fix(desktop): offer safe resume after completed tool timeout - #4075
fix(desktop): offer safe resume after completed tool timeout#4075liugddx wants to merge 4 commits into
Conversation
jackwener
left a comment
There was a problem hiding this comment.
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
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
24d79ce to
1eb6ab0
Compare
|
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
left a comment
There was a problem hiding this comment.
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
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
Summary
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_RESUMErollout flag. The existingturn.resume.query/startplanner remains the authority for whether continuation may actually start.Related: #4074
Validation
npm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build:mainnode --test "dist/main/__tests__/session-status-presentation.test.js" "dist/main/__tests__/interrupted-resume.test.js"npx biome checkon changed files