feat(acp/desktop): classify turn failures and persist them per agent#2240
feat(acp/desktop): classify turn failures and persist them per agent#2240Orland00 wants to merge 4 commits into
Conversation
Add a stable, machine-readable error_class discriminant (timeout, transport, agent_error, protocol, exited, cancelled, panic, error) to the turn_error and agent_panic observer payloads, additive alongside the existing outcome/error/code fields. Extracted into a pure classify_turn_failure(&PromptOutcome) so the mapping is unit-testable independent of the emit call sites, and it mirrors the existing outcome_label/is_transport_error groupings rather than inventing a new taxonomy. Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
…discarding them activeAgentTurnsStore previously treated turn_error/agent_panic the same as turn_completed: endTurn() ran and the outcome/error/code payload was discarded, so the "Working in #channel" badge just vanished with no trace of the failure. Add a per-agent TurnFailure record (outcome, error, code, errorClass, timestamp) captured from the turn_error/agent_panic payload — including the new additive error_class field from the backend commit — and exposed via getLastTurnFailureForAgent / useLastTurnFailure. It is cleared only on a subsequent turn_completed (a successful completion is the "agent is healthy again" signal; a mere turn_started is not, so the badge doesn't flicker off and back on). Both new branches sit behind the same per-agent watermark gate as before, so the existing stale-replay guarantees (a replayed turn_error/agent_panic with a null turnId must not resurrect or re-trigger effects on a live/completed turn) are unchanged — verified with a new regression test. The failure map is also folded into the community-switch save/restore snapshot alongside the other four maps. Wired into ManagedAgentRow's existing StatusBlock, next to the process-exit-sourced `agent.lastError` surface, rendered through friendlyTurnErrorCopy so JSON-RPC codes still get friendly copy. Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
2128600 to
dc4869b
Compare
Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
Signed-off-by: Orlando Lopez <orlando.lf19@gmail.com>
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Agent-authored review (two independent passes; findings converged, so treat the blocker as high-confidence).
Really solid contribution overall — the watermark-gate analysis is correct, errorClass being nullable for mixed-version fleets is the right call, the classification fn is pure with every branch tested, and the stale-replay regression test is exactly the right non-happy-path. The failure map also correctly joins reset + community-switch save/restore.
I re-ran the gates locally at ce7f1228: cargo test -p buzz-acp 580/0, clippy -D warnings clean, fmt --check clean, desktop typecheck/lint/test (3339/0)/build all green. The checks in the PR description are accurate.
Requesting changes for one correctness issue in the clear path (inline below): turn_completed is emitted on every exit path by TurnCompletionGuard (crates/buzz-acp/src/pool.rs:3218-3258), not just success, so a cancelled turn wipes a persisted failure and never re-records it. Details and a minimal suggested fix on the store's turn_completed branch. The other inline comments are non-blocking nits — take or leave them.
| case "turn_completed": | ||
| endTurn( | ||
| agentPubkey, | ||
| event.turnId ?? null, | ||
| event.channelId ?? null, | ||
| Date.parse(event.timestamp), | ||
| ); | ||
| // A successful completion is the "agent is healthy again" signal — | ||
| // clear any previously-persisted failure so its badge disappears. | ||
| clearLastTurnFailure(key); |
There was a problem hiding this comment.
Blocking — turn_completed is not “successful completion”, so this clear fires on cancelled turns too.
TurnCompletionGuard (crates/buzz-acp/src/pool.rs:3218-3258) emits turn_completed on every exit path — success, error, timeout, cancel, panic (its own doc comment says so). Two consequences:
- A cancelled turn wrongly clears a persisted failure.
PromptOutcome::Cancelledemits noturn_error(crates/buzz-acp/src/lib.rs:3271-3280— it just returns the agent to the pool), but its guard-dropturn_completedstill arrives, so a user who cancels a turn erases the badge from the previous genuinely-failed turn. That contradicts the PR's own design note (“clear on next successful completion”) — a cancel proves nothing about health, same asturn_started. - Failed turns only render correctly by emission-order luck. A failed turn emits
turn_completed(guard drop, lowerseq) thenturn_error(main loop, higherseq), so the store happens to clear-then-set. Nothing documents that the badge's correctness depends on that ordering.
Minimal fix: add an "outcome" field to the turn_completed payload (it's json!({}) today) and clear only when it's absent-or-"ok". Treating absent as success keeps mixed-version behavior identical to this PR (an older harness's cancellations would retain the bug, but that's a strictly-smaller surface than shipping it for current builds — worth a one-line comment either way). Please also add a regression test for failure → cancelled turn → failure remains.
| * from a harness build that predates the field. */ | ||
| errorClass: string | null; | ||
| /** Agent-host clock ms when the failure was recorded (parsed from the event). */ | ||
| timestamp: number; |
There was a problem hiding this comment.
Nit: timestamp is stored and asserted in tests but never rendered anywhere — dead field today. Fine if a tooltip/relative-time display is coming; otherwise it's removable.
| /// classes for wording differences. | ||
| fn classify_turn_failure(outcome: &PromptOutcome) -> &'static str { | ||
| match outcome { | ||
| PromptOutcome::Ok(_) => "error", |
There was a problem hiding this comment.
Nit: PromptOutcome::Ok(_) => "error" silently mislabels an input that should be unreachable at both emit sites. A short comment (or debug_assert!) naming this as a defensive default would keep a future caller from trusting it as a real classification.
| data-testid="managed-agent-last-turn-error" | ||
| > | ||
| Last turn error ( | ||
| {lastTurnFailure.errorClass ?? lastTurnFailure.outcome}):{" "} |
There was a problem hiding this comment.
Nit: this renders the raw machine token in user-facing copy (“Last turn error (agent_error): …”). A tiny humanizing map — or the prettify helper already in agentSessionUtils — would read better. Product call, non-blocking.
| {friendlyError.copy} | ||
| </p> | ||
| ) : null} | ||
| {lastTurnFailure ? ( |
There was a problem hiding this comment.
Nit: for error_class == "exited" this line will stack with the process-exit friendlyError line just above in the same StatusBlock — two error lines describing one crash. Consider suppressing the turn-failure line when a process-level error is already shown.
2 participants
Closes #1659.
Problem
Failed turns still burn tokens; when timeouts, transport flakes, auth expiry and panics all collapse into one vanishing "Turn error" badge, that spend is impossible to attribute or reduce. As discussed on the issue, the backend already derives a granular
outcome_label— the collapse happens in the desktop store, which handlesturn_error/agent_panicin the same fall-through asturn_completedand discards the entire error payload.Implementation
Two commits (split-able into separate PRs on request):
feat(acp)— additiveerror_classfield on theturn_errorandagent_panicpayloads: a stable machine discriminant (timeout/transport/agent_error/protocol/exited/cancelled/panic) derived in a pureclassify_turn_failure(&PromptOutcome)fn from the same data that drivesoutcome_labeland the existingis_transport_errorgrouping. Existing payload fields untouched; old frontends unaffected. No central schema doc exists for observer event kinds (grepped docs/, AGENTS.md, NOSTR.md), so the fn's doc comment is the documentation site, matching the existing convention.feat(desktop)—activeAgentTurnsStorenow splits the terminal-event fall-through:turn_completedclears a new per-agentlastFailureByAgententry;turn_error/agent_panicpersist{outcome, error, code, error_class, timestamp}. Both remain inside the pre-existing per-agent watermark gate, so the stale-replay guarantee (replayedturn_errorwith nullturnIdmust not resurrect state) is preserved — covered by a new regression test. The persisted failure renders inManagedAgentRow's existingStatusBlockthrough the existingfriendlyTurnErrorCopy, next to the process-exitlastErrorline — no new UI surface. Clearing happens on the next successful completion, not on turn start: a turn starting proves nothing about health and would make the badge flicker.Design notes:
errorClassis nullable on the frontend so a mixed-version fleet (older harness without the field) degrades gracefully tooutcome.resetActiveAgentTurnsStoreand the community-switch save/restore snapshot like the other four maps.Testing
classify_turn_failurebranch + payload assertions in the existingerror_outcome_emission_tests;cargo test -p buzz-acp580 passed,cargo clippy -p buzz-acp --all-targets -- -D warningsclean,cargo fmt --all -- --checkclean,cargo check --workspaceclean.pnpm -C desktop test3339 passed / 0 failed;typecheckandlint(biome) clean.Manual test: trigger any turn failure (e.g. kill the agent process mid-turn) → the agent row shows "Last turn error (exited): …" persistently; run a successful turn → the line clears.
Deferred
ManagedAgentRow's newmanaged-agent-last-turn-errornode (repo has no component-level tests for this row today) — happy to add a Playwright assertion if you want one.