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

REST is the weaker door, and it is the door a lead falls back to when MCP drops #297

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

Two independent read-only hunts — one scoped to MCP-vs-REST parity, one scoped to placement and the two outage states — converged on the same conclusion from different directions. I verified every claim below myself.

Neither gap causes a wrong spawn. REST spawn goes through the same CompositePeerLauncher gate (FleetApp.java:454-458), so a refusal is still a refusal. These are visibility and contract gaps, not state defects, and I want that stated plainly rather than inflated.

What makes them worth fixing is when they bite. listMembers carries this comment in its own body:

the out-of-band path a lead falls back to when its MCP mount drops reads this endpoint

So REST is weakest exactly when it is load-bearing: during a degradation, when the richer door is already gone.

Gap 1 — two read-only endpoints throw outside the JSON error contract

FleetApp has a herdrError(Context, HerdrException) helper (:718) and four handlers that use it (:270, :286, :539, :684). healthz catches a herdr transport failure at :270 and turns it into a clean 503 — so this file already treats "herdr is briefly unreachable" as an expected, mappable condition.

These two do not:

private void agents(Context ctx) {
    ...
    ctx.status(200).json(Map.of("agents",
            workers.list().stream()...));      // bare — HerdrException escapes
}

private void listMembers(Context ctx) {
    ...
    Map<String, Agent> live = workers.list().stream()   // bare — HerdrException escapes

And there is no global handler to catch them: grep -rn "\.exception(" fleetd/src/main/java returns nothing. HerdrException is a RuntimeException, so Javalin's default handling takes over and the response leaves the {error, detail} envelope every other failure path in the file guarantees (400/401/403/404/409/502/503 all use it).

Direction of harm: a client that parses {error, detail} uniformly breaks, at the moment it is asking why the fleet looks unreachable. The MCP twin returns clean typed error text for the identical failure.

Reachable path: herdr briefly unreachable — the exact condition healthz exists to report.

Gap 2 — GET /profiles reports neither outage state

ctx.status(200).json(Map.of(
        "profiles", workers.profiles(),
        "default", workers.defaultProfile() == null ? "" : workers.defaultProfile()));

Its MCP sibling FleetMcp.profiles (:1017-1050) reports quarantined and coolingOff, each with the credential id and the remaining seconds. GET /members likewise carries no capacity block at all, where fleet_list's capacityView (:1229-1268) does.

Direction of harm: a lead on the REST fallback during an outage window cannot tell "busy, frees in 40s" from "broken". It burns turns re-probing or escalates something that was about to clear on its own. It is never told a wrong thing — it is told nothing.

What is already correct — recorded so nobody re-litigates it

The placement hunt looked hard for the classic "one outage state checked, the sibling not" shape and did not find it. I spot-checked its reasoning and it holds:

  • PlacementPolicyUtil.available() and emptyException() both consult quarantine and cooling-off together, with mutually-exclusive bucketing.
  • FixedPlacementPolicy.select() checks both states in all three places it needs to: the default check, the fallback loop, and the error-message builder.
  • CompositePeerLauncher.spawn() enforces both on the explicit-profile branch (:332-334) and derives both sets once into one PlacementContext on the placement branch (:349-351).
  • Fleetd.java:212-225 and :639-648 wire one shared BackendQuarantine and one shared BackendOutagePolicy into both the enforcement path and the reporting path — so the "two copies that drift" shape is ruled out at the instance level, which is the right level.
  • PlacementPolicyTest, BackendOutagePolicyTest, CompositePeerLauncherTest and FleetMcpTest cover both states alone, combined, and their precedence (quarantineWinsOverCoolingOffWhenBothAreActiveOnTheSameCredential, quarantinedAndCoolingOffIsNotDoubleCounted).

That is a genuinely well-covered area. The defect is not in the states; it is that one front door never reports them.

Scope

  1. Map HerdrException for GET /agents and GET /members, using the existing herdrError helper. Do not invent a new error shape.
  2. Add the two outage states to GET /profiles, reading from the same shared instances FleetMcp reads. Do not compute them a second time — the whole reason this area is currently sound is that there is one instance of each, and a second copy is how it stops being sound. If the wiring is not reachable from FleetApp today, say so in your report and describe what you would have to pass in; do not force it with a static or a new singleton.
  3. GET /members capacity block: do not add it. Report what it would take, in one paragraph, and stop. I want to decide the response shape myself before it becomes a compatibility surface — listMembers already carries a deprecated workers alias because a key was renamed once, and I am not repeating that.

Rules

  • Prove each fix with a test that fails without it. FleetAppTest currently has no test that makes workers.list() fail and then hits GET /members or GET /agents — a grep for quarantine/coolingOff across FleetAppTest, FleetAppTwoDaemonTest and FleetAppAuthTest returns nothing. You are adding the first coverage here, so state plainly what you added.
  • The two REST error responses must match the envelope the rest of the file uses. Read herdrError before writing anything.
  • Do not change FleetMcp. It is the correct side.
  • Do not change placement, quarantine or the outage policy. They are correct; see above.

Shape check

When done, look for the same shape — a rule enforced on the MCP door and absent on the REST door — in FleetApp.java only. Report each in one line and do not fix any of it. Two known ones you can skip, both already ruled out as non-defects: sendMessage's missing profileTargetError check (it fails safely downstream, so the only cost is a vaguer message), and PrimaryRegistry not being wired into FleetApp (REST callers have no herdr-pane identity, so the mechanism does not apply — it is not a skipped rule).

Two independent read-only hunts — one scoped to MCP-vs-REST parity, one scoped to placement and the two outage states — converged on the same conclusion from different directions. **I verified every claim below myself.** Neither gap causes a wrong spawn. REST spawn goes through the same `CompositePeerLauncher` gate (`FleetApp.java:454-458`), so a refusal is still a refusal. These are visibility and contract gaps, not state defects, and I want that stated plainly rather than inflated. What makes them worth fixing is *when* they bite. `listMembers` carries this comment in its own body: > the out-of-band path a lead falls back to when its MCP mount drops reads this endpoint So REST is weakest exactly when it is load-bearing: during a degradation, when the richer door is already gone. ## Gap 1 — two read-only endpoints throw outside the JSON error contract `FleetApp` has a `herdrError(Context, HerdrException)` helper (`:718`) and four handlers that use it (`:270`, `:286`, `:539`, `:684`). `healthz` catches a herdr transport failure at `:270` and turns it into a clean `503` — so this file already treats "herdr is briefly unreachable" as an expected, mappable condition. These two do not: ```java private void agents(Context ctx) { ... ctx.status(200).json(Map.of("agents", workers.list().stream()...)); // bare — HerdrException escapes } private void listMembers(Context ctx) { ... Map<String, Agent> live = workers.list().stream() // bare — HerdrException escapes ``` And there is **no global handler to catch them**: `grep -rn "\.exception(" fleetd/src/main/java` returns nothing. `HerdrException` is a `RuntimeException`, so Javalin's default handling takes over and the response leaves the `{error, detail}` envelope every other failure path in the file guarantees (400/401/403/404/409/502/503 all use it). **Direction of harm:** a client that parses `{error, detail}` uniformly breaks, at the moment it is asking why the fleet looks unreachable. The MCP twin returns clean typed error text for the identical failure. **Reachable path:** herdr briefly unreachable — the exact condition `healthz` exists to report. ## Gap 2 — `GET /profiles` reports neither outage state ```java ctx.status(200).json(Map.of( "profiles", workers.profiles(), "default", workers.defaultProfile() == null ? "" : workers.defaultProfile())); ``` Its MCP sibling `FleetMcp.profiles` (`:1017-1050`) reports `quarantined` and `coolingOff`, each with the credential id and the remaining seconds. `GET /members` likewise carries no capacity block at all, where `fleet_list`'s `capacityView` (`:1229-1268`) does. **Direction of harm:** a lead on the REST fallback during an outage window cannot tell "busy, frees in 40s" from "broken". It burns turns re-probing or escalates something that was about to clear on its own. It is never told a wrong thing — it is told nothing. ## What is already correct — recorded so nobody re-litigates it The placement hunt looked hard for the classic "one outage state checked, the sibling not" shape and **did not find it**. I spot-checked its reasoning and it holds: - `PlacementPolicyUtil.available()` and `emptyException()` both consult quarantine *and* cooling-off together, with mutually-exclusive bucketing. - `FixedPlacementPolicy.select()` checks both states in all three places it needs to: the default check, the fallback loop, and the error-message builder. - `CompositePeerLauncher.spawn()` enforces both on the explicit-profile branch (`:332-334`) and derives both sets once into one `PlacementContext` on the placement branch (`:349-351`). - `Fleetd.java:212-225` and `:639-648` wire **one** shared `BackendQuarantine` and **one** shared `BackendOutagePolicy` into both the enforcement path and the reporting path — so the "two copies that drift" shape is ruled out at the instance level, which is the right level. - `PlacementPolicyTest`, `BackendOutagePolicyTest`, `CompositePeerLauncherTest` and `FleetMcpTest` cover both states alone, combined, and their precedence (`quarantineWinsOverCoolingOffWhenBothAreActiveOnTheSameCredential`, `quarantinedAndCoolingOffIsNotDoubleCounted`). That is a genuinely well-covered area. The defect is not in the states; it is that one front door never reports them. ## Scope 1. Map `HerdrException` for `GET /agents` and `GET /members`, using the existing `herdrError` helper. Do **not** invent a new error shape. 2. Add the two outage states to `GET /profiles`, reading from the same shared instances `FleetMcp` reads. **Do not compute them a second time** — the whole reason this area is currently sound is that there is one instance of each, and a second copy is how it stops being sound. If the wiring is not reachable from `FleetApp` today, say so in your report and describe what you would have to pass in; do not force it with a static or a new singleton. 3. `GET /members` capacity block: **do not add it.** Report what it would take, in one paragraph, and stop. I want to decide the response shape myself before it becomes a compatibility surface — `listMembers` already carries a deprecated `workers` alias because a key was renamed once, and I am not repeating that. ## Rules - Prove each fix with a test that fails without it. `FleetAppTest` currently has **no** test that makes `workers.list()` fail and then hits `GET /members` or `GET /agents` — a grep for quarantine/coolingOff across `FleetAppTest`, `FleetAppTwoDaemonTest` and `FleetAppAuthTest` returns nothing. You are adding the first coverage here, so state plainly what you added. - The two REST error responses must match the envelope the rest of the file uses. Read `herdrError` before writing anything. - Do not change `FleetMcp`. It is the correct side. - Do not change placement, quarantine or the outage policy. They are correct; see above. ## Shape check When done, look for the same shape — **a rule enforced on the MCP door and absent on the REST door** — in `FleetApp.java` only. Report each in one line and **do not fix any of it**. Two known ones you can skip, both already ruled out as non-defects: `sendMessage`'s missing `profileTargetError` check (it fails safely downstream, so the only cost is a vaguer message), and `PrimaryRegistry` not being wired into `FleetApp` (REST callers have no herdr-pane identity, so the mechanism does not apply — it is not a skipped rule).
ltms commented 2026-09-04 07:19:32 +02:00
Author
Owner
Copy Link
Copy Source

Merged as f34361b, corrected in 9debc0d. Real merge built green at 1307 tests.

The wiring is right, and it is the part I checked hardest. Fleetd now names quarantineSource and outageSource once and hands the same instances to both FleetMcp and FleetApp. The one production new FleetApp(...) call site uses the new constructor, so nothing ships inert — worth stating explicitly, because a new dependency that silently defaults to a no-op is how a feature lands dead with a green build.

The worker also respected the three-item split: it did not build the GET /members capacity block, and said so rather than leaving me to notice.

My correction — and the ticket is what caused the problem

The merged GET /profiles was a character-for-character copy of FleetMcp.profiles's loop, in a different file.

That is the #284 shape one level up. Sharing the QuarantineSource/OutageSource instances stops the two doors reading different facts. It does nothing to stop them reporting those facts differently: rename a row key, add a field, and the edit lands on one door and not the other. The two then disagree about a live outage — which is the exact failure #284 was.

This was my instruction, not a worker mistake. The ticket said "read from the same shared instances" and "do not change FleetMcp". Together those made copying the loop the only legal move. The worker followed both rules exactly and the result was a second copy. I should have asked for the extraction.

Fixed by extracting FleetMcp.profilesView and calling it from both doors. Its javadoc records why shared inputs are necessary but not sufficient.

My mutation — testing the coupling, not the fix

The worker proved the fix works by reverting each of the three changes and quoting the failures. That is the right proof for the feature. It is not a proof of the property my correction adds, so I mutated for that instead: I renamed a row key in the one shared builder and checked that both doors fail.

FleetMcpTest.profilesReportsAQuarantinedCredential:503
  {"profiles":["ltms-local"],...,"quarantined":{"ltms-local":
   {"credentialId":"shared-openai","MUTANT_renamed_key":1800}}}
  ==> expected: <true> but was: <false>

FleetAppTest.profilesReportsQuarantineAndCoolingOffFromTheSameSharedSources:272
  NullPointer Cannot invoke "JsonNode.asLong()" because the return value of
  "JsonNode.get(String)" is null

One edit, both doors. Before the correction that same edit would have left the REST test passing and the two doors silently disagreeing.

One thing I will not overclaim

That mutation also caught capacityView (:1275, :1283), which emits the same two key names for fleet_list. I looked at whether to fold that in as well and decided not to: it is a different response shape — per-profile capacity rows, not the profiles map — so it is shared vocabulary rather than a duplicated rule, and its javadoc at :1222-1229 already cross-references fleet_profiles and spells out where the two deliberately differ.

The material change is that every site emitting these keys now lives in one file, adjacent. The REST copy was the dangerous one precisely because an editor of FleetMcp would never have seen it.

Shape check — three more, filed as follow-up, not fixed

The worker found three further "MCP enforces it, REST skips it" instances and correctly left them alone. I have not verified these myself yet; recording them as reported:

  • POST /members (spawnMember) — no catch (HerdrException) around sessions.acquire, where FleetMcp.spawn catches and returns a clean error.
  • DELETE /members/{paneId} (stopMember) — no try/catch at all around sessions.release, where FleetMcp.stop catches.
  • POST /sessions/{id}/reply (replyMessage) — reads content with .asText("") and no required-check, so a missing content field silently becomes an empty-string reply, where FleetMcp.reply rejects null content with "content is required".

The third is the interesting one and is not the same severity as the other two. The first two produce a bare 500 — ugly, visible, and already accepted elsewhere in this file with passing tests. The third writes a wrong value silently: an empty reply is indistinguishable from a member that genuinely answered with nothing. I will file that separately.

Merged as `f34361b`, corrected in `9debc0d`. Real merge built green at **1307 tests**. The wiring is right, and it is the part I checked hardest. `Fleetd` now names `quarantineSource` and `outageSource` once and hands **the same instances** to both `FleetMcp` and `FleetApp`. The one production `new FleetApp(...)` call site uses the new constructor, so nothing ships inert — worth stating explicitly, because a new dependency that silently defaults to a no-op is how a feature lands dead with a green build. The worker also respected the three-item split: it did not build the `GET /members` capacity block, and said so rather than leaving me to notice. ## My correction — and the ticket is what caused the problem The merged `GET /profiles` was a **character-for-character copy** of `FleetMcp.profiles`'s loop, in a different file. That is the #284 shape one level up. Sharing the `QuarantineSource`/`OutageSource` instances stops the two doors reading different facts. It does nothing to stop them *reporting* those facts differently: rename a row key, add a field, and the edit lands on one door and not the other. The two then disagree about a live outage — which is the exact failure #284 was. **This was my instruction, not a worker mistake.** The ticket said "read from the same shared instances" and "do not change `FleetMcp`". Together those made copying the loop the only legal move. The worker followed both rules exactly and the result was a second copy. I should have asked for the extraction. Fixed by extracting `FleetMcp.profilesView` and calling it from both doors. Its javadoc records why shared inputs are necessary but not sufficient. ## My mutation — testing the coupling, not the fix The worker proved the fix works by reverting each of the three changes and quoting the failures. That is the right proof for the feature. It is not a proof of the property my correction adds, so I mutated for that instead: I renamed a row key in the **one** shared builder and checked that **both** doors fail. ``` FleetMcpTest.profilesReportsAQuarantinedCredential:503 {"profiles":["ltms-local"],...,"quarantined":{"ltms-local": {"credentialId":"shared-openai","MUTANT_renamed_key":1800}}} ==> expected: <true> but was: <false> FleetAppTest.profilesReportsQuarantineAndCoolingOffFromTheSameSharedSources:272 NullPointer Cannot invoke "JsonNode.asLong()" because the return value of "JsonNode.get(String)" is null ``` One edit, both doors. Before the correction that same edit would have left the REST test passing and the two doors silently disagreeing. ## One thing I will not overclaim That mutation also caught `capacityView` (`:1275`, `:1283`), which emits the same two key names for `fleet_list`. I looked at whether to fold that in as well and decided not to: it is a different response shape — per-profile capacity rows, not the profiles map — so it is shared vocabulary rather than a duplicated rule, and its javadoc at `:1222-1229` already cross-references `fleet_profiles` and spells out where the two deliberately differ. The material change is that **every site emitting these keys now lives in one file, adjacent**. The REST copy was the dangerous one precisely because an editor of `FleetMcp` would never have seen it. ## Shape check — three more, filed as follow-up, not fixed The worker found three further "MCP enforces it, REST skips it" instances and correctly left them alone. I have not verified these myself yet; recording them as reported: - `POST /members` (`spawnMember`) — no `catch (HerdrException)` around `sessions.acquire`, where `FleetMcp.spawn` catches and returns a clean error. - `DELETE /members/{paneId}` (`stopMember`) — no try/catch at all around `sessions.release`, where `FleetMcp.stop` catches. - `POST /sessions/{id}/reply` (`replyMessage`) — reads content with `.asText("")` and no required-check, so a **missing `content` field silently becomes an empty-string reply**, where `FleetMcp.reply` rejects null content with "content is required". The third is the interesting one and is not the same severity as the other two. The first two produce a bare 500 — ugly, visible, and already accepted elsewhere in this file with passing tests. The third writes a wrong value silently: an empty reply is indistinguishable from a member that genuinely answered with nothing. I will file that separately.
ltms closed this issue 2026-09-04 07:20:14 +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#297
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?