fix(cloud-agent-next): preserve classified assistant terminal failures - #4659
fix(cloud-agent-next): preserve classified assistant terminal failures#4659eshurakov wants to merge 1 commit into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Executive Summary
Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (30 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 05739c0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 05739c0)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (34 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit 07d0a00)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (34 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit cbd5b8e)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe classifier and diagnostic-safety guarantees hold up across all reviewed paths (no raw gateway bodies reach logs/reports, migration is a safe drop-only change, SQLite prefix substr replacement is parameterized and equivalent to the old LIKE match); the only finding is a low-risk over-matching regex in the new negative-balance heuristic. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (27 files)
Reviewed by claude-sonnet-5 · Input: 74 · Output: 22.3K · Cached: 2.7M Review guidance: REVIEW.md from base branch |
Squashed from five branch commits: - Preserve classified assistant terminal failures (payment_required, model_missing) through to PostgreSQL reporting with a shared terminal classifier, bounded wrapper health diagnostics, and a literal substr failure lookup replacing the SQLite LIKE prefix match - Design and implement reliable e2e health checks: warm-reuse preparation classification, bounded stable-absence cleanup verification, created-session tracking, and raised dev sandbox max_instances for the full 15-row matrix - Destroy isolated sandboxes when deleting their only session - Require account context for negative-balance credit detection - Align e2e health design quiescence window with implementation
05739c0 to
8d9dd19
Compare
| killSandboxFamily, | ||
| listSandboxContainers, | ||
| waitForSandboxFamilyGone, | ||
| removeSandboxFamily, |
There was a problem hiding this comment.
CRITICAL: killSandboxFamily and waitForSandboxFamilyGone are removed from this import but are still called later in the file
The "clean stale sandbox containers from previous runs" block (around lines 136-137, unchanged by this diff) still calls killSandboxFamily(container) and waitForSandboxFamilyGone(container, 30_000), but this import statement no longer imports either symbol (only removeSandboxFamily, sandboxFamilyKey, and waitForSandboxCleanupQuiescence are now imported from ./sandbox-control.js). This will throw a ReferenceError on module load/execution whenever stale containers exist from a prior run — a common occurrence for this harness — crashing smoke.ts before the matrix even starts. Both symbols are still exported from sandbox-control.ts, so re-adding them to this import should fix it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| while (true) { | ||
| const containers = await listSandboxContainers(executeDocker); | ||
| const observedAt = now(); | ||
| if (observedAt > deadline) return false; |
There was a problem hiding this comment.
WARNING: Deadline check runs before the stability check, causing a false-negative on the exact tick quiescence is reached
In waitForSandboxCleanupQuiescence, if (observedAt > deadline) return false; executes at the top of every loop iteration, before the postBaselineSandboxes.length === 0 / observedAt - absentSince >= stableMs check below. If the stability window (stableMs) is satisfied on the same poll tick that also crosses deadline, the function still returns false, declaring cleanup "unstable" even though the required continuous-absence window was actually met. This is a source of rare flaky failures in the smoke harness when cleanup finishes right at the timeout boundary. Consider checking stability before the deadline short-circuit (or checking deadline only after evaluating postBaselineSandboxes).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| { failureStage: 'agent_activity', failureCode: 'model_missing' }, | ||
| { failureStage: 'agent_activity', failureCode: 'wrapper_error_after_activity' }, | ||
| { failureStage: 'agent_activity', failureCode: 'payment_required' }, | ||
| { failureStage: 'agent_activity', failureCode: 'model_missing' }, |
There was a problem hiding this comment.
SUGGESTION: Duplicate classification entries
Lines 38-39 ({ failureStage: 'agent_activity', failureCode: 'payment_required' } / 'model_missing') duplicate lines 35-36 added by the same PR. Since CloudAgentRunFailureClassifications feeds a Set-based validator, the duplicates are functionally harmless, but they look like an unintended copy-paste and should be removed for clarity.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| tsx services/cloud-agent-next/test/e2e/smoke.ts | ||
| ``` | ||
|
|
||
| The dev `SandboxSmall` capacity is 16 because this matrix creates fourteen |
There was a problem hiding this comment.
SUGGESTION: Session count doesn't match the matrix
This states the matrix creates "fourteen" isolated sessions, but DEFAULT_MATRIX in smoke.ts has fifteen entries (including the two new classified-failure-report rows added by this PR), each creating one session. The capacity math (16 vs demand) still holds, but the stated count should read "fifteen".
| The dev `SandboxSmall` capacity is 16 because this matrix creates fourteen | |
| The dev `SandboxSmall` capacity is 16 because this matrix creates fifteen |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| headers: { 'Content-Type': 'application/json' }, | ||
| const stdout = result.stdout ?? ''; | ||
| const markerIndex = stdout.lastIndexOf(responseMetadataMarker); | ||
| if (markerIndex === -1) { |
There was a problem hiding this comment.
SUGGESTION: Missing response-metadata marker silently falls back to a 200 status
If the curl -w marker is absent from stdout (e.g., truncated output or an unexpected exec transport issue), this returns status: 200 by default, masking what could actually be a failed request. Given this PR's explicit goal of surfacing wrapper failure diagnostics rather than hiding them, treating a missing marker as a diagnostic/parse failure (e.g., 500) would be more consistent with that goal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Production failure analysis (2026-07-19 → 21) found every sampled
agent_activity / assistant_errorrun was really a model-gateway rejection (402 insufficient credits, free-tier 429, managed-provider 5xx), but two lossy steps collapsed explicit wrapper codes before PostgreSQL reporting:CloudAgentSessionoverwrote every failedmessage.updatedterminal transition withassistant_error, andqueue-reports.tsremappedpayment_required/model_missingback to generic phase-based codes.classifyAssistantTerminalFailure) returns a structured failure code plus bounded safe message, used by bothmessage.updatedand fatal-wrapper ingest.payment_requiredandmodel_missingnow persist unchanged; everything else staysassistant_error. Credit detection no longer uses exact-string matching, so appended gateway bodies (Payment Required: {...}, negative-balance text) classify correctly — raw gateway bodies still never reach reports or logs.CloudAgentRunFailureClassificationsand drop the duplicatedcloud_agent_session_runs_failure_classification_checkconstraint (drop-only migration 0191) instead of maintaining a second copy of the classification matrix in the database.LIKE 'prefix%'with literalsubstr(entity_id, 1, length(prefix)) = prefix, removing the pattern engine implicated inLIKE or GLOB pattern too complexflush failures;%/_in prefixes are now data, not patterns.x-kilo-request-id) — never the body. The wrapper wraps/healthin a top-level error boundary returning a bounded structuredHEALTH_CHECK_FAILEDresponse carrying the same correlation ID.classified-failure-reportlifecycle scenario (payment/model) asserting terminal classification and safe diagnostic in the persisted run row, plus a fake-LLMmodel-missingscenario.Deployment order: apply migration 0191 before deploying the worker that emits the new stage/code pairs.
Verification
Visual Changes
N/A
Reviewer Notes
Automated checks run locally, all passing:
cloud-agent-nextunit tests (2233 passed; one unrelated load-sensitive git-hook test timed out once under parallel load and passes consistently in isolation), integration tests (210 passed),typecheck(tsgo + wrapper),lint(0 problems)pnpm drizzle check(migration consistency) andpackages/db/src/schema.test.ts(37 passed; note the web jest config does not discoverpackages/dbtests without arootsoverride — pre-existing, reproduced onmain)worker-utilstests (300 passed) for theCloudAgentRunFailureClassificationschangepnpm format:changed/git diff --checkcleanReviewer focus: the classifier regexes in
safe-failure-projection.ts(they only ever emit allowlisted messages), the curl-wresponse-metadata marker parsing inExecCurlWrapperTransport, and the drop-only migration.