Feat(code review) keep and map error codes - #4788
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe test now omits Files Reviewed (1 file)
Previous Review Summary (commit 68034d6)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 68034d6)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryOne new test ( Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (8 files)
Reviewed by claude-sonnet-5 · Input: 24 · Output: 3.1K · Cached: 425.3K Review guidance: REVIEW.md from base branch |
exercise the structured path
…-audit-w1-pr-safety WORKFLOW_LEARNINGS.md: both sides appended Orchestrator entries; kept all three (PR-review E2E env traps + EXITCODE false-trigger + Kilobot no-findings reading). #4788's code-review error-code changes touch only the review-status route, disjoint from this PR's surface — behavior-neutral resolution, no reruns per the base-advance rule.
Summary
cloud-agent-next already sends a structured failure object on the code review
status callback (
{ stage, code, subtype, attempts, message }), but thereceiver only read
failure.codefor a single case (workspace_setup_failedcombined with
sandbox_storage_full) and ignored it otherwise. Everything elsewas stored with a NULL
terminal_reasonand only a human-readable sentence inerror_message, so the admin Error Analysis had to recover the category bypattern-matching that sentence. Roughly a quarter of failures matched nothing
and fell into "Other".
This maps every
CloudAgentFailureCodeandWorkspaceFailureSubtypeonto aterminal reason at the callback boundary, so classification no longer round
trips through English.
terminal_reasonis an untyped text column with noconstraint, so widening the allowlist needs no migration.
Receiver side only. cloud-agent-next is unchanged. Existing rows keep their NULL
terminal_reasonand continue to use the message fallback; there is no backfill.Changes
apps/web/src/lib/code-reviews/terminal-reason-from-failure.ts, mappingall 20 failure codes and 13 workspace subtypes to a terminal reason. Both
tables use
satisfies Record<...>, so adding a code upstream withoutcategorizing it here is a type error rather than a silent fall through to
"Other".
assistant_errorby exact match on the safe messages produced byclassifyAssistantFailure. That code covers every provider failure includingrate limiting, which is the largest single bucket, so a 1:1 mapping would have
merged rate limits into a generic category and made the chart worse. Backed by
a
Maprather than an object literal because the key is caller supplied textand an object lookup for
constructoror__proto__returns a truthyinherited member.
CODE_REVIEW_TERMINAL_REASONS. Onlysetup_command_failedis marked benign, since that is the customer's ownsetup script. The rest stay as system failures so they still count toward
error spike alerting.
CloudAgentTerminalReasontype union in worker-utils with aruntime
CLOUD_AGENT_TERMINAL_REASONSarray and derive the type from it, thenbuild the zod enum in
services/code-review-infra/src/types.tsfrom thatimport. That schema ends in
.catch(undefined), so any reason missing fromits hand-maintained copy was silently coerced away and lost from the
orchestrator's in-memory state while the DB row kept the correct value.
comes from the structured payload. Both existing checks key off error text,
and a structured
model_missingfailure carries the generic "No model wasselected", so status would stay
failedinstead ofcancelledand thecustomer would see generic copy instead of the actionable message.
CASEexpressions in the admin routerinto a single
buildErrorCategoryExpr(terminalReasonColumn, errorMessageColumn),following the existing
excludeBillingErrorConditionpattern. It readsterminal_reasonfirst and keeps the message patterns underneath for olderrows.
Verification
No manual testing. The change sits on the internal status callback, which needs
a real cloud-agent-next failure of each code to exercise end to end. Covered by
unit tests instead:
apps/webaffected suites (460 tests), including 4 new callback testsasserting
wrapper_failed,assistant_rate_limited, cancelled status formodel_missing, and failed status forpayment_requiredCODE_REVIEW_TERMINAL_REASONSandCLOUD_AGENT_TERMINAL_REASONS, which live in packages that cannot importeach other
worker-utils(323) andcode-review-infra(57) suitesVisual Changes
N/A. No component or layout changes. The admin Error Analysis chart will show
new category names once failures start arriving with a structured reason, but
that needs production data to screenshot.
Reviewer Notes
sandbox_errorandinterruptedare matched last in the SQL, after themessage patterns. Both are generic buckets rather than causes, so matching
them earlier would mask the specific category on existing rows. This is the
behavior the admin router tests cover.
than bound parameters. The expression appears in both SELECT and GROUP BY, and
parameter placeholders are renumbered by position, so Postgres would stop
recognizing the two as the same expression.
a message in
classifyAssistantFailureis reworded, the map stops matching andfalls back to the generic
assistant_failed, which is no worse than currentbehavior. The cleaner fix is to carry the assistant reason in the structured
failure itself, which would be a sender side change.