Skip to content

fix(desktop): offer safe resume after completed tool timeout - #4075

Open
liugddx wants to merge 4 commits into
apache:mainfrom
liugddx:fix/post-tool-timeout-recovery-pr
Open

fix(desktop): offer safe resume after completed tool timeout#4075
liugddx wants to merge 4 commits into
apache:mainfrom
liugddx:fix/post-tool-timeout-recovery-pr

Conversation

@liugddx

@liugddx liugddx commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • expose safe continuation guidance when a failed Turn timed out after a completed Tool Result
  • make the Desktop resume candidate detector require a completed tool result for timeout recovery
  • preserve fail-closed Runtime planner validation; incomplete or errored tools remain in inspection/parked flows
  • add renderer presentation and candidate-selection regression tests

Scope

This is the first Desktop recovery slice for #4074. It does not change Runtime tool execution, does not re-run tools, and does not change the MAKA_RUNTIME_SAFE_BOUNDARY_RESUME rollout flag. The existing turn.resume.query/start planner remains the authority for whether continuation may actually start.

Related: #4074

Validation

  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop run build:main
  • node --test "dist/main/__tests__/session-status-presentation.test.js" "dist/main/__tests__/interrupted-resume.test.js"
  • npx biome check on changed files

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 28, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed exact head 24d79cea01006c5994854702eb59653884b9a30b. I found one correctness issue in the new candidate rule.

[P2] Require every tool boundary to be settled before showing Safe Resume

latestInterruptedResumeTurnId() currently treats one completed tool as sufficient, even when another tool in the same failed Turn has no durable result. This is a normal persisted shape: after one successful tool, a later tool_call can time out before its tool_result is written. materializeTurns() then projects the two tools as [completed, interrupted], but the new .some(status === 'completed') check still returns the Turn ID and Desktop renders the Safe Resume action.

The Runtime planner correctly fails closed for the same boundary: the unmatched call becomes pending_tool_result / dangling_tool_state, so clicking the action only produces a parked recovery error. This makes Desktop promise a recovery path that its authority will reject, and it contradicts the PR description's statement that incomplete tools remain gated.

The new test does not cover the production shape. It checks a hand-built Turn containing only running, while an unfinished tool in a terminal Turn is projected as interrupted; it also never combines a completed tool with the unfinished one. Replacing the predicate with “at least one tool, and every tool is completed” left both new tests green, confirming that they do not distinguish the faulty rule.

Please gate the action on the whole tool set (or on an equally strict projection of the Runtime planner) and add a regression that goes through materializeTurns() with one completed tool plus one missing result. If a completed-plus-errored Turn should remain an inspection flow as the PR description says, the same all-completed rule also closes that mixed case.

Separately, this head now conflicts with current main after #4073 in app-shell-turn-view-model.ts and session-status-presentation.ts. Please rebase and preserve #4073's newer separation between the failure reason and execution-state copy. The exact-head hosted test check is green, and the local Desktop suite passed 1,629/1,629 tests; those checks do not remove the mixed-state defect above.


Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

@liugddx
liugddx force-pushed the fix/post-tool-timeout-recovery-pr branch from 24d79ce to 1eb6ab0 Compare August 28, 2026 15:08
@liugddx

liugddx commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Addressed the review: timeout Safe Resume now requires a non-empty tool set where every tool is completed; mixed [completed, interrupted] and errored/incomplete tool states remain gated. Added a regression using materializeTurns() to cover the persisted mixed shape. Rebased onto current main (including #4073) and force-pushed; new head is 1eb6ab0.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed exact head 1eb6ab0067722378afb6c9f8fdd00c24340739da. The previous mixed-tool finding is fixed, and I found no remaining blocking issues.

The timeout recovery candidate now requires a non-empty tool set in which every rendered tool has a durable completed result. A persisted Turn with one completed tool followed by a call whose result never arrived materializes as [completed, interrupted] and is correctly excluded. Errored and running tools are excluded as well, while app_restarted keeps its existing recovery behavior. The Runtime resume planner remains the final authority when the action is used.

The new regression goes through materializeTurns() and reproduces the actual mixed persisted shape. As a falsification check, restoring the old some(completed) predicate made both mixed-state regressions fail; restoring the fix made them pass again. The complete Desktop suite passed locally (1,663/1,663), along with the full workspace test build, formatting, and diff checks.

This head is based directly on the current main; the previous conflicts with #4073 are gone and the merge result is clean. The exact-head hosted test check is successful.


Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants