feat(cloud-agent-next): attribute incomplete Kilo session failures - #5068
feat(cloud-agent-next): attribute incomplete Kilo session failures#5068eshurakov wants to merge 2 commits into
Conversation
…fic terminal failure codes - Preserve watchdog failure codes (wrapper_no_output, wrapper_ping_timeout, wrapper_disconnected) after agent activity instead of collapsing them into wrapper_error_after_activity - Detect finish=length output-limit terminations on the root session in the wrapper and fail fast with kilo_output_limit - Fail code-review runs whose final root assistant message has no visible text with kilo_empty_terminal_response instead of sealing them as complete - Thread both codes through the DB type union, worker-utils classification (platform/wrapper_liveness), queue-report allowlist, safe-failure projection, telemetry diagnostics, and web terminal-reason mapping - Resolve cloud.status and persisted error event messages from the structured failure code for the new codes so all user-facing surfaces agree
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the 14 files changed since commit Files Reviewed (14 files)
Previous Review Summary (commit cdc751e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit cdc751e)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe new wrapper-side empty-response/output-limit detection in Overview
Issue Details (click to expand)WARNING
Files Reviewed (14 files)
Verified non-issues: taxonomy threading is consistent across all seven enumeration sites (DB union, worker-utils codes/classifier, queue-report allowlist, protocol enum, telemetry, safe-failure projection, web reason map) with no typos; the model-not-found diagnostics gating is preserved; omitting Reviewed by kimi-k3 · Input: 60.8K · Output: 12.4K · Cached: 601.1K Review guidance: REVIEW.md from base branch |
Idle without an explicit error is success. Remove kilo_empty_terminal_response and the visible-text tracker; keep output-limit fail-fast and watchdog code preservation. Skip kilo_output_limit while finalizing so post-completion turns cannot false-fail a sealed run.
1 participant
Summary
Why
Twelve code-review sessions on 2026-08-04/05 ended with incomplete Kilo results (output-limit terminations, unresponsive wrappers), but the Worker terminalized all of them identically —
wrapper_error_after_activity/wrapper_liveness, up to ~90s after the real cause. The terminal record erased which watchdog fired and why, making the failure class invisible to dashboards and support. The wrapper is the only component that observes Kilo's event stream directly, so it is the right place to attribute the mechanically detectable endings; the DO stays a durable coordinator.What was done
wrapper_no_output,wrapper_ping_timeout,wrapper_disconnected) when agent activity was observed, instead of collapsing it intowrapper_error_after_activity; the{agent_activity, code}pairs were added to the queue-report allowlist so those reports are not silently dropped by telemetry validation.message.updatedwithfinish === 'length'(or aMessageOutputLengthError) raises terminal failurekilo_output_limitin ~1s, and the completion signal is intentionally not armed for that message. Child sub-agent messages hitting output limits are explicitly excluded and never terminalize the run. Detection is skipped whilestate.isFinalizingso post-completion turns (e.g. condense-on-complete) cannot false-fail a sealed run.platform/wrapper_liveness, so reporting taxonomy is unchanged), safe-failure projection, telemetry diagnostics, the web terminal-reason map (assistant_failed), and the DBtext().$type<>()union — no migration.cloud.statusbroadcast and persisted error-event message from the structured failure code forkilo_output_limit, so live status,cloud.message.failed, callbacks, and push all show the same per-code message instead of a generic "Assistant request failed".Not in this PR: empty-terminal-response / visible-text failure. Idle without an explicit error is success; a tools-only empty final assistant message is complete.
High-level architecture
sequenceDiagram participant Kilo as Kilo server (sandbox) participant Wrapper as Wrapper (sandbox) participant Ingest as Ingest WS (Worker) participant DO as Session DO (supervisor) participant Web as Web / callback Kilo->>Wrapper: SSE message.updated / session.idle alt finish = "length" on root assistant message Wrapper->>Ingest: fatal error event (failureCode=kilo_output_limit) else normal completion Wrapper->>Ingest: completion signal (unchanged path) end Ingest->>DO: forwardIngestTerminalEvent(status=failed, failureCode) DO->>DO: terminalizeSessionMessageOnce — stage from agent activity, per-code safe message DO-->>Web: cloud.message.failed + callbackWorker-side liveness paths (
runMaintenanceno-output/ping watchdogs, disconnect grace) follow the same settlement flow and now keep their specific code when agent activity was observed.Architecture decision
Decision: Attribute incomplete-session endings in the wrapper from Kilo SDK event shapes (
finish === 'length') and preserve the specific post-activity watchdog code, rather than inferring causes Worker-side after watchdog expiry.Context: Once a session goes silent, the Worker has no bounded authoritative Kilo terminal query — disconnect or liveness expiry terminalizes accepted work as a generic wrapper failure. Only the wrapper sees the event stream where output-limit endings are mechanically distinguishable.
Rationale: Wrapper-side detection fails the run in ~1s with a precise code instead of ~90s later with a collapsed one, and it matches the service's ownership boundary: Kilo/job behavior lives in the wrapper, the DO remains a durable coordinator.
Alternatives considered:
error.name === 'MessageOutputLengthError'. The SDK types that error, but the current Kilo server never constructs it — it appends an ignored warning text part and persistsfinish: "length"— so the predicate would ship as a well-tested no-op. (The error-name check is still OR-ed in for forward-compatibility.)wrapper_error_after_activity. That erases which watchdog fired at the terminal record; downstream consumers already map the specific codes, so preservation is behavior-neutral for classification.Consequences:
kilo_output_limitshares the existingwrapper_livenessreason bucket, so dashboards keep their current taxonomy while gaining the specific code. Detection is deliberately narrow (root session only; skipped while finalizing). Rollback skew is a known gap: an old worker receiving an unknownfailureCodedrops the terminal frame, and the watchdog terminalizes ~90s later with a generic code; ingest schema hardening for that case is a noted follow-up, not part of this PR.Verification
No manual verification performed. The wrapper detection path is covered by a unit suite (
test/unit/wrapper/terminal-failure-attribution.test.ts) and the Worker paths by extended supervisor/ingest unit tests; the local fake-LLM e2e harness (services/cloud-agent-next/test/e2e) is manual and was not run.Visual Changes
N/A
Reviewer Notes
failure_codeistext().$type<CloudAgentSessionRunFailureCode>(), so the union extension is TS-only.failureCodeparsing in the ingest schema for rollback skew.