fix(agent-server): recover a completed turn reported as error_during_execution (#1870)

[trinity-ability]

Fixes #1870

Claude Code can report is_error: true / subtype: error_during_execution for a turn that actually finished. The reproduction is a fan-out turn: the model reaches stop_reason: end_turn, a background subagent's <task-notification> lands after it, the follow-on turn is interrupted, and the CLI's terminal-state check sees a non-terminal last message — so it reports the whole turn as failed ([ede_diagnostic] result_type=user last_content_type=n/a stop_reason=null). _finalize_headless_result treated that as terminal and raised HTTP 502, discarding a complete assistant answer that was sitting on disk. Observed 4× in ~30 runs of one schedule.

This adds a new, additive recovery surface consulted from inside that branch. On positive on-disk evidence the completed answer is recovered and the turn returns 200 / SUCCESS; on anything else the 502 is unchanged (character-identical, modulo one indent level).

---

⚠️ READ FIRST — bounds, rollout, and what was NOT verified

1. Permanent coverage bound: agents with execution_timeout_seconds <= 600 are NOT fixed

The fix reads the session JSONL. That file exists only when session persistence is on. headless_executor auto-enables persistence for timeout_seconds > 600 only, and task_execution_service.execute_task never passes persist_session=True. So every schedule or webhook whose agent has execution_timeout_seconds <= 600 writes no JSONL, and #1870 remains unfixed for it — silently.

There is no fallback. This was measured, not assumed: the stream-json events carry no completion signal at all — message.stop_reason is None in 179/179 real assistant records, versus 99.96% populated on disk. There is nothing in memory to gate on.

The only operator lever is raising the agent's execution timeout above 600s (PUT /api/agents/{name}/timeout). When diagnosing "the fix isn't firing", check for event=jsonl_persistence_auto_enabled in the agent's logs first — its absence is the answer, not a gate problem.

2. Rollout requirement: base-image rebuild + COLD agent recreate

The change is entirely inside trinity-agent-base. A running fleet keeps discarding completed turns until ./scripts/deploy/build-base-image.sh runs and each agent is cold-recreated. A plain restart does not always adopt the rebuilt base image (#1809).

3. VERIFICATION GAPS — disclosed, not softened

The verify-local agent-exercise and integration stages did NOT run on this host. The harness could not mint an admin token against its freshly-wiped DB. This reproduced identically on a sibling branch, so it is a host/harness issue rather than something this change caused — but it means those stages provide no evidence here.

The plan's §4.4 live-container acceptance check remains OUTSTANDING and has been promoted to REQUIRED. Write the #1870 tail into a real agent's ~/.claude/projects/-home-developer/ and drive finalize against it. Synthetic fixtures plus a rebuilt base image can ship a silent no-op — this is the real gate, and it has not been done.

What DID pass: backend build + import-smoke, agent build + import-smoke, boot + health. The built image was content-verified to contain the new code (message.id grouping, _recover_completed_turn_from_jsonl, _safe_stop_reason, _RECOVERY_NOTICE, recovered_terminal), compiled to cpython-314 inside the image.

tests/unit is in NO required CI check. The 4 required checks on dev run no unit tests, so the 97 tests below gate nothing on merge. The live-container check in §4.4 is the real gate.

---

What changed

File	Change
docker/base-image/agent_server/services/jsonl_recovery.py	new _recover_completed_turn_from_jsonl, _is_main_thread, _is_before, _safe_stop_reason
docker/base-image/agent_server/services/headless_executor.py	_try_recover_completed_turn, _RECOVERY_NOTICE, the nested if in the execution_error branch
docker/base-image/agent_server/models.py	ExecutionMetadata.recovered_terminal (additive, defaulted)
docs/memory/requirements/scheduling.md	new §10.4.3
docs/memory/feature-flows/parallel-headless-execution.md	revision-history row + a new section; corrected a stale error-table row
tests/	test_1870_completed_turn_recovery.py (81), 5 new #1673 pins, a committed real captured transcript fixture, registry entry

Why the #678 recovery could not be reused. _recover_response_from_jsonl picks its boundary by walking backward from the end of file to the newest string-content user record — and the trailing <task-notification> is one. The boundary lands past the completed answer and the forward scan returns nothing. The notification is inside any plausible time window, so a since parameter would not have helped either: the boundary rule itself is the defect. #1870 therefore adds a new function; the #678 path is untouched and carries zero regression risk.

Three independent gates, all required — main-thread only (isSidechain/isMeta excluded from marker selection, the boundary walk and text collection) × turn-scoped (marker timestamp within [task_start_iso, now + 300s], fail-closed on both ends) × finished (the last qualifying assistant record must itself carry stop_reason == "end_turn"; max_tokens and stop_sequence rejected).

The recovered answer is the marker message's message.id group, fail-closed when that group holds no text — never a text window ending at the marker. A thinking-enabled final message is written as two records sharing one message.id (thinking then text) and both carry end_turn; measured over 1,075 real transcripts, 40.6% of markers are thinking-only. _read_jsonl_records drops the final partial line on an interrupted write — and #1870 is the interrupted-tail case — so a window rule would silently store the turn's narration without the answer as a 200 SUCCESS. Every decline is logged with a reason split by the action it implies, so a fail-closed gate that stops firing is distinguishable from "the bug never happened".

---

4. Repo-level defect found during this run (NOT caused by this PR)

tests/requirements-test.txt declares fastapi>=0.115.0 — a floor, not a pin. A fresh venv therefore resolves whatever FastAPI is latest, and tests/unit/test_1069_voip_call_path_param.py:214 imports get_flat_dependant from fastapi.dependencies.utils, a symbol newer FastAPI dropped:

ImportError: cannot import name 'get_flat_dependant' from 'fastapi.dependencies.utils'

This is invisible in CI — base and head are both red, so the diff matrix absorbs it — while breaking every fresh local environment that resolves past the drop.

Honest measurement caveat: on this host (FastAPI 0.136.1, Python 3.14) the symbol is still present and the file passes 4/4. The defect is the unpinned floor and the resulting resolution roulette, not a currently-red test on every machine. Recommend pinning FastAPI in tests/requirements-test.txt or reworking the import to a version-tolerant form.

Separately, tests/unit/test_1771{a,b,c}_*_properties.py (4 files) fail collection on this host with ModuleNotFoundError: No module named 'hypothesis' — also pre-existing and unrelated to this branch; they were excluded from the run below.

---

5. Behaviour note — the recovery notice flows downstream

_RECOVERY_NOTICE is prepended into the stored response, so it flows into {{previous_response}} loop templating and fan-out joins. That is deliberate and pinned by test — a recovered turn must never be indistinguishable from a clean one, and the notice is (see item 9) the only signal that actually persists onto the execution row. But a schedule whose downstream parses structured output will now see a markdown blockquote prepended — only on a turn that would otherwise have been a total loss.

---

6. Doc conflicts with PR #1938 (#1849) are EXPECTED — 2 mechanical, code overlap is ZERO

Both PRs touch docs/memory/feature-flows/parallel-headless-execution.md:

1. The revision-history row at the top anchor — same anchor fix(agent-server): never surface Claude Code's [ede_diagnostic] header as the error cause (#1849) #1938 uses. Keep both rows, newest first.
2. The "execution_error" | 503 → 502 table row. This branch corrects it (it has been wrong since bug: agent-server masks error_during_execution as a successful context: fork placeholder — failed session resumes recorded as success, Sessions-tab fallback never fires #1673 gave the case a dedicated 502 branch).

When resolving, do NOT re-gate on the error string — #1938 rewrites that text. This branch's gate is on-disk stop_reason, never the error message, precisely so the two are orthogonal.

Code overlap is zero.

7. #1853 collision note

Both this PR and #1853 edit the same execution_error branch. This PR keeps the raise HTTPException(502, …) character-identical modulo one indent level (verified mechanically), so the hand-resolve is six lines.

8. Design decision (user-directed)

Recovered turns return 200 / SUCCESS plus a distinguishing signal (recovered_terminal on the metadata + a notice in the response), so a recovered turn is never indistinguishable from a clean one. No new TaskExecutionStatus value was introduced — apply_result writes SUCCESS naturally and the #1083 async callback path inherits the fix with no extra wiring.

---

9. Follow-ups — LISTED, DELIBERATELY NOT FILED

No GitHub issues were created for these. File them if/when they are wanted.

1. Teach the backend to read recovered_terminal. Traced, not assumed: apply_result's success branch cherry-picks exactly six metadata keys (cache_read_tokens/cache_creation_tokens/input_tokens, context_window, cost_usd, session_id, compact_events) and drops the rest; schedule_executions has no metadata column. So today the marker dies on the wire — recovered_terminal is agent-side + on-the-wire only. apply_result's success branch is the single chokepoint.
2. The Option B verification ask. On the instance that filed bug: completed turn discarded on error_during_execution — jsonl_recovery never consulted, response lost despite end_turn in the JSONL #1870: compare the recovered text against what the retry produced for the 2 cases where the retry succeeded, and confirm that agent's execution_timeout_seconds > 600.
3. Typed ExecutionMetadata.error_subtype instead of string matching on error_type.
4. The /api/chat path has no execution_error branch at all — the same class of loss is simply unhandled there.
5. Surface the <= 600s gap in the UI, or reconsider the auto-persist threshold, so operators are not silently uncovered.
6. 3 pre-existing Async chat_with_agent: long execution silently fails with null response (reader-thread) #678 log sites interpolate session_id raw (same log-forging shape this PR fixed in its own new lines via !r) — left out of scope deliberately.
7. tests/unit is order-flaky on dev itself — 4 randomized runs on clean origin/dev gave green / 2 errors / 2 failed / green.

---

10. Release-note DRAFT TEXT (draft only — no docs/releases/<next>.md was created; 0.8.5 is the latest tag)

Fixed — completed turns are no longer discarded when Claude Code mislabels them as failed (#1870).
Claude Code can report error_during_execution for a turn that actually finished (a background subagent's notification landing after stop_reason: end_turn). The agent server now consults the session transcript for positive evidence of completion before discarding the answer, and surfaces the recovered response as a success with a visible recovery notice.
Requires a base-image rebuild (./scripts/deploy/build-base-image.sh) plus a cold agent recreate — a plain restart does not always adopt the new image.
Bound: the recovery reads the session JSONL, which is only written when session persistence is on (auto-enabled above a 600s execution timeout). Agents with execution_timeout_seconds <= 600 are not covered, and there is no fallback — raise the agent's timeout above 600s to opt in.

---

Testing

python3 -m pytest tests/unit/test_1870_completed_turn_recovery.py tests/unit/test_1673_execution_error_not_success.py -q
→ 97 passed

Driven off a committed real captured CC 2.1.220 transcript tail (tests/unit/fixtures/, redacted — structure byte-preserved, free-text values substituted), not a typed fixture: the plan's first draft asserted from the issue's rendered table that both trailing records were string-content, and measurement over 1,075 real transcripts showed [Request interrupted by user…] is list content (261 list / 2 str).

🤖 Generated with Claude Code

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[vybe]

/validate-pr — not merged, held on your own gate

Ran this in the merge sweep with #1838/#1910/#1954/#1959/#1752/#1955/#1956. Everything mechanical passes: Fixes #1870, no secrets/IPs/env changes, no schema or config surface, no new top-level backend module, requirements §10.4.3 + the parallel-headless flow doc updated, 97 tests with a real captured transcript fixture.

Held because the PR itself says the real gate hasn't run:

The plan's §4.4 live-container acceptance check remains OUTSTANDING and has been promoted to REQUIRED. Write the #1870 tail into a real agent's ~/.claude/projects/-home-developer/ and drive finalize against it. Synthetic fixtures plus a rebuilt base image can ship a silent no-op — this is the real gate, and it has not been done.

That reasoning is right, and it's reinforced by two other things in the body: tests/unit is in none of the four required checks on dev (Analyze (python), Analyze (javascript-typescript), schema-parity, verify-non-root), so the 97 tests gate nothing on merge; and the verify-local agent-exercise and integration stages didn't run on the host. Merging now would land a change whose only real verification is the one that was skipped.

To unblock

1. Run the §4.4 live-container check and paste the result.
2. Note the rollout order in the release notes — this is entirely inside trinity-agent-base, so it needs build-base-image.sh plus a cold recreate per agent (a plain restart doesn't reliably adopt the rebuilt image, bug: a rebuilt agent base image is never picked up — no image-drift predicate, so the v0.8.5 "stop/start-with-recreate" upgrade step is a no-op #1809).

Separately worth filing

The coverage bound is a silent one: agents with execution_timeout_seconds <= 600 write no JSONL, so #1870 stays unfixed for them with no signal beyond an absent event=jsonl_persistence_auto_enabled log line. Measuring 179/179 stop_reason: None in-stream to prove there's no fallback is the right way to establish that — but "the fix silently doesn't apply below a timeout threshold nobody sets deliberately" deserves its own issue rather than living only in this PR body.

The fastapi>=0.115.0 floor-not-pin defect you found is also worth its own issue — it's a repo-wide fresh-venv breakage, not part of #1870.

The stale "merge conflict against dev" bot comment above is a false positive from the nightly's missing merge-base — that's exactly what #1952 fixes.

[vybe]

Validated via /validate-pr — additive, fail-closed recovery; miss path character-identical to today's 502 (pinned by the #1673 tests); 97 named regression tests off a redacted real fixture; full doc traceability (requirements §10.4.3 + feature flow). Merging; RELEASE is conditioned on the PR's own §4.4 live-container acceptance check + base-image rebuild with cold agent recreate (#1809).