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

MessageService.Outcome leaks .name() onto the wire; its sibling ReplyOutcome pins a wireName 400 lines up #578

New Issue
Open
opened 2026-09-12 14:26:10 +02:00 by ltms · 3 comments
ltms commented 2026-09-12 14:26:10 +02:00
Owner
Copy Link
Copy Source

Found while verifying #571. Not a live defect. A structure problem, and the fix pattern already exists in the same file.

The two enums

MessageService.java:166 — ReplyOutcome, done properly:

RESOLVED_SEND("resolved_send", true,
        "delivered — resolved the fleet_send that was waiting for it"),
RESOLVED_ASYNC_TICKET("resolved_async_ticket", true,
        "delivered — resolved a pending async ticket (visible to fleet_poll)"),
QUEUED("queued", false,
        "queued — no send or ticket was waiting; held in the inbox for a later drain");

/** Stable machine-readable name for a JSON/metrics label (REST's {@code outcome} field). */
public String wireName() { return wireName; }

The wire token is pinned in the constant and deliberately decoupled from .name(). The javadoc says why: it is the one place both fleet_reply and REST word this.

MessageService.java:66 — Outcome, not done properly: 8 bare constants with javadoc and nothing else. No wireName, no toString(), no Jackson annotation, no serializer. Measured today on main at 204da67.

The consequence

Outcome's wire form is whatever .name() happens to return, at two sites:

MessageService.java:1371   "no reply — " + r.outcome().name().toLowerCase()
FleetMcp.java:807          r.outcome().name().toLowerCase().replace("timed_out_", "")

:1371 is the designed path for timeout outcomes, not an oversight — its own comment says the timeout/busy outcomes carry no reason, so it falls back to the outcome name.

So renaming a constant silently changes the protocol, and adding one silently adds a token. Both are refactors that look local and are not. FleetMcp.java:807 is worse than a rename hazard: it derives the token by string surgery on the constant name (.replace("timed_out_", "")), so the wire form depends on the constant's spelling in two ways at once.

ReplyOutcome has none of these properties, in the same class, written by the same project.

Why this is not urgent

Measured today, all with positive controls:

  • Outcome.values() / Outcome.valueOf: 0. Nothing parses a string back into this enum. It is write-only to the wire, so the whole round-trip hazard class — an old persisted value failing to parse in a new binary, or the reverse — does not exist here. That is normally the expensive part of this problem and it is already absent.
  • No reader outside the Java source roots. git grep repo-wide for the constant names outside src/main/java and src/test/java returns 8 hits, all prose (4 files under docs/, 2 comments in fleetd.example.yaml). Wire tokens (backend_exhausted, timed_out_, no reply —) outside *.java: zero. Control: git grep reaches non-Java files fine here.

So today the only consumers are genuinely external — a script, a dashboard, or a Claude session parsing MCP output — and none of them is in this repo to break.

A future change that adds a valueOf reintroduces the entire round-trip class silently. That is the trigger to watch for, and it is the reason to record the absence rather than rely on it.

What is wanted

Give Outcome a wireName, following ReplyOutcome exactly. Then:

  1. Every wire token is pinned in the constant and visible in one place.
  2. Renaming a constant is a local refactor again.
  3. FleetMcp.java:807's .replace("timed_out_", "") string surgery goes away — it becomes a wireName() read.

Match the sibling's shape rather than inventing a second convention. If ReplyOutcome's shape is wrong, that is a different ticket about both of them.

Acceptance

  • A test that each constant's wireName() is the exact string a consumer sees today, so this change is provably not a protocol change. Name the three serialized surfaces (FleetApp.writeReply's "status", FleetMcp.java:807, MessageService.java:1371) and pin each.
  • A test that renaming a constant does not change its wireName(). That is the whole point; assert it directly.
  • Every switch over Outcome stays exhaustive and default-less (#571 establishes this). Prove it the same way: delete the defaults first, then add a temporary constant, then every site must error. Order matters — a default is what suppresses the compile error, so a proof built on compile errors cannot see one.
  • State in the PR that Outcome.values() / valueOf is still 0, with the control. If it is no longer 0, stop and say so — the round-trip class is back and this ticket needs redesigning.
  • Mutation proof per new test: locate every line fresh; anchor counted with grep -Fxc (not awk -v, which escape-processes the value and silently counts 0 on any line holding a tab); count down by exactly one; red with the test's own assertion message; restored byte-identical under shasum -a 256; green control.
  • mvn -o clean install from fleetd/ (no POM at the repo root). Never mvn -q — it hides the test count. Report the Maven line and an independent sum over target/surefire-reports/*.txt, and name the profile.

Do not start until #571 has merged

It adds a ninth constant and edits the same declaration.

Out of scope

  • Changing any wire token's value. This is a refactor that must be provably invisible to every consumer. Changing a string is a separate, louder decision.
  • ReplyOutcome itself. It is the model here, not the subject.

Related: #571 (adds the ninth constant, and is where the door enumeration was worked out), #512 (one symbol carrying two states).

Found while verifying #571. **Not a live defect. A structure problem**, and the fix pattern already exists in the same file. ## The two enums `MessageService.java:166` — `ReplyOutcome`, done properly: ```java RESOLVED_SEND("resolved_send", true, "delivered — resolved the fleet_send that was waiting for it"), RESOLVED_ASYNC_TICKET("resolved_async_ticket", true, "delivered — resolved a pending async ticket (visible to fleet_poll)"), QUEUED("queued", false, "queued — no send or ticket was waiting; held in the inbox for a later drain"); /** Stable machine-readable name for a JSON/metrics label (REST's {@code outcome} field). */ public String wireName() { return wireName; } ``` The wire token is **pinned in the constant** and deliberately decoupled from `.name()`. The javadoc says why: it is the one place both `fleet_reply` and REST word this. `MessageService.java:66` — `Outcome`, not done properly: 8 bare constants with javadoc and nothing else. No `wireName`, no `toString()`, no Jackson annotation, no serializer. Measured today on `main` at 204da67. ## The consequence `Outcome`'s wire form is whatever `.name()` happens to return, at two sites: ``` MessageService.java:1371 "no reply — " + r.outcome().name().toLowerCase() FleetMcp.java:807 r.outcome().name().toLowerCase().replace("timed_out_", "") ``` `:1371` is the designed path for timeout outcomes, not an oversight — its own comment says the timeout/busy outcomes carry no reason, so it falls back to the outcome name. So **renaming a constant silently changes the protocol, and adding one silently adds a token.** Both are refactors that look local and are not. `FleetMcp.java:807` is worse than a rename hazard: it derives the token by *string surgery* on the constant name (`.replace("timed_out_", "")`), so the wire form depends on the constant's spelling in two ways at once. `ReplyOutcome` has none of these properties, in the same class, written by the same project. ## Why this is not urgent Measured today, all with positive controls: - **`Outcome.values()` / `Outcome.valueOf`: 0.** Nothing parses a string back into this enum. It is **write-only to the wire**, so the whole round-trip hazard class — an old persisted value failing to parse in a new binary, or the reverse — does not exist here. That is normally the expensive part of this problem and it is already absent. - **No reader outside the Java source roots.** `git grep` repo-wide for the constant names outside `src/main/java` and `src/test/java` returns 8 hits, all prose (4 files under `docs/`, 2 comments in `fleetd.example.yaml`). Wire tokens (`backend_exhausted`, `timed_out_`, `no reply —`) outside `*.java`: **zero**. Control: `git grep` reaches non-Java files fine here. So today the only consumers are genuinely external — a script, a dashboard, or a Claude session parsing MCP output — and none of them is in this repo to break. **A future change that adds a `valueOf` reintroduces the entire round-trip class silently.** That is the trigger to watch for, and it is the reason to record the absence rather than rely on it. ## What is wanted Give `Outcome` a `wireName`, following `ReplyOutcome` exactly. Then: 1. Every wire token is pinned in the constant and visible in one place. 2. Renaming a constant is a local refactor again. 3. `FleetMcp.java:807`'s `.replace("timed_out_", "")` string surgery goes away — it becomes a `wireName()` read. Match the sibling's shape rather than inventing a second convention. If `ReplyOutcome`'s shape is wrong, that is a different ticket about both of them. ## Acceptance - A test that each constant's `wireName()` is the exact string a consumer sees today, so this change is provably **not** a protocol change. Name the three serialized surfaces (`FleetApp.writeReply`'s `"status"`, `FleetMcp.java:807`, `MessageService.java:1371`) and pin each. - A test that renaming a constant does **not** change its `wireName()`. That is the whole point; assert it directly. - Every switch over `Outcome` stays exhaustive and `default`-less (#571 establishes this). Prove it the same way: delete the defaults first, then add a temporary constant, then every site must error. **Order matters — a `default` is what suppresses the compile error, so a proof built on compile errors cannot see one.** - State in the PR that `Outcome.values()` / `valueOf` is still 0, with the control. If it is no longer 0, stop and say so — the round-trip class is back and this ticket needs redesigning. - Mutation proof per new test: locate every line fresh; anchor counted with `grep -Fxc` (**not** `awk -v`, which escape-processes the value and silently counts 0 on any line holding a tab); count down by exactly one; red with the test's own assertion message; restored byte-identical under `shasum -a 256`; green control. - `mvn -o clean install` from `fleetd/` (no POM at the repo root). Never `mvn -q` — it hides the test count. Report the Maven line **and** an independent sum over `target/surefire-reports/*.txt`, and name the profile. ## Do not start until #571 has merged It adds a ninth constant and edits the same declaration. ## Out of scope - Changing any wire token's **value**. This is a refactor that must be provably invisible to every consumer. Changing a string is a separate, louder decision. - `ReplyOutcome` itself. It is the model here, not the subject. Related: #571 (adds the ninth constant, and is where the door enumeration was worked out), #512 (one symbol carrying two states).
ltms commented 2026-09-12 15:03:26 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 1 — the acceptance as filed cannot be satisfied. Do not start this ticket from the description alone.

Credit: the fleet01 lead (opus) found this. They read my own door-5 table across instead of down and saw that it lists different tokens for the same constant. I then measured it here. Their count was three vocabularies; the measurement found four, plus a fifth surface that is not a per-constant mapping at all.

What the ticket got wrong

The acceptance said: "each wireName() pinned to the exact string emitted today". That assumes one string per constant. There is not one. Which today?

If you pin one accessor and route all the call sites through it, at least two surfaces change what they emit — and the PR still reads as having proved invisibility, because the pinning exercise was done and every constant does have a pinned string. That is a single-value-for-several-states defect inside the fix for a single-value-for-several-states defect, and it passes review because the acceptance sounds satisfied.

The measured table — Outcome constant × surface → exact token, today

Commands: sed -n '628,655p' .../rest/FleetApp.java, sed -n '800,812p' .../mcp/FleetMcp.java, sed -n '1360,1372p' .../msg/MessageService.java, sed -n '645,656p' .../msg/MessageService.java, all run 2026-09-12 on main at 204da67.

constant A REST status (FleetApp:641-650) B MCP text (FleetMcp:807) C ticket detail (MessageService:1371) D metrics label (MessageService:645) E replySource
REPLIED — (200, no status) — — replied reply
COMPLETED_UNREPLIED — (200, no status) — — completion_fallback transcript
QUESTION question — — (null — no metric) —
STALE_TURN stale_turn — — (null — no metric) —
TIMED_OUT_WORKING working working timed_out_working timeout —
TIMED_OUT_QUEUED queued queued timed_out_queued timeout —
BUSY busy busy busy timeout —
WORKER_FAILED failed — reason, else worker_failed failed —
BACKEND_EXHAUSTED backend_exhausted — reason, else backend_exhausted backend_exhausted —

Read the TIMED_OUT_WORKING row: working, working, timed_out_working, timeout — three distinct tokens for one constant, all live at the same time.

This is not a paper reading. A fleet_poll I ran today returned, from the running daemon:

[failed — no reply — timed_out_working]

That is surface C in production. Surface D for the same delegation is timeout.

Surface D was missing from the ticket entirely, and it is the worst one to break

sendOutcomeLabel (MessageService:645) is not a fourth text surface. It feeds count(FleetMetrics.SENDS, "outcome", label) at :639 — a metrics label. Rename a metrics label and no code fails, no test fails, and every dashboard and alert built on the old value silently stops matching. It is the surface with the least feedback when it breaks.

The structural point: wireName() is the wrong shape for two of the five surfaces

This goes past "under-specified". A per-constant accessor is a one-to-one tool. Two surfaces are deliberately not one-to-one:

  • D is many-to-one on purpose. TIMED_OUT_WORKING, TIMED_OUT_QUEUED and BUSY all collapse to timeout. That is a deliberate grouping for the metric, not an accident to be normalised away.
  • E is not a function of the constant at all. replySource is a predicate: r.outcome() == REPLIED ? "reply" : "transcript" (MessageService:1364, and the same shape at FleetApp:638). No per-constant accessor can express it.

So a single wireName() cannot carry D or E even in principle. Any design that tries will either flatten a deliberate grouping or invent a token nobody emits today.

The replacement acceptance — per surface, captured before, asserted after

  1. Write the table above into a test first, as it is today, and watch it pass before you change any production code. A characterisation test that is not green on unchanged main is not characterising anything. Name it so its job is obvious, e.g. OutcomeWireVocabularyTest.
  2. Assert per surface, not per invariant. Five surfaces means five sets of assertions. One combined test whose total is non-zero is exactly how a zero at one surface hides — see the per-site rule this repo has now hit four times (#561, #572, #575, and the #577 sweep's two findings).
  3. Then pick one of these two shapes. Either is acceptable; say in the PR which you picked and why:
    • one accessor per surface — wireName() for REST, plus a separate short/label form for FleetMcp:807, with the REST and MCP vocabularies pinned separately; or
    • keep :807's string surgery but derive it from wireName() instead of name(), so the spelling is pinned once and the derivation stays visible.
  4. Leave D and E alone unless you can state what they gain. They are correct today. Routing them through a per-constant accessor is the defect this comment exists to prevent.
  5. FleetMcp:807 is doubly exposed. r.outcome().name().toLowerCase().replace("timed_out_", "") depends on the prefix spelling and on the constant's name. Renaming a constant changes the emitted token with no compile error and no test failure. Pinning wireName() alone does not fix this — the surgery has to move onto the pinned string, or the token has to be spelled out per constant.
  6. The PR states, for every one of the five surfaces, that its emitted tokens are byte-identical before and after — with the test that proves it. "Invisible" is a per-surface claim now, not one claim.

Still true from the original ticket

  • The house pattern already exists 100 lines up in the same file: ReplyOutcome (MessageService:166) pins its wire token in the constant, with a javadoc saying why. Nobody has to be persuaded the pattern is right — only that it was applied unevenly. That is the argument to lead the PR with.
  • Still blocked on #571. Do not start until #571 merges.
  • values()/valueOf on Outcome measured 0 (control: healthz matched 5 files, so the search reaches non-Java files). Nothing parses a string back into this enum, so the round-trip hazard class — an old persisted token failing in a new binary — does not exist today. Put that zero and its control in the PR. It is the only record the class was ever absent, and a future valueOf reintroduces it silently.
## CORRECTION 1 — the acceptance as filed cannot be satisfied. Do not start this ticket from the description alone. Credit: the fleet01 lead (opus) found this. They read my own door-5 table across instead of down and saw that it lists **different tokens for the same constant**. I then measured it here. Their count was three vocabularies; the measurement found **four**, plus a fifth surface that is not a per-constant mapping at all. ### What the ticket got wrong The acceptance said: *"each `wireName()` pinned to the exact string emitted today"*. That assumes one string per constant. There is not one. **Which today?** If you pin one accessor and route all the call sites through it, at least two surfaces change what they emit — and the PR still reads as having proved invisibility, because the pinning exercise was done and every constant does have a pinned string. That is a single-value-for-several-states defect inside the fix for a single-value-for-several-states defect, and it passes review because the acceptance *sounds* satisfied. ### The measured table — `Outcome` constant × surface → exact token, today Commands: `sed -n '628,655p' .../rest/FleetApp.java`, `sed -n '800,812p' .../mcp/FleetMcp.java`, `sed -n '1360,1372p' .../msg/MessageService.java`, `sed -n '645,656p' .../msg/MessageService.java`, all run 2026-09-12 on `main` at `204da67`. | constant | **A** REST `status` (`FleetApp:641-650`) | **B** MCP text (`FleetMcp:807`) | **C** ticket `detail` (`MessageService:1371`) | **D** metrics label (`MessageService:645`) | **E** `replySource` | |---|---|---|---|---|---| | `REPLIED` | — (200, no `status`) | — | — | `replied` | `reply` | | `COMPLETED_UNREPLIED` | — (200, no `status`) | — | — | `completion_fallback` | `transcript` | | `QUESTION` | `question` | — | — | *(null — no metric)* | — | | `STALE_TURN` | `stale_turn` | — | — | *(null — no metric)* | — | | `TIMED_OUT_WORKING` | `working` | `working` | `timed_out_working` | `timeout` | — | | `TIMED_OUT_QUEUED` | `queued` | `queued` | `timed_out_queued` | `timeout` | — | | `BUSY` | `busy` | `busy` | `busy` | `timeout` | — | | `WORKER_FAILED` | `failed` | — | reason, else `worker_failed` | `failed` | — | | `BACKEND_EXHAUSTED` | `backend_exhausted` | — | reason, else `backend_exhausted` | `backend_exhausted` | — | Read the `TIMED_OUT_WORKING` row: **`working`, `working`, `timed_out_working`, `timeout`** — three distinct tokens for one constant, all live at the same time. **This is not a paper reading.** A `fleet_poll` I ran today returned, from the running daemon: ``` [failed — no reply — timed_out_working] ``` That is surface C in production. Surface D for the same delegation is `timeout`. ### Surface D was missing from the ticket entirely, and it is the worst one to break `sendOutcomeLabel` (`MessageService:645`) is not a fourth text surface. It feeds `count(FleetMetrics.SENDS, "outcome", label)` at `:639` — a **metrics label**. Rename a metrics label and no code fails, no test fails, and every dashboard and alert built on the old value silently stops matching. It is the surface with the least feedback when it breaks. ### The structural point: `wireName()` is the wrong *shape* for two of the five surfaces This goes past "under-specified". A per-constant accessor is a one-to-one tool. Two surfaces are deliberately not one-to-one: - **D is many-to-one on purpose.** `TIMED_OUT_WORKING`, `TIMED_OUT_QUEUED` and `BUSY` all collapse to `timeout`. That is a deliberate grouping for the metric, not an accident to be normalised away. - **E is not a function of the constant at all.** `replySource` is a *predicate*: `r.outcome() == REPLIED ? "reply" : "transcript"` (`MessageService:1364`, and the same shape at `FleetApp:638`). No per-constant accessor can express it. So a single `wireName()` cannot carry D or E even in principle. Any design that tries will either flatten a deliberate grouping or invent a token nobody emits today. ### The replacement acceptance — per surface, captured before, asserted after 1. **Write the table above into a test first, as it is today, and watch it pass before you change any production code.** A characterisation test that is not green on unchanged `main` is not characterising anything. Name it so its job is obvious, e.g. `OutcomeWireVocabularyTest`. 2. **Assert per surface, not per invariant.** Five surfaces means five sets of assertions. One combined test whose total is non-zero is exactly how a zero at one surface hides — see the per-site rule this repo has now hit four times (#561, #572, #575, and the #577 sweep's two findings). 3. **Then pick one of these two shapes.** Either is acceptable; say in the PR which you picked and why: - **one accessor per surface** — `wireName()` for REST, plus a separate short/label form for `FleetMcp:807`, with the REST and MCP vocabularies pinned separately; or - **keep `:807`'s string surgery but derive it from `wireName()`** instead of `name()`, so the spelling is pinned once and the derivation stays visible. 4. **Leave D and E alone unless you can state what they gain.** They are correct today. Routing them through a per-constant accessor is the defect this comment exists to prevent. 5. **`FleetMcp:807` is doubly exposed.** `r.outcome().name().toLowerCase().replace("timed_out_", "")` depends on the *prefix spelling* **and** on the *constant's name*. Renaming a constant changes the emitted token with no compile error and no test failure. Pinning `wireName()` alone does not fix this — the surgery has to move onto the pinned string, or the token has to be spelled out per constant. 6. **The PR states, for every one of the five surfaces, that its emitted tokens are byte-identical before and after** — with the test that proves it. "Invisible" is a per-surface claim now, not one claim. ### Still true from the original ticket - The house pattern already exists **100 lines up in the same file**: `ReplyOutcome` (`MessageService:166`) pins its wire token in the constant, with a javadoc saying why. Nobody has to be persuaded the pattern is right — only that it was applied unevenly. That is the argument to lead the PR with. - Still blocked on #571. Do not start until #571 merges. - `values()`/`valueOf` on `Outcome` measured **0** (control: `healthz` matched 5 files, so the search reaches non-Java files). Nothing parses a string back into this enum, so the round-trip hazard class — an old persisted token failing in a new binary — does not exist today. **Put that zero and its control in the PR.** It is the only record the class was ever absent, and a future `valueOf` reintroduces it silently.
ltms commented 2026-09-12 15:18:01 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 2 — the table must be GENERATED, not typed. And the vocabulary it pins is protected by nothing today.

Credit again to the fleet01 lead (opus), who ran these greps on their own tree and flagged this as needing to arrive before anyone writes the table by hand. I re-measured every number below on main at 204da67. Their tree is 91 commits behind, so their counts were a lower bound; mine are the live ones.

1. The leak shape is a house idiom, not an Outcome problem

grep -rn 'name()\s*\.\s*toLowerCase()' --include='*.java' over non-test sources on main:

15 hits across 5 files — and they cover at least six different enums:

file lines
mcp/FleetMcp.java 807, 1124, 1156, 1189, 1950, 1979
rest/FleetApp.java 772, 801, 835, 847
session/SessionManager.java 851, 880
msg/MessageService.java 1371, 1399
metrics/FleetMetrics.java 81

So "an enum's spelling must not be its wire token" has instances in five files, and this ticket had it scoped to one enum. Same escalation #577 went through: not a forgotten site, a house idiom that nothing asserts.

(One correction to the peer's figures, offered because it changes nothing in the argument: their count was "15 hits, 6 files"; the hits match exactly, the file count is 5. Their own sample list also names five.)

2. Their §1 question, answered: ONE site, not two

.replace("timed_out_", "") occurs exactly once on main, at FleetMcp.java:807. They saw it at :692 on their tree. 115 lines of drift over 91 commits is ordinary, so it is the same site moved, not a second one. The blast radius of the doubly-exposed case does not double.

3. The vocabulary this ticket pins is asserted by NOTHING

Across all 144 test .java files on main, searching for the quoted literal:

token test files
"timed_out_working" 0
"timed_out_queued" 0
"backend_exhausted" 0
"worker_failed" 0
"stale_turn" 0

Controls, so those zeros mean something. The peer reported honestly that their intended control ("rendezvous") also returned 0 and therefore proved nothing. Two controls do work: "working" matches 5 test files and "queued" matches 2, proving the search reaches test sources and matches quoted literals in them. I added a third of my own — "replied" matches 1 file. The zeros are real.

Read twice, as the rule says. "Absent from tests" could mean the strings are pinned some other way. Combined with point 1, the honest reading is the plain one: no test goes red if a refactor changes these strings.

This changes what the rewritten acceptance is. It is not extra safety on top of an existing net. It is the only net. If the "captured before, asserted after" step is skipped or done loosely, the PR proves nothing whatsoever about invisibility.

4. FleetMetrics.java:81 — a third arity, and it defeats a hand-written table

This site passes the arity check in CORRECTION 1: it is 1:1, like A/B/C. It is still the worst one, because the label set is built by iterating the enum:

// One gauge per state so a scrape shows the whole census even when a state is empty —
// an absent series and a zero series read very differently on a dashboard.
for (MemberSession.State state : MemberSession.State.values()) {
    String label = state.name().toLowerCase();
    m.gauge(SESSIONS, () -> countIn(sessions, state), "state", label);
}

There is no written list of tokens for a reviewer to find. Add a constant and a new gauge series appears; rename one and a series silently vanishes while a new one appears under a different name. The comment directly above reasons about exactly this harm and pins nothing.

So: the before/after table must be GENERATED from values(), not typed. A hand-written constant × surface table omits this site completely — and it is the site that most needs covering, because it is the only one whose tokens are produced rather than written.

Note this is MemberSession.State, a different enum from Outcome. Two consequences:

  • The scope of this ticket is wrong as filed, and point 1 above says how wrong.
  • My door-3 zero was per-enum and must not be read as tree-wide. values()/valueOf is 0 for Outcome. It is plainly not 0 for MemberSession.State — :81 is a values() call. When that zero goes in the PR, it must say which enum it was measured for.

5. The updated acceptance

Replacing point 1 of CORRECTION 1:

  1. Generate the constant × surface → token table by iterating values() for every enum in scope, and write the generated output into the test as the expected fixture. Do not type the table. A typed table cannot see an auto-enumerated site, which is the one site whose tokens drift without anyone editing a string.
  2. Everything else in CORRECTION 1 stands: green on unchanged main first, assert per surface, leave D and E alone, :807 is doubly exposed.
  3. Sort the work by exposure, not by tidiness. FleetMcp:807 is doubly exposed (constant name and literal prefix) and is the one to fix. Surface E is immune and needs nothing. Surface D is frozen — do not touch it.
  4. For any surface whose consumers cannot be enumerated from this tree, "pin it" is not available and the only correct acceptance is UNCHANGED, BYTE FOR BYTE. That covers the metrics labels, whose consumers are dashboards in another system, and it covers surface C, whose consumers include anything parsing fleet_poll output — I was reading one in my own tool output while writing CORRECTION 1.
  5. Scope decision, mine: this ticket stays on Outcome. The five-file idiom is filed separately rather than swallowed here — a unit that grows a fifth time stops being reviewable, which is the same reason #578 was kept out of #571.
## CORRECTION 2 — the table must be GENERATED, not typed. And the vocabulary it pins is protected by nothing today. Credit again to the fleet01 lead (opus), who ran these greps on their own tree and flagged this as needing to arrive before anyone writes the table by hand. I re-measured every number below on `main` at `204da67`. Their tree is 91 commits behind, so their counts were a lower bound; mine are the live ones. ### 1. The leak shape is a house idiom, not an `Outcome` problem `grep -rn 'name()\s*\.\s*toLowerCase()' --include='*.java'` over non-test sources on `main`: **15 hits across 5 files** — and they cover **at least six different enums**: | file | lines | |---|---| | `mcp/FleetMcp.java` | 807, 1124, 1156, 1189, 1950, 1979 | | `rest/FleetApp.java` | 772, 801, 835, 847 | | `session/SessionManager.java` | 851, 880 | | `msg/MessageService.java` | 1371, 1399 | | `metrics/FleetMetrics.java` | 81 | So "an enum's spelling must not be its wire token" has instances in five files, and this ticket had it scoped to one enum. Same escalation #577 went through: not a forgotten site, a **house idiom that nothing asserts**. *(One correction to the peer's figures, offered because it changes nothing in the argument: their count was "15 hits, 6 files"; the hits match exactly, the file count is 5. Their own sample list also names five.)* ### 2. Their §1 question, answered: ONE site, not two `.replace("timed_out_", "")` occurs **exactly once** on `main`, at `FleetMcp.java:807`. They saw it at `:692` on their tree. 115 lines of drift over 91 commits is ordinary, so it is **the same site moved**, not a second one. The blast radius of the doubly-exposed case does not double. ### 3. The vocabulary this ticket pins is asserted by NOTHING Across all **144** test `.java` files on `main`, searching for the quoted literal: | token | test files | |---|---| | `"timed_out_working"` | **0** | | `"timed_out_queued"` | **0** | | `"backend_exhausted"` | **0** | | `"worker_failed"` | **0** | | `"stale_turn"` | **0** | **Controls, so those zeros mean something.** The peer reported honestly that their intended control (`"rendezvous"`) also returned 0 and therefore proved nothing. Two controls do work: `"working"` matches **5** test files and `"queued"` matches **2**, proving the search reaches test sources and matches quoted literals in them. I added a third of my own — `"replied"` matches **1** file. The zeros are real. Read twice, as the rule says. "Absent from tests" could mean the strings are pinned some other way. Combined with point 1, the honest reading is the plain one: **no test goes red if a refactor changes these strings.** **This changes what the rewritten acceptance is.** It is not extra safety on top of an existing net. **It is the only net.** If the "captured before, asserted after" step is skipped or done loosely, the PR proves nothing whatsoever about invisibility. ### 4. `FleetMetrics.java:81` — a third arity, and it defeats a hand-written table This site passes the arity check in CORRECTION 1: it is 1:1, like A/B/C. It is still the worst one, because **the label set is built by iterating the enum**: ```java // One gauge per state so a scrape shows the whole census even when a state is empty — // an absent series and a zero series read very differently on a dashboard. for (MemberSession.State state : MemberSession.State.values()) { String label = state.name().toLowerCase(); m.gauge(SESSIONS, () -> countIn(sessions, state), "state", label); } ``` There is no written list of tokens for a reviewer to find. Add a constant and a new gauge series appears; rename one and a series silently vanishes while a new one appears under a different name. The comment directly above **reasons about exactly this harm** and pins nothing. **So: the before/after table must be GENERATED from `values()`, not typed.** A hand-written constant × surface table omits this site completely — and it is the site that most needs covering, because it is the only one whose tokens are produced rather than written. Note this is `MemberSession.State`, a **different enum** from `Outcome`. Two consequences: - The scope of this ticket is wrong as filed, and point 1 above says how wrong. - **My door-3 zero was per-enum and must not be read as tree-wide.** `values()`/`valueOf` is 0 for `Outcome`. It is plainly **not** 0 for `MemberSession.State` — `:81` is a `values()` call. When that zero goes in the PR, it must say *which enum* it was measured for. ### 5. The updated acceptance Replacing point 1 of CORRECTION 1: 1. **Generate the constant × surface → token table by iterating `values()`** for every enum in scope, and write the generated output into the test as the expected fixture. Do not type the table. A typed table cannot see an auto-enumerated site, which is the one site whose tokens drift without anyone editing a string. 2. Everything else in CORRECTION 1 stands: green on unchanged `main` first, assert per surface, leave D and E alone, `:807` is doubly exposed. 3. **Sort the work by exposure, not by tidiness.** `FleetMcp:807` is doubly exposed (constant name **and** literal prefix) and is the one to fix. Surface E is immune and needs nothing. Surface D is frozen — do not touch it. 4. **For any surface whose consumers cannot be enumerated from this tree, "pin it" is not available and the only correct acceptance is UNCHANGED, BYTE FOR BYTE.** That covers the metrics labels, whose consumers are dashboards in another system, and it covers surface C, whose consumers include anything parsing `fleet_poll` output — I was reading one in my own tool output while writing CORRECTION 1. 5. **Scope decision, mine:** this ticket stays on `Outcome`. The five-file idiom is filed separately rather than swallowed here — a unit that grows a fifth time stops being reviewable, which is the same reason #578 was kept out of #571.
ltms commented 2026-09-12 15:45:44 +02:00
Author
Owner
Copy Link
Copy Source

CORRECTION 3 — the acceptance must be a PROPERTY, not a SHAPE. Do not implement this ticket until this is read.

Credit: the fleet01 lead found this. I measured it again here on main 634d33b and it is worse than they reported.

The defect in my acceptance

My acceptance said: "each wireName() pinned to the exact string emitted today".

That criterion is trivially true of an implementation that does not fix anything. A derived
accessor — return name().toLowerCase(Locale.ROOT); — gives every constant a pinned string, so the
before/after table matches perfectly, the test passes, the review is honest, and the rename coupling
survives untouched. The ticket would close with the hazard exactly where it started.

New acceptance, and it replaces the old wording:

Renaming the constant must not change the emitted token.

That is falsifiable. A derived implementation fails it on the first rename. It cannot be satisfied by
adding a method. If we want it mechanical: the acceptance test renames a constant in a scratch build
and asserts the token is unchanged.

Why the old wording was easy to satisfy the wrong way — measured

There are three wireName() implementations in this tree, not one house pattern. Two are
explicit, one is derived, and the derived one has the most call sites:

implementation form call sites (main tree, excluding its own file)
mcp/FleetTool.java:49-56 explicit — constructor argument (SEND("fleet_send")) 12
msg/MessageService.java:185-196 (ReplyOutcome) explicit — constructor argument 1
peer/MemberRole.java:56-58 derived — name().toLowerCase(Locale.ROOT) 14 
cd fleetd/src/main/java
grep -rn 'String wireName' ../../../..//fleetd/src --include='*.java'     # the three declarations
grep -rn 'FleetTool\.[A-Z_]*\.wireName()' . --include='*.java' | wc -l    # 12
grep -rn 'outcome\.wireName()' . --include='*.java' | wc -l               # 1
# MemberRole sites: MemberRegistry 82/317/350/354 · FleetConfig 866/1273 · FleetMcp 2036
#                   ClaudeCodeLauncher 393 · HerdrPeerLauncher 364/547/566 · OpenCodeLauncher 368
#                   CharterReceipt 35 · SessionManager 850                    = 14

So "apply the house pattern, add wireName()" is an instruction with two readings, and an
implementer picking the most-cited exemplar picks the one that does not decouple anything.

The same file settles which discipline was intended, and it is not the derived one.
MemberRole.configKey(), one method below wireName(), is an explicit switch
(ARCHITECT -> "architects") with a javadoc saying the pool name is "not always the wire name".
Same class, same authors, both tools, chosen per case. The method name wireName records the word,
not the decision.

Independent of this ticket: MemberRole.wireName() is load-bearing past display

Renaming a MemberRole constant today changes an argv value passed to claude/opencode
(ClaudeCodeLauncher:393, OpenCodeLauncher:368), a charter filename on disk
(HerdrPeerLauncher:364), config keys (FleetConfig:866, :1273) and a registry key
(MemberRegistry:82). That is a filesystem lookup breaking silently. fleet01 measured that;
I have re-read all five sites here and they are as described. It argues for making
MemberRole.wireName() explicit whatever happens to Outcome.

The general rule I am recording from this

AN ACCEPTANCE CRITERION THAT NAMES A CONSTRUCT CAN BE SATISFIED BY THE CONSTRUCT.
"Add X and pin it" is passed by any X. Only a criterion written as behaviour under a change
("rename it and the token must not move") can fail. This is the same shape as a mutation test:
the useful criterion is the one you cannot pass by adding code.

This is the ninth time the defect in a unit was in my brief rather than in the work.

## CORRECTION 3 — the acceptance must be a PROPERTY, not a SHAPE. Do not implement this ticket until this is read. Credit: the fleet01 lead found this. I measured it again here on `main` `634d33b` and it is worse than they reported. ### The defect in my acceptance My acceptance said: *"each `wireName()` pinned to the exact string emitted today"*. That criterion **is trivially true of an implementation that does not fix anything**. A derived accessor — `return name().toLowerCase(Locale.ROOT);` — gives every constant a pinned string, so the before/after table matches perfectly, the test passes, the review is honest, and the rename coupling survives untouched. The ticket would close with the hazard exactly where it started. **New acceptance, and it replaces the old wording:** > **Renaming the constant must not change the emitted token.** That is falsifiable. A derived implementation fails it on the first rename. It cannot be satisfied by adding a method. If we want it mechanical: the acceptance test renames a constant in a scratch build and asserts the token is unchanged. ### Why the old wording was easy to satisfy the wrong way — measured There are **three** `wireName()` implementations in this tree, not one house pattern. Two are explicit, one is derived, and the derived one has the most call sites: | implementation | form | call sites (main tree, excluding its own file) | |---|---|---| | `mcp/FleetTool.java:49-56` | **explicit** — constructor argument (`SEND("fleet_send")`) | 12 | | `msg/MessageService.java:185-196` (`ReplyOutcome`) | **explicit** — constructor argument | 1 | | `peer/MemberRole.java:56-58` | **derived** — `name().toLowerCase(Locale.ROOT)` | 14 | ```bash cd fleetd/src/main/java grep -rn 'String wireName' ../../../..//fleetd/src --include='*.java' # the three declarations grep -rn 'FleetTool\.[A-Z_]*\.wireName()' . --include='*.java' | wc -l # 12 grep -rn 'outcome\.wireName()' . --include='*.java' | wc -l # 1 # MemberRole sites: MemberRegistry 82/317/350/354 · FleetConfig 866/1273 · FleetMcp 2036 # ClaudeCodeLauncher 393 · HerdrPeerLauncher 364/547/566 · OpenCodeLauncher 368 # CharterReceipt 35 · SessionManager 850 = 14 ``` So "apply the house pattern, add `wireName()`" is an instruction with **two readings**, and an implementer picking the most-cited exemplar picks the one that does not decouple anything. **The same file settles which discipline was intended, and it is not the derived one.** `MemberRole.configKey()`, one method below `wireName()`, is an explicit switch (`ARCHITECT -> "architects"`) with a javadoc saying the pool name is *"not always the wire name"*. Same class, same authors, both tools, chosen per case. The method name `wireName` records the word, not the decision. ### Independent of this ticket: `MemberRole.wireName()` is load-bearing past display Renaming a `MemberRole` constant today changes an argv value passed to `claude`/`opencode` (`ClaudeCodeLauncher:393`, `OpenCodeLauncher:368`), a charter **filename** on disk (`HerdrPeerLauncher:364`), config keys (`FleetConfig:866`, `:1273`) and a registry key (`MemberRegistry:82`). That is a filesystem lookup breaking silently. fleet01 measured that; I have re-read all five sites here and they are as described. It argues for making `MemberRole.wireName()` explicit **whatever happens to `Outcome`**. ### The general rule I am recording from this **AN ACCEPTANCE CRITERION THAT NAMES A CONSTRUCT CAN BE SATISFIED BY THE CONSTRUCT.** "Add X and pin it" is passed by any X. Only a criterion written as *behaviour under a change* ("rename it and the token must not move") can fail. This is the same shape as a mutation test: the useful criterion is the one you cannot pass by adding code. This is the ninth time the defect in a unit was in **my brief** rather than in the work.
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#578
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?