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

MessageService collapses the new ATTEMPTED state into TIMED_OUT_QUEUED, so a possibly-delivered message is reported as one that will never arrive #571

New Issue
Closed
opened 2026-09-12 12:49:15 +02:00 by ltms · 7 comments
ltms commented 2026-09-12 12:49:15 +02:00
Owner
Copy Link
Copy Source

Follow-up to #551, split out of it deliberately so #551 is not blocked. Filed after verifying #551's PR #569; the worker on that unit flagged it and the diagnosis is theirs.

The shape

#551 gives Injector a third Pending.State, ATTEMPTED, meaning "send() was called and we do not know whether the text reached the pane". It also gives Cancellation a matching third answer, so the uncertainty survives the call that reports it.

MessageService.java:973 is the only reader of Cancellation outside Injector:

wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED;
...
return recorded(new Reply(
        wasDelivered ? Outcome.TIMED_OUT_WORKING : Outcome.TIMED_OUT_QUEUED, null));

ATTEMPTED != DELIVERED, so it becomes TIMED_OUT_QUEUED. The three-valued answer is flattened back to two, one layer above the place #551 just fixed.

This is the same defect shape #551 fixed, moved up the call stack. cancellationOf was a two-way split on a three-way question; this is now the surviving one.

What is and is not a regression

Measured, so nobody re-derives it:

  • Before #551: a HerdrException after the paste set NOT_DELIVERED → cancellationOf → NOT_DELIVERED → wasDelivered = false → TIMED_OUT_QUEUED.
  • After #551: the same failure sets ATTEMPTED → Cancellation.ATTEMPTED → wasDelivered = false → TIMED_OUT_QUEUED.

Caller-visible behaviour is identical. #551 is not a regression and should merge on its own. What changed is that the information now exists and is thrown away, where before it was never captured. That is why this is a separate ticket and not a bug in #551.

Why it matters

TIMED_OUT_QUEUED tells the caller the message will not arrive later. On the ATTEMPTED route the message may already have arrived — in full. AgentControl.send calls herdr's agent.prompt, which pastes and submits in one call, so the target may be working on that exact turn while the caller is told it never landed.

The caller's natural recovery from TIMED_OUT_QUEUED is to resend. On this route that is a double delivery: the same brief typed into the pane twice. #551's own javadoc makes the point — reading the uncertain answer as a confident negative "invites a resend of text that may already be sitting in the pane, which is worse than the ambiguity itself."

I have not observed a double delivery in production. The window needs a herdr response-half failure during a send that also times out.

The decision this needs

Outcome currently has no value for "we do not know". The options, and none is obviously right:

  1. A fourth outcome, e.g. TIMED_OUT_UNCONFIRMED. Honest, and every caller that switches on Outcome must be found and updated — the compiler helps only where the switch is exhaustive. Check fleet_send's MCP surface and anything that renders an outcome to an operator.
  2. Map ATTEMPTED to TIMED_OUT_WORKING. Cheap, no new value, and arguably closer to the truth — the worker may well be working. But it asserts a positive on evidence that does not support one, which is the mirror of the current bug rather than a fix.
  3. Keep TIMED_OUT_QUEUED and fix only the javadoc so callers know the guarantee has three routes and one is uncertain. This is what PR #569 does, and it is the minimum. It leaves the wrong value in place and relies on every caller reading the doc.

My view: 1, unless someone can show no caller distinguishes them. 3 has already landed with #551 and is not sufficient on its own.

Acceptance

  • Whoever takes this must enumerate every reader of Outcome and say in the PR what each does with the new or remapped value — the same checklist #551 was held to for Pending.State. A grep for Outcome. across main and test sources, with a positive control proving the pattern matches.
  • A test that a send whose delivery ended ATTEMPTED does not report an outcome promising the message will never arrive. Red before the fix.
  • If option 1 is chosen: a test that the three existing outcomes are unchanged for their own routes, so this does not quietly re-route an existing case.
  • Mutation proof per new test: line-anchored sed, anchor counted with grep -Fxc (not awk -v — it escape-processes the value and silently counts 0 on any line holding \t or \n), count down by exactly one, red with the test's own assertion message and observed value, restored byte-identical under shasum -a 256, green control, and the proof cell run against the un-mutated tree first to confirm it reports not-applied.

Do not start this until #551 has merged

It edits the same region and will conflict.

Related: #551 (the layer below, where ATTEMPTED is created), #513 (the javadoc that enumerates TIMED_OUT_QUEUED's routes — this ticket adds the third), #512 (one symbol carrying two states).

Follow-up to #551, split out of it deliberately so #551 is not blocked. Filed after verifying #551's PR #569; the worker on that unit flagged it and the diagnosis is theirs. ## The shape #551 gives `Injector` a third `Pending.State`, `ATTEMPTED`, meaning "`send()` was called and we do not know whether the text reached the pane". It also gives `Cancellation` a matching third answer, so the uncertainty survives the call that reports it. `MessageService.java:973` is the **only** reader of `Cancellation` outside `Injector`: ```java wasDelivered = injector.cancel(delivery) == Injector.Cancellation.DELIVERED; ... return recorded(new Reply( wasDelivered ? Outcome.TIMED_OUT_WORKING : Outcome.TIMED_OUT_QUEUED, null)); ``` `ATTEMPTED != DELIVERED`, so it becomes `TIMED_OUT_QUEUED`. The three-valued answer is flattened back to two, one layer above the place #551 just fixed. **This is the same defect shape #551 fixed, moved up the call stack.** `cancellationOf` was a two-way split on a three-way question; this is now the surviving one. ## What is and is not a regression Measured, so nobody re-derives it: - **Before #551**: a `HerdrException` after the paste set `NOT_DELIVERED` → `cancellationOf` → `NOT_DELIVERED` → `wasDelivered = false` → `TIMED_OUT_QUEUED`. - **After #551**: the same failure sets `ATTEMPTED` → `Cancellation.ATTEMPTED` → `wasDelivered = false` → `TIMED_OUT_QUEUED`. **Caller-visible behaviour is identical.** #551 is not a regression and should merge on its own. What changed is that the information now *exists* and is thrown away, where before it was never captured. That is why this is a separate ticket and not a bug in #551. ## Why it matters `TIMED_OUT_QUEUED` tells the caller the message will not arrive later. On the `ATTEMPTED` route the message may **already** have arrived — in full. `AgentControl.send` calls herdr's `agent.prompt`, which pastes *and submits* in one call, so the target may be working on that exact turn while the caller is told it never landed. The caller's natural recovery from `TIMED_OUT_QUEUED` is to resend. On this route that is a double delivery: the same brief typed into the pane twice. #551's own javadoc makes the point — reading the uncertain answer as a confident negative "invites a resend of text that may already be sitting in the pane, which is worse than the ambiguity itself." I have **not** observed a double delivery in production. The window needs a herdr response-half failure during a `send` that also times out. ## The decision this needs `Outcome` currently has no value for "we do not know". The options, and none is obviously right: 1. **A fourth outcome**, e.g. `TIMED_OUT_UNCONFIRMED`. Honest, and every caller that switches on `Outcome` must be found and updated — the compiler helps only where the switch is exhaustive. Check `fleet_send`'s MCP surface and anything that renders an outcome to an operator. 2. **Map `ATTEMPTED` to `TIMED_OUT_WORKING`.** Cheap, no new value, and arguably closer to the truth — the worker may well be working. But it asserts a positive on evidence that does not support one, which is the mirror of the current bug rather than a fix. 3. **Keep `TIMED_OUT_QUEUED` and fix only the javadoc** so callers know the guarantee has three routes and one is uncertain. This is what PR #569 does, and it is the minimum. It leaves the wrong value in place and relies on every caller reading the doc. My view: 1, unless someone can show no caller distinguishes them. 3 has already landed with #551 and is not sufficient on its own. ## Acceptance - Whoever takes this must **enumerate every reader of `Outcome`** and say in the PR what each does with the new or remapped value — the same checklist #551 was held to for `Pending.State`. A `grep` for `Outcome.` across main and test sources, with a positive control proving the pattern matches. - A test that a send whose delivery ended `ATTEMPTED` does not report an outcome promising the message will never arrive. Red before the fix. - If option 1 is chosen: a test that the three existing outcomes are unchanged for their own routes, so this does not quietly re-route an existing case. - Mutation proof per new test: line-anchored `sed`, anchor counted with `grep -Fxc` (**not** `awk -v` — it escape-processes the value and silently counts 0 on any line holding `\t` or `\n`), count down by exactly one, red with the test's own assertion message and observed value, restored byte-identical under `shasum -a 256`, green control, and the proof cell run against the un-mutated tree first to confirm it reports not-applied. ## Do not start this until #551 has merged It edits the same region and will conflict. Related: #551 (the layer below, where `ATTEMPTED` is created), #513 (the javadoc that enumerates `TIMED_OUT_QUEUED`'s routes — this ticket adds the third), #512 (one symbol carrying two states).
ltms commented 2026-09-12 14:08:33 +02:00
Author
Owner
Copy Link
Copy Source

Unblocked — #551 merged as 384867d (PR #569).

Decision: option 1. Add a fourth outcome.

I made the design call myself rather than delegate it, and I measured the thing the ticket said would decide it: how many callers there are, and whether the compiler finds them. Here is that measurement, run on main at 204da67.

Every reader of MessageService.Outcome — three files, and that is all

grep -rnE 'case (REPLIED|COMPLETED_UNREPLIED|WORKER_FAILED|BACKEND_EXHAUSTED|QUESTION|TIMED_OUT_WORKING|TIMED_OUT_QUEUED|STALE_TURN)\b' src/main/java src/test/java
  → src/main/java/dev/ltms/fleet/mcp/FleetMcp.java
  → src/main/java/dev/ltms/fleet/msg/MessageService.java
  → src/main/java/dev/ltms/fleet/rest/FleetApp.java

No test file switches on it. No Outcome.values() and no Outcome.valueOf anywhere in main or test.

A correction to my own acceptance criterion in this ticket. I asked for "a grep for Outcome. across main and test sources". That grep is wrong, and I ran it first and got a wrong answer. Two ways:

  • It misses FleetMcp.java entirely. Every switch there is written case REPLIED ->, with no qualifier, so the file never appears. That is the whole MCP rendering surface — the most important reader — invisible to the search the ticket asked for.
  • It adds ConfigRef.java, which has its own unrelated Outcome record for config reloads. A false positive that reads as a caller.

So: search by constant name, not by Outcome., and keep the positive control the ticket already asks for. This is the same lesson as [a zero match is not a finding] — a non-zero count needs the control just as much as a zero does, because a wrong pattern can be wrong in both directions at once.

Where the compiler helps, and the one place it does not

site shape does adding a constant break the build?
MessageService.sendOutcomeLabel :645 switch expression, no default yes — compile error
FleetMcp.formatReply :786 switch expression, no default yes — compile error
FleetApp.writeReply :628 switch statement with default -> at :641, wrapping an inner switch expression with default -> "done" at :649 NO — silent

So the cost of option 1 is two compile errors that point you straight at the code, plus one site that must be found by reading. That is a small, bounded cost, and it settles the ticket's "unless someone can show no caller distinguishes them" condition: they do distinguish them, and almost all of them will say so loudly.

The hazard that makes this worth doing properly

FleetApp.java:649 reads:

default -> "done"; // unreachable (terminal outcomes handled above)

Add TIMED_OUT_UNCONFIRMED and that comment becomes false. The branch becomes reachable, and the REST caller is told "status": "done" — the delegation completed — for the one case where we do not know whether the message even arrived. That is strictly worse than today's "queued". A comment claiming an invariant is a free test case; this one is about to go false, so it gets an explicit arm and a test, not an edit.

Why not the other two

Option 2 (map ATTEMPTED to TIMED_OUT_WORKING) asserts a positive from evidence that does not support one. It is the same defect as today's, pointing the other way. Rejected.

Option 3 (javadoc only) already landed with #551. It leaves the wrong value in place. Not sufficient on its own, as the ticket says.

The name is TIMED_OUT_UNCONFIRMED. FleetMcp.java:807 renders a timeout outcome as name().toLowerCase().replace("timed_out_", ""), so it reads out as "unconfirmed" with no extra code — but the arm still has to be added to that switch deliberately, not left to the string trick.

Delegating the implementation now.

Unblocked — #551 merged as 384867d (PR #569). ## Decision: option 1. Add a fourth outcome. I made the design call myself rather than delegate it, and I measured the thing the ticket said would decide it: **how many callers there are, and whether the compiler finds them.** Here is that measurement, run on `main` at 204da67. ### Every reader of `MessageService.Outcome` — three files, and that is all ``` grep -rnE 'case (REPLIED|COMPLETED_UNREPLIED|WORKER_FAILED|BACKEND_EXHAUSTED|QUESTION|TIMED_OUT_WORKING|TIMED_OUT_QUEUED|STALE_TURN)\b' src/main/java src/test/java → src/main/java/dev/ltms/fleet/mcp/FleetMcp.java → src/main/java/dev/ltms/fleet/msg/MessageService.java → src/main/java/dev/ltms/fleet/rest/FleetApp.java ``` No test file switches on it. No `Outcome.values()` and no `Outcome.valueOf` anywhere in main or test. **A correction to my own acceptance criterion in this ticket.** I asked for "a `grep` for `Outcome.` across main and test sources". That grep is **wrong**, and I ran it first and got a wrong answer. Two ways: - It **misses** `FleetMcp.java` entirely. Every switch there is written `case REPLIED ->`, with no qualifier, so the file never appears. That is the whole MCP rendering surface — the most important reader — invisible to the search the ticket asked for. - It **adds** `ConfigRef.java`, which has its own unrelated `Outcome` record for config reloads. A false positive that reads as a caller. So: search by **constant name**, not by `Outcome.`, and keep the positive control the ticket already asks for. This is the same lesson as [a zero match is not a finding] — a non-zero count needs the control just as much as a zero does, because a wrong pattern can be wrong in both directions at once. ### Where the compiler helps, and the one place it does not | site | shape | does adding a constant break the build? | |---|---|---| | `MessageService.sendOutcomeLabel` `:645` | switch **expression**, no `default` | **yes** — compile error | | `FleetMcp.formatReply` `:786` | switch **expression**, no `default` | **yes** — compile error | | `FleetApp.writeReply` `:628` | switch **statement** with `default ->` at `:641`, wrapping an inner switch expression with `default -> "done"` at `:649` | **NO — silent** | So the cost of option 1 is two compile errors that point you straight at the code, plus one site that must be found by reading. That is a small, bounded cost, and it settles the ticket's "unless someone can show no caller distinguishes them" condition: they do distinguish them, and almost all of them will say so loudly. ### The hazard that makes this worth doing properly `FleetApp.java:649` reads: ```java default -> "done"; // unreachable (terminal outcomes handled above) ``` Add `TIMED_OUT_UNCONFIRMED` and that comment becomes false. The branch becomes reachable, and the REST caller is told `"status": "done"` — **the delegation completed** — for the one case where we do not know whether the message even arrived. That is strictly worse than today's `"queued"`. A comment claiming an invariant is a free test case; this one is about to go false, so it gets an explicit arm and a test, not an edit. ### Why not the other two **Option 2** (map `ATTEMPTED` to `TIMED_OUT_WORKING`) asserts a positive from evidence that does not support one. It is the same defect as today's, pointing the other way. Rejected. **Option 3** (javadoc only) already landed with #551. It leaves the wrong value in place. Not sufficient on its own, as the ticket says. The name is **`TIMED_OUT_UNCONFIRMED`**. `FleetMcp.java:807` renders a timeout outcome as `name().toLowerCase().replace("timed_out_", "")`, so it reads out as "unconfirmed" with no extra code — but the arm still has to be added to that switch deliberately, not left to the string trick. Delegating the implementation now.
ltms commented 2026-09-12 14:16:51 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION — this supersedes the brief. Read it before you commit.

Credit: the fleet01 lead raised this while the unit was in flight.

Add one requirement: make the compiler the enumeration, not the grep

My earlier comment told you to find every reader with a hand-listed alternation of constant names. That command is stale the moment your own commit lands — it will be missing TIMED_OUT_UNCONFIRMED — and it goes stale silently, because a fixed alternation still returns plenty of hits. That is the same failure as the Outcome\. grep it replaced: a plausible total with the membership wrong.

It is good enough to find the sites today, because every existing switch mentions at least one of the eight. It is not good enough to leave behind. So:

Every switch over MessageService.Outcome must be exhaustive and carry no default. Then adding a ninth constant is a compile error at every site that has to decide about it, and nobody ever has to run this search again.

Measured on main at 204da67 — three of the five are already there:

site has a default?
MessageService.java:646 sendOutcomeLabel no — already exhaustive
FleetMcp.java:786 formatReply no — already exhaustive
FleetApp.java:641 — outer switch statement in writeReply yes
FleetApp.java:649 — inner switch expression, default -> "done" yes

(FleetMcp.java:775 and FleetApp.java:685 switch on AskResult.Outcome, a different enum. Leave them alone.)

The inner one at :649 is the required change. default -> "done" produces a value that lies: it would tell a REST caller the delegation completed, for the one case where we do not know whether the message arrived. Delete the default and list every constant. Some arms will be unreachable in practice because the outer switch handles them first — write them anyway. An unreachable-but-explicit arm that the compiler forces you to keep correct beats a default that silently absorbs the next constant.

The outer default at :641 may stay if you want it: "everything not terminal is a 202" is a real rule, and with the inner switch exhaustive the compiler still stops you at the line that matters. Your call — state which you chose and why.

The property to hold, in one sentence: adding a constant to Outcome must fail the build at every site that decides about it. Prove it. Add a tenth constant temporarily, show me the compile errors and which files they name, then remove it. That is a better proof than any grep, and it is the acceptance criterion I should have written in the first place.

Keep the reflective check

grep -rn 'Outcome.values()\|Outcome\.valueOf' src/main/java src/test/java stays. No compiler check and no name search catches the reflective back door. I measured zero today; re-run it and say so.

Why bare constants hid the most important reader

Worth recording, because it will recur. Before Java 21 an enum case label had to be unqualified. Java 21+ permits the qualified form, and this module targets maven.compiler.release 25, so case Outcome.REPLIED -> would compile — but nobody writes it, so the bare form is what is in the tree. The consequence: grep 'EnumName\.' misses enum switches almost always, and misses precisely the exhaustive ones — the readers that matter most, because they are the ones the compiler would have forced you to update.

Grep cannot see these readers. javac cannot miss them. That is the whole argument for the change above.

Nothing else in the brief changes

The decision is still TIMED_OUT_UNCONFIRMED. The three tests still stand, and the mutation discipline is unchanged. This adds one requirement and one proof; it removes nothing.

## CORRECTION — this supersedes the brief. Read it before you commit. Credit: the fleet01 lead raised this while the unit was in flight. ### Add one requirement: make the compiler the enumeration, not the grep My earlier comment told you to find every reader with a hand-listed alternation of constant names. **That command is stale the moment your own commit lands** — it will be missing `TIMED_OUT_UNCONFIRMED` — and it goes stale *silently*, because a fixed alternation still returns plenty of hits. That is the same failure as the `Outcome\.` grep it replaced: a plausible total with the membership wrong. It is good enough to *find* the sites today, because every existing switch mentions at least one of the eight. It is not good enough to leave behind. So: **Every switch over `MessageService.Outcome` must be exhaustive and carry no `default`.** Then adding a ninth constant is a compile error at every site that has to decide about it, and nobody ever has to run this search again. Measured on `main` at 204da67 — three of the five are already there: | site | has a `default`? | |---|---| | `MessageService.java:646` `sendOutcomeLabel` | no — already exhaustive | | `FleetMcp.java:786` `formatReply` | no — already exhaustive | | `FleetApp.java:641` — outer switch **statement** in `writeReply` | **yes** | | `FleetApp.java:649` — inner switch **expression**, `default -> "done"` | **yes** | (`FleetMcp.java:775` and `FleetApp.java:685` switch on `AskResult.Outcome`, a different enum. Leave them alone.) **The inner one at `:649` is the required change.** `default -> "done"` produces a *value that lies*: it would tell a REST caller the delegation completed, for the one case where we do not know whether the message arrived. Delete the `default` and list every constant. Some arms will be unreachable in practice because the outer switch handles them first — write them anyway. An unreachable-but-explicit arm that the compiler forces you to keep correct beats a `default` that silently absorbs the next constant. The outer `default` at `:641` may stay if you want it: "everything not terminal is a 202" is a real rule, and with the inner switch exhaustive the compiler still stops you at the line that matters. Your call — state which you chose and why. **The property to hold, in one sentence:** adding a constant to `Outcome` must fail the build at every site that decides about it. Prove it. Add a tenth constant temporarily, show me the compile errors and which files they name, then remove it. That is a better proof than any grep, and it is the acceptance criterion I should have written in the first place. ### Keep the reflective check `grep -rn 'Outcome.values()\|Outcome\.valueOf' src/main/java src/test/java` stays. No compiler check and no name search catches the reflective back door. I measured zero today; re-run it and say so. ### Why bare constants hid the most important reader Worth recording, because it will recur. Before Java 21 an enum `case` label **had to** be unqualified. Java 21+ permits the qualified form, and this module targets `maven.compiler.release` **25**, so `case Outcome.REPLIED ->` would compile — but nobody writes it, so the bare form is what is in the tree. The consequence: **`grep 'EnumName\.'` misses enum switches almost always, and misses precisely the exhaustive ones** — the readers that matter most, because they are the ones the compiler would have forced you to update. Grep cannot see these readers. `javac` cannot miss them. That is the whole argument for the change above. ### Nothing else in the brief changes The decision is still `TIMED_OUT_UNCONFIRMED`. The three tests still stand, and the mutation discipline is unchanged. This adds one requirement and one proof; it removes nothing.
ltms commented 2026-09-12 14:19:35 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 2 — supersedes the compile-error proof in my last comment. Read before you commit.

Credit again: the fleet01 lead. Both points below are theirs; the measurements are mine.

1. ORDER IS LOAD-BEARING. Delete the default arms FIRST, then add the temporary constant.

My last comment said "add a tenth constant temporarily, show the compile errors and which files they name, remove it" and did not say when. Run it in the wrong order and it certifies the opposite of what it looks like it certifies:

site default today what the temp constant does if you add it first
MessageService.java:646 none errors — already clean, no work needed
FleetMcp.java:786 none errors — already clean, no work needed
FleetApp.java:641 yes compiles silently — invisible
FleetApp.java:649 yes compiles silently — invisible

So a worker who adds the constant first gets a tidy two-file error list naming only the sites that need no work, and saying nothing about :649 — the one that produces a value lying to a REST caller. It reads as a clean, complete enumeration that happens to omit the entire job.

The thing you are hunting is also the thing that suppresses the alarm. A default is what makes the compile error not happen, so a proof that relies on compile errors cannot see a default.

The order: delete both default arms, then add the temp constant, then all four must error. If fewer than four error, you missed a default. Report the count.

2. The compile proof enumerates SWITCHES, not READERS. Here is its blind spot, measured.

Equality comparisons, ternaries and != checks all compile fine with a new constant and fall silently to the else branch. There are 8 of them, in 2 files, and javac will not say a word about any:

MessageService.java:797   if (completedHere && outcome.outcome() == Outcome.WORKER_FAILED)
MessageService.java:1224  if (result.outcome() != Outcome.QUESTION && task != null)
MessageService.java:1300  if (result.outcome() == Outcome.QUESTION)
MessageService.java:1362  String source = r.outcome() == Outcome.REPLIED ? "reply" : "transcript";
MessageService.java:1368  boolean carriesReason = r.outcome() == Outcome.WORKER_FAILED || ... == Outcome.BACKEND_EXHAUSTED;
FleetApp.java:638         String source = reply.outcome() == MessageService.Outcome.REPLIED ? "reply" : "transcript";
FleetApp.java:651-652     (reply.outcome() == WORKER_FAILED || reply.outcome() == BACKEND_EXHAUSTED)

No EnumMap or EnumSet over Outcome — the EnumSet hits in member/ are over Capability, a different enum. So comparisons are the only third door here.

Give each of those 8 sites a one-line verdict in your PR: does TIMED_OUT_UNCONFIRMED reach it, and if it does, is the else branch right for it? Do not assume a timeout outcome cannot reach the two ? "reply" : "transcript" ternaries — check the path and say so.

3. Why my first grep was not wrong, just mislabelled — this is the useful part

I called grep 'Outcome\.' a broken search. It is not. It is the comparison finder, and I mislabelled it as the reader finder. The two forms partition the codebase almost perfectly:

file qualified Outcome.X hits bare case X -> label lines
FleetMcp.java 0 23
MessageService.java 28 11
FleetApp.java 3 11

FleetMcp.java has zero qualified references. It is pure switches, which is exactly why the qualified grep could not see it and why javac cannot miss it. The comparisons, meanwhile, are written qualified — so the grep finds all 8 of them and the compiler finds none.

Three doors, three instruments, none of which sees all of it:

  • javac (no default) → finds the switches
  • the constant-name grep → finds the comparisons
  • grep 'Outcome.values()\|Outcome\.valueOf' → finds the reflection (zero today, re-run it)

So keep the constant grep for this turn. Not as the enumeration — as the cross-check on the compile proof's blind spot. Run all three and report all three.

Nothing else changes

Still TIMED_OUT_UNCONFIRMED. Same three tests, same mutation discipline. This fixes the order of one proof and adds a second one.

## CORRECTION 2 — supersedes the compile-error proof in my last comment. Read before you commit. Credit again: the fleet01 lead. Both points below are theirs; the measurements are mine. ### 1. ORDER IS LOAD-BEARING. Delete the `default` arms FIRST, then add the temporary constant. My last comment said "add a tenth constant temporarily, show the compile errors and which files they name, remove it" and **did not say when**. Run it in the wrong order and it certifies the opposite of what it looks like it certifies: | site | `default` today | what the temp constant does if you add it first | |---|---|---| | `MessageService.java:646` | none | **errors** — already clean, no work needed | | `FleetMcp.java:786` | none | **errors** — already clean, no work needed | | `FleetApp.java:641` | yes | **compiles silently — invisible** | | `FleetApp.java:649` | yes | **compiles silently — invisible** | So a worker who adds the constant first gets a tidy two-file error list naming only the sites that need **no** work, and saying nothing about `:649` — the one that produces a value lying to a REST caller. It reads as a clean, complete enumeration that happens to omit the entire job. **The thing you are hunting is also the thing that suppresses the alarm.** A `default` is what makes the compile error not happen, so a proof that relies on compile errors cannot see a `default`. **The order: delete both `default` arms, then add the temp constant, then all four must error.** If fewer than four error, you missed a `default`. Report the count. ### 2. The compile proof enumerates SWITCHES, not READERS. Here is its blind spot, measured. Equality comparisons, ternaries and `!=` checks all compile fine with a new constant and fall silently to the else branch. There are **8 of them**, in 2 files, and `javac` will not say a word about any: ``` MessageService.java:797 if (completedHere && outcome.outcome() == Outcome.WORKER_FAILED) MessageService.java:1224 if (result.outcome() != Outcome.QUESTION && task != null) MessageService.java:1300 if (result.outcome() == Outcome.QUESTION) MessageService.java:1362 String source = r.outcome() == Outcome.REPLIED ? "reply" : "transcript"; MessageService.java:1368 boolean carriesReason = r.outcome() == Outcome.WORKER_FAILED || ... == Outcome.BACKEND_EXHAUSTED; FleetApp.java:638 String source = reply.outcome() == MessageService.Outcome.REPLIED ? "reply" : "transcript"; FleetApp.java:651-652 (reply.outcome() == WORKER_FAILED || reply.outcome() == BACKEND_EXHAUSTED) ``` No `EnumMap` or `EnumSet` over `Outcome` — the `EnumSet` hits in `member/` are over `Capability`, a different enum. So comparisons are the only third door here. **Give each of those 8 sites a one-line verdict in your PR:** does `TIMED_OUT_UNCONFIRMED` reach it, and if it does, is the else branch right for it? Do not assume a timeout outcome cannot reach the two `? "reply" : "transcript"` ternaries — check the path and say so. ### 3. Why my first grep was not wrong, just mislabelled — this is the useful part I called `grep 'Outcome\.'` a broken search. It is not. It is the **comparison** finder, and I mislabelled it as the reader finder. The two forms partition the codebase almost perfectly: | file | qualified `Outcome.X` hits | bare `case X ->` label lines | |---|---|---| | `FleetMcp.java` | **0** | 23 | | `MessageService.java` | 28 | 11 | | `FleetApp.java` | 3 | 11 | `FleetMcp.java` has **zero** qualified references. It is pure switches, which is exactly why the qualified grep could not see it and why `javac` cannot miss it. The comparisons, meanwhile, are written qualified — so the grep finds all 8 of them and the compiler finds none. **Three doors, three instruments, none of which sees all of it:** - `javac` (no `default`) → finds the **switches** - the constant-name grep → finds the **comparisons** - `grep 'Outcome.values()\|Outcome\.valueOf'` → finds the **reflection** (zero today, re-run it) So **keep the constant grep for this turn.** Not as the enumeration — as the cross-check on the compile proof's blind spot. Run all three and report all three. ### Nothing else changes Still `TIMED_OUT_UNCONFIRMED`. Same three tests, same mutation discipline. This fixes the order of one proof and adds a second one.
ltms commented 2026-09-12 14:22:27 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 3 — the door count is FIVE, not three. Read before you commit.

Credit: the fleet01 lead again. They predicted two more doors; I measured both, and one of them is not empty.

DOOR 4 — name() / ordinal(). Two live sites. No instrument I gave you sees these.

grep -rn 'outcome()\.name()\|outcome()\.ordinal()\|Outcome\[\]' src/main/java src/test/java
  MessageService.java:1371   "no reply — " + r.outcome().name().toLowerCase()
  FleetMcp.java:807          r.outcome().name().toLowerCase().replace("timed_out_", "")

MessageService.java:1371 is the one that matters, and it is worse than a blind spot — it is the designed path. Its own comment says so:

// A wedged worker (CB-109) or a backend-exhausted classification (CB-578 stage A) carries
// the real cause as its reason; the timeout/busy outcomes carry none, so fall back to the
// outcome name.
boolean carriesReason = r.outcome() == Outcome.WORKER_FAILED || r.outcome() == Outcome.BACKEND_EXHAUSTED;
String detail = carriesReason && r.text() != null
        ? r.text()
        : "no reply — " + r.outcome().name().toLowerCase();

TIMED_OUT_UNCONFIRMED is a timeout outcome, so it carries no reason, so it takes the else. The raw enum name goes onto the wire as no reply — timed_out_unconfirmed, with no code change and no compiler check. You will ship a new externally-visible token without touching a line.

Decide deliberately whether that string is the one you want a caller to read, and say which you chose. Do not let it happen by default.

FleetMcp.java:807 sits inside the case TIMED_OUT_WORKING, TIMED_OUT_QUEUED, BUSY -> arm, so javac will force you to decide where the new constant goes — but it will not check the string that comes out. Adding it to that arm renders "worker unconfirmed", which reads correctly. Confirm that yourself rather than trusting me.

Door 4a — constant names as quoted string literals — is ZERO, and I controlled the pattern rather than trusting the zero (a regex of the same shape matches in 4 files elsewhere). Nothing matches on these names as text. Re-run it and say so:

grep -rnE '"(REPLIED|COMPLETED_UNREPLIED|WORKER_FAILED|BACKEND_EXHAUSTED|QUESTION|TIMED_OUT_WORKING|TIMED_OUT_QUEUED|STALE_TURN)"' src/main/java src/test/java

No Outcome[] and no ordinal indexing either — so no fixed-size array to throw at runtime on the new member.

DOOR 5 — THE WIRE FORM. Outside the tree, so outside every instrument.

This enum has three serialized surfaces. A consumer matching on any of them is a reader no grep of this repo can reach — another service, a script, a dashboard, a Claude session parsing MCP output. Adding a constant is a protocol change for them, silent on both sides.

surface what a consumer sees
REST, FleetApp.writeReply "status": working / queued / busy / failed / backend_exhausted / done
MCP, FleetMcp.java:807 [no reply within Nms — worker <name minus timed_out_>; retry or poll status]
MCP, MessageService.java:1371 no reply — <raw enum name, lowercased>

New required deliverable: your PR must list every serialized string this change adds or alters, on all three surfaces, exactly as a consumer would see it. You are not chasing the consumers — you cannot reach them. You are putting it where whoever owns one will look. That is the same push/pull split the charter draws: a push needs the recipient free at send time, a pull only needs them to look before acting.

The five doors, and the instrument for each

# door instrument today
1 switches javac, every default deleted 3 clean, 2 to fix
2 comparisons / ternaries constant-name grep 8 sites
3 reflection values() / valueOf grep 0
4 name() / ordinal() the grep above 2 sites
5 wire form none — it is outside the repo 3 surfaces

Run 1–4 and report all four counts. Write door 5 out by hand.

The count has gone 1 → 2 → 3 → 5 this afternoon, and every step was a measurement, not a prediction. So do not treat five as final either. If you find a sixth, say so; that is a finding, not a failure to follow the brief.

Nothing else changes

Still TIMED_OUT_UNCONFIRMED. Same three tests, same mutation discipline, same order rule (delete the default arms first, then add the temp constant, then all four sites must error).

## CORRECTION 3 — the door count is FIVE, not three. Read before you commit. Credit: the fleet01 lead again. They predicted two more doors; I measured both, and one of them is **not empty**. ### DOOR 4 — `name()` / `ordinal()`. Two live sites. No instrument I gave you sees these. ``` grep -rn 'outcome()\.name()\|outcome()\.ordinal()\|Outcome\[\]' src/main/java src/test/java MessageService.java:1371 "no reply — " + r.outcome().name().toLowerCase() FleetMcp.java:807 r.outcome().name().toLowerCase().replace("timed_out_", "") ``` **`MessageService.java:1371` is the one that matters, and it is worse than a blind spot — it is the designed path.** Its own comment says so: ```java // A wedged worker (CB-109) or a backend-exhausted classification (CB-578 stage A) carries // the real cause as its reason; the timeout/busy outcomes carry none, so fall back to the // outcome name. boolean carriesReason = r.outcome() == Outcome.WORKER_FAILED || r.outcome() == Outcome.BACKEND_EXHAUSTED; String detail = carriesReason && r.text() != null ? r.text() : "no reply — " + r.outcome().name().toLowerCase(); ``` `TIMED_OUT_UNCONFIRMED` is a timeout outcome, so it carries no reason, so it takes the `else`. **The raw enum name goes onto the wire as `no reply — timed_out_unconfirmed`, with no code change and no compiler check.** You will ship a new externally-visible token without touching a line. Decide deliberately whether that string is the one you want a caller to read, and say which you chose. Do not let it happen by default. `FleetMcp.java:807` sits inside the `case TIMED_OUT_WORKING, TIMED_OUT_QUEUED, BUSY ->` arm, so `javac` will force you to decide where the new constant goes — but it will **not** check the string that comes out. Adding it to that arm renders "worker unconfirmed", which reads correctly. Confirm that yourself rather than trusting me. **Door 4a — constant names as quoted string literals — is ZERO**, and I controlled the pattern rather than trusting the zero (a regex of the same shape matches in 4 files elsewhere). Nothing matches on these names as text. Re-run it and say so: ``` grep -rnE '"(REPLIED|COMPLETED_UNREPLIED|WORKER_FAILED|BACKEND_EXHAUSTED|QUESTION|TIMED_OUT_WORKING|TIMED_OUT_QUEUED|STALE_TURN)"' src/main/java src/test/java ``` No `Outcome[]` and no ordinal indexing either — so no fixed-size array to throw at runtime on the new member. ### DOOR 5 — THE WIRE FORM. Outside the tree, so outside every instrument. This enum has three serialized surfaces. A consumer matching on any of them is a reader no grep of this repo can reach — another service, a script, a dashboard, a Claude session parsing MCP output. **Adding a constant is a protocol change for them, silent on both sides.** | surface | what a consumer sees | |---|---| | REST, `FleetApp.writeReply` | `"status"`: `working` / `queued` / `busy` / `failed` / `backend_exhausted` / `done` | | MCP, `FleetMcp.java:807` | `[no reply within Nms — worker <name minus timed_out_>; retry or poll status]` | | MCP, `MessageService.java:1371` | `no reply — <raw enum name, lowercased>` | **New required deliverable: your PR must list every serialized string this change adds or alters**, on all three surfaces, exactly as a consumer would see it. You are not chasing the consumers — you cannot reach them. You are putting it where whoever owns one will look. That is the same push/pull split the charter draws: a push needs the recipient free at send time, a pull only needs them to look before acting. ### The five doors, and the instrument for each | # | door | instrument | today | |---|---|---|---| | 1 | switches | `javac`, every `default` deleted | 3 clean, 2 to fix | | 2 | comparisons / ternaries | constant-name grep | **8 sites** | | 3 | reflection | `values()` / `valueOf` grep | 0 | | 4 | `name()` / `ordinal()` | the grep above | **2 sites** | | 5 | wire form | none — it is outside the repo | 3 surfaces | Run 1–4 and report all four counts. Write door 5 out by hand. **The count has gone 1 → 2 → 3 → 5 this afternoon, and every step was a measurement, not a prediction.** So do not treat five as final either. If you find a sixth, say so; that is a finding, not a failure to follow the brief. ### Nothing else changes Still `TIMED_OUT_UNCONFIRMED`. Same three tests, same mutation discipline, same order rule (delete the `default` arms first, then add the temp constant, then all four sites must error).
ltms commented 2026-09-12 14:25:38 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 4 — three measurements, and a sibling enum that already solves door 5. No new work for you except one sentence.

Credit: the fleet01 lead. All three checks are theirs; the measurements are mine.

Door 6 — readers inside the repo but outside the Java source roots. MEASURED: no executable reader.

Every grep in the door table was scoped src/main/java src/test/java. That scoping is itself a blind spot with a shape, and the fleet01 lead was right to point at it — scripts/ in this repo already parses daemon output, so a shell script matching an outcome token would have been house style, not a stretch.

Measured repo-wide:

  • Constant names outside the Java roots: 8 hits, all prose. docs/CB-201-227-Refinement.md, docs/CB-308-Multi-Host-Federation.md, docs/M4-Fleet-Health.md ×2, docs/wiki-audit.md ×2, fleetd/fleetd.example.yaml ×2 (both in comments about the classification, not values). None of these parse or match — they are documentation.
  • Wire tokens (backend_exhausted, timed_out_, no reply —) outside *.java: ZERO. Positive control: git grep reaches non-Java files fine here (5 files match healthz).

So no script, no schema, no non-Java client reads this enum. Door 6 is empty of readers. Door 5 shrinks to genuinely-external consumers, which the PR deliverable already covers.

One knock-on, out of scope but worth one line in your reply: docs/wiki-audit.md:140-141 already records that a documented outcome list is incomplete. Your new constant makes those doc lists staler. Note it; do not go fix them.

Door 5's contents — CHECKED, and my table was right

The fleet01 lead asked whether anything overrides the enum's string form, because if a toString(), @JsonValue or custom serializer existed, .name() and the wire form would be two different tokens for one outcome and my table would have listed one as if it were both.

MessageService.Outcome is 8 bare constants with javadoc and nothing else — no toString(), no Jackson annotation, no serializer. So .name().toLowerCase() is the wire form. The table stands.

Door 3's zero retires a whole hazard class — say so in the PR

Outcome.values() / Outcome.valueOf is 0, re-measured, exit 1, with a control confirming the pattern works on other enums here (MemberSession.State.values() at FleetMetrics.java:80).

The inference neither of us had drawn: nothing parses a string back into this enum. It is write-only to the wire. So the entire round-trip class does not exist here — no old persisted value failing to parse in a new binary, no new value failing to parse in an old reader. That is normally the most expensive part of adding an enum constant, and it is already ruled out.

Put that in the PR explicitly. It is the only place that will ever record the absence, and a future change that adds a valueOf reintroduces the whole class silently.

The thing I found while checking: ReplyOutcome already does this properly, 400 lines up

MessageService.java:166 declares a sibling enum that is everything Outcome is not:

RESOLVED_SEND("resolved_send", true,
        "delivered — resolved the fleet_send that was waiting for it"),
...
/** Stable machine-readable name for a JSON/metrics label (REST's {@code outcome} field). */
public String wireName() { return wireName; }

An explicit wireName, pinned in the constant, deliberately decoupled from .name(). The house pattern for a wire-facing enum in this exact file already exists, and Outcome does not follow it — which is precisely why :1371 leaks .name().toLowerCase() onto the wire.

Do NOT fix that in this PR. Giving Outcome a wireName touches 8 constants and 3 call sites and is its own change; folding it in would make a unit that already carries four corrections unreviewable. I have filed it separately.

What I want from you instead is one sentence in the PR: that TIMED_OUT_UNCONFIRMED introduces the wire token timed_out_unconfirmed implicitly, via .name(), rather than by a pinned wireName like its sibling — and that this is known and deferred, not overlooked.

Door table, current

# door instrument measured today
1 switches javac, defaults deleted 3 clean, 2 to fix
2 comparisons / ternaries constant-name grep 8 sites
3 reflection values() / valueOf 0 — and that retires the round-trip class
4 name() / ordinal() .name() grep 2 sites
5 wire form (external) none — announce it 3 surfaces
6 repo, outside Java roots git grep with pathspec excludes 0 readers (8 prose mentions)

Six doors, two of them now measured empty. Still do not treat six as final.

Nothing else changes

TIMED_OUT_UNCONFIRMED. Same three tests, same mutation discipline, same order rule.

## CORRECTION 4 — three measurements, and a sibling enum that already solves door 5. No new work for you except one sentence. Credit: the fleet01 lead. All three checks are theirs; the measurements are mine. ### Door 6 — readers inside the repo but outside the Java source roots. MEASURED: no executable reader. Every grep in the door table was scoped `src/main/java src/test/java`. **That scoping is itself a blind spot with a shape**, and the fleet01 lead was right to point at it — `scripts/` in this repo already parses daemon output, so a shell script matching an outcome token would have been house style, not a stretch. Measured repo-wide: - **Constant names outside the Java roots: 8 hits, all prose.** `docs/CB-201-227-Refinement.md`, `docs/CB-308-Multi-Host-Federation.md`, `docs/M4-Fleet-Health.md` ×2, `docs/wiki-audit.md` ×2, `fleetd/fleetd.example.yaml` ×2 (both in comments about the classification, not values). None of these parse or match — they are documentation. - **Wire tokens (`backend_exhausted`, `timed_out_`, `no reply —`) outside `*.java`: ZERO.** Positive control: `git grep` reaches non-Java files fine here (5 files match `healthz`). So no script, no schema, no non-Java client reads this enum. **Door 6 is empty of readers.** Door 5 shrinks to genuinely-external consumers, which the PR deliverable already covers. One knock-on, out of scope but worth one line in your reply: `docs/wiki-audit.md:140-141` already records that a documented outcome list is incomplete. Your new constant makes those doc lists staler. Note it; do not go fix them. ### Door 5's contents — CHECKED, and my table was right The fleet01 lead asked whether anything overrides the enum's string form, because if a `toString()`, `@JsonValue` or custom serializer existed, `.name()` and the wire form would be **two different tokens for one outcome** and my table would have listed one as if it were both. `MessageService.Outcome` is **8 bare constants with javadoc and nothing else** — no `toString()`, no Jackson annotation, no serializer. So `.name().toLowerCase()` *is* the wire form. The table stands. ### Door 3's zero retires a whole hazard class — say so in the PR `Outcome.values()` / `Outcome.valueOf` is **0**, re-measured, exit 1, with a control confirming the pattern works on other enums here (`MemberSession.State.values()` at `FleetMetrics.java:80`). The inference neither of us had drawn: **nothing parses a string back into this enum.** It is write-only to the wire. So the entire round-trip class does not exist here — no old persisted value failing to parse in a new binary, no new value failing to parse in an old reader. That is normally the most expensive part of adding an enum constant, and it is already ruled out. **Put that in the PR explicitly.** It is the only place that will ever record the absence, and a future change that adds a `valueOf` reintroduces the whole class silently. ### The thing I found while checking: `ReplyOutcome` already does this properly, 400 lines up `MessageService.java:166` declares a sibling enum that is everything `Outcome` is not: ```java RESOLVED_SEND("resolved_send", true, "delivered — resolved the fleet_send that was waiting for it"), ... /** Stable machine-readable name for a JSON/metrics label (REST's {@code outcome} field). */ public String wireName() { return wireName; } ``` An explicit `wireName`, pinned in the constant, deliberately decoupled from `.name()`. **The house pattern for a wire-facing enum in this exact file already exists, and `Outcome` does not follow it** — which is precisely why `:1371` leaks `.name().toLowerCase()` onto the wire. **Do NOT fix that in this PR.** Giving `Outcome` a `wireName` touches 8 constants and 3 call sites and is its own change; folding it in would make a unit that already carries four corrections unreviewable. I have filed it separately. What I want from you instead is **one sentence in the PR**: that `TIMED_OUT_UNCONFIRMED` introduces the wire token `timed_out_unconfirmed` implicitly, via `.name()`, rather than by a pinned `wireName` like its sibling — and that this is known and deferred, not overlooked. ### Door table, current | # | door | instrument | measured today | |---|---|---|---| | 1 | switches | `javac`, defaults deleted | 3 clean, 2 to fix | | 2 | comparisons / ternaries | constant-name grep | 8 sites | | 3 | reflection | `values()` / `valueOf` | **0 — and that retires the round-trip class** | | 4 | `name()` / `ordinal()` | `.name()` grep | 2 sites | | 5 | wire form (external) | none — announce it | 3 surfaces | | 6 | repo, outside Java roots | `git grep` with pathspec excludes | **0 readers** (8 prose mentions) | Six doors, two of them now measured empty. Still do not treat six as final. ### Nothing else changes `TIMED_OUT_UNCONFIRMED`. Same three tests, same mutation discipline, same order rule.
ltms commented 2026-09-12 15:14:55 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 5 — HOLD on PR #580. The new FleetMcp arm is unasserted, and I proved it is a gap this PR creates.

The work is good. All six doors were addressed, the exhaustiveness proof ran in the required order, and both new tests carry real mutation proofs. Nothing below is a criticism of the reported work — it is the mutation I run against the half the worker did not change.

The survivor

Branch worker/571-attempted-outcome-5739f7-2, extracted fresh. Pristine FleetMcp.java sha fcba00011880827c.

Baseline: Tests run: 1768, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. That matches the worker's reported number exactly.

Mutation — FleetMcp.java:811, the new arm, located fresh. Anchor grep -Fxc 1 → 0, marker present. Replaced the "delivery unconfirmed" text with the generic queued-timeout text.

Tests run: 1768, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Survived. Restored; sha matches fcba00011880827c.

So the MCP text that exists specifically to warn a caller against a blind retry can be replaced with the text that invites one, and nothing fails.

The control, which is what makes this a finding rather than a guess

A survivor has three causes. I ruled out the other two with a positive control — mutating the pre-existing sibling arm at :806 (TIMED_OUT_WORKING, TIMED_OUT_QUEUED, BUSY):

[ERROR] FleetMcpTest.sendTimesOutWithAWorkingNote:298
        got: [MUTCTL surface is untested]queued; retry or poll status]
        ==> expected: <true> but was: <false>
[INFO] BUILD FAILURE

Killed. So:

  • formatReply is reachable from tests — FleetMcpTest:436 also asserts its [question] arm.
  • The MCP timeout surface is covered — the old arm is pinned.
  • The build is not missing a profile — same default profile, same 1768 total, both runs.

The only remaining explanation is the real one: this PR adds a tenth arm to a nine-arm switch on a surface that is otherwise asserted, and does not assert the new one. That is a per-site gap created here, not inherited — which is why it is fair to hold the PR for it rather than file it separately.

Worth naming: this is the exact shape of #577, the sweep running in parallel with this ticket. One method, several arms carrying one invariant, coverage at some sites and not others. It is easy to miss precisely because the total is healthy — 323 assertions in FleetMcpTest, 1768 green.

A correction to my own method, for the record

My first pass grepped the test tree for no reply within and got zero, and I was about to read that as "the whole MCP timeout surface is untested". That zero was wrong. The covering test asserts on a fragment (queued; retry or poll status]), so my pattern could not see it. The control is what corrected me, not the grep. A zero match is a fact about the pattern until a positive probe says otherwise — and here the positive probe reversed the conclusion, from "pre-existing gap, file separately" to "gap created here, hold the PR".

What is required to merge

One test in FleetMcpTest, next to sendTimesOutWithAWorkingNote, which is the model to copy.

  1. Assert formatReply's output for TIMED_OUT_UNCONFIRMED — specifically that it carries the delivery-unconfirmed wording and does not tell the caller to retry the way the queued/working arm does. The distinction between those two messages is the entire point of this ticket; pin it.
  2. Prove it is red under the mutation above: replace the :811 arm's text with the :806 arm's text, show the named failure, restore, paste the matching shasum -a 256.
  3. Nothing else changes. Do not touch production code — the production behaviour is already correct.

Also fine as-is, no action needed

Your call on MessageService's .name().toLowerCase() fallback for the new outcome is accepted. You are right that it already renders the other three timeout outcomes the same raw way, and that TaskView.reason is not the REST or MCP surface. Stating it as a deliberate choice rather than letting it pass silently is exactly what I wanted. That whole area is #578's problem, and #578 has since grown a correction of its own — the wire vocabulary is not one vocabulary but four, so leaving it alone here was the right call for a second reason you could not have known.

Your flag that comments 17128/17129 produced no code change, and that you were the one who judged that, is also right, and it is the reason I looked hard at doors 4 and 5 rather than taking the verdicts on trust. That flag did its job.

## CORRECTION 5 — HOLD on PR #580. The new `FleetMcp` arm is unasserted, and I proved it is a gap this PR creates. The work is good. All six doors were addressed, the exhaustiveness proof ran in the required order, and both new tests carry real mutation proofs. Nothing below is a criticism of the reported work — it is the mutation I run against **the half the worker did not change**. ### The survivor Branch `worker/571-attempted-outcome-5739f7-2`, extracted fresh. Pristine `FleetMcp.java` sha `fcba00011880827c`. **Baseline:** `Tests run: 1768, Failures: 0, Errors: 0, Skipped: 0` — BUILD SUCCESS. That matches the worker's reported number exactly. **Mutation** — `FleetMcp.java:811`, the new arm, located fresh. Anchor `grep -Fxc` 1 → 0, marker present. Replaced the "delivery unconfirmed" text with the generic queued-timeout text. ``` Tests run: 1768, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` **Survived.** Restored; sha matches `fcba00011880827c`. So the MCP text that exists specifically to **warn a caller against a blind retry** can be replaced with the text that invites one, and nothing fails. ### The control, which is what makes this a finding rather than a guess A survivor has three causes. I ruled out the other two with a positive control — mutating the **pre-existing sibling arm** at `:806` (`TIMED_OUT_WORKING, TIMED_OUT_QUEUED, BUSY`): ``` [ERROR] FleetMcpTest.sendTimesOutWithAWorkingNote:298 got: [MUTCTL surface is untested]queued; retry or poll status] ==> expected: <true> but was: <false> [INFO] BUILD FAILURE ``` **Killed.** So: - `formatReply` **is** reachable from tests — `FleetMcpTest:436` also asserts its `[question]` arm. - The MCP timeout surface **is** covered — the old arm is pinned. - The build is **not** missing a profile — same default profile, same 1768 total, both runs. The only remaining explanation is the real one: **this PR adds a tenth arm to a nine-arm switch on a surface that is otherwise asserted, and does not assert the new one.** That is a per-site gap created here, not inherited — which is why it is fair to hold the PR for it rather than file it separately. Worth naming: this is the exact shape of #577, the sweep running in parallel with this ticket. One method, several arms carrying one invariant, coverage at some sites and not others. It is easy to miss precisely because the *total* is healthy — 323 assertions in `FleetMcpTest`, 1768 green. ### A correction to my own method, for the record My first pass grepped the test tree for `no reply within` and got **zero**, and I was about to read that as "the whole MCP timeout surface is untested". That zero was wrong. The covering test asserts on a *fragment* (`queued; retry or poll status]`), so my pattern could not see it. **The control is what corrected me, not the grep.** A zero match is a fact about the pattern until a positive probe says otherwise — and here the positive probe reversed the conclusion, from "pre-existing gap, file separately" to "gap created here, hold the PR". ### What is required to merge One test in `FleetMcpTest`, next to `sendTimesOutWithAWorkingNote`, which is the model to copy. 1. Assert `formatReply`'s output for `TIMED_OUT_UNCONFIRMED` — specifically that it carries the delivery-unconfirmed wording and does **not** tell the caller to retry the way the queued/working arm does. The distinction between those two messages is the entire point of this ticket; pin it. 2. **Prove it is red** under the mutation above: replace the `:811` arm's text with the `:806` arm's text, show the named failure, restore, paste the matching `shasum -a 256`. 3. Nothing else changes. Do not touch production code — the production behaviour is already correct. ### Also fine as-is, no action needed Your call on `MessageService`'s `.name().toLowerCase()` fallback for the new outcome is **accepted**. You are right that it already renders the other three timeout outcomes the same raw way, and that `TaskView.reason` is not the REST or MCP surface. Stating it as a deliberate choice rather than letting it pass silently is exactly what I wanted. That whole area is #578's problem, and #578 has since grown a correction of its own — the wire vocabulary is not one vocabulary but four, so leaving it alone here was the right call for a second reason you could not have known. Your flag that comments 17128/17129 produced no code change, and that you were the one who judged that, is also right, and it is the reason I looked hard at doors 4 and 5 rather than taking the verdicts on trust. That flag did its job.
ltms commented 2026-09-12 15:30:30 +02:00
Author
Owner
Copy Link
Copy Source

Merged. Verified independently, including the CORRECTION 5 gap.

Merged to main as part of 634d33b.

My own verification, not the worker's numbers:

  • Merged main + #571 + #581 + #562 in a scratch worktree. All three merge clean. Tests run: 1784, Failures: 0, Errors: 0, Skipped: 0, confirmed twice — the Maven Results: block and an independent sum over 132 surefire report files. The arithmetic checks: 1766 + 3 (#571) + 7 (#581) + 8 (#562) = 1784, so no test was lost in the merge.
  • The CORRECTION 5 gap is closed. I re-ran my own mutation on the merged tree — changing the TIMED_OUT_UNCONFIRMED arm's wording to the queued/working wording — and it is now killed by FleetMcpTest.sendTimesOutWithAnUnconfirmedNoteNotARetryInvitation:327. Exactly one test, no crowd.
  • Read the production diff myself. The three-way branch replacing the old two-way ternary is correct, and sendOutcomeLabel adds the new constant to the existing timeout group rather than giving it its own label — which preserves that surface's deliberate many-to-one grouping. That was the right call and it matches what #578 now requires.

A note on my own method, worth recording. My first attempt at the verification mutation used a line-anchored sed on what turned out to be a three-line arm. It replaced line 1 with a complete statement and orphaned lines 2–3, so the tree did not compile and the build failed with no test results at all. A compile failure proves nothing either way. I added a mvn -o compile gate before the test build and re-ran with an in-string substitution that cannot change structure. A line-anchored mutation assumes the statement is one line — check that first, and gate on compilation before reading any test result.

Follow-ups, both already filed:

  • #578 — the wire token. Two corrections deep now: the same enum emits four different tokens across four live surfaces, and one surface is a predicate rather than a value, so a single wireName() is the wrong shape. Still blocked until it is picked up.
  • #586 — the name().toLowerCase() idiom is not confined to this enum: 15 sites across 5 files, and no test pins any of the long-form tokens.

Closing. Good work — the doors were measured rather than assumed, and flagging your own judgment call on comments 17128/17129 is what sent me to check doors 4 and 5 by hand.

## Merged. Verified independently, including the CORRECTION 5 gap. Merged to `main` as part of `634d33b`. **My own verification, not the worker's numbers:** - Merged `main` + #571 + #581 + #562 in a scratch worktree. All three merge clean. **`Tests run: 1784, Failures: 0, Errors: 0, Skipped: 0`**, confirmed twice — the Maven `Results:` block and an independent sum over 132 surefire report files. The arithmetic checks: 1766 + 3 (#571) + 7 (#581) + 8 (#562) = 1784, so no test was lost in the merge. - **The CORRECTION 5 gap is closed.** I re-ran my own mutation on the merged tree — changing the `TIMED_OUT_UNCONFIRMED` arm's wording to the queued/working wording — and it is now killed by `FleetMcpTest.sendTimesOutWithAnUnconfirmedNoteNotARetryInvitation:327`. Exactly one test, no crowd. - Read the production diff myself. The three-way branch replacing the old two-way ternary is correct, and `sendOutcomeLabel` adds the new constant to the existing `timeout` group rather than giving it its own label — which preserves that surface's deliberate many-to-one grouping. That was the right call and it matches what #578 now requires. **A note on my own method, worth recording.** My first attempt at the verification mutation used a line-anchored `sed` on what turned out to be a **three-line** arm. It replaced line 1 with a complete statement and orphaned lines 2–3, so the tree did not compile and the build failed with no test results at all. A compile failure proves nothing either way. I added a `mvn -o compile` gate before the test build and re-ran with an in-string substitution that cannot change structure. **A line-anchored mutation assumes the statement is one line** — check that first, and gate on compilation before reading any test result. **Follow-ups, both already filed:** - **#578** — the wire token. Two corrections deep now: the same enum emits four different tokens across four live surfaces, and one surface is a predicate rather than a value, so a single `wireName()` is the wrong shape. Still blocked until it is picked up. - **#586** — the `name().toLowerCase()` idiom is not confined to this enum: 15 sites across 5 files, and no test pins any of the long-form tokens. Closing. Good work — the doors were measured rather than assumed, and flagging your own judgment call on comments 17128/17129 is what sent me to check doors 4 and 5 by hand.
ltms closed this issue 2026-09-12 15:31:13 +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#571
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?