feat(chat): surface daemon failure_detail to sharpen run-error guidance - #5321
Conversation
|
The contract → daemon → web propagation path is easy to follow here, especially the way you kept it aligned with the existing |
ScreenshotsRendered with the real
|
|
Thanks for adding the screenshots — that closes the visual gap from earlier. The main blocker now is the persistence path @nettee called out: 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
|
截图已更新:托管服务改名为 Open Design Cloud(原 AMR)。切换卡文案现为「切换到 Open Design Cloud 并重试」/「可切换到 Open Design Cloud 官方模型服务」。上方图片链接指向同名文件,已随 |
mrcfps
left a comment
There was a problem hiding this comment.
@app/open-design-crew Thanks for the clear propagation work here; the status/SSE fields and client-side mapping are easy to follow. I rechecked the current head and the blocking persistence gap from the existing unresolved thread is still present: packages/contracts/src/api/chat.ts now allows persisted status:error events to carry failureCategory / failureDetail, but apps/daemon/src/server.ts runSseEventToPersistedAgentEvent() still serializes daemon error SSE frames as only { kind: 'status', label: 'error', detail }. Because daemon-side message persistence uses that producer, a failed run that completes without the live web error handler saving the message, or a conversation reloaded before that save happens, will lose the new classification and fall back to the coarse/raw error UI. Please persist the structured error code plus the finalize-time failureCategory / failureDetail on the daemon-owned assistant-message event path, and cover it with a regression that reads the stored assistant message after a failed hard-quota/CLI-missing run.
|
Thanks — the current head is still blocked on the same daemon-owned persistence gap, and @mrcfps has now confirmed it on recheck as well. Once |
|
@app/open-design-crew I'm holding off on generating review comments for #5321 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
|
🧪 This PR changes user-visible run-error behavior, so it will need a manual QA pass before merge. It also has conflicts with |
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.
nettee
left a comment
There was a problem hiding this comment.
Checked the current ae98f47 head across the daemon/contracts/web diff. One blocking persistence gap remains in the new failure-detail propagation; details inline.
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.
nettee
left a comment
There was a problem hiding this comment.
Checked the current 6d521e7 head end-to-end across the daemon/contracts/web changes. One blocking issue remains in the live web propagation path; details inline.
|
Thanks for the detailed write-up — that sounds like the right fix for the daemon-owned persistence path, and the new persisted-message regression is exactly the missing coverage reviewers were asking for. Once the refreshed checks land, the next useful step is a fresh pass from the blocking reviewers on |
|
Thanks — it looks like the daemon-side persistence piece is in, and the remaining blocker has moved to the live web reattach path @nettee just called out. The next fix needs |
…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.
mrcfps
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current f4eced7 head across the daemon status/SSE propagation, persisted assistant-message enrichment, web reattach/error merging paths, failure-detail UI mapping, contracts, and the focused regression coverage. The previous persistence and reload/reattach blockers appear addressed on this head; git diff --check origin/main...HEAD is clean. I could not execute the focused Vitest suites in this prepared worktree because node_modules is missing (vitest was not available), so this approval is based on static review plus the added test coverage in the diff. Thanks for iterating on the edge cases and tightening the failure guidance path. 🙂
nettee
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current f4eced7 head across the daemon status/SSE propagation, daemon-side persisted-message enrichment, web reload/reattach error merging, failure-detail UI mapping, and the new focused regression coverage. The previously blocking persistence and same-detail reattach gaps appear addressed in the changed ranges, and git diff --check origin/main...HEAD is clean. I could not execute the focused test suites in this prepared worktree because node_modules is missing, so this approval is based on static review plus the added targeted tests in the diff. Thanks for tightening the failure guidance path and covering the reload edge cases.
|
Thanks for tightening the live web path on |
|
🧪 Queued for QA validation — this head changes the run-error / reload guidance path in a way that needs a manual pass before merge. Nothing needed from you on this step; we’ll update here once QA has gone through the failure-detail flows. |
Keep PR failure-guidance i18n wording and incorporate main's settings.amrModelUpgradeHint across conflicted locale files. Co-authored-by: Cursor <cursoragent@cursor.com>
nettee
left a comment
There was a problem hiding this comment.
Found one blocking issue in the updated web regression coverage; details inline.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/web/tests/runtime/amr-guidance.test.ts RIGHT line 192
This test still calls resolveRunFailureUi with the old two-argument shape, so after the new detail parameter was inserted it now passes 'amr' as detail and leaves agentId undefined. That means this case falls through to the generic retry branch instead of the AMR upgrade branch, so the focused regression suite described in the PR will fail once apps/web/tests/runtime/amr-guidance.test.ts is executed. Please update this call to resolveRunFailureUi('AMR_TIER_UPGRADE_REQUIRED', null, 'amr') so the test is actually exercising the AMR entitlement path again.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
mrcfps
left a comment
There was a problem hiding this comment.
@app/open-design-crew Thanks for continuing to tighten the failure-detail propagation path. I found one current-head blocker in the web unit coverage after the resolveRunFailureUi signature change: an existing AMR tier-upgrade case is still calling the helper with the old two-argument shape, so it now passes amr as the detail instead of the agent and should fail. Details inline.
|
Thanks — the two current-head blocking reviews are pointing at the same remaining issue on Updating that case to |
The main merge kept a two-argument call site that passed agentId as detail after the signature gained an explicit detail parameter. Co-authored-by: Cursor <cursoragent@cursor.com>
mrcfps
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current 14bc861 head across the daemon status/SSE propagation, daemon-side persisted-message enrichment, web reload/reattach error merging, failure-detail UI mapping, and the focused regression coverage. The earlier persistence, reattach, and stale resolveRunFailureUi call-shape blockers are addressed in the current changed ranges; git diff --check origin/main...HEAD is clean. I could not execute the focused Vitest suites in this prepared worktree because node_modules is missing, so this approval is based on static review plus the added targeted tests in the diff. Thanks for the careful iteration on the failure guidance path. 🙂
nettee
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current 14bc861 head across the daemon status/SSE propagation, daemon-side persisted-message enrichment, web reload/reattach error merging, failure-detail UI mapping, and the focused regression coverage. The earlier persistence, reattach, and stale resolveRunFailureUi call-shape blockers are addressed in the current changed ranges, and git diff --check origin/main...HEAD is clean. I could not execute the focused Vitest suites in this prepared worktree because node_modules is missing, so this approval is based on static review plus the added targeted tests in the diff. Thanks for tightening the failure guidance path and covering the reload edge cases.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@xiaoche-hub Rechecked the current head and the blocker matches your note: GitHub still isn't reporting a fresh Once that required check is actually run on this head, the PR should be back in shape for queue/merge. |
Update unit and e2e selectors that still expected pre-rebrand strings for account status, handoff website link, and switch-and-retry CTA. Co-authored-by: Cursor <cursoragent@cursor.com>
nettee
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current fda7539 head across the failure-detail propagation path, the persisted-message/reload recovery flow, and the final follow-up diff since the last clean pass. The earlier blockers remain addressed in the changed ranges, and this head’s additional changes are limited to aligning the AMR/Open Design Cloud test assertions with the current copy. I could not execute the focused suites in this prepared worktree because node_modules is absent, so this approval is based on static review plus the targeted regression coverage already in the diff. Thanks for tightening the edge cases and cleaning up the remaining test drift.
mrcfps
left a comment
There was a problem hiding this comment.
@app/open-design-crew I reviewed the current fda7539 head across the daemon status/SSE failure-detail propagation, daemon-side persisted-message enrichment, web reload/reattach error merging, failure-detail UI mapping, contract changes, i18n keys, and the final Open Design Cloud test assertion updates. The earlier persistence, reattach, and stale resolveRunFailureUi call-shape blockers remain addressed in the changed ranges, and git diff --check origin/main...HEAD is clean. I could not execute the focused Vitest or Playwright suites in this prepared worktree because node_modules is missing, so this approval is based on static review plus the targeted regression coverage in the diff. Thanks for the careful iteration on this failure-guidance path.
|
QA 验收记录(head 验证范围:
本地验证:
GitHub 状态:
结论:验收通过。建议将 |
AmyShang-alt
left a comment
There was a problem hiding this comment.
QA accepted on head fda7539. Focused daemon/web regressions and current GitHub CI are green; needs-validation has been moved to validated.
















































Why
Author's use case: while wiring #5308 (mapping
error_code→ a named failure type + fix for #895), I hit causes the code alone can't tell apart. The daemon already computes a much finer classification —failure_category/failure_detailinrun-failure-classification.ts— but it never leaves the daemon (used only for retry policy + telemetry). So the chat is stuck guessing from a coarse code.The pain: distinct failures collapse onto one code and get the wrong advice:
RATE_LIMITED. feat(web): surface actionable run-failure guidance for common errors #5308 shows "wait and retry" for both — but retrying a hard quota is futile and just reproduces the failure.AGENT_EXECUTION_FAILED, so it can't be routed to the "install the CLI" card.This PR closes that gap by giving the client the classification the daemon already has.
What users will see
How it works
Mirrors the already-proven
resumablepropagation path end-to-end:failureCategory/failureDetailtoChatRunStatusResponse, the persistedstatus:errorevent, andChatSseEndPayload(reusing the analyticsTrackingRunFailure*unions so producer/consumer can't drift).run.failureCategory/Detailat the same finalize point that setsrun.resumable; emit them instatusBody+ the SSEendframe.ProjectView/useConversationChatpersist it onto the assistant message;resolveRunFailureUi(code, detail, agentId)applies aDETAIL_FAILURE_UIoverride layer (after the AMR/Antigravity agent branches, before the generic code branches) sodetailcan correct a too-coarse code.Tests
run-failure-detail-status.test.ts): a hard-quota run's/api/runs/:idstatus now surfacesfailureCategory: 'rate_limit'+failureDetail: 'hard_quota'. Red onmain/feat(web): surface actionable run-failure guidance for common errors #5308 (fields absent from the DTO), green here.amr-guidance.test.ts): hard_quota / workspace_credits override RATE_LIMITED to a no-Retry "Quota exhausted" card; a transient 429 keeps Retry;cli_not_installedroutes to the install-CLI card; Antigravity's switch-model flow still wins over the detail override.pnpm guard,pnpm typecheck, and the adjacent daemon suites (resume-on-failure, sse-response, run-failure-classification) all pass.Surface area
packages/contracts) — new optional DTO / SSE fields.endframe.chat.runError.title.quotaExhausted,quotaExhaustedMessage,workspaceCreditsMessageacross all 19 locale files.odrun-status JSON now carries the samefailureCategory/failureDetailvia the shared DTO, so the machine-readable surface stays in parity with the UI.Screenshots
TODO: attach the "Quota exhausted" (no-Retry, switch-card) and text-detected "Agent not installed" cards from the running web app.