Add opt-in Pro Plan routing profile#1865
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces 'Pro Plan' mode, a model-routing workflow that utilizes deepseek-v4-pro for planning and review phases and deepseek-v4-flash for implementation. It includes a new ProPlanRouter state machine to manage phase transitions and updates the TUI and engine to enforce plan confirmation, handle automatic model switching, and provide phase-specific status updates. Feedback identifies the use of unstable Rust 'let chains' which could cause compilation errors on stable toolchains. Additionally, several new user-facing strings and labels are hardcoded in English and should be moved to the localization system to maintain internationalization standards.
| if let Some(router) = app.pro_plan_router.as_mut() | ||
| && router.phase() == crate::tui::pro_plan::ProPlanPhase::Done |
There was a problem hiding this comment.
The use of 'let chains' (if let ... && ...) is an unstable Rust feature (see RFC 2497). Unless this repository explicitly requires a nightly toolchain via a rust-toolchain.toml file, this will cause compilation errors on stable Rust. Please refactor these to use nested if let statements or matches!. This pattern also appears on lines 1582, 1633, and 5590.
| crate::tui::pro_plan::ProPlanFollowUp::ReviewImplementation => { | ||
| "Review the implementation against the accepted plan. Do not edit files during review. If it is correct, include `<pro_plan review=\"approved\">`; if changes are needed, include `<pro_plan review=\"changes_requested\">` and list the fixes." | ||
| } | ||
| crate::tui::pro_plan::ProPlanFollowUp::AddressReviewFeedback => { | ||
| "Address the review feedback using the accepted plan, then summarize the changes and include `<pro_plan execute_complete=\"true\">`." | ||
| } |
There was a problem hiding this comment.
These follow-up messages and status labels are hardcoded in English. To maintain consistency with the project's internationalization (i18n) standards, these strings should be moved to MessageId in crates/tui/src/localization.rs and retrieved using the tr() function. This applies to several new user-facing strings in this file, including:
- Phase labels (lines 1654-1657)
- System messages (lines 5532, 5566, 5601)
- Tool blocking status (line 1976)
| ("Accept plan", "Start implementation with approvals"), | ||
| ( | ||
| "Accept plan (YOLO)", | ||
| "Start implementation in YOLO mode (auto-approve)", | ||
| "Start implementation with auto-approval", | ||
| ), | ||
| ("Revise plan", "Ask follow-ups or request plan changes"), | ||
| ( | ||
| "Exit Plan mode", | ||
| "Exit to Agent", | ||
| "Return to Agent mode without implementation", | ||
| ), |
# Conflicts: # crates/tui/src/tui/ui.rs
|
Addressed the current review pass in e915c79/e5efe93:
Local validation:
|
|
Polished this for reviewability in 872691c:
The PR is now marked ready for review. |
|
Independent review: Read the routing logic ( Design. 4-state FSM (Plan->Execute->Review->Done) gated on explicit Cost. Each full cycle is Pro(plan) + Flash(exec) + Pro(review) - measurably more expensive than auto-model on small tasks, cheaper than all-Pro. Important: entering ProPlan force-disables Mode interaction. ProPlan added to the mode cycle (Plan->Agent->YOLO->ProPlan->Plan). Auto-model save/restore is the only state-coupling risk - if mode-set panics mid-transition, restore won't run. Tests. 10 unit tests on the FSM, 1 integration test on auto-model save/restore, fail-closed tests for v0.8.48 merge. CONFLICTING: ui.rs (156-line block - new ProPlan helpers vs PR #2256 sidebar work), plan_prompt.rs (2), engine/tests.rs (1), README, docs/MODES.md. All mechanical but ui.rs touches the same dispatch path as v0.8.48. |
|
Thanks @dumbjack — I re-checked this during the harvest pass. I am leaving it unmerged for now because it is a large mode/engine/UI change, currently conflicts with main, and only has the lightweight security check on the current PR surface. What would make this mergeable:
The idea is still interesting; it just needs a fresh integration pass before I can responsibly land it. |
|
Thanks @dumbjack. I rechecked this against current I wouldn’t want to merge or partially harvest this until it is rebased and revalidated on the current |
|
Fresh integration pass pushed in db795e7. What changed for the current review concerns:
Validation on stable Rust 1.95.0:
Note: the first broad test pass hit a one-off local MCP SSE connection reset in |
|
Thanks @dumbjack, this is much closer now. I rechecked the latest head and the Pro Plan design still fits CodeWhale well: Pro for plan/review, Flash for execution, with the existing Plan Confirmation gate and fail-closed read-only paths.\n\nI’m going to hold off on merging until the required CodeWhale checks actually run and pass on this head. GitHub reports the branch as mergeable, but the PR is still blocked because the required ruleset checks are missing here. Before landing I’d also like one final maintainer pass over the broad planning-trigger heuristic and the YOLO re-implementation loop semantics.\n\nSuggested validation: cargo fmt --all -- --check, cargo check -p codewhale-tui, focused pro_plan / plan_prompt / localization tests, and a full cargo test -p codewhale-tui --quiet. |
|
Thank you for exploring the Pro Plan idea. The general goal still fits: CodeWhale should help route work thoughtfully across model strengths. I am leaving this unmerged for now because the PR is blocked and the current product direction is to be less opinionated in the main model/menu surface, giving users clearer control over choices. A refreshed version could be very compelling if it moves the policy-heavy routing into opt-in profiles or subagent defaults, keeps the main menu neutral, and explains when the tool is choosing versus when the user is choosing. That would keep the useful intelligence without making the default experience feel over-prescribed. |
# Conflicts: # crates/tui/src/commands/config.rs # crates/tui/src/core/engine/tests.rs # crates/tui/src/localization.rs # crates/tui/src/tui/app.rs # crates/tui/src/tui/ui.rs # docs/MODES.md
|
Closing, @dumbjack — thank you for the Pro Plan routing exploration. As discussed earlier in the thread, automatic plan-based routing doesn't match the direction we want for default behavior. If you'd like to rework it as an explicit opt-in routing profile (config-gated, no default change), we'd welcome that as a fresh PR. |
Summary
/mode pro-planrouting profile, not a fourth default mode in theTabcycle or/modepicker.AppMode::ProPlanmaps to read-only Plan behavior, and missing Flash routes fall back to the resolved Pro model.Why this helps
This follows the same broad workflow idea as Claude Code's public Opus Plan pattern: use the strongest model where planning and review have the most leverage, then use a faster model for the implementation pass. In CodeWhale this is deliberately opt-in so the main model/menu surface stays neutral and users can tell when they are choosing a workflow versus when CodeWhale is choosing a phase model inside that workflow.
The profile is intended for larger or riskier coding tasks, not as a default for tiny edits.
Safety / Reviewability
/mode pro-planis the explicit entry point; numeric mode shortcuts remain1,2, and3for the visible picker rows.auto_model, disables it while routed by phase, and restores it on exit.Validation
cargo fmt --all -- --checkcargo check -p codewhale-tuicargo test -p codewhale-tui pro_plan --quiet(20 passed)cargo test -p codewhale-tui plan_prompt --quiet(6 passed)cargo test -p codewhale-tui localization --quiet(8 passed)cargo test -p codewhale-tui --quiet(fullcodewhale-tuipass: 4006 passed, 4 ignored, plus integration targets)Greptile Summary
This PR introduces
AppMode::ProPlan, a new opt-in routing profile that plans and reviews withdeepseek-v4-proand executes withdeepseek-v4-flash(falling back to the Pro model when Flash is unavailable on the active provider). It wires the existing Plan Confirmation gate into the flow so execution never starts without explicit user approval.ProPlanRouter(pro_plan.rs) drives the four-phase lifecycle (Plan → Execute → Review → Done) via XML markers emitted by the model; phase transitions are gated onTurnOutcomeStatus::Completedonly, preventing drift on aborted turns.should_force_update_plan_stepreplaces the oldturn.tool_calls.is_empty()check, so theupdate_plangate stays active until a successful call is made rather than lifting after any first tool error.ProPlanConfig::for_providerresolves and validates the Flash route at mode-entry time, defaulting to Pro when the active provider does not advertise Flash.MessageIdvariants cover all user-visible Pro Plan strings across six shipped locales; model-facing instructions remain hardcoded English.Confidence Score: 5/5
Safe to merge. The raw ProPlanMode fails closed to read-only tools and Never approval on every path that bypasses phase resolution, and phase transitions are correctly gated on completed turns only.
The new state machine, provider routing, tool-gate enforcement, and UI wiring are all well-covered by tests. The same-mode guard in set_mode prevents restore-value corruption. The one notable finding — common action words triggering unnecessary planning-instruction injection on question turns — does not produce incorrect behaviour because the injected instruction tells the model to answer normally for questions.
crates/tui/src/tui/ui.rs — the should_add_pro_plan_planning_instruction word-list would benefit from a question-start short-circuit to reduce context noise on long planning sessions.
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant UI as TUI participant R as ProPlanRouter participant EP as Engine Plan tools participant EA as Engine Agent tools participant M as Model U->>UI: /mode pro-plan UI->>R: new(ProPlanConfig::for_provider) UI-->>U: "Phase=Plan, Model=deepseek-v4-pro" U->>UI: Add feature X UI->>UI: append pro_plan_planning instruction UI->>EP: "SendMessage(mode=Plan, model=pro)" EP->>M: request with update_plan gate M->>EP: calls update_plan EP-->>UI: TurnCompleted UI->>R: apply_turn_completion sets has_generated_plan UI-->>U: Plan Confirmation prompt U->>UI: Accept plan UI->>R: start_execution(auto_approve) UI->>EA: "follow-up Proceed mode=Agent model=flash" EA->>M: execute with write tools M->>EA: changes plus execute_complete marker EA-->>UI: TurnCompleted UI->>R: transition to Review UI->>EP: "auto follow-up Review implementation mode=Plan model=pro" EP->>M: review read-only M->>EP: review approved or changes_requested EP-->>UI: TurnCompleted UI->>R: transition to Done or Execute UI-->>U: PRO-PLAN DoneReviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile