1fcda74596
main already ships fleetd#164 points 1 and 2 (MIN_TURN_NANOS floor + hard-fail on an empty/unreadable scrape, commit3bfa828, already an ancestor of main). The rescued worker branch worker/cb-164-empty-scrape-false-success-1a80af-3 (851ebca) reimplemented the same two behaviours via a separate, more invasive mechanism (elapsedNanos threaded through TurnListener/Injector/Fleetd) that would conflict with main's already-shipped clock-injection design if merged wholesale. Port forward only what main is genuinely missing: the narrow BACKEND_ERROR pattern (`(?i)\bAPI Error\s*:`) that classifies a cleanly-scraped-but-backend-rejected turn as a failure naming the member, instead of letting the error text resolve as a completed reply. See REPORT-cb164.md for the full reasoning, what was deliberately not ported, and the sabotage proof for the new tests.
149 lines
8.4 KiB
Markdown
149 lines
8.4 KiB
Markdown
# CB-164 report — worker/cb-164-rebase-885863-8
|
|
|
|
## Headline finding — please read this before the diff
|
|
|
|
**`main` already ships a full fix for issue #164 points 1 and 2, done separately from the
|
|
rescued branch.** Commit `3bfa828` ("fleetd#164: an empty or suspiciously fast scrape must
|
|
fail, never resolve as a success") is already an ancestor of `origin/main` (verified with
|
|
`git merge-base --is-ancestor 3bfa828 origin/main`). It was written the same day as the
|
|
rescued commit (2026-08-28), by the same author, but on `main` directly. No later commit
|
|
touches `CompletionResolver.java` after it.
|
|
|
|
What `main` already has, before any change of mine:
|
|
|
|
- `CompletionResolver.MIN_TURN_NANOS` = 2 seconds — the exact "too fast to be a real
|
|
completion" floor the brief described, just under a different name and a different
|
|
mechanism (an injectable `LongSupplier nowNanos` clock read at delivery and again at
|
|
resolution, stored on the `InFlight` record — no interface or caller changes needed).
|
|
- A hard fail on any empty or unreadable scrape, via the existing `fail()` →
|
|
`Rendezvous.resolveFailure()` → `MessageService.Outcome.WORKER_FAILED` path — the same
|
|
pattern already used for the CB-109 wedge case. `FleetMcp.formatReply` already renders
|
|
`WORKER_FAILED` as `"[worker failed — turn ended in an unrecoverable state]\n" + text`,
|
|
never as blank content. I read this chain end to end and confirmed it (not just from the
|
|
commit message).
|
|
|
|
So **"Part 2" as described in the brief — the one called out as the most important — was
|
|
already done.** What I did **not** find on `main`: the narrow `BACKEND_ERROR` pattern
|
|
(`(?i)\bAPI Error\s*:`) from the rescued branch. That is genuinely new.
|
|
|
|
### Why I did not cherry-pick 851ebca, and what I did instead
|
|
|
|
`851ebca` implements the same floor-check idea a second time, but via a structurally
|
|
different, more invasive mechanism: it threads a real measured `elapsedNanos` through
|
|
`TurnListener.onTurnComplete`, `Injector` (a new `turnStartedAtNanos` field, a new
|
|
constructor, a new `LongSupplier` clock), and `Fleetd`'s anonymous `TurnListener`. Cherry-
|
|
picking that wholesale onto current `main` would have created **two competing timing
|
|
mechanisms for the same floor check** in the same class — main's already-shipped
|
|
delivery-clock read, and a second, parameter-threaded one from the rescued branch — with
|
|
no clear answer for which one should win if they ever disagreed. Reintroducing the
|
|
Injector/TurnListener/Fleetd signature changes for that also seemed hard to justify: main's
|
|
already-tested approach measures essentially the same interval (delivery to resolution)
|
|
with no interface changes at all.
|
|
|
|
Given that, I read the instruction *"if you cannot tell which behaviour a hunk is meant to
|
|
have, keep both behaviours and say so"* as pointing at genuinely uncertain hunks — not at
|
|
knowingly wiring in two redundant implementations of the identical check. So instead of a
|
|
mechanical cherry-pick, I hand-ported only the parts of `851ebca` that are **not** already
|
|
on `main`:
|
|
|
|
1. Added `CompletionResolver.BACKEND_ERROR` — the exact narrow pattern from the brief,
|
|
`(?i)\bAPI Error\s*:`, with the same "keep it narrow" comment.
|
|
2. Added a classification check for it, placed right after the existing `BACKEND_EXHAUSTED`
|
|
pattern-match block (same position, same shape: `firstMatchingLine` against
|
|
`assistantBlock`, then `fail(target, turn, reason)` naming the member and the matched
|
|
line) and before the success path (`fleetd/src/main/java/dev/ltms/fleet/inject/
|
|
CompletionResolver.java`).
|
|
|
|
I did **not** port:
|
|
- `MIN_COMPLETED_TURN_NANOS` / the `elapsedNanos` parameter threading through
|
|
`TurnListener`/`Injector`/`Fleetd` — redundant with `main`'s already-shipped
|
|
`MIN_TURN_NANOS`/`LongSupplier` mechanism, and touching those three extra files for no
|
|
behavioural gain seemed like avoidable risk.
|
|
- The rescued branch's `visibleTurn = assistantBlock.isBlank() ? raw : assistantBlock`
|
|
fallback (falls back to the raw pane when the parsed assistant block is blank, so a
|
|
TUI-hidden error/exhausted line still classifies). This is a real, separate improvement,
|
|
but on `main`'s current check order the empty-tail fail already fires *before* the
|
|
exhausted/backend-error pattern checks run, so the fallback would be dead code unless I
|
|
also reordered those checks ahead of the empty-tail fail. That reorder is a bigger,
|
|
separate behavioural change than either "Part 1" or "Part 2" asked for, so I left it out
|
|
and am flagging it here rather than making that call silently. **Out-of-scope note for
|
|
the lead**, not fixed: a raw-screen-only exhausted/backend-error line (no `⏺` marker, so
|
|
`lastAssistantBlock` returns blank) is still swallowed into the generic "empty scrape"
|
|
failure rather than being classified specifically.
|
|
|
|
I did not use `fleet_ask` for this: the brief's own instruction for exactly this kind of
|
|
conflict ("keep both, say so") already pre-authorized me to decide and document rather than
|
|
block, and a live `fleet_ask` has only a ~55s window per prior fleet notes, so I judged
|
|
documenting clearly here was more reliable than gambling on that window. If this call is
|
|
wrong, it's easy to undo — the diff is 3 files, +85/-0 lines.
|
|
|
|
## Files changed (worktree-relative)
|
|
|
|
- `fleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java` — added the
|
|
`BACKEND_ERROR` pattern constant and the classification check (+17 lines).
|
|
- `fleetd/src/test/java/dev/ltms/fleet/inject/CompletionResolverTest.java` — 3 new unit
|
|
tests for the `BACKEND_ERROR` classification (+46 lines).
|
|
- `fleetd/src/test/java/dev/ltms/fleet/msg/MessageServiceTest.java` — 1 new end-to-end test
|
|
driving the same scenario through `MessageService` (+22 lines).
|
|
|
|
No changes to `Fleetd.java`, `Injector.java`, or `TurnListener.java` — see above for why.
|
|
|
|
## Build — verbatim result
|
|
|
|
Full unpiped `mvn clean install` from `fleetd/`, exit code `0`:
|
|
|
|
```
|
|
[INFO] Tests run: 1036, Failures: 0, Errors: 0, Skipped: 0
|
|
[INFO] BUILD SUCCESS
|
|
```
|
|
|
|
No `[ERROR]` lines anywhere in the full log (checked with `grep -n "\[ERROR\]"` over the
|
|
whole log, not a piped/truncated view).
|
|
|
|
## Sabotage proof for the new tests
|
|
|
|
Commented out the new check in `CompletionResolver.java`:
|
|
|
|
```java
|
|
// String backendError = firstMatchingLine(assistantBlock, BACKEND_ERROR);
|
|
// if (backendError != null) {
|
|
// fail(target, turn, "member " + target + " ended on a backend error: " + backendError);
|
|
// return;
|
|
// }
|
|
```
|
|
|
|
Ran just the 4 new tests:
|
|
|
|
```
|
|
mvn -q test -Dtest='CompletionResolverTest#classifiesABackendErrorLineAsAFailureInsteadOfACompletedReply+theBackendErrorReasonNamesTheMemberAndCarriesTheMatchedLine+aCaseInsensitiveApiErrorLineIsStillClassifiedAsABackendError,MessageServiceTest#backendErrorScrapeThroughMessageServiceFailsInsteadOfBecomingReplyText'
|
|
```
|
|
|
|
All 4 failed, exactly as expected without the fix:
|
|
|
|
```
|
|
[ERROR] Tests run: 4, Failures: 4, Errors: 0, Skipped: 0
|
|
[ERROR] CompletionResolverTest.aCaseInsensitiveApiErrorLineIsStillClassifiedAsABackendError:612
|
|
the pattern is case-insensitive ==> expected: <FAILED> but was: <COMPLETION>
|
|
[ERROR] CompletionResolverTest.classifiesABackendErrorLineAsAFailureInsteadOfACompletedReply:582
|
|
a backend rejection is a failure, not a completed reply ==> expected: <FAILED> but was: <COMPLETION>
|
|
[ERROR] CompletionResolverTest.theBackendErrorReasonNamesTheMemberAndCarriesTheMatchedLine:597
|
|
the failure names the member: API Error: 400 invalid request body ==> expected: <true> but was: <false>
|
|
[ERROR] MessageServiceTest.backendErrorScrapeThroughMessageServiceFailsInsteadOfBecomingReplyText:104
|
|
a backend rejection must use the caller's failure outcome, not a completed reply
|
|
==> expected: <WORKER_FAILED> but was: <COMPLETED_UNREPLIED>
|
|
```
|
|
|
|
Then restored the fix (put the block back exactly as committed) and re-ran the full
|
|
`mvn clean install` — green again, see above (`Tests run: 1036, Failures: 0`).
|
|
|
|
## What I could not confirm
|
|
|
|
- I have no IDE tooling and no way to run the daemon live — only `mvn` in this worktree.
|
|
Everything reported above is from that command, nothing else.
|
|
- I did not attempt the "visibleTurn/raw-fallback" reorder described above — flagged as an
|
|
open, separate item, not fixed, not tested.
|
|
- Points 3 (broader backend-error surfacing) and 4 (spawn-time profile quarantine) of the
|
|
issue are untouched, as instructed.
|
|
- I have not verified this against a live daemon or a real crashed backend — only against
|
|
the unit/integration test fixtures in this repo.
|