fix(desktop): reconcile goal arm across reconnects - #3363
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
Codex-assisted review performed under the maintainer-approved review workflow.
hqhq1025
left a comment
There was a problem hiding this comment.
The previous reconciliation-wait finding is fixed on this revision. The deadline is limited to replacement waits after a dispatched control enters reconciliation, expiry removes the waiter and returns the control-specific unavailable result, and goal.arm is not replayed. Exact-head build:test, the Desktop suite (996 tests), Desktop typecheck, lint, and format checks passed.
Codex-assisted review performed under the maintainer-approved review workflow.
|
Could you please run CI for this PR? All local tests and checks are passing. Thank you! |
|
The Knip failure has been fixed in 6780df0 by renaming the Goal dialog test from |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — I reviewed this fresh against the current head rather than leaning on the earlier approval, since commits landed on the branch after it.
No P0–P3 findings in the Goal logic. The part I went in most skeptical about is the part that convinced me, so it is worth saying where I expected to find a hole and did not.
My worry with "dispatch once, reconcile only through replacement-candidate reads" is causality: a read of canonical state cannot prove that the state you are looking at was produced by your mutation. Someone else arming an equivalent Goal concurrently would look identical from the reading side. What this does instead is narrower and correct — it computes whether the current Goal is a new generation with matching Session, condition and budgets, and the dialog says so in as many words: the state matches the request, but whether your submission is the one that landed is not confirmed. So the concurrent case degrades to unknown, never to a false "confirmed". A same-goalId Goal, a different field, a different Session, or no Goal at all all fail to report matching. That is the honest version of this reconciliation and it is what I would want here.
The reconcile path is also entered only on the specific control + dispatched + connection_lost shape rather than on any failure, the replacement wait is fenced to the same target epoch, and post-wait calls are query-only — so a lost response cannot turn into a second dispatch. A different epoch reports a target change instead of reconciling, which is right: a new Host is not evidence about the old Host's causality.
On lifecycle, which is where "keep the uncertain outcome visible and locked" usually goes wrong: the 15s deadline bounds only the replacement wait, and the dialog locks after any reconciled-or-unavailable result rather than hanging. The UI uncertainty is not persisted across a restart, but the authority is — if the original mutation did land, the Goal is still unfinished and a second arm is refused by the one-unfinished-Goal-per-Session rule from canonical state; if it did not land, retrying is genuinely a first submission. So there is no orphaned reconciliation lock to clean up.
On the new reusable reconciled-control path having exactly one consumer: I went looking for premature generalisation here and I do not think that is what this is. What the shared part carries is the router's own invariants — a dispatched control is never replayed, replacement reads are query-only, waits do not cross an epoch. Those belong to the reconnecting IPC router, not to Goal. Making it Goal-specific would push transport and reconnect semantics back into the Session domain and duplicate a state machine, which is the more expensive shape even at one consumer.
CI, as a merge gate rather than a code finding — and I do not think you need to change anything for it.
test is currently red on this head, failing the whole-repo build at packages/cli/src/pi-tui-runner.ts for a missing midTurn. That field became required in #3379, and this PR does not touch the CLI at all.
The reason is timing rather than anything in your branch. This run was created at 11:35:28Z, and a pull_request run tests the merge commit computed at event time, not the state of main when it actually executes. The commit that added midTurn: 'local' to that file landed on main at 11:49:56Z — fourteen minutes later. So the merge base under test predates the fix, and re-running will not help, because a re-run replays the same SHA.
A fresh push — a rebase onto current main is the natural one — recomputes the merge commit and should clear it. Please do not go looking for something to fix in pi-tui-runner.ts; nothing on your side caused this.
I am not approving only because I do not approve against a red exact head, not because of anything in the change.
Review assisted by AI (Claude Opus 5 and GPT-5.6, reviewed independently). Findings were verified against the files and workflow-run timestamps at this head; the reviewers are accountable for them.
6780df0 to
e2a28ad
Compare
|
Rebased onto the current |
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
e2a28ad to
23c56f9
Compare
|
Rebased onto the current Local verification on |
Summary
goal.arm, comparing canonical Goal state after a dispatched connection loss without replaying the mutation.Fixes #3234
Verification
npm test -w @maka/runtime-host— 1036 passednpm test -w @maka/desktop— 995 passednpm run typecheck -w @maka/desktop— passednpm run format:check— passednpm run lint— passednpm run e2e -w @maka/desktop -- goal-dialog-budget.spec.ts— 1 passedReview focus
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the Desktop reconciliation seam, Goal integration, renderer behavior, tests, conflict resolution, and verification under user direction.
Checklist
Does this PR entail a change in behavior?