feat(plugins): turn_failed hook — observe non-clean turn exits

[brandonedley]

What

Adds a turn_failed plugin hook (registered in VALID_HOOKS, emitted from finalize_turn at the same point the turn-exit diagnostic is logged) so observability plugins can capture non-clean turn exits: pending-tool premature stops, non-text_response exit reasons, guardrail/exhaustion exits. Observers only — a failing handler can never break finalization, and deliberate user interrupts (/stop) never fire it.

Why

Failure-capture pipelines (e.g. feeding an agent-failure sink or a kanban triage queue) currently have no supported way to observe "the turn ended badly" — the closest hooks (api_request_error, on_session_end) fire at the wrong granularity. This has been running in production on a fork for a week feeding an automated failure-ingest pipeline.

Bonus fix included

While rebasing onto current main we found the #43849/#44100 persist-invariant (append an assistant row whenever a final_response was delivered) lands before the turn-exit diagnostic, so messages[-1] can no longer be "tool" there — which silently degraded the existing Turn ended with pending tool result WARNING for any premature stop that delivered partial text. The third commit captures the tail role before persist-time mutations and uses it for both the diagnostic and the new hook.

Testing

tests/run_agent/test_turn_failed_hook.py — 9 tests covering: fires on pending-tool stop, fires on non-text exit reasons, never fires on healthy completion, never fires on user interrupt, kwargs payload shape, handler-exception isolation, and the pre-persist tail classification. pytest tests/run_agent/ -k 'final or persist or turn' — 221 passed.

🤖 Generated with Claude Code

[teknium1]

Thanks for identifying the post-persistence tail-classification regression; current main does append a recovery assistant row before the diagnostic reads messages[-1] (agent/turn_finalizer.py:223-229, :249-285).

Problems

• The new emission at agent/turn_finalizer.py:300 only runs when finalize_turn() is reached. Current main has non-clean terminal exits that return directly from conversation_loop, including invalid-response exhaustion at agent/conversation_loop.py:1600-1607 and disabled-compaction context overflow at :3135-3144. Those failures would not emit turn_failed, despite the hook's stated all-non-clean-exit contract.
• tests/run_agent/test_turn_failed_hook.py covers the finalizer path, but not a failed direct-return path that bypasses it.

Suggested changes

• Centralize terminal-result emission/finalization, or cover every failed direct return with the same observer-only emission contract.
• Add an integration test for one bypassing failure path, such as the invalid-response terminal return at agent/conversation_loop.py:1600.

Automated hermes-sweeper review.

[teknium1]

This only covers the post-loop fallthrough. Current failed returns such as agent/conversation_loop.py:1600-1607 (invalid-response exhaustion) and :3135-3144 (compaction-disabled overflow) bypass finalize_turn() entirely, so they cannot emit this hook. Please centralize terminal failure emission or cover those direct-return paths before claiming all non-clean exits.

[brandonedley]

Both problems addressed in 794698445, and the branch is rebased onto current main. Taking your points in order.

Problem 1 — emission only from finalize_turn

Confirmed, and worse than the two paths you named. I inventoried run_conversation with an AST walk rather than trusting a grep: there are 24 terminal returns that build a result dict and return directly, and every one sets completed: False.

Two things that inventory changed about the fix:

• failed is the wrong predicate. Only 13 of the 24 set failed; the other 11 have no such key — including the disabled-compaction context-overflow exits you named. Keying on failed would have missed almost half, so the sweep keys on completed is False.
• 3 of the 24 set interrupted: True and must stay silent, consistent with the user-/stop gate already in _should_emit_turn_failed.

Your line references point at an older tree, incidentally — on current main the invalid-response exhaustion is agent/conversation_loop.py:2530 and the context-overflow exit is :4166. Same paths, and both are covered.

Suggested change 1 — "centralize, or cover every failed direct return"

Took the centralize option rather than instrumenting 24 sites, since the per-site approach requires every future site to remember:

• emit_turn_failed(agent, **fields) — one emitter, idempotent per turn via an agent._turn_failed_emitted latch reset in turn setup. finalize_turn now routes through it, and sets the latch even when it deliberately stays silent, so the sweep can never override an intentional non-emit.
• emit_turn_failed_for_unfinalized_exit(agent, result) — runs once in the run_conversation forwarder in run_agent.py, which every caller (cli.py, batch_runner.py, the agent's own entry points) passes through. Emits only when completed is False, the turn has not already emitted, and the exit was not an interrupt.

_should_emit_turn_failed is untouched and remains the sole classifier — the guard you already reviewed keeps its contract.

The reason I'd argue for the contract-based sweep over per-site instrumentation: a 25th terminal path is covered the moment it honours completed: False, with no new emission call. To keep that from being an unchecked assumption, there's an AST test asserting every terminal return carrying final_response also carries completed — so if someone adds a path that breaks the premise, that test fails rather than the failure silently going unobserved.

Problem 2 / Suggested change 2 — a test for a bypassing path

test_bypass_sweep_fires_on_invalid_response_exhaustion covers the invalid-response terminal return you named specifically. Ten more alongside it:

• context overflow emits despite having no failed key (the 11-path class above)
• silent on user interrupt; silent on healthy completion
• silent on shapes that cannot be positively identified as failed — None, non-dict, {}, completed: None
• no double-fire when finalize_turn already emitted
• a raising observer cannot break turn teardown
• the AST source invariant

+11 tests. Full suite 2455 passed (was 2444), no regressions.

That raising-observer test earned its place immediately — it caught a real bug in my first draft. This module imports logger lazily to avoid an agent.conversation_loop import cycle, so referencing it at module scope turned a failing observer into a NameError, i.e. the handler meant to swallow a plugin error would itself have raised. The lazy import now sits inside its own guard.

One adjacent bug I found but deliberately did not bundle

The forwarder already classifies the relay outcome right after run_conversation returns:

elif terminal.get("failed") is True:
    relay_outcome = "failed"
else:
    relay_outcome = "success"

11 of the 24 terminal returns never set failed. Three of those do set interrupted and so land correctly on "cancelled" — but the remaining 8 non-clean exits are recorded as relay "success" (conversation_loop.py lines 2835, 2963, 3033, 3050, 5564, 5693, 5782, 5866).

That looks like a genuine observability defect independent of this hook, but fixing it changes relay-metrics semantics, so it does not belong in an observer-only PR. Happy to file it separately, or fold it in here if you'd rather see them together — your call.