fix(ui): clarify safe-recovery failure and pre-progress turn status (#1358) - #1366
Merged
Conversation
Part of #1358. When safe recovery exhausts its budget after a completed side-effecting tool (e.g. a posted GitHub comment), the turn previously ended with a weak grey caption while the prominent item stayed the completed tool card — reading as if the tool were still stuck. - notice.tsx renders a separated, titled status row (stroked icon + ink title + explanation), not the old single-line caption. No semantic colour flood: the operation usually succeeded, so it must not read as an error. - Copy adapts to context: a completed tool earlier in the turn means an external side effect already landed, so the notice reassures it is done ("Action completed" / 操作已完成) and does not nudge a redo; otherwise the reply simply never started ("Reply incomplete" / 回复未完成). Both attribute the failure to the network or model provider (not PawWork) and give a next step (retry / switch models). - i18n: split the flat safeRetryFailed key into sideEffect/default title+body for en/zh/zht; drop the stale flat key from the dormant locales (runtime loads en + zh, others fall back to en). - session-safe-retry-contract.test.ts rewritten to lock the new presentation and copy intent. - recovery-presentation snap (fixture + target) renders both cases through the real AssistantParts → tool card + notice pipeline as durable regression coverage. Visual check: bun run snap recovery-presentation (docs/design/preview/screenshots/recovery-presentation.png). Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
Before the provider sends its first chunk, the turn showed "Thinking" / "思考中" even though the model had not started responding — building the request, connecting, or waiting for the stream to be accepted all read as reasoning (#1358). Split the turn status by whether any provider-output part (text / reasoning / tool — the UI mirror of the backend isProviderProgressEvent set) exists yet: - no such part: "Connecting" / "连接中" (data-phase="connecting") - after the first one: "Thinking" / "思考中" (data-phase="thinking") A step-start part is intentionally excluded; it can precede the first provider chunk and would otherwise make a connection wait read as reasoning. Safe recovery now names the retry attempt ("Recovering... attempt #N" / "正在恢复…第 N 次") so the row reads as model-recovery progress, not a stuck tool. Terminal failure before first progress is already covered by the safe_retry_failed notice ("Reply incomplete" / "回复未完成"). UI-only: the assistant message carries zero parts until the first provider progress event creates one, so no backend status/schema change is needed. Verify: - packages/ui: typecheck, full unit suite (732 pass), lint clean - packages/app: typecheck - e2e @smoke W1 connecting indicator (real session-turn memo, hang path) passed - snap turn-status-phase grid reviewed (连接中 / 思考中 / 正在恢复…第 2 次) Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThe PR splits the session turn "connecting" phase from "thinking" by adding a ChangesConnecting Phase Split + Side-Effect-Aware Safe-Retry Notice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…pleted" Independent review (#1358) found the `afterToolRun` predicate over-claimed: any completed tool — including a read-only read/glob/grep/webfetch/tool_info — flipped the notice to "Action completed / 操作已完成". So a turn that only ran a grep before the stream dropped would falsely assert an external action landed, the exact honesty defect the side-effect copy exists to avoid. Gate on side-effecting tools only: exclude the backend READ_ONLY_TOOLS set (mirrors run-observability/sanitize.ts). Everything else — bash, apply_patch, or an unknown/custom tool — still counts, erring toward reassurance so a real side effect is never under-warned (a redo would repeat it, which #1358 calls out). Renamed the memo to `afterSideEffectingTool` for accuracy. The recovery-presentation snap gains a read-only column (completed grep) that proves the notice falls back to the default "回复未完成", not "操作已完成". Verify: - packages/ui: typecheck clean, full unit suite 732 pass, lint clean - packages/app: typecheck clean - snap recovery-presentation: 3 columns reviewed (side-effect 操作已完成 / read-only 回复未完成 / no-tool 回复未完成) Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
Independent review (#1358) found the side-effect body said "Try again later / 请稍后重试" right after "the previous action already went through". After a real side effect, that nudges the user to repeat it (re-comment, re-run a command) — the exact thing #1358 says to avoid. Side-effect copy now says the action already ran and must not be repeated, and points the next step at regenerating the *reply* (or switching models), never a plain retry. The default body is unchanged: nothing landed there, so "请稍后重试" stays safe. zh copy is the user's wording; en/zht mirror it. Full-width CJK punctuation. Verify: - packages/ui: typecheck clean, full unit suite 732 pass (contract test now locks the no-redo side-effect copy and the safe default retry) - snap recovery-presentation: side-effect column reads "无需重复 … 可稍后重新生成回复" Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
… test The e2e-smoke-tagging inventory locks the exact set of @smoke test titles. Renaming the W1 test ("thinking" → "connecting indicator", #1358) left the inventory stale, failing unit-opencode (and the unit-windows-opencode-* and the `check` aggregate gate). Updated the entry to the new title and kept the list in sorted order (connecting sorts before rendered). Verify: packages/opencode `bun test test/config/e2e-smoke-tagging.test.ts` — 2 pass, 0 fail. Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
github-actions
Bot
added
the
harness
…1358) The post-tool model continuation runs as a NEW assistant message (the turn loop creates one message per step), so a completed side-effecting tool and the trailing safe_retry_failed notice land on DIFFERENT messages of the same turn. The UI sees one part at a time and cannot scan sibling messages, so the earlier UI-side scan of the notice's own message could never see the tool — it would always fall back to the default copy even after bash/apply_patch ran. Make the backend the single source of truth: when writing the notice, scan the whole turn via turnHasCompletedSideEffect() and stamp a `sideEffect` flag on the NoticePart. The UI now just reads the field — no useData, no part scan, no READ_ONLY_TOOLS duplication. Side-effect classification stays in one place (RunObservability.toolEffect): bash/apply_patch/unknown count as side effects, read-only tools do not. - opencode: new pure helper safe-retry-notice.ts (+6 unit tests covering the cross-message, read-only, no-tool, still-running, unknown-tool, and different-turn cases); processor stamps sideEffect; NoticePart schema gains the optional field. - sdk: regenerate NoticePart.sideEffect (scoped 1-line). - ui: notice.tsx reads part().sideEffect; contract test asserts the field drives the copy and the old UI scan is gone. - snap: recovery-presentation rebuilt as a real cross-message topology, zh/en side by side (中英对照). Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app/e2e/snap/recovery-presentation.snap.ts (1)
14-19: 💤 Low valueConsider extracting the repeated 30-second timeout to a named constant.
The timeout value
30_000appears four times across the helper functions and assertions. Extracting it would improve maintainability if the timeout needs adjustment in the future.♻️ Optional refactor
+const SNAPSHOT_READY_TIMEOUT = 30_000 + async function waitForThemeBoot(page: Page): Promise<void> { await page.waitForFunction( () => getComputedStyle(document.documentElement).getPropertyValue("--bg-base").trim().length > 0, null, - { timeout: 30_000 }, + { timeout: SNAPSHOT_READY_TIMEOUT }, ) } async function capture(name: string, block: Locator): Promise<Shot> { - await expect(block).toBeVisible({ timeout: 30_000 }) + await expect(block).toBeVisible({ timeout: SNAPSHOT_READY_TIMEOUT }) return { name, buf: await block.screenshot() } }Then use
SNAPSHOT_READY_TIMEOUTin the remaining assertions on lines 44 and 61.Also applies to: 44-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/e2e/snap/recovery-presentation.snap.ts` around lines 14 - 19, Extract the hardcoded timeout value 30_000 into a named constant at the top of the file (for example, SNAPSHOT_READY_TIMEOUT) and replace all four occurrences of the timeout value throughout the helper functions (in the capture function's expect call and in other timeout assertions) with this constant reference. This will make the code more maintainable and allow for easier adjustments to the timeout value in the future.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/app/e2e/snap/recovery-presentation.snap.ts`:
- Around line 14-19: Extract the hardcoded timeout value 30_000 into a named
constant at the top of the file (for example, SNAPSHOT_READY_TIMEOUT) and
replace all four occurrences of the timeout value throughout the helper
functions (in the capture function's expect call and in other timeout
assertions) with this constant reference. This will make the code more
maintainable and allow for easier adjustments to the timeout value in the
future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de903660-6aa8-4575-a019-6c129425c95a
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (33)
packages/app/e2e/session/session-w1-contracts.spec.tspackages/app/e2e/snap/fixtures/recovery-presentation-snap-fixture.tsxpackages/app/e2e/snap/fixtures/turn-status-phase-snap-fixture.tsxpackages/app/e2e/snap/recovery-presentation.snap.tspackages/app/e2e/snap/turn-status-phase.snap.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/session/processor.tspackages/opencode/src/session/safe-retry-notice.test.tspackages/opencode/src/session/safe-retry-notice.tspackages/opencode/test/config/e2e-smoke-tagging.test.tspackages/ui/src/components/message-part/parts/notice.csspackages/ui/src/components/message-part/parts/notice.tsxpackages/ui/src/components/session-retry.tsxpackages/ui/src/components/session-safe-retry-contract.test.tspackages/ui/src/components/session-thinking-phase-contract.test.tspackages/ui/src/components/session-turn.tsxpackages/ui/src/i18n/ar.tspackages/ui/src/i18n/br.tspackages/ui/src/i18n/bs.tspackages/ui/src/i18n/da.tspackages/ui/src/i18n/de.tspackages/ui/src/i18n/en.tspackages/ui/src/i18n/es.tspackages/ui/src/i18n/fr.tspackages/ui/src/i18n/ja.tspackages/ui/src/i18n/ko.tspackages/ui/src/i18n/no.tspackages/ui/src/i18n/pl.tspackages/ui/src/i18n/ru.tspackages/ui/src/i18n/th.tspackages/ui/src/i18n/tr.tspackages/ui/src/i18n/zh.tspackages/ui/src/i18n/zht.ts
💤 Files with no reviewable changes (14)
- packages/ui/src/i18n/pl.ts
- packages/ui/src/i18n/th.ts
- packages/ui/src/i18n/ko.ts
- packages/ui/src/i18n/es.ts
- packages/ui/src/i18n/no.ts
- packages/ui/src/i18n/ja.ts
- packages/ui/src/i18n/ru.ts
- packages/ui/src/i18n/tr.ts
- packages/ui/src/i18n/ar.ts
- packages/ui/src/i18n/de.ts
- packages/ui/src/i18n/da.ts
- packages/ui/src/i18n/fr.ts
- packages/ui/src/i18n/bs.ts
- packages/ui/src/i18n/br.ts
… grep (#1358) Address the P3 review note: the contract test asserted the notice's behavior by grepping its source for `part().sideEffect`, "no useData", "no READ_ONLY_TOOLS", etc. — brittle, and it never actually rendered anything. Replace that block with a real render (the repo's Vite-SSR fixture pattern): mount the notice with ONLY the backend `sideEffect` flag — no tool part, no DataProvider, no turn context — and assert the rendered copy: - sideEffect=true → "Action completed" / 操作已完成, variant=side-effect, no redo - sideEffect=false → "Reply incomplete" / 回复未完成, variant=default - sideEffect=undefined (older notices) → safe default Because nothing tool-shaped is ever mounted, a correct copy proves the UI reads the field alone and never scans or classifies tools. The same render covers the zh locale, so a copy regression in either variant fails the test. The contract file keeps only the locale-content guards (no-redo wording, en/zh/ zht parity, old flat key removed) and the retry status-row check; the brittle component source grep is gone. Claude-Session: https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two presentation fixes for the safe-recovery / pre-progress turn (runtime behavior was already correct). Three independent review passes (Codex + two subagents) drove three further correctness fixes — the last a P1 cross-message topology bug (see Review).
Terminal
safe_retry_failednotice — replaced the weak single-line caption with a separated, titled status row (stroked icon + ink title + explanation). Copy adapts to context, driven by a backend-stampedsideEffectflag on the notice:The classification lives in one place on the backend (
turnHasCompletedSideEffect→RunObservability.toolEffect): bash/apply_patch/unknown count as side effects, the read-only set (read/glob/grep/webfetch/tool_info) does not, erring toward reassurance so a real side effect is never under-warned. The UI just readspart.sideEffect.Pre-first-provider-progress status — the turn no longer says Thinking / 思考中 while it is still connecting. Split by whether any provider-output part (text / reasoning / tool — the UI mirror of the backend
isProviderProgressEventset) exists yet:data-phase="connecting")data-phase="thinking")Safe recovery now names the attempt: Recovering... attempt #N / 正在恢复…第 N 次.
Why
#1358: after a side-effecting tool (e.g. a posted GitHub comment) completed, the model continuation failed before first provider progress and safe recovery exhausted its budget. The runtime state was correct, but the UI made it look like the previous tool card was still stuck, and the pre-progress wait was mislabeled "Thinking" even though no provider progress had been observed. The goal is honest, reassuring presentation: say the operation already happened, attribute the failure outside the harness (network / provider), don't nudge a redo of the side effect, and don't render a connection wait as model reasoning.
Cross-message topology (the P1 fix): the turn loop creates a new assistant message per step, so the side-effecting tool completes in one step's message while the trailing
safe_retry_failednotice lands on the next (failed-continuation) message. A UI scan of the notice's own message can therefore never see the tool — it would always fall back to the default copy. Only the backend, which can scan the whole turn at notice-write time, can classify this reliably; it stampssideEffectonto the notice and the UI trusts the field.A
step-startpart is intentionally excluded from the "provider started" signal: it can precede the first provider chunk. The assistant message carries zero provider parts until the first progress event creates one, so no backend status change is needed for fix #2.Related Issue
Closes #1358
Human Review Status
Pending
Review
Three independent review passes (Codex
codex exec+ two separate subagents), all run against the branch. They converged on three real issues, now fixed:turnHasCompletedSideEffectscans the whole turn) and stamped asideEffectflag on theNoticePart; the UI now reads the field — nouseData, no part scan, noREAD_ONLY_TOOLSduplication. The recovery snap was rebuilt to the real cross-message topology (tool on message A, notice on message B) to prove it.grepfalsely showed "操作已完成". Fixed to side-effecting tools only (read-only set excluded), with a read-only column in the snap proving the fallback to "回复未完成".part().sideEffect, "nouseData", "noREAD_ONLY_TOOLS") without rendering anything. Converged into a real render (notice-render.test.tsx, the Vite-SSR fixture pattern): the notice is mounted with only thesideEffectflag — no tool part, noDataProvider— and the rendered copy is asserted (true → 操作已完成 no-redo, false/undefined → 回复未完成, plus the zh locale). Because nothing tool-shaped is ever mounted, a correct copy proves the UI reads the field alone and never classifies tools. The contract file keeps only locale-content/parity guards.All reviewers confirmed the connecting↔thinking equivalence is airtight (multi-step reconnect, reasoning-only, tool-only all correct) and that the 4th #1358 state ("model did not start responding") is adequately carried by the "Reply incomplete" notice. Consciously accepted:
zhtis not loaded at runtime but kept consistent.Review Focus
turnHasCompletedSideEffect(packages/opencode/src/session/safe-retry-notice.ts): the whole-turn scan, the read-only exclusion, and the "err toward reassurance for unknown tools" choice. Stamped inprocessor.tsat notice-write time.providerStartedinsession-turn.tsx: mirrors the backendisProviderProgressEventset; excludesstep-start.Risk Notes
Fix #1 now spans backend + SDK + UI: an optional
sideEffectfield onNoticePart(schema + scoped 1-line SDK regen), a pure backend helper, and the processor stamping it. The field is optional, so existing notices and persisted sessions parse unchanged. No recovery-policy, status-schema, or provider-behavior change. Fix #2 is UI-only. The pre-progress and side-effect signals are validated against the backend's own classifications and exercised by the @smoke W1 hang path, the new backend unit tests, and the recovery snap.How To Verify
```text
opencode unit (processor-effect end-to-end + message-v2 schema + safe-retry-notice helper): 122 pass, 0 fail
└ safe-retry-notice.test.ts: 6 pass (cross-message side-effect=true, read-only=false, no-tool=false, still-running=false, unknown-tool=true, different-turn=false)
└ processor-effect.test.ts still writes the notice through the real processor path with the new field — green
packages/ui typecheck: clean (tsgo --noEmit)
packages/app typecheck: clean (tsgo -b)
packages/opencode typecheck: clean
packages/ui unit suite: 735 pass, 0 fail (notice-render.test.tsx renders the notice from the sideEffect field alone — no tool/DataProvider — and asserts the en+zh copy per variant; contract file keeps locale-parity guards only)
lint (changed src): 0 errors
e2e @smoke W1 connecting indicator (real session-turn memo, assistant.hang path): 1 passed
snap recovery-presentation: rebuilt to real cross-message topology, 中英对照 (zh + en), 3 scenarios each (side-effect 操作已完成 no-redo / read-only 回复未完成 / no-tool 回复未完成) reviewed
snap turn-status-phase: 连接中 / 思考中 / 正在恢复…第 2 次 reviewed
```
Screenshots or Recordings
Checklist
bugapp,uiP2Pending,Approved by @<reviewer>, orNot required: <reason>sdk/.../types.gen.ts) is a scoped 1-line regen for the new optional field.dev, and my PR title and commit messages use Conventional Commits in English.https://claude.ai/code/session_01UUsFz2KqaDQpF8TzBJoBZF
Summary by CodeRabbit
New Features
Tests