Auto-run approved plans with Ferment V2 - #1142
Conversation
Kimchi Code Review
Summary📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Comprehensive coverage: unit tests for the new PromptEditor indicator and gradient border, Ferment V2 command summary neutralization, approved-plan executor registry, reducer presentation round-tripping, permissions integration (auto/resume/pause, tool visibility, execute routing), and full E2E TUI tests for both manual Ferment V2 from plan mode and automatic approved-plan execution. 📝 Found 2 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Comprehensive coverage: unit tests for the new PromptEditor indicator and gradient border, Ferment V2 command summary neutralization, approved-plan executor registry, reducer presentation round-tripping, permissions integration (auto/resume/pause, tool visibility, execute routing), and full E2E TUI tests for both manual Ferment V2 from plan mode and automatic approved-plan execution.
📝 Found 2 issue(s). See inline comments for details.
| @@ -410,6 +424,34 @@ export default function fermentV2Extension(pi: ExtensionAPI): void { | |||
| const cleared = clearCompletionDecision(currentFermentV2) | |||
There was a problem hiding this comment.
🚨🔀 Concurrency
pauseActiveFermentV2ForPlanMode commits a paused Ferment V2 revision directly via commitFermentV2 without serializing through the per-session mutationTails queue used by serializeUserMutation. If the user switches to Plan mode (or the MODE_CHANGED event fires) while an approved-plan start or /ferment-v2 command is yielded inside serializeUserMutation, the pause mutates currentFermentV2 out from under the in-flight mutation. When that mutation resumes, assertUnchanged(captured) throws and the operation fails, even though ensureFermentV2ExecutionMode had already tried to switch to Auto.
💡 Suggestion: Route the pause through serializeUserMutation (or a shared session lock) so Plan-mode pauses cannot interleave with in-flight Ferment V2 mutations. For example, enqueue a source: "plan_mode" mutation that checks the active status again at execution time before calling setFermentV2Status.
| if (getPermissionMode(sessionId)?.mode !== "plan") return | ||
| setPermissionMode(sessionId, { mode: "auto", initiatedBy: "user", source: "runtime" }) | ||
| } | ||
|
|
There was a problem hiding this comment.
ℹ️
pauseActiveFermentV2ForPlanMode fires abortEvaluation() with void and immediately checkpoints/commits the paused state. If the evaluator is slow to stop, a late turn result can still arrive after the pause is recorded. The downstream matchesFermentV2 guard will drop it, but the immediate "paused" notification may race with a final status/tool update, giving the user a briefly inconsistent view.
💡 Suggestion: Consider awaiting abortEvaluation() (or its settlement) before checkpointFermentV2 so the pause is committed only after the active evaluator has actually stopped, provided that does not introduce a re-entrant deadlock with abortEvaluation's own cleanup path.
What was wrong
Approving a Plan-mode plan with Execute only sent the legacy execution trigger. With Ferment V2 enabled, the approved plan did not enter the persistent execution controller, and starting or resuming execution from Plan mode left write tools unavailable.
Fix
Validation
pnpm run checkpassed with two unrelated existing lint warnings.pnpm run build:binarypassed.pnpm run test: 9,759 passed and 10 skipped; three unrelatedsrc/extensions/dap/integration.test.tscases failed and reproduced when run alone.Type of Change
Checklist
CONTRIBUTING.mdand agree to the CLApnpm run test) — see the unrelated DAP failures abovepnpm run check)