fix(acp): make session/set_mode plan idempotent on sessions created in plan mode - #3407
fix(acp): make session/set_mode plan idempotent on sessions created in plan mode#3407ApexYash11 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: ac44034 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac44034f27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sessionId: this.sessionId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return false; |
There was a problem hiding this comment.
Preserve plan cancellation when status reads fail
When getPlan() rejects while the engine is actually in plan mode (for example, status() hits a non-missing plan-file I/O error), returning false treats the state as inactive. That makes init() report default, and more importantly makes setMode('default' | 'auto' | 'yolo') skip cancelPlan() before updating permission/currentModeId and emitting a non-plan mode, leaving the engine in plan mode while ACP says it exited; propagate the unknown state or still issue the cancel for non-plan targets.
Useful? React with 👍 / 👎.
Related Issue
Resolve #3356
Problem
Over ACP,
session/set_modewithmodeId: "plan"fails with-32603 Internal error("Already in plan mode") right aftersession/newreportscurrentModeId: "default". Whendefault_plan_mode = true, the engine enters plan mode at session creation, but the ACP server's local mode state stays"default"andsetModere-enters plan unconditionally, tripping the engine's guard. ACP clients that apply a configured plan mode right aftersession/newabort session creation entirely.What changed
In
packages/acp-server/src/session.ts:currentModeIdfrom the engine's actual plan state during session init, sosession/newtruthfully reports"plan"when the engine entered plan mode at create time.setMode's plan toggle idempotent — only callenterPlan/cancelPlanwhen the engine's plan state differs from the requested mode — so re-asserting the active mode no longer throws "Already in plan mode".Added an integration test in
packages/acp-server/test/config.test.tsthat boots withdefault_plan_mode = true, assertssession/newreportsplan, and verifiessession/set_mode "plan"succeeds.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.