fix(adapter): surface error on agent EOF without final response

[howie]

What problem does this solve?

When a bridged (non-ACP-native) agent crashes on a backend error, OpenAB shows the user a vague _(no response)_ (or, worse, a partially-streamed buffer presented as a complete answer) instead of the real failure reason.

agy does not natively speak ACP — we bridge it. So when an AI backend (e.g. Gemini) returns HTTP 500 or a quota-exhausted error, agy can crash/exit without first emitting an ACP final response carrying that error. The failure chain:

1. AI backend returns HTTP 500 / quota exhausted.
2. agy crashes/exits — never sends an ACP error notification.
3. OpenAB's ACP stdout pipe reaches EOF.
4. The recv loop's rx.recv() -> None branch breaks out, but response_error is still None.
5. final_content is empty → falls through to the _(no response)_ sentinel.

The user is left with no idea their request hit a quota limit or server error.

This was surfaced in the OpenAB community discussion. Because agy itself doesn't support ACP, the bridging layer (OpenAB side) must supply a sensible error on pipe EOF rather than letting it fall through.

Closes #

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1519250661664100455

At a Glance

Before:
  backend 500/quota  ->  agy crashes (no ACP error)  ->  pipe EOF
        ->  recv() == None  ->  break (response_error stays None)
        ->  final_content empty  ->  "_(no response)_"   [user confused]

After:
  backend 500/quota  ->  agy crashes (no ACP error)  ->  pipe EOF
        ->  recv() == None  ->  response_error = "Agent process exited unexpectedly"
        ->  final_content: "⚠️ Agent process exited unexpectedly"   [empty buffer]
                        or "⚠️ Agent process exited unexpectedly\n\n<partial text>"  [partial stream]

Termination paths:
  liveness check sees dead process   -> "⚠️ Agent process died"               (already handled)
  pipe EOF, no final response        -> "⚠️ Agent process exited unexpectedly" (THIS FIX)
  agent sends proper ACP error       -> "⚠️ Server Error ... quota exceeded"   (already handled)

Prior Art & Industry Research

Not applicable — defensive bug fix on the ACP bridge. Adds error surfacing on an existing break
path; no new architecture, dependency, or protocol. It makes the EOF branch consistent with the
sibling !conn.alive() branch, which already sets response_error = "Agent process died".

Proposed Solution

In crates/openab-core/src/adapter.rs, the recv loop EOF branch records an explicit error when the
pipe closes without a final response and no error was already recorded:

None => {
    if response_error.is_none() {
        response_error = Some("Agent process exited unexpectedly".into());
    }
    break;
}

The downstream final_content builder already renders response_error as ⚠️ {err} (standalone
when there is no body, or prepended to any partial content), so no other change is needed.

Why this approach?

• Smallest correct fix on the bridge side. agy can't be relied on to emit an ACP error before
  crashing, so the bridge detects the abnormal EOF itself, mirroring the existing
  !conn.alive() branch.
• No buffer-content guard. An earlier revision gated this on text_buf.is_empty(). Multi-model
  mob review (Codex + a Claude multi-finder review) showed that guard is unsound: it is defeated by
  the session-reset pre-seed (text_buf already holds the expiry notice), by send-once mode (the
  delivered body is a post-answer_start slice, not the full buffer), and by native streaming
  (partial text shown as a complete answer). The guard is also unnecessary: a successful turn is
  always signalled by the id-bearing JSON-RPC response to session/prompt, which breaks the recv
  loop before EOF — so the EOF branch is reachable only on abnormal termination, regardless of
  buffer contents. When partial text was streamed, final_content prepends the warning to it,
  preserving the output while flagging the truncation.

Alternatives Considered

• Gate on text_buf.is_empty() (earlier revision): rejected after mob review — masks crashes on
  reset turns, send-once tool turns, and native-streaming partials (see above).
• Synthetic final-response timeout instead of acting on EOF: rejected — EOF already definitively
  signals the pipe closed; a timeout only adds latency for the same outcome.
• Fix only agy (emit ACP error before exit): rejected as the sole fix — agy isn't guaranteed to
  emit an error before crashing, so the bridge must be robust to silent EOF. A complementary agy-side
  fix is still worthwhile but out of scope for this repo.

Known Issues / Follow-ups

• Reaction emoji on crashed turns (pre-existing, deferred): the EOF-injected response_error
  drives the ⚠️ message banner but not delivery_failed, so dispatch still reacts with success
  (🆗) rather than error (❌). This gap is shared with the existing Agent process died /
  hard-timeout / coded-error branches — this PR only joins the pattern. Recommended as a separate PR
  that wires response_error → ❌ uniformly for all error branches.

Validation

Rust changes:

• cargo build -p openab-core passes
• cargo test -p openab-core --lib adapter — 43 passed, 0 failed
• adapter.rs clippy-clean
• Multi-model mob review (Codex + Claude multi-finder): converged to LGTM after the
  text_buf.is_empty() guard was dropped

CI note:

• The check job (cargo clippy --workspace -- -D warnings) is currently RED on
  crates/openab-core/src/pre_seed.rs:289 (manual_is_multiple_of, rust-1.96 lint) — pre-existing
  on main (landed via feat: add pre_seed phase — S3 zip download before pre_boot #1189/feat(pre_seed): support .tar.gz/.tgz archives #1196/build: make pre-seed a default feature #1197), not part of this diff. The merge gate will stay red until
  main is fixed; flagging for maintainers.

All PRs:

• Manual testing — reproduce by forcing a bridged agent to exit on a backend 500/quota error and
  confirm the user sees ⚠️ Agent process exited unexpectedly (with any partial text preserved)
  instead of _(no response)_. No automated test seam exists for this deeply-nested recv loop;
  the sibling error paths are likewise covered only by manual repro against a live agent.

[howie]

Final Aggregated Review — PR #1198

Mode

group-review (2/2 voices active: Claude 8-finder/14-verified workflow + Codex)

Outcome (after fix + re-review)

• Consensus fix applied (commit 6697c21): dropped text_buf.is_empty(); EOF now always sets
  response_error when none was recorded. Resolves findings 1–3.
• Codex re-review: LGTM (no findings). Claude voice (lead): LGTM — findings 1–3 resolved,
  false-positive concern was refuted at review time, fix: pull --rebase before push in bump-chart #4 deferred (user decision), fix: use PR for chart bump instead of direct push #5 kept distinct.
• All 43 adapter unit tests pass; cargo build -p openab-core clean.
• Merge blocked only by the pre-existing pre_seed.rs:289 clippy lint on main (see Out-of-PR
  note) — user decision: leave for maintainers.

Consensus Critical (must fix)

None.

Consensus Important (must fix)

Both voices converge on a single root cause: the text_buf.is_empty() guard predicates error
surfacing on buffer contents, but buffer contents are not a reliable proxy for "did the turn
complete". Three confirmed crash classes still slip through silently:

1. Session-reset turn masks the crash (adapter.rs:705-707 pre-seeds text_buf with
   "⚠️ _Session expired, starting fresh..._"). On a reset turn text_buf.is_empty() is always
   false, so a bridged-agent crash at EOF never sets response_error — the user sees only the
   reset notice. Confirmed by 6 Claude finders + verifiers.

2. Guard measures the wrong buffer (send-once mode). The guard reads the full pre-split
   text_buf, but the delivered body is the post-answer_start slice. A turn that streams
   pre-tool narration, runs a tool (answer_start advances to buffer end at adapter.rs:955),
   then crashes with no post-tool text → text_buf non-empty but delivered body empty → no error.
   Confirmed by Codex + multiple Claude verifiers.

3. Native-streaming partial-text crash shown as complete. Partial text flushed live, then
   backend 500/quota closes stdout → response_error stays None by the guard → truncated answer
   shown unprefixed, indistinguishable from a deliberately short reply. (Codex's primary finding +
   Claude finding.)

Unifying fix (resolves all three): drop the text_buf.is_empty() conjunct; on EOF set
response_error whenever none was recorded. This is provably safe — both voices' verifiers
independently REFUTED the "successful turn closes stdout without a final response → false
positive" concern: a successful ACP turn is always signaled by the id-bearing JSON-RPC response to
session/prompt, which breaks the loop at the id-branch before EOF. The EOF branch is therefore
reachable only on abnormal termination, regardless of buffer contents. (Independently corroborated
by an Explore trace of acp/connection.rs:170-172,255-257 + agy-acp/src/main.rs:46-52,149-172.)

The original spec's text_buf.is_empty() rationale ("agent streamed all text then closed
normally") describes a path that does not exist in this ACP/JSON-RPC implementation.

Disputed / scope decision (user decides)

4. EOF error is display-only; does not set delivery_failed (Claude single-voice, CONFIRMED at
   verify index 29). A crashed turn shows the ⚠️ banner but dispatch still reacts with success
   (🆗) because the closure return is governed by delivery_failed, never by response_error.
   Pre-existing & shared: the existing Agent process died (833), hard-timeout (838), and
   coded-error branches have the same gap — my branch only joins the existing pattern, it does not
   introduce it. Wiring response_error → ❌ reaction is a broader change to the shared
   error/delivery contract affecting all four branches.
   • Lead recommendation: OUT OF SCOPE for this focused EOF fix. Defer to a separate PR that
     fixes the reaction signal for all error branches uniformly; note as Known Issue here.

Actionable NIT

5. recv-vs-timeout wording race (PLAUSIBLE). The same crash can surface as
   "Agent process exited unexpectedly" (EOF arm wins) or "Agent process died" (liveness arm
   wins) depending on a tokio::select! race.
   • Lead decision: keep distinct wording. Both messages are accurate and reflect two genuine
     detection mechanisms (stdout EOF vs liveness probe); the distinction aids triage rather than
     harming it. Documented here rather than forced into a single string. Not a defect.

Voices unavailable

• agy (Gemini): not installed (cache GEMINI_OK=0). 2-voice mob.

Out-of-PR note

• CI check job is red on a pre-existing clippy lint at pre_seed.rs:289
  (manual_is_multiple_of, added by PRs feat: add pre_seed phase — S3 zip download before pre_boot #1189/feat(pre_seed): support .tar.gz/.tgz archives #1196/build: make pre-seed a default feature #1197 on main, rust-1.96). Not part of this
  PR's diff. Blocks the merge gate until main is fixed — surfaced separately for the user.

[chaodu-agent]

LGTM ✅ — Minimal, correct defensive fix for the silent-EOF-on-bridged-agent-crash path.

What This PR Does

When a bridged (non-ACP-native) agent crashes on a backend error (e.g. HTTP 500, quota exhausted), it exits without emitting an ACP error notification. Previously the recv loop's EOF branch simply broke out, leaving response_error as None and the user seeing a vague _(no response)_. This PR surfaces an explicit "Agent process exited unexpectedly" error on that path, consistent with the existing !conn.alive() ("Agent process died") branch.

How It Works

In the rx.recv() => None branch (pipe EOF without a prior final response), sets response_error = Some("Agent process exited unexpectedly".into()) when no error was already recorded. The downstream final_content builder already renders this as ⚠️ {err}. No other code changes needed.

Findings

#	Severity	Finding	Location
1	🟢	Correct pattern — mirrors !conn.alive() branch exactly	adapter.rs:815-835
2	🟢	Sound reasoning for dropping the text_buf.is_empty() guard — well-documented in comments	adapter.rs:823-831
3	🟢	response_error.is_none() guard avoids overwriting errors already set by other paths	adapter.rs:833

Finding Details

🟢 F1: Correct pattern reuse

The fix follows the exact same response_error = Some(...) + break pattern used by the sibling !conn.alive() and hard-timeout branches. Minimal and consistent.

🟢 F2: Thorough design rationale in comments

The inline comments explain why the text_buf.is_empty() guard was dropped (pre-seed buffer, send-once mode, native streaming partials) — this is excellent documentation for future maintainers and avoids re-litigating the same design question.

🟢 F3: Defensive is_none() guard

Protects against double-setting response_error if a future code path sets it before reaching EOF. Good defensive practice.

Baseline Check

• PR opened: 2026-06-25
• Main already has: response_error handling for !conn.alive() ("Agent process died") and hard timeout; the EOF branch was a bare break
• Net-new value: Surfaces an explicit error on the EOF-without-final-response path, eliminating the silent _(no response)_ failure mode for bridged agents

CI Status

• All smoke tests (14 matrix configs + 14 unified): ✅ PASS
• check (clippy): ❌ — pre-existing manual_is_multiple_of lint on pre_seed.rs:289 (from main, landed via feat: add pre_seed phase — S3 zip download before pre_boot #1189/feat(pre_seed): support .tar.gz/.tgz archives #1196/build: make pre-seed a default feature #1197), unrelated to this diff
• check (second run): ❌ — PR body template enforcement (missing Discord Discussion URL), not a code issue

What's Good (🟢)

• Single-line behavioral fix with extensive, well-reasoned inline documentation
• Consistent with existing error-surfacing patterns in the same recv loop
• PR description is exemplary: clear before/after diagrams, alternatives considered, known issues documented
• Multi-model mob review already performed and converged

[antigenius0910]

LGTM ✅ — independent re-verification of the bug and the fix.

Bug reproduces in code

Confirmed in crates/openab-core/src/adapter.rs on main:

• :815-816 — EOF arm is a bare break and never sets response_error.
• :1033-1038 — when final_content is empty and response_error is None, falls through to the _(no response)_ sentinel.
• Sibling termination paths already break the symmetry: :820 (Agent process died), :825 (hard timeout), :845 (coded ACP error) all set response_error. The EOF arm is the only error termination that stays silent — this PR closes that asymmetry.

Fix is provably safe

The mob-review claim that "successful turns never reach the EOF arm" verifies directly at :835-848:

if let Some(notification_id) = notification.id {
    if notification_id != request_id { continue; }
    if let Some(ref err) = notification.error { ... }
    break;   // matching id ⇒ always breaks here, before any EOF
}

A successful ACP turn closes with a JSON-RPC response carrying the matching request_id, which trips this branch before the pipe close can be observed. So the EOF arm is reachable only on abnormal termination, regardless of text_buf state. Dropping the text_buf.is_empty() guard is therefore correct — keeping it would have masked the three confirmed crash classes already documented in the aggregated review (session-reset pre-seed, send-once post-tool slice, native-streaming partial).

Scope check

Searched open issues for an existing report: none filed (PR is Discord-driven, no Closes #).

• #997 (codex-acp LLM call hang) looks adjacent but is a different defect — that one is an intra-turn LLM stall with no EOF and no events, which is the liveness/inactivity-timeout territory, not the EOF arm. This PR does not address #997 and should not be conflated with it.
• #1101, #976, #1075, #990, #1090 — unrelated.

CI

• 27/28 green; the only red is the pre-existing manual_is_multiple_of lint on pre_seed.rs:289 from main (landed via #1189/#1196/#1197) — not from this diff.
• All 14 smoke-test configs + 14 unified-build configs pass, including agy-acp (the bridged-agent path this fix targets).

Known follow-ups (not blockers)

• The aggregated review's Finding #4 — response_error doesn't drive delivery_failed, so dispatch still reacts 🆗 instead of ❌ on crashed turns — is pre-existing and shared with the three sibling error branches (Agent process died, hard-timeout, coded-error). Wiring response_error → delivery_failed uniformly is a broader contract change and is correctly deferred to a separate PR.

Approving.