fix(session): terminalize stale question blockers#947
Conversation
📝 WalkthroughWalkthroughSession.messages now terminates stale external-result questions: running question tool parts without valid external results are converted to error state with cancellation metadata and interruption flags when their ExternalResult lookup returns not-found. ChangesStale Question Terminalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to automatically terminalize stale, running external-result questions when retrieving session messages. It adds a helper function terminalizeStaleExternalResultQuestions that iterates through message parts, identifies stale questions, and updates their state to an error status. Additionally, a comprehensive unit test has been added to verify this behavior. The reviewer suggested refactoring the sequential for loops in the new helper function to use Effect.all for parallel processing, which would improve performance and better align with effect-ts idioms.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/opencode/test/session/session.test.ts`:
- Around line 740-825: The test calls ExternalResult.__resetForTests() only
before and after the Instance.provide block, which can leak state if the test
body throws; wrap the entire async callback passed to Instance.provide (the
function that creates the session and updates messages/parts using
SessionNs.create, SessionNs.updateMessage, SessionNs.updatePart) in a
try/finally and move the ExternalResult.__resetForTests() call into the finally
block so it always runs (keep or remove the pre-call as desired, but ensure the
finally calls ExternalResult.__resetForTests() to guarantee cleanup).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed03086f-ff00-429a-b00b-df9c405b4690
📒 Files selected for processing (2)
packages/opencode/src/session/session.tspackages/opencode/test/session/session.test.ts
|
Addressed the P2 test coverage follow-up in 52c35ce. Session.messages now has regression coverage for the stale not_found case plus the two safety branches: live pending ExternalResult entries stay running, and unready external-result questions stay running.\n\nValidation run locally:\n- bun test test/session/session.test.ts — 25 passed\n- bun test test/server/tool-respond-route.test.ts — 7 passed\n- bun run typecheck — passed |
Root cause: The desktop message loader uses the paginated session message endpoint. PR #947 repaired stale external-result question parts through Session.messages(), but GET /session/:sessionID/message?limit=... still called MessageV2.page() directly, so reopened sessions could keep showing an unanswerable running question card after the in-memory ExternalResult was gone. Change: - Add Session.messagesPage() as the paginated session-layer reader. - Reuse stale external-result question terminalization for paginated responses. - Keep MessageV2.page() as a raw storage pagination helper with no repair side effects. - Route paginated message responses through Session.messagesPage() while preserving cursor headers. - Add route-level regressions for stale, live pending, unready, and older-cursor stale question cases. Verification: - RED: stale paginated route returned running before the fix. - RED follow-up: older cursor page returned running when the route was temporarily restored to direct MessageV2.page(). - bun test test/server/session-messages.test.ts: 9 passed. - bun test test/session/session.test.ts -t "messages": 3 passed. - bun test test/server/tool-respond-route.test.ts: 7 passed. - bun run typecheck: passed. - PR CI: passed, including ci/check, unit-opencode, unit-app, desktop-smoke, e2e-artifacts, codeql, dependency-review, commit-lint, and pr-triage. Review follow-up: - Added the older before-cursor regression requested in review. - Kept shared fixture extraction out of scope to avoid broad test-structure churn. Risk: Low. The behavior change is limited to the public paginated session message API and only terminalizes question tool parts that are already marked externalResultReady and have no live ExternalResult entry.
Summary
Terminalize stale question blockers when session history is read, so a persisted
runningquestion whose in-memory external-result Deferred was lost during restart no longer reopens as an active dock. There is no linked issue; this came from a user-provided exported session that reproduced a blocked conversation after app restart.Why
External-result question prompts depend on an in-memory Deferred. After a full app/server restart, that Deferred is gone, but the persisted tool part can still say
runningwithexternalResultReady: true. The app then renders a submit-ready question dock that can only fail withno_pending_tool_call, leaving the session stuck.The fix reconciles durable session history with the live external-result registry at the backend session-read boundary. If the registry no longer has a pending entry for a ready running question, PawWork persists the part as
errorwith interrupted metadata instead of exposing it as a live blocker.Related Issue
No GitHub issue yet. This PR is scoped to the exported-session bug report where a question dock survived restart and blocked the session from continuing.
Human Review Status
Pending
Review Focus
Please focus on whether
Session.messages()is the right reconciliation boundary and whether the stale check is narrow enough: it only terminalizesquestiontool parts that arerunning,externalResultReady === true, and missing fromExternalResult.lookup().Risk Notes
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changed.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests