Feat remove phases from kimchi leaving plan - #1093
Conversation
Kimchi Code Review
Summary📊 Review Score: 92/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Tests were updated across status-line, agent-runner, prompts, ferment, and status-line config. A new regression test in 📝 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 |
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 92/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests were updated across status-line, agent-runner, prompts, ferment, and status-line config. A new regression test in src/extensions/agents/manager/agent-runner.test.ts verifies that subagents no longer mutate parent-session phase/tag state, and another confirms guidelines are resolved from the persona role rather than the orchestrator phase.
📝 Found 2 issue(s). See inline comments for details.
| Kimi writes the plan (interfaces, file paths, method signatures) to a spec | ||
| file. Spawns one minimax-m2.7 subagent with the spec file attached for the | ||
| build phase. After the subagent returns, kimi reads the output and verifies. | ||
| builder role. After the subagent returns, kimi reads the output and verifies.. |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The sentence ends with two consecutive periods (verifies..), a documentation typo introduced by the diff.
💡 Suggestion: Change verifies.. to verifies.
| ``` | ||
|
|
||
| **Expected:** plan phase (heavy model) + multiple implementation subagents, 3–6 subagents, <15 min, clean package separation, comprehensive tests, stdlib only. | ||
| **Expected:** planner (heavy) + builder role + build both, 3–6 subagents, <15 min, clean separation, comprehensive tests, stdlib only. |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
The expected-outcome sentence contains the fragment planner (heavy) + builder role + build both, which is grammatically unclear and could confuse benchmark operators about whether both roles perform implementation work or whether two separate builds are required.
💡 Suggestion: Rephrase to something explicit, e.g. planner (heavy) + builder role (standard), 3–6 subagents or planner (heavy) and builder role (standard) both participate in implementation.
…es-from-kimchi-leaving-plan
…es-from-kimchi-leaving-plan # Conflicts: # src/sandbox/worker/acp-client.ts
1 participant
Linked issue
Closes #
What does this PR do?
Removes Kimchi's model-controlled workflow phase state machine (
explore,research,plan,build, andreview). Planning remains the existing read-only permission mode, while multi-model specialization remains role-based.Multi-model routing is not being replaced. The orchestrator already selected specialized models and created workers through
Agent;set_phasewas a parallel layer of self-reported session state around that flow. After this change, the orchestrator delegates concrete work directly by role without first changing a global phase.Removed
set_phase,/phase, thePhasestate, and parent/child phase propagation.phase:*request tags, phase status-line rendering, and phase-change UI.explore -> research -> plan -> build -> reviewpipeline.set_phase. The user's Pi thinking controls remain, and each delegatedAgentcall can still set an explicit thinking level.src/extensions/model-catalog/implementation; orchestration now owns the canonical model registry and reference helpers.Added
ExitPlanMode(plan)tool instead of parsing<!-- PLAN_COMPLETE -->or<done>markers.What stays
default,plan,auto, andyolo.Agent-based worker creation, explicit per-worker model/thinking/budget selection, and parallel independent build chunks.model:*tag, and Ferment's separate persisted milestone phases.This is primarily a simplification and regression-risk reduction, not a claim of direct benchmark improvement. The expected multi-model work path is substantially the same, minus the phase-management ceremony and its cross-cutting state.
Validation
pnpm run checkpnpm run test— 466 files passed, 3 skipped; 8,886 tests passed, 13 skippedgit diff --checkChecklist
pnpm run test)pnpm run check)