fix #85782: surface terminal TUI lifecycle errors#85996
Conversation
|
Codex review: needs maintainer review before merge. Reviewed May 30, 2026, 4:57 PM ET / 20:57 UTC. Summary PR surface: Source +81, Tests +162, Other +42. Total +285 across 5 files. Reproducibility: yes. from source inspection and the PR proof: current main handles lifecycle phase:error separately from chat state:error, so a lifecycle-only terminal failure can update the footer without rendering and clearing through the chat error path. I did not run a local failing repro because this review is read-only. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land or reject this PR through normal maintainer review after accepting the TUI state-machine and test-runner automation risks, keeping the focused regression coverage with the implementation if it lands. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection and the PR proof: current main handles lifecycle phase:error separately from chat state:error, so a lifecycle-only terminal failure can update the footer without rendering and clearing through the chat error path. I did not run a local failing repro because this review is read-only. Is this the best way to solve the issue? Yes, the proposed direction is a maintainable fix: reuse the terminal chat-error cleanup path for terminal lifecycle errors, defer through a retry grace, and cover dedupe/retry behavior. The explicit Vitest config-target fix is a separate automation repair but is guarded with focused tests. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 9c2744f1e12b. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +81, Tests +162, Other +42. Total +285 across 5 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Brave Signal Puff Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
4e0f37b to
c7bf5d6
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
c7bf5d6 to
4431edb
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
This comment was marked as spam.
This comment was marked as spam.
a9dddb7 to
a1d885e
Compare
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
a1d885e to
fa3f82a
Compare
|
Maintainer refresh pushed at Verification:
Maintainer fixes added during refresh:
|
steipete
left a comment
There was a problem hiding this comment.
Verification for head d11f0f5:
- pnpm exec oxfmt --check src/tui/tui-event-handlers.ts src/tui/tui-event-handlers.test.ts test/scripts/test-projects.test.ts scripts/test-projects.test-support.mjs src/agents/model-catalog-visibility.test.ts
- node scripts/run-vitest.mjs src/tui/tui-event-handlers.test.ts
- node scripts/run-vitest.mjs src/tui/tui-event-handlers.test.ts src/tui/tui-command-handlers.test.ts test/scripts/test-projects.test.ts src/agents/model-catalog-visibility.test.ts
- git diff --check
- autoreview --mode local: no accepted/actionable findings
- autoreview --mode branch --base origin/main: no accepted/actionable findings
CI:
- CI run 26694374647: checks-node-agentic-agents-core rerun passed at job 78676538366 after an unrelated timeout/interrupted flake in src/agents/code-mode.test.ts.
- TUI PTY run 26694374606 passed.
- Required check dependency-guard passed at run 26694374142 job 78676243471.
Known proof gap: ci-timings-summary is a reporting helper and is currently failing; GitHub does not list it as a required check for this PR.
|
CI follow-up: ci-timings-summary rerun passed at CI run 26694374647 job 78676883672. Real behavior proof passed at run 26694499928 job 78676827000. |
|
Rebased proof for head 63293c0:
CI after rebase:
|
Summary
errorwhile the transcript showed no run error and the active run stayed locked; a later delayed chat error could also duplicate the visible error.src/tui/tui-event-handlers.ts, focused TUI coverage, and the test-project runner coverage for explicit leaf config targets plus the slow catalog visibility shard timeout.Fixes #85782
Real behavior proof
run error: <message>, clears the active run, leaves input unblocked, deduplicates the delayed embedded chat error for the same run, and the explicit Vitest config target lane succeeds on the current head.a9dddb7aa83e836ece7baf7e49559e98f20af3f1git rev-parse --short HEAD node --import tsx /media/vdc/code/ai/aispace/openclaw-issue-85782-evidence/embedded-tui-lifecycle-proof.mts node scripts/run-vitest.mjs src/tui/tui-event-handlers.test.ts src/tui/tui-command-handlers.test.ts node scripts/run-vitest.mjs test/scripts/test-projects.test.ts pnpm exec vitest run --config test/vitest/vitest.agents-core.config.ts src/agents/model-catalog-visibility.test.ts pnpm check:test-types gh pr checks 85996 --repo openclaw/openclaw$ git rev-parse --short HEAD
a9dddb7
$ node --import tsx /media/vdc/code/ai/aispace/openclaw-issue-85782-evidence/embedded-tui-lifecycle-proof.mts
OpenClaw embedded TUI lifecycle error proof
{
"backendEvents": [
{
"event": "agent",
"stream": "lifecycle",
"phase": "error",
"error": "provider exploded"
},
{
"event": "chat",
"state": "error",
"error": "provider exploded"
}
],
"systemMessages": [
"run error: provider exploded"
],
"dismissedPendingRuns": [
"run-embedded-error-proof"
],
"activityStatuses": [
"error",
"error"
],
"activeChatRunId": null,
"historyLoads": 1,
"renders": 2,
"inputUnblocked": true,
"delayedChatErrorObserved": true,
"duplicateTranscriptErrors": 1
}
$ node scripts/run-vitest.mjs src/tui/tui-event-handlers.test.ts src/tui/tui-command-handlers.test.ts
RUN v4.1.7 /media/vdc/code/ai/aispace/openclaw-issue-85782-push
Test Files 2 passed (2)
Tests 89 passed (89)
Start at 01:55:22
Duration 10.19s (transform 7.72s, setup 885ms, import 8.81s, tests 164ms, environment 0ms)
$ node scripts/run-vitest.mjs test/scripts/test-projects.test.ts
RUN v4.1.7 /media/vdc/code/ai/aispace/openclaw-issue-85782-push
Test Files 1 passed (1)
Tests 88 passed (88)
Start at 01:59:35
Duration 13.03s (transform 1.19s, setup 401ms, import 490ms, tests 11.85s, environment 0ms)
$ pnpm exec vitest run --config test/vitest/vitest.agents-core.config.ts src/agents/model-catalog-visibility.test.ts
RUN v4.1.7 /media/vdc/code/ai/aispace/openclaw-issue-85782-push
Test Files 1 passed (1)
Tests 3 passed (3)
Start at 01:59:54
Duration 11.92s (transform 8.45s, setup 772ms, import 9.97s, tests 919ms, environment 0ms)
$ pnpm check:test-types
state: "error"; the transcript rendered onerun error: provider exploded,activeChatRunIdbecamenull, and the delayed chat error did not add a duplicate transcript error. The current-head TUI tests, test-project runner regression, focused agents-core config command, test typecheck, and selected PR checks all passed.Regression Test Plan
src/tui/tui-event-handlers.test.ts;test/scripts/test-projects.test.ts;src/agents/model-catalog-visibility.test.tsendedAtrenders the error, dismisses pending text, clears the active run, preserves retryable lifecycle errors, ignores the delayed chat error for the same run, and explicit leaf Vitest config targets are planned as whole config runs.Root Cause
phase: "error"path only updated activity status, while visible transcript errors and active-run cleanup lived in the separate chatstate: "error"path; after lifecycle rendering was added, delayed embedded chat errors for the same run still needed an idempotency guard. The CI wrapper also rejected explicit leaf Vitest config paths as unmatched test file targets instead of treating them as whole config runs.