feat(chat): map long-tail failure_detail values to specific run-error cards - #5329
Conversation
lefarcen
added
risk/medium
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hey @open-design-crew — the rollout context here is clear, especially the split between the earlier detail-plumbing PR and this long-tail copy/mapping pass. Before pool review picks it up, could you reshape the body to include the repo template's explicit Validation section and the full Surface area checklist so reviewers can see the user-visible scope at a glance?
Visual regression reviewHead: 0 changed · 53 unchanged · 0 new without baseline · 0 failed Unchanged cases
Visual diff is advisory only and does not block merging. |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
Blocking finding:
Location: AGENTS.md section Git commit policy, applied to the current head commit metadata for 522f28894d09df2bbd57f879518d24a01d85972a. That commit includes Co-authored-by: Cursor <cursoragent@cursor.com>, but the repository policy says commits must not include Co-authored-by trailers or any other co-author metadata. Please amend the commit to remove that trailer and force-push the same code content.
I did not find a code-level correctness issue in the changed failure-detail mapping, locale keys, or focused resolver test while reviewing the changed ranges. I could not rerun the web typecheck or focused Vitest locally because this prepared worktree has no node_modules installed (tsc/vitest were unavailable).
|
Thanks — the current blocker looks narrow. Please address @PerishCode's review on commit |
The daemon already classifies each run failure into failure_category / failure_detail (run-failure-classification.ts) for retry policy and telemetry, but kept it daemon-internal — so the chat could only key error guidance off the coarse errorCode. That collapses distinct causes that share one code: a hard quota and a transient 429 both arrive as RATE_LIMITED, and a text-detected missing CLI leaks in as the opaque AGENT_EXECUTION_FAILED. Propagate the classification to the client (contract DTO + SSE end frame, mirroring the proven `resumable` path) and let resolveRunFailureUi take `detail` as a first-class input that can override a too-coarse code: - hard_quota / workspace_credits_exhausted -> a distinct "Quota exhausted" card that drops the futile Retry (primaryAction 'none') and steers to the hosted-AMR switch card instead. - cli_not_installed (text-detected) -> the same install-the-CLI card the AGENT_UNAVAILABLE code path already renders. A transient 429 without a hard-quota detail still offers Retry, and the Antigravity terminal switch-model flow still wins over the generic detail override. Stacks on #5308 (the code->guidance mapping this detail layer refines).
The hosted model service is now Open Design Cloud, but the run-error cards this PR sharpens (switch / auth / balance), plus settings account + wallet labels, avatar console, onboarding gate, and the handoff website link, still showed the old "AMR" (or bare "Open Design") name. Normalize every user-visible brand mention to "Open Design Cloud" across all 19 locales. Value-only: no keys, agent ids, URLs, or raw daemon error strings touched.
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
I found two blockers in this pass.
Blocking finding: AGENTS.md section Git commit policy, applied to the current PR commit metadata. The commits eb9b2c994103f8de51eafb55a8885a7607d6d7eb, ae98f47e3f42ea16bf8a3ba8d64ce92c960925a3, and dec41307b03baea3a88a374a739f31ec773fd3be each still include Co-authored-by: Cursor <cursoragent@cursor.com>, but the repository policy says commits must not include Co-authored-by trailers or any other co-author metadata. Please amend the branch to remove those trailers and force-push the same code content.
There is also an inline blocker on the web streaming fallback path where the new failure classification fields are still dropped for one terminal status probe. I could not rerun the focused Vitest checks locally because this prepared worktree does not have vitest installed in node_modules.
|
Thanks — this round has two concrete blockers to clear before the PR can move again:
Also, this now carries user-visible runtime behavior on 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
lefarcen
added
size/XL
The daemon emits the terminal `error` SSE frame from the child-close
handler before a run is finalized, so `failureCategory` / `failureDetail`
(computed at finalize) weren't known when that frame was first persisted
to the assistant message — the stored `status:error` event kept only
`{ detail, code }`. A failure persisted without the live web error
handler saving the message, or a conversation reloaded before that save
landed, therefore fell back to the coarse errorCode UI and lost the
specific fix guidance this PR adds.
Stamp the finalize-time classification onto the persisted assistant
message: enrich the last `status:error` event in place (append one only
if a failed run somehow never persisted an error frame). Covered by a
daemon regression that reads the stored assistant message via the
conversation-messages API after a failed hard-quota run.
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
I found one blocking issue in the streaming fallback path. The new failure classification fields are still dropped in the error-frame recovery status probe, so the long-tail cards can regress to the generic error UI on that path.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
|
Thanks — the commit-trailer cleanup looks resolved on the current head The remaining blocker is still @PerishCode's streaming-fallback review on this head: the error-frame recovery path needs to preserve |
…recovery paths Two live-propagation gaps let the sharpened run-error card degrade back to the coarse/raw UI even though the daemon now sends the classification: - appendErrorStatusEvent() no-op'd whenever the message already had a trailing status:error with the same detail. In the reload/reattach race (reload reads the daemon-persisted error frame first, then the run finishes and onError arrives with the finalize-time classification) that dropped the new code / failureCategory / failureDetail. Merge those into the existing event in place instead; no-op only when nothing is added. - The error-frame-then-status recovery probe in providers/daemon.ts broke out on a terminal failed status without copying failureCategory / failureDetail (unlike the post-loop status fallback), so markErrorRunFailure stamped null and the failureDetail mapping fell back to the generic card on the reconnect path. Mirror the two assignments before the break. Covered by chat-events (in-place merge) and daemon-sse-reconnect (error frame + terminal status fetch, no end frame) regressions.
… cards #5321 pipes the daemon's failure_detail to the chat but only mapped the high/medium-frequency causes (hard_quota, workspace_credits, cli_not_installed). This wires the recoverable long tail the daemon already classifies — timeout, inactivity_timeout, empty_output, session_resume_expired, git_bash_missing — each to a named failure type + one-line fix with a plain Retry. These are engine-neutral outcomes, so they resolve in a new AGENT_AGNOSTIC_DETAIL_FAILURE_UI layer BEFORE the AMR/Antigravity agent branches (which would otherwise return a generic retry for AMR). Adds the title/message i18n keys across all 19 locales and unit coverage for the new mappings.
|
Thanks — we see the current head From here the blocker is in reviewer hands: once @PerishCode re-reviews this head, we can see whether anything else is still outstanding. The |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current head against the changed ranges, including the daemon status/SSE failure classification plumbing, persisted assistant-message enrichment, web error-event propagation, failure-detail UI mapping, locale keys, shared contracts, and the follow-up reconnect coverage for the prior streaming fallback blocker. The previous blocker is addressed on this head, and I did not find any remaining actionable correctness or maintainability issues in scope. Nice work closing the long-tail failure guidance and adding focused coverage around the recovery paths.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
I found one blocking issue in the current head metadata.
Blocking finding: Location: AGENTS.md section Git commit policy, applied to the current head commit metadata for 76489621f51be8387a5d813029dde4ba6d999d79. The merge commit body includes Co-authored-by: Cursor <cursoragent@cursor.com>, but the repository policy says commits must not include Co-authored-by trailers or any other co-author metadata. Please amend the head commit to remove that trailer and force-push the same code content.
I did not find a code-level correctness issue in the changed long-tail failure-detail mapping, locale keys, or focused resolver test while reviewing the changed ranges. I could not rerun the focused web Vitest locally because this prepared worktree does not have vitest installed in node_modules.
|
Thanks — the current blocker is @PerishCode's latest review on head Once that commit metadata is cleaned up, this can go back for reviewer re-check. |
Resolve i18n and run-failure guidance conflicts while preserving both the long-tail error cards and current main behavior.
7648962 to
2386ac6
Compare
|
The blocking finding is addressed on the new head |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current head against the changed ranges, including the long-tail failureDetail resolver mapping, the new title/message i18n keys across all locale files, and the focused amr-guidance coverage. The previous commit-metadata blocker is resolved on this head, the new details resolve before the generic AMR/agent fallbacks as intended, and I did not find any remaining actionable correctness or maintainability issues in scope. Nice work closing out the long-tail failure cards with clear copy and targeted coverage.
|
QA validation pass on head
Removing |
lefarcen
removed
the
needs-validation
…nt (nexu-io#5519) A PR ejected from the merge queue by the needs-validation gate leaves no trace on the PR itself: the gate fails on the queue's transient ref, the PR's own checks stay green, and mergeQueueEntry silently goes null. The only way to learn why the PR fell out of the queue is to dig through merge_group workflow runs, which nobody watches (observed on nexu-io#5329). Producer: the merge_group gate in ci.yml now emits a handoff/comment artifact targeting the labeled PR right before it blocks, and uploads it on the failure path. Consumer: comment.atom.yml accepts merge_group workflow_run events; since a merge_group run's head_sha is the queue's synthetic merge commit, artifacts from those runs are bound to their producing run by run_id, and the base-freshness skip is waived (PRs ahead in the queue move the base while the run completes). The live-PR head check still guards staleness. Topology coverage pins the new boundary: comment.atom consumes pull_request + merge_group; autofix/report stay pull_request-only.
2 participants
Part of #895. Stacked on #5321 (base =
feat/run-failure-detail-surface-895); review/merge that first, then this retargets tomain.Why
#5321 taught the chat to consume the daemon's fine-grained
failure_detail, but only wired the high/medium-frequency causes (hard_quota,workspace_credits_exhausted,cli_not_installed). The daemon already classifies a long tail of other recoverable outcomes that still fell through to the opaque "task failed" card showing only raw stderr. Now that the detail is on the frontend, closing this tail is just a mapping + copy — the cheap finish the #895 strategy deferred.What users will see
A failed run caused by one of these now shows a named failure type + a one-line fix instead of the raw error text, with a plain Retry:
timeout,inactivity_timeout) — "took too long / went quiet, retry (optionally narrow the task)".empty_output) — "finished without output, usually temporary, retry".session_resume_expired) — "the resumed session expired and was reset, retry to start fresh".git_bash_missing, Windows) — "install Git for Windows, then retry".How
AGENT_AGNOSTIC_DETAIL_FAILURE_UImap inapps/web/src/runtime/amr-guidance.ts. These are engine-neutral outcomes, so they resolve before the AMR/Antigravity agent branches (unlike the quota/credits overrides, which must defer to those agents' own flows) — otherwise an AMR run would fall through to a generic retry.apps/web/src/i18n/types.ts+locales/*).apps/web/tests/runtime/amr-guidance.test.tsasserting each detail maps to its named type + retry for every agent (claude/codex/amr/null).Test plan
pnpm --filter @open-design/web typecheck(validates all 19 locales carry the new keys)pnpm --filter @open-design/web exec vitest run tests/runtime/amr-guidance.test.ts— 19/19 passpnpm guardSurface area
Pre-existing broad web-suite failures (ChatComposer/lexical/jsdom) are unrelated to this change and also fail on the base.