diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 2d33e0f..b03b0c4 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -4,22 +4,54 @@ All notable changes to MandoCode will be documented in this file. ## [Unreleased] -**Runs on .NET 10, still runs on .NET 8.** MandoCode now ships both builds in one package. The -same `dotnet tool install -g MandoCode` gives a .NET 10 machine the .NET 10 build and a .NET 8 -machine the .NET 8 build, with nothing to choose and nothing to configure. Dependencies moved to -current releases in the same pass. +**Plans can now survive real work instead of being an all-or-nothing suggestion.** MandoCode's new +workflow planner runs each step as durable work the host can observe, pause, retry, revise, and +resume after a restart. It remains opt-in for this release while we finish the long-running model +soak and local-model token measurements. MandoCode also now ships .NET 10 and .NET 8 builds in the +same package, with nothing for users to choose or configure. ### Why this matters (plain-language summary) +- **A plan no longer disappears with the process.** If MandoCode closes after step two, the first + two steps stay settled and the remaining work can resume instead of starting over. The CLI shows + the saved plan at startup and supports both Resume and Discard. +- **The user stays in charge when the plan changes.** `/plan ` forces a plan on demand. Before + execution, steps can be selected and edited in place; later steps are refreshed so a renamed file + or changed expected value does not leave the rest of the plan stale. If execution disproves the + plan, MandoCode can propose a revised remainder and waits for approval before using it. +- **Failures are reported honestly.** Retry, skip, revise, and cancel are distinct decisions. A run + that reaches the end after skipping failed work says so instead of reporting an unqualified + success. +- **The rollout is controlled.** The workflow engine is selected with `planner=workflow`; the + existing planner remains available while soak and token-cost work finishes. - **There is nothing for you to do.** Existing installs keep working exactly as they did. If you update .NET later, the next `dotnet tool update -g MandoCode` quietly moves you onto the newer build. `mandocode --doctor` reports which runtime you're on if you're curious. - **.NET 8 stops getting security fixes in November 2026.** Rather than cut anyone off on a release they didn't expect, MandoCode carries both builds through that window and drops the .NET 8 one after it. The README says so now, so the removal is announced well ahead of time. -- **Cost/risk: low.** No feature or behavior changed. The full suite runs against both builds - independently, so a difference between them fails the build rather than reaching you. +- **Cost/risk: contained.** The framework and execution changes are substantial, which is why the + new planner is opt-in. Both target frameworks run the same suite, Desktop has its own host-level + coverage, and the approval boundary remains in front of every revised plan and file change. + +### Added +- **Durable workflow planning behind `planner=workflow`.** Each plan step moves through a Microsoft + Agent Framework workflow with checkpointed run state. `/plan`, `/plan-resume`, and + `/plan-discard` inspect and control unfinished work without replaying completed steps. +- **Deterministic `/plan `.** The command uses a proposal-only model call with no access to + normal project tools. Providers that cannot honor forced tool choice fall back to constrained + JSON and finally to a safe one-step proposal rather than silently ignoring the command. +- **Retry and replan after failure.** A failed step can be retried with revised instructions, + skipped, used as the starting point for a replacement suffix, or used to cancel the plan. ### Changed +- **Plan execution is no longer nested inside `propose_plan`.** The model finishes proposing, the + host presents the plan, and execution begins only after that turn drains. This removes the root + cause of duplicate work, stale proposal state, and several spinner/cancellation workarounds. +- **Plan review shows the instruction that will actually run.** The CLI step picker uses keyboard + rows and the same prompt component as chat, prefilled for in-place editing. Editing an earlier + step regenerates only the dependent suffix and returns the whole plan for review. +- **Progress reports what each step is doing.** Step narration, retry state, completion counts, and + resumed execution all use the workflow's durable cursor rather than inferred step numbers. - **Targets .NET 8 and .NET 10.** The published package carries both, and NuGet selects the match for your machine at install time. `net10.0` is listed first, so it is the default target for Visual Studio F5 and for `dotnet run -f`. @@ -30,7 +62,11 @@ current releases in the same pass. exists and when it goes away. ### Test coverage -The full suite runs twice, once per build. 505 tests green on .NET 8 and 505 green on .NET 10. +The suite now contains 618 cases per target and runs on both .NET 8 and .NET 10. New coverage locks +down workflow topology, checkpoint envelopes and storage, resume context, retry behavior, plan +revision, semantic step outcomes, proposal handoff, input handling, and version labels. Desktop's +host suite adds 239 passing cases, and the planner was also exercised through real CLI terminal and +Desktop sessions, including forced process termination between steps. ### Internal - **Moved the AI orchestration layer off Semantic Kernel, onto Microsoft's newer Agent diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index a5064c9..3f200d9 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -6,6 +6,7 @@ @inject AIService AI @inject MandoCodeConfig Config @inject TaskPlannerService TaskPlanner +@inject PlanRunnerSelector PlanRunners @inject PlanHandoff PlanHandoff @inject FileAutocompleteProvider FileProvider @inject TokenTrackingService TokenTracker @@ -62,17 +63,12 @@ Bridged via InstructionPromptCoordinator (TCS) so the imperative handler can await VDOM input. *@ @if (InstructionCoordinator.IsActive) { - + } @* Approval menu — surfaced by DiffApprovalHandler for write / command / delete / MCP @@ -182,10 +178,6 @@ private string _wizardSelectValue = ""; private TaskCompletionSource? _wizardSelectTcs; - // Instruction input — bound to the VDOM TextInput surfaced when DiffApprovalHandler - // requests instructions via InstructionPromptCoordinator. - private string _instructionValue = ""; - // Approval menu options — projected from ApprovalSelectCoordinator.Options into the // ApprovalSelect component's own Option type. Rebuilt whenever the coordinator's state // changes (see OnApprovalMenuStateChanged) so the render path stays allocation-free. @@ -382,6 +374,10 @@ ? $"[green]Conversation restored ({restoredCount} messages) — continuing where you left off.[/]" : "[dim]No previous session to continue for this folder — starting fresh.[/]"); AnsiConsole.WriteLine(); + + // Same gesture, so the same place: "continue where I left off" should mention a + // plan that was still running as well as the conversation. + ShowUnfinishedPlanNotice(); } // Initialize imperative input with the shared state machine @@ -953,6 +949,28 @@ continue; } + if (command == "plan-resume") + { + await HandlePlanCommandAsync("resume"); + continue; + } + + if (command == "plan-discard") + { + await HandlePlanCommandAsync("discard"); + continue; + } + + if (command == "plan" || command.StartsWith("plan ")) + { + // GetCommandName lower-cases for dispatch. Recover arguments from the original + // input so paths, identifiers, and quoted goal text keep their casing. + var planCommand = input.TrimStart()[1..]; + var planSpace = planCommand.IndexOf(' '); + await HandlePlanCommandAsync(planSpace >= 0 ? planCommand[(planSpace + 1)..].Trim() : ""); + continue; + } + if (command == "help") { Console.WriteLine(); @@ -988,6 +1006,9 @@ table.AddRow("/mcp remove ", "Remove an MCP server from config"); table.AddRow("/mcp tools ", "List tools exposed by connected MCP servers (server optional)"); table.AddRow("/mcp-reload", "Restart MCP servers and re-register their tools"); + table.AddRow("/plan ", "Force a step-by-step plan (no goal shows an unfinished plan)"); + table.AddRow("/plan-resume", "Continue an unfinished plan"); + table.AddRow("/plan-discard", "Forget an unfinished plan"); table.AddRow("/clear", "Clear conversation history"); table.AddRow("/exit", "Exit MandoCode"); AnsiConsole.Write(table); @@ -1240,7 +1261,6 @@ /// private void HandleInstructionSubmit(string value) { - _instructionValue = ""; InstructionCoordinator.Submit(value ?? string.Empty); } @@ -1643,6 +1663,11 @@ private async Task ProcessDirectRequestAsync(string input) { + // A proposal belongs to the turn that produced it. Without this, a plan proposed during a + // turn the user then cancelled would sit in the single slot and execute at the end of some + // later, unrelated turn. + PlanHandoff.ClearPendingProposal(); + // Reset operation tracking for the new request _recentReadCount = 0; _recentReadFiles.Clear(); @@ -1786,6 +1811,26 @@ } AnsiConsole.WriteLine(); + + // Spinner ownership note: ExecuteAgentModelCallAsync starts a spinner per model call + // and never stops it — it has always relied on the caller. Before plans were deferred + // they ran inside the enumerator drain above, so that finally cleaned up. Now they run + // out here, so the plan run needs its own guarantee or the last step's spinner keeps + // animating after everything has finished. + // + // The model's turn is fully drained. Only now do we run any plan it proposed — the + // plan is a peer of the chat turn, not a child of the propose_plan tool call. Running + // it here is what lets the outer stall watchdog and request ceiling stay untouched + // (they've already completed), lets the prompt gate be held normally, and leaves the + // model with no open turn afterwards to redo the work in. + try + { + await RunPendingPlanAsync(); + } + finally + { + Spinner.Stop(); + } } catch (OperationCanceledException) { @@ -1886,6 +1931,7 @@ // Plain labels — ApprovalSelect carries the palette (green = proceed, warm gold = // redirect/stop), so these stay markup-free for clean comparisons and scrollback echo. private const string ExecutePlanLabel = "Execute plan"; + private const string EditPlanStepLabel = "Edit a step"; private const string RejectPlanLabel = "Reject (answer without a plan)"; private const string CancelRequestLabel = "Cancel request"; @@ -1900,6 +1946,7 @@ private static readonly ApprovalSelect.Option[] _planSelectOptions = { new(ExecutePlanLabel, Color.Green), + new(EditPlanStepLabel, Color.DeepSkyBlue1), new(RejectPlanLabel, new Color(255, 200, 80)), new(CancelRequestLabel, new Color(255, 200, 80)), }; @@ -1946,6 +1993,302 @@ _planSelectTcs?.TrySetResult(choice); } + /// + /// One line during --continue startup when this project has an unfinished plan. + /// + /// + /// + /// Shown only for --continue, because that is this app's existing "pick up where I left + /// off" gesture. Launching without it means a fresh session, and a plan from a previous one is + /// noise there. It also keeps the two halves of "where I left off" together: a resumed plan + /// runs with the conversation it came from rather than without it. + /// + /// + /// It notifies rather than resuming. Resuming writes files, and doing that automatically at + /// launch would let a plan the user had walked away from start changing their project before + /// they had read a single line. + /// + /// + private void ShowUnfinishedPlanNotice() + { + try + { + if (!PlanRunners.SupportsResume) return; + + var saved = PlanRunners.FindResumable(out _); + if (saved == null) return; + + var outstanding = PlanCheckpointStore.OutstandingSteps(saved); + if (outstanding == 0) return; + + var done = saved.Steps.Count - outstanding; + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine( + $"[yellow]Unfinished plan:[/] {Spectre.Console.Markup.Escape(saved.Goal)} " + + $"[dim]({done} of {saved.Steps.Count} steps done)[/]"); + AnsiConsole.MarkupLine("[dim]/plan-resume[/] to continue, [dim]/plan-discard[/] to forget it."); + AnsiConsole.WriteLine(); + } + catch { /* a notice must never stop the session starting */ } + } + + /// + /// `/plan` — inspect, resume or discard the plan recorded for this project. + /// + /// + /// Only the workflow engine records progress, so only it can be resumed; the legacy runner + /// keeps the plan in a local variable that dies with the process. The command says so rather + /// than reporting "nothing to resume", which would be indistinguishable from having lost work. + /// + private async Task HandlePlanCommandAsync(string arg) + { + AnsiConsole.WriteLine(); + + if (!string.IsNullOrWhiteSpace(arg) && arg is not "resume" and not "discard") + { + await ForcePlanAsync(arg); + return; + } + + if (!PlanRunners.SupportsResume) + { + AnsiConsole.MarkupLine( + "[yellow]Plans are only resumable on the workflow planner.[/]"); + AnsiConsole.MarkupLine("[dim]Enable it with:[/] /config set planner workflow"); + AnsiConsole.WriteLine(); + return; + } + + if (arg == "discard") + { + PlanRunners.DiscardResumable(); + AnsiConsole.MarkupLine("[dim]Saved plan discarded.[/]"); + AnsiConsole.WriteLine(); + return; + } + + var saved = PlanRunners.FindResumable(out var refusal); + + if (refusal != null) + { + // Explain rather than silently offering nothing — the plan is on disk, it just can't + // be safely continued as things stand. + AnsiConsole.MarkupLine($"[yellow]{Spectre.Console.Markup.Escape(refusal)}[/]"); + AnsiConsole.MarkupLine("[dim]Use[/] /plan-discard [dim]to forget it.[/]"); + AnsiConsole.WriteLine(); + return; + } + + if (saved == null) + { + AnsiConsole.MarkupLine("[dim]No unfinished plan for this project.[/]"); + AnsiConsole.WriteLine(); + return; + } + + var outstanding = PlanCheckpointStore.OutstandingSteps(saved); + var done = saved.Steps.Count - outstanding; + + if (arg != "resume") + { + AnsiConsole.MarkupLine( + $"[deepskyblue1]Unfinished plan:[/] {Spectre.Console.Markup.Escape(saved.Goal)}"); + AnsiConsole.MarkupLine($"[dim]{done} of {saved.Steps.Count} steps done.[/]"); + AnsiConsole.WriteLine(); + DisplaySavedPlan(saved); + AnsiConsole.MarkupLine("[dim]/plan-resume[/] to continue, [dim]/plan-discard[/] to forget it."); + AnsiConsole.WriteLine(); + return; + } + + AnsiConsole.MarkupLine( + $"[green]Resuming:[/] {Spectre.Console.Markup.Escape(saved.Goal)} " + + $"[dim]({outstanding} step(s) left)[/]"); + AnsiConsole.WriteLine(); + + // The plan was started in a process that no longer exists, so nothing has told the agent + // what request it is fulfilling. Steps treat that request as authoritative for WHERE work + // happens, so without it a resumed plan can write to the wrong place entirely. + AI.SetRequestContext(saved.Goal); + + // Steps already Completed or Skipped keep that status, so the runner steps over them and + // only outstanding work executes. ResumeAsync also seeds what earlier steps produced. + var plan = PlanCheckpointStore.ToPlan(saved); + + _requestCts = new CancellationTokenSource(); + StartCancelKeyListener(); + try + { + using var execution = PlanHandoff.BeginResumedExecution(saved.FileOperations); + var runner = PlanRunners.Current as WorkflowPlanRunner; + var progress = runner != null + ? runner.ResumeAsync(plan, saved, _requestCts.Token) + : PlanRunners.Current.ExecutePlanAsync(plan, _requestCts.Token); + + await foreach (var progressEvent in progress) + { + await HandleProgressEventAsync(progressEvent, plan, _requestCts.Token); + } + + var manifest = PlanHandoff.BuildManifest(plan, PlanHandoff.FileOperations); + AI.AppendAssistantNote(manifest); + } + catch (OperationCanceledException) + { + AnsiConsole.MarkupLine("[yellow]Resume cancelled.[/]"); + } + finally + { + Spinner.Stop(); + StopCancelKeyListener(); + var oldCts = Interlocked.Exchange(ref _requestCts, null); + oldCts?.Dispose(); + } + + AnsiConsole.WriteLine(); + } + + private async Task ForcePlanAsync(string goal) + { + _requestCts = new CancellationTokenSource(); + StartCancelKeyListener(); + var rejected = false; + try + { + Spinner.Start("Creating plan..."); + var proposal = await AI.GeneratePlanAsync(goal, cancellationToken: _requestCts.Token); + Spinner.Stop(); + _lastPlanOutcome = PlanTurnOutcome.None; + var manifest = await PlanHandoff.ProcessAsync( + proposal.Goal, proposal.Steps, _requestCts.Token, originalRequest: goal); + rejected = _lastPlanOutcome == PlanTurnOutcome.Rejected; + if (_lastPlanOutcome == PlanTurnOutcome.Executed && !string.IsNullOrWhiteSpace(manifest)) + AI.AppendAssistantNote(manifest); + } + catch (OperationCanceledException) + { + AnsiConsole.MarkupLine("[yellow]Planning cancelled.[/]"); + } + catch (Exception ex) + { + AnsiConsole.MarkupLine($"[red]Could not create plan:[/] {Spectre.Console.Markup.Escape(ex.Message)}"); + } + finally + { + Spinner.Stop(); + StopCancelKeyListener(); + var oldCts = Interlocked.Exchange(ref _requestCts, null); + oldCts?.Dispose(); + } + + if (rejected) + { + await ProcessDirectRequestAsync( + goal + "\n\n[system: the user reviewed the forced plan and chose to skip stepwise " + + "execution. Answer the request directly now. Do not call propose_plan.]"); + PlanHandoff.ClearPendingProposal(); + } + } + + private static void DisplaySavedPlan(PlanRunState saved) + { + var table = new Spectre.Console.Table { Border = TableBorder.Rounded }; + table.AddColumn("Step"); + table.AddColumn("Description"); + table.AddColumn("Status"); + + foreach (var step in saved.Steps) + { + var status = step.Status switch + { + TaskStepStatus.Completed => "[green]done[/]", + TaskStepStatus.Skipped => "[dim]skipped[/]", + TaskStepStatus.Failed => "[red]failed[/]", + _ => "[yellow]pending[/]", + }; + table.AddRow( + step.Number.ToString(), + Spectre.Console.Markup.Escape(step.Description), + status); + } + + AnsiConsole.Write(table); + AnsiConsole.WriteLine(); + } + + private enum PlanTurnOutcome { None, Executed, Rejected, Cancelled } + + private PlanTurnOutcome _lastPlanOutcome = PlanTurnOutcome.None; + + // Bounds the answer-directly follow-up to a single extra turn. Without it a model that responds + // to a rejection by proposing another plan could ping-pong indefinitely. + private int _planFollowUpDepth; + + /// + /// Runs a plan the model proposed during the turn that just finished, then records the outcome + /// in chat history. No-op when no plan was proposed. + /// + /// + /// An executed plan's manifest goes into history as an assistant note rather than being fed back + /// to the model for a closing turn. That is deliberate: giving the model an open turn after a + /// plan is exactly what used to make it redo the work, and no amount of "the files already + /// exist" phrasing reliably stopped it. + /// + /// A rejected plan is the one case that does need another turn — the user asked for a direct + /// answer instead of stepwise execution, and before plans were deferred the model got that for + /// free by simply continuing its open turn. + /// + /// + private async Task RunPendingPlanAsync() + { + if (!PlanHandoff.HasPendingProposal) return; + + // Already inside the post-rejection follow-up: the user has just declined a plan, so + // silently running another one would be the opposite of what they asked for. + if (_planFollowUpDepth > 0) + { + PlanHandoff.ClearPendingProposal(); + return; + } + + var ct = _requestCts?.Token ?? CancellationToken.None; + + // The user cancelled this turn; running the plan it proposed is the opposite of what they + // asked for, and leaving it queued would run it at the end of some later, unrelated turn. + if (ct.IsCancellationRequested) + { + PlanHandoff.ClearPendingProposal(); + return; + } + + _lastPlanOutcome = PlanTurnOutcome.None; + var manifest = await PlanHandoff.RunPendingPlanAsync(ct); + + if (_lastPlanOutcome == PlanTurnOutcome.Rejected) + { + _planFollowUpDepth++; + try + { + await ProcessDirectRequestAsync( + "[system: the user reviewed your proposed plan and chose to skip stepwise " + + "execution. Answer their original request directly now. Do not call propose_plan.]"); + } + finally + { + _planFollowUpDepth--; + } + return; + } + + if (string.IsNullOrWhiteSpace(manifest)) return; + + AI.AppendAssistantNote(manifest); + + // Keep --continue honest: the plan's outcome is part of this turn. + try { SessionResumeStore.Save(ProjectRoot.ProjectRoot, AI.ExportHistoryJson()); } + catch { /* persistence must never break the chat */ } + } + private async Task HandleProposedPlanAsync(TaskPlan plan, CancellationToken ct) { string choice; @@ -1959,55 +2302,71 @@ // Stop the outer "Thinking..." spinner so it doesn't fight the approval prompt. Spinner.Stop(); - AnsiConsole.WriteLine(); - DisplayPlan(plan); - - // VDOM select instead of a Spectre SelectionPrompt — Spectre's blocking - // ReadKey races RazorConsole's keyboard pump and intermittently lost arrow - // presses (see ApprovalSelect.razor). Suppress() is still required: the - // Escape listener reads the same console and would steal keys from the - // pump exactly the way the pump stole them from Spectre. + // One suppression scope spans the approval menu AND every follow-up step picker/editor. + // Ending it after the first menu lets the background Escape listener race RazorConsole's + // keyboard pump and silently steal arrow presses and typed characters. using (KeyCoordinator.Suppress()) { - choice = await PromptPlanChoiceAsync(ct); + // Loops so editing a step returns to the menu: reviewing a plan is iterative, and + // being forced to approve or reject immediately after one edit defeats the point. + while (true) + { + AnsiConsole.WriteLine(); + DisplayPlan(plan); + + // VDOM select instead of a Spectre SelectionPrompt — Spectre's blocking + // ReadKey races RazorConsole's keyboard pump and intermittently lost arrow + // presses (see ApprovalSelect.razor). + choice = await PromptPlanChoiceAsync(ct); + + if (choice != EditPlanStepLabel) break; + + await EditPlanStepAsync(plan, ct); + } } } if (choice == CancelRequestLabel) { + _lastPlanOutcome = PlanTurnOutcome.Cancelled; AnsiConsole.MarkupLine("[dim]Plan cancelled.[/]"); AnsiConsole.WriteLine(); - // Returning a "stop here" string is a POLITE REQUEST that small models ignore — - // observed live: the model received it and simply executed the cancelled plan's - // steps itself via direct tool calls. Cancel the request token so the turn - // mechanically unwinds (same path as Esc); the string below only matters in the - // narrow window before the cancellation lands. + // Cancel the token rather than trusting a "stop here" string. This predates deferred + // execution — observed live, the model received exactly that string and went on to + // execute the cancelled plan's steps itself via direct tool calls. The model has no + // open turn to do that in any more, but cancelling is still what unwinds the plan run + // itself (same path as Esc), so it stays. _requestCts?.Cancel(); return "User cancelled the request. Stop here and do not take further action."; } if (choice == RejectPlanLabel) { - AnsiConsole.MarkupLine("[dim]Plan rejected — continuing without stepwise execution.[/]"); + _lastPlanOutcome = PlanTurnOutcome.Rejected; + AnsiConsole.MarkupLine("[dim]Plan rejected — answering directly instead.[/]"); AnsiConsole.WriteLine(); - return "User rejected the proposed plan. Respond to the original request directly without calling propose_plan again."; + // The caller turns this into a fresh follow-up turn: with execution deferred there is + // no open turn left for the model to answer in, so one is started explicitly. + return "User rejected the proposed plan."; } + _lastPlanOutcome = PlanTurnOutcome.Executed; + AnsiConsole.WriteLine(); AnsiConsole.MarkupLine("[green]Executing plan...[/]"); AnsiConsole.WriteLine(); try { - await foreach (var progressEvent in TaskPlanner.ExecutePlanAsync(plan, ct)) + await foreach (var progressEvent in PlanRunners.Current.ExecutePlanAsync(plan, ct)) { - HandleProgressEvent(progressEvent, plan); + await HandleProgressEventAsync(progressEvent, plan, ct); } } catch (OperationCanceledException) { Spinner.Stop(); - TaskPlanner.CancelPlan(plan); + PlanRunners.Current.CancelPlan(plan); } AnsiConsole.WriteLine(); @@ -2015,6 +2374,10 @@ { AnsiConsole.MarkupLine("[green]Plan completed successfully![/]"); } + else if (plan.Status == TaskPlanStatus.CompletedWithIssues) + { + AnsiConsole.MarkupLine("[yellow]Plan completed with skipped or failed steps.[/]"); + } else if (plan.Status == TaskPlanStatus.Cancelled) { AnsiConsole.MarkupLine("[yellow]Plan was cancelled.[/]"); @@ -2041,18 +2404,32 @@ "Do NOT create more files, call more tools, or propose another plan."; } + /// + /// Renders the proposed plan for approval, including each step's instruction. + /// + /// + /// The instruction is shown because it is the text actually sent to the model — the + /// description is a ≤60-character label for this table and nothing else. Showing only the + /// label meant approving work you had not read: two steps can look identical in summary and + /// target completely different files. + /// private void DisplayPlan(TaskPlan plan) { var table = new Spectre.Console.Table() .Border(TableBorder.Rounded) .AddColumn(new TableColumn("Step").Centered()) - .AddColumn(new TableColumn("Description")); + .AddColumn(new TableColumn("Description")) + .AddColumn(new TableColumn("What it will do")); foreach (var step in plan.Steps) { + var instruction = step.Instruction ?? ""; + // Wrapped rather than truncated: a clipped instruction is the same problem as showing + // none — you cannot approve what you cannot read. table.AddRow( $"[deepskyblue1]{step.StepNumber}[/]", - Spectre.Console.Markup.Escape(step.Description) + Spectre.Console.Markup.Escape(step.Description), + $"[dim]{Spectre.Console.Markup.Escape(instruction)}[/]" ); } @@ -2062,7 +2439,149 @@ AnsiConsole.WriteLine(); } - private void HandleProgressEvent(TaskProgressEvent progressEvent, TaskPlan plan) + /// + /// Lets the user rewrite a step's instruction before the plan runs. + /// + /// + /// Edits the instruction, not the description: the description is a display label, so changing + /// it would alter what the table says without changing what actually happens — the worst of + /// both. The step's own text is the thing with consequences. + /// + private async Task EditPlanStepAsync( + TaskPlan plan, + CancellationToken ct, + int minimumStepNumber = 1, + bool reviseFollowingSteps = true) + { + // This method owns two consecutive VDOM inputs (row selector, then text editor). + // Keep the background Escape listener out even if a future caller forgets to suppress it. + using var keyboardOwnership = KeyCoordinator.Suppress(); + + AnsiConsole.WriteLine(); + + const string goBackLabel = "Go back"; + var selectableSteps = plan.Steps + .Where(candidate => candidate.StepNumber >= minimumStepNumber) + .ToArray(); + if (selectableSteps.Length == 0) + { + AnsiConsole.MarkupLine("[dim]No step selected.[/]"); + AnsiConsole.WriteLine(); + return false; + } + + var stepLabels = selectableSteps + .Select(candidate => $"Step {candidate.StepNumber} — {candidate.Description}") + .Append(goBackLabel) + .ToArray(); + var stepOptions = stepLabels + .Select((label, index) => new ApprovalSelectCoordinator.Option( + label, + index < selectableSteps.Length ? Color.DeepSkyBlue1 : new Color(255, 200, 80))) + .ToArray(); + + // ApprovalSelect keeps a stable row shape while repainting both the highlight and cursor. + // RazorConsole's generic Select left its '>' marker behind on the first row even while the + // highlight moved. The custom component was written specifically to avoid that VDOM diff bug. + AnsiConsole.MarkupLine("[deepskyblue1]Select a step to edit (Up/Down, Enter):[/]"); + var selectedLabel = await ApprovalMenu.RequestAsync(stepOptions); + + if (selectedLabel == goBackLabel) + { + AnsiConsole.MarkupLine("[dim]No step selected.[/]"); + AnsiConsole.WriteLine(); + return false; + } + + var selectedIndex = Array.IndexOf(stepLabels, selectedLabel); + if (selectedIndex < 0 || selectedIndex >= selectableSteps.Length) + return false; + + var step = selectableSteps[selectedIndex]; + + var revised = await InstructionCoordinator.RequestAsync( + $"Edit step {step.StepNumber} instruction (Enter to save):", + step.Instruction); + + if (string.IsNullOrWhiteSpace(revised) || + string.Equals(revised.Trim(), step.Instruction?.Trim(), StringComparison.Ordinal)) + { + AnsiConsole.MarkupLine("[dim]Left unchanged.[/]"); + AnsiConsole.WriteLine(); + return false; + } + + var trimmed = revised.Trim(); + step.Instruction = trimmed; + // A stale label makes the review table misleading, so always derive it from the edited text. + step.Description = trimmed.Length > 60 ? trimmed[..57] + "..." : trimmed; + + AnsiConsole.MarkupLine($"[green]Step {step.StepNumber} updated.[/]"); + AnsiConsole.WriteLine(); + + if (reviseFollowingSteps && plan.Steps.Any(candidate => candidate.StepNumber > step.StepNumber)) + await ReviseFollowingStepsAfterEditAsync(plan, step, ct); + + return true; + } + + private async Task ReviseFollowingStepsAfterEditAsync( + TaskPlan plan, + TaskStep editedStep, + CancellationToken ct) + { + AnsiConsole.MarkupLine( + $"[dim]Updating steps after {editedStep.StepNumber} so they stay consistent with your edit...[/]"); + Spinner.Start("Updating dependent steps..."); + + try + { + var earlier = plan.Steps + .Where(step => step.StepNumber < editedStep.StepNumber) + .Select(step => $"Step {step.StepNumber}: {step.Description}\nInstruction: {step.Instruction}"); + var later = plan.Steps + .Where(step => step.StepNumber > editedStep.StepNumber) + .Select(step => $"Step {step.StepNumber}: {step.Description}\nInstruction: {step.Instruction}"); + var context = + $"The user edited step {editedStep.StepNumber}. Their edited instruction is authoritative and must not be changed:\n" + + $"{editedStep.Instruction}\n\n" + + "Earlier steps that must remain unchanged:\n" + string.Join("\n\n", earlier) + "\n\n" + + "Return only replacement steps that come after the edited step. Update paths, values, and verification " + + "expectations so they are consistent with the edit. State each replacement as the actual work to execute; " + + "never say to replace, update, or revise a plan step, and never refer to old step numbers. " + + "Do not repeat earlier or edited steps.\n\n" + + "Current later steps to replace:\n" + string.Join("\n\n", later); + + var revision = await AI.GeneratePlanAsync(plan.OriginalRequest, context, ct); + if (revision.Steps.Length == 1 && + revision.Steps[0].description == "Complete the requested goal" && + revision.Steps[0].instruction == plan.OriginalRequest.Trim()) + throw new InvalidOperationException("The model did not return a usable dependent-step revision."); + + var candidate = PlanRevision.CreateFollowingCandidate(plan, editedStep.StepNumber, revision); + PlanRevision.ApplyFollowing(plan, editedStep.StepNumber, candidate); + AnsiConsole.MarkupLine( + "[green]Dependent steps updated.[/] [dim]Review the complete plan before executing it.[/]"); + AnsiConsole.WriteLine(); + } + catch (OperationCanceledException) { throw; } + catch (Exception ex) + { + AnsiConsole.MarkupLine( + $"[yellow]Could not update dependent steps automatically:[/] {Spectre.Console.Markup.Escape(ex.Message)}"); + AnsiConsole.MarkupLine("[dim]Review the later steps manually before execution.[/]"); + AnsiConsole.WriteLine(); + } + finally + { + Spinner.Stop(); + } + } + + private async Task HandleProgressEventAsync( + TaskProgressEvent progressEvent, + TaskPlan plan, + CancellationToken ct) { switch (progressEvent.ProgressType) { @@ -2097,11 +2616,30 @@ SpinnerService.SetTaskbarError(progressEvent.CurrentStep * 100 / progressEvent.TotalSteps); AnsiConsole.MarkupLine($"[red]Step {progressEvent.CurrentStep} failed:[/] {Spectre.Console.Markup.Escape(progressEvent.Message ?? "Unknown error")}"); + // Cancellation is already the user's terminal decision. Diff/command approval + // cancellation reports a StepFailed event while also marking the plan cancelled; + // do not follow that with a contradictory retry/replan/skip prompt. + if (plan.Status == TaskPlanStatus.Cancelled || ct.IsCancellationRequested) + { + SpinnerService.ClearTaskbarProgress(); + break; + } + // Ask user what to do - // Palette: gold = redirect (skip), red = destructive (kill the plan). + // Palette: green = try again, gold = redirect (skip), red = destructive. + const string retryStepLabel = "[green]Retry this step[/]"; + const string revisePlanLabel = "[deepskyblue1]Revise the remaining plan[/]"; const string skipStepLabel = "[rgb(255,200,80)]Skip this step and continue[/]"; const string cancelPlanLabel = "[red]Cancel the plan[/]"; + var stepToDecide = plan.Steps.FirstOrDefault(s => s.StepNumber == progressEvent.CurrentStep); + + // Retry is offered only on the workflow engine. The legacy runner walks the steps + // with a foreach and has already moved past this one — it has no way back. + var choices = PlanRunners.UsingWorkflowEngine + ? new[] { retryStepLabel, revisePlanLabel, skipStepLabel, cancelPlanLabel } + : new[] { skipStepLabel, cancelPlanLabel }; + string failChoice; using (KeyCoordinator.Suppress()) { @@ -2109,22 +2647,37 @@ new SelectionPrompt() .Title("[deepskyblue1]How would you like to proceed?[/]") .HighlightStyle(SelectionHighlight) - .AddChoices(new[] { skipStepLabel, cancelPlanLabel }) + .AddChoices(choices) ); } if (failChoice == cancelPlanLabel) { - TaskPlanner.CancelPlan(plan); + PlanRunners.Current.CancelPlan(plan); } - else + else if (failChoice == retryStepLabel && stepToDecide != null) { - var failedStep = plan.Steps.FirstOrDefault(s => s.StepNumber == progressEvent.CurrentStep); - if (failedStep != null) - { - TaskPlanner.SkipStep(plan, failedStep); - SpinnerService.SetTaskbarWarning(progressEvent.CurrentStep * 100 / progressEvent.TotalSteps); - } + // Pending is the signal triage reads as "run this one again". + stepToDecide.Status = TaskStepStatus.Pending; + AnsiConsole.MarkupLine($"[dim]Retrying step {progressEvent.CurrentStep}...[/]"); + AnsiConsole.WriteLine(); + } + else if (failChoice == revisePlanLabel && stepToDecide != null) + { + var decision = await ReplanAfterFailureAsync( + plan, + stepToDecide, + progressEvent.Message ?? "Unknown error", + ct); + if (decision == CliReplanDecision.Cancel) + PlanRunners.Current.CancelPlan(plan); + else if (decision == CliReplanDecision.KeepCurrent) + PlanRunners.Current.SkipStep(plan, stepToDecide); + } + else if (stepToDecide != null) + { + PlanRunners.Current.SkipStep(plan, stepToDecide); + SpinnerService.SetTaskbarWarning(progressEvent.CurrentStep * 100 / progressEvent.TotalSteps); } break; @@ -2140,6 +2693,100 @@ } } + private enum CliReplanDecision { Applied, KeepCurrent, Cancel } + + private async Task ReplanAfterFailureAsync( + TaskPlan plan, + TaskStep failedStep, + string error, + CancellationToken ct) + { + Spinner.Start("Revising plan..."); + AnsiConsole.MarkupLine("[dim]Revising the unfinished portion of the plan...[/]"); + + TaskPlan candidate; + try + { + var settled = plan.Steps + .Where(step => step.StepNumber < failedStep.StepNumber) + .Select(step => $"Step {step.StepNumber} [{step.Status}]: {step.Description}\nResult: {step.Result ?? "(none)"}"); + var remaining = plan.Steps + .Where(step => step.StepNumber >= failedStep.StepNumber) + .Select(step => $"Step {step.StepNumber}: {step.Description}\nInstruction: {step.Instruction}"); + var context = + $"Failed step {failedStep.StepNumber}: {failedStep.Description}\n" + + $"Failure: {error}\n\n" + + "Settled earlier steps:\n" + string.Join("\n\n", settled) + "\n\n" + + "Return only replacements for the failed and later steps. State each replacement as the actual work to " + + "execute; never say to replace, update, or revise a plan step, and never refer to old step numbers. " + + "Do not repeat settled steps.\n\n" + + "Current failed and remaining steps to replace:\n" + string.Join("\n\n", remaining); + + var revision = await AI.GeneratePlanAsync(plan.OriginalRequest, context, ct); + if (revision.Steps.Length == 1 && + revision.Steps[0].description == "Complete the requested goal" && + revision.Steps[0].instruction == plan.OriginalRequest.Trim()) + throw new InvalidOperationException("The model did not return a usable revision."); + + candidate = PlanRevision.CreateCandidate(plan, failedStep.StepNumber, revision); + } + catch (OperationCanceledException) { throw; } + catch (Exception ex) + { + AnsiConsole.MarkupLine($"[red]Could not revise the plan:[/] {Spectre.Console.Markup.Escape(ex.Message)}"); + return CliReplanDecision.KeepCurrent; + } + finally + { + Spinner.Stop(); + } + + const string useLabel = "[green]Use revised plan[/]"; + const string editLabel = "[deepskyblue1]Edit a step[/]"; + const string keepLabel = "[rgb(255,200,80)]Keep current plan and skip this step[/]"; + const string cancelLabel = "[red]Cancel the plan[/]"; + string choice; + + // Keep the background Escape listener suppressed across the revised-plan menu and + // its nested row selector/text editor. Releasing ownership between those VDOM inputs + // makes the listener consume their keys before RazorConsole sees them. + using (KeyCoordinator.Suppress()) + { + while (true) + { + AnsiConsole.WriteLine(); + DisplayPlan(candidate); + AnsiConsole.MarkupLine( + $"[dim]Steps before {failedStep.StepNumber} are settled and will not run again.[/]"); + + choice = AnsiConsole.Prompt( + new SelectionPrompt() + .Title("[deepskyblue1]Review the revised remaining plan[/]") + .HighlightStyle(SelectionHighlight) + .AddChoices(useLabel, editLabel, keepLabel, cancelLabel)); + + if (choice == editLabel) + { + await EditPlanStepAsync( + candidate, + ct, + failedStep.StepNumber, + reviseFollowingSteps: false); + continue; + } + if (choice == cancelLabel) return CliReplanDecision.Cancel; + if (choice == keepLabel) return CliReplanDecision.KeepCurrent; + + PlanRevision.ApplyApproved(plan, failedStep.StepNumber, candidate); + AnsiConsole.MarkupLine( + $"[green]Revised plan approved[/] [dim]— resuming at step {failedStep.StepNumber} of {plan.Steps.Count}.[/]"); + AnsiConsole.WriteLine(); + return CliReplanDecision.Applied; + } + } + } + + private void HandleSkillsListCommand() { var skills = Skills.GetAll(); diff --git a/src/MandoCode/Components/Banner.razor b/src/MandoCode/Components/Banner.razor index debdd53..048632d 100644 --- a/src/MandoCode/Components/Banner.razor +++ b/src/MandoCode/Components/Banner.razor @@ -1,4 +1,5 @@ @using Spectre.Console; +@using System.Reflection; @code { // Gate the easter-egg roll so it happens at most once per process launch. @@ -20,9 +21,7 @@ } } - var version = typeof(Program).Assembly.GetName().Version; - var versionStr = version != null ? $"v{version.Major}.{version.Minor}.{version.Build}" : ""; - var gradientText = BuildGradientBanner(versionStr); + var gradientText = BuildGradientBanner(ResolveVersionLabel()); var panel = new Spectre.Console.Panel(new Spectre.Console.Markup("\n\n" + gradientText + "\n\n")); panel.Border(BoxBorder.Rounded); @@ -31,6 +30,9 @@ AnsiConsole.Write(panel); } + private static string ResolveVersionLabel() + => MandoCode.Services.VersionLabel.ForAssembly(typeof(Program).Assembly); + // ════════════════════════════════════════════ // Gradient MANDOCODE figlet text // ════════════════════════════════════════════ diff --git a/src/MandoCode/Components/PromptInput.razor b/src/MandoCode/Components/PromptInput.razor index 81bc4f4..6992098 100644 --- a/src/MandoCode/Components/PromptInput.razor +++ b/src/MandoCode/Components/PromptInput.razor @@ -6,7 +6,9 @@ OnAutocompleteRequested { get; set; } [Parameter] public bool Disabled { get; set; } [Parameter] public string InitialValue { get; set; } = ""; + [Parameter] public string Label { get; set; } = ""; + [Parameter] public Color LabelColor { get; set; } = Color.Green; + [Parameter] public string Placeholder { get; set; } = "type / for commands, @ for files"; + [Parameter] public bool ChatFeaturesEnabled { get; set; } = true; + [Parameter] public bool AllowEmpty { get; set; } [Inject] private InputStateMachine SM { get; set; } = default!; private string _inputValue = ""; private DateTime _lastInputAt = DateTime.MinValue; private int _lastLength = 0; + private bool _initialValueApplied; protected override void OnParametersSet() { - if (!string.IsNullOrEmpty(InitialValue)) + if (!_initialValueApplied && !string.IsNullOrEmpty(InitialValue)) { _inputValue = InitialValue; - InitialValue = ""; // Consume it so it doesn't re-apply on next render + _initialValueApplied = true; } } @@ -55,13 +63,14 @@ _inputValue = sanitized; - var action = SM.UpdateText(sanitized); + var action = ChatFeaturesEnabled ? SM.UpdateText(sanitized) : InputAction.Noop; _lastInputAt = DateTime.UtcNow; _lastLength = sanitized.Length; // When autocomplete triggers, hand off to imperative mode - if (action == InputAction.ShowCommandDropdown || action == InputAction.ShowFileDropdown) + if (ChatFeaturesEnabled && + (action == InputAction.ShowCommandDropdown || action == InputAction.ShowFileDropdown)) { await OnAutocompleteRequested.InvokeAsync(sanitized); } @@ -70,7 +79,7 @@ private void HandleSubmit(string value) { if (Disabled) return; - if (string.IsNullOrWhiteSpace(value)) return; + if (!AllowEmpty && string.IsNullOrWhiteSpace(value)) return; // Paste-burst guard: a Submit firing within 30ms of the last OnInput is the '\r' // from a paste burst, not a real user Enter. Humans can't reach Enter that fast. @@ -78,11 +87,13 @@ if (sinceInput < 30) { SetValue(value); - SM.UpdateText(value); + if (ChatFeaturesEnabled) + SM.UpdateText(value); return; } - SM.SubmitInput(value); + if (ChatFeaturesEnabled) + SM.SubmitInput(value); var submitted = value; _inputValue = ""; diff --git a/src/MandoCode/MandoCode.csproj b/src/MandoCode/MandoCode.csproj index b92a213..f2e1f73 100644 --- a/src/MandoCode/MandoCode.csproj +++ b/src/MandoCode/MandoCode.csproj @@ -55,7 +55,15 @@ Every change is covered by automated tests — the suite grew to 486 checks. agent-framework-migration.md. --> - + + + + + + diff --git a/src/MandoCode/Models/ConfigKeySetter.cs b/src/MandoCode/Models/ConfigKeySetter.cs index 5637be9..0c18366 100644 --- a/src/MandoCode/Models/ConfigKeySetter.cs +++ b/src/MandoCode/Models/ConfigKeySetter.cs @@ -142,6 +142,30 @@ public static SetResult TrySet(MandoCodeConfig config, string key, string value) } return Fail("Error: Value must be 'true' or 'false'"); + case "planner": + case "plannerengine": + // Deliberately separate from taskPlanning, which decides whether there is a + // planner at all (it gates registering propose_plan). Overloading that key + // would make "planning off" and "old engine" the same state and render any + // A/B between the two engines uninterpretable. + var planner = value.Trim().ToLowerInvariant(); + if (planner is "default" or "auto" or "clear") + { + config.PlannerEngine = null; + return new(true, "✓ Planner engine reset to this build's default", ApplyScope.KernelRebuild); + } + if (planner == MandoCodeConfig.PlannerEngineLegacy) + { + config.PlannerEngine = planner; + return new(true, "✓ Planner engine set to: legacy", ApplyScope.KernelRebuild); + } + if (planner == MandoCodeConfig.PlannerEngineWorkflow) + { + config.PlannerEngine = planner; + return new(true, "✓ Planner engine set to: workflow (experimental)", ApplyScope.KernelRebuild); + } + return Fail("Error: Value must be 'legacy', 'workflow', or 'default'"); + case "streaming": case "responsestreaming": var streamMode = value.Trim().ToLowerInvariant(); @@ -249,6 +273,7 @@ public static string DescribeKeys(MandoCodeConfig config) => maxContinuations {config.MaxAutoContinuations} ({MandoCodeConfig.MinMaxAutoContinuations}-{MandoCodeConfig.MaxMaxAutoContinuations}) renderTimeout {config.MarkdownRenderTimeoutSeconds}s ({MandoCodeConfig.MinMarkdownRenderTimeoutSeconds}-{MandoCodeConfig.MaxMarkdownRenderTimeoutSeconds}) taskPlanning {config.EnableTaskPlanning} + planner {config.PlannerEngine ?? "default"} (legacy | workflow | default) diffApprovals {config.EnableDiffApprovals} webSearch {config.EnableWebSearch} tavilyKey {(string.IsNullOrWhiteSpace(config.TavilyApiKey) ? "not set" : MandoCodeConfig.MaskApiKey(config.TavilyApiKey))} (Tavily API key for reliable web search — free at https://app.tavily.com; "clear" to remove) diff --git a/src/MandoCode/Models/GeneratedPlan.cs b/src/MandoCode/Models/GeneratedPlan.cs new file mode 100644 index 0000000..7d13fc0 --- /dev/null +++ b/src/MandoCode/Models/GeneratedPlan.cs @@ -0,0 +1,9 @@ +using MandoCode.Plugins; + +namespace MandoCode.Models; + +/// +/// A plan produced by the model in proposal-only mode. No tools other than +/// propose_plan are available while this value is generated. +/// +public sealed record GeneratedPlan(string Goal, PlanStepProposal[] Steps); diff --git a/src/MandoCode/Models/MandoCodeConfig.cs b/src/MandoCode/Models/MandoCodeConfig.cs index 7f03ad6..7b2067c 100644 --- a/src/MandoCode/Models/MandoCodeConfig.cs +++ b/src/MandoCode/Models/MandoCodeConfig.cs @@ -304,6 +304,31 @@ public static int RecommendedContextLength(string? modelTag) [JsonPropertyName("enableTaskPlanning")] public bool EnableTaskPlanning { get; set; } = true; + /// Planner engine: the original in-tool-call runner. + public const string PlannerEngineLegacy = "legacy"; + + /// Planner engine: the MAF workflow graph (lands in a later phase). + public const string PlannerEngineWorkflow = "workflow"; + + /// + /// Which planner engine to use, or null for whatever this build defaults to. + /// Distinct from , which decides whether there is a planner + /// at all. + /// + /// + /// Nullable on purpose, and it must stay that way. When the default eventually flips to + /// , a null here still means "follow the build" while an + /// explicit "legacy" still means "the user chose this" — so the flip is a one-line change with + /// no config migration and no version bump. With a non-nullable default the two states are + /// indistinguishable, and Migrate() would have to guess, exactly as it already has to + /// for ModelResponseTimeoutSeconds. Also written only when set, so older builds — whose reader + /// has no UnmappedMemberHandling and whose Save() reserializes the whole object — don't + /// silently drop a key they never knew about. + /// + [JsonPropertyName("planner")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string? PlannerEngine { get; set; } + /// /// Enable fallback parsing for function calls output as JSON text. /// Some local models output function calls as text instead of proper tool calls. diff --git a/src/MandoCode/Models/SlashCommands.cs b/src/MandoCode/Models/SlashCommands.cs index 5706daa..a52bb41 100644 --- a/src/MandoCode/Models/SlashCommands.cs +++ b/src/MandoCode/Models/SlashCommands.cs @@ -21,6 +21,9 @@ public static class SlashCommands { "/copy", "Copy last AI response to clipboard" }, { "/copy-code", "Copy code blocks from last AI response" }, { "/command", "Run a shell command (also: !)" }, + { "/plan", "Force planning for a goal, or show an unfinished plan" }, + { "/plan-resume", "Continue an unfinished plan where it left off" }, + { "/plan-discard", "Forget an unfinished plan" }, { "/clear", "Clear conversation history" }, { "/learn", "Learn about LLMs and local AI models" }, { "/retry", "Retry Ollama connection" }, diff --git a/src/MandoCode/Models/SystemPrompts.cs b/src/MandoCode/Models/SystemPrompts.cs index 6bfb259..7d9cb07 100644 --- a/src/MandoCode/Models/SystemPrompts.cs +++ b/src/MandoCode/Models/SystemPrompts.cs @@ -101,20 +101,22 @@ summarize web content clearly — don't dump raw text at the user." 1. Call the appropriate function 2. Wait for the result 3. Use that result to formulate a helpful, conversational response to the user especially when assisting with coding tasks. -4. If the user needs help with coding tasks. Make a plan before executing any functions. Communicate your plan to the user in natural language. +4. For coding tasks, think through the approach before calling functions. If the work spans several files or systems, use propose_plan (see MULTI-STEP PLANNING below) rather than narrating a plan in prose — a proposed plan is reviewable and gets executed for you; a described one is neither. PROGRESS UPDATES (IMPORTANT): When working on multi-step tasks (creating projects, building games, refactoring multiple files, etc.): -- Before each major step, output a clear status line showing what you're currently doing. Use a format like: - ⚙️ (Step 1/5) Setting up project structure... - ⚙️ (Step 2/5) Creating world generation system... - ✅ (Step 2/5) World generation complete! - ⚙️ (Step 3/5) Building inventory UI... +- Before each major piece of work, output a clear status line saying what you are doing right now. Use a format like: + ⚙️ Setting up project structure... + ⚙️ Creating world generation system... + ✅ World generation complete! + ⚙️ Building inventory UI... +- Do NOT number these lines and do NOT invent a total. You cannot know how many there will be, and + when a plan is running MandoCode already shows the real step number above your output — a made-up + count contradicts it and makes the progress display look wrong. - NEVER use square brackets in your progress lines or status updates. Use parentheses instead. -- After completing each step, briefly confirm it's done before moving to the next +- After finishing each piece, briefly confirm it's done before moving to the next - At the end, provide a summary of everything that was created or changed - This helps the user see real-time progress instead of waiting in silence for a large final output -- Always number your steps so the user knows how far along you are MULTI-STEP PLANNING: For requests that clearly require multiple distinct operations on different files @@ -128,10 +130,12 @@ any work. - Lookups (""show me the config"", ""find all usages of X"") - Content you can produce in one response -When you call propose_plan, the user approves or rejects. If approved, each step -is executed one at a time with full context. If rejected, they may redirect you -or cancel. You will receive a summary string when planning completes — treat it -as the final outcome and respond conversationally. +propose_plan returns as soon as the plan is queued — it does NOT wait for the plan +to run. When it returns, your job for this turn is finished: reply with one short +sentence telling the user their plan is ready to review, and stop. Do not start +the work, do not call other tools, and do not call propose_plan again. The user +is shown the plan for approval immediately afterwards, and the approved steps are +executed for you. Do NOT call propose_plan from inside a plan step that is already running. diff --git a/src/MandoCode/Models/TaskPlan.cs b/src/MandoCode/Models/TaskPlan.cs index a1ebeac..4ee37a1 100644 --- a/src/MandoCode/Models/TaskPlan.cs +++ b/src/MandoCode/Models/TaskPlan.cs @@ -93,6 +93,9 @@ public enum TaskPlanStatus /// All steps completed successfully. Completed, + /// The plan reached the end, but one or more steps were skipped after a failure. + CompletedWithIssues, + /// User cancelled the plan. Cancelled, diff --git a/src/MandoCode/Models/TaskProgressEvent.cs b/src/MandoCode/Models/TaskProgressEvent.cs index 114e684..14597f6 100644 --- a/src/MandoCode/Models/TaskProgressEvent.cs +++ b/src/MandoCode/Models/TaskProgressEvent.cs @@ -93,7 +93,9 @@ public class TaskProgressEvent : StreamEvent TotalSteps = plan.Steps.Count, CurrentStep = plan.Steps.Count, Plan = plan, - Message = "All steps completed successfully" + Message = plan.Status == TaskPlanStatus.CompletedWithIssues + ? "Plan reached the end with skipped or failed steps" + : "All steps completed successfully" }; /// diff --git a/src/MandoCode/Program.cs b/src/MandoCode/Program.cs index 1dad660..a731b1a 100644 --- a/src/MandoCode/Program.cs +++ b/src/MandoCode/Program.cs @@ -164,6 +164,19 @@ static async Task Main(string[] args) return new TaskPlannerService(aiService, cfg); }); + // Which engine actually runs a plan is decided per plan, not here — see + // PlanRunnerSelector, which re-reads the `planner` key so the choice can be flipped + // mid-session without losing history. + services.AddSingleton(provider => + new AiServicePlanStepExecutor(provider.GetRequiredService())); + + services.AddSingleton(provider => new PlanRunnerSelector( + provider.GetRequiredService(), + provider.GetRequiredService(), + provider.GetRequiredService(), + provider.GetRequiredService(), + provider.GetRequiredService())); + // Register MusicPlayerService as singleton services.AddSingleton(provider => { diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index dd7d278..752999e 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -393,7 +393,14 @@ private void BuildAgent() // version ships — see memory agent-framework-migration.md. var baseAgent = ollamaClient.AsAIAgent(new ChatClientAgentOptions { - Name = "MandoCode", + // Id is pinned, not left to MAF to synthesize, because workflow-executor identity + // derives from BOTH Id and Name. BuildAgent runs again on every MCP reconcile and + // every KernelRebuild-scoped /config set, so a synthesized Id would change + // mid-session and orphan any checkpoint written before it — silently, with no error. + // Must never incorporate anything volatile (model, temperature, project path). + // See PlanExecutorIds for the rest of the identity scheme. + Id = PlanExecutorIds.GeneralistAgentId, + Name = PlanExecutorIds.GeneralistAgentName, ChatOptions = new Microsoft.Extensions.AI.ChatOptions { Instructions = _systemPrompt, @@ -446,6 +453,225 @@ private void BuildAgent() private static AIFunction NamedTool(Delegate method, string name) => AIFunctionFactory.Create(method, new AIFunctionFactoryOptions { Name = name }); + /// + /// Generates a plan without entering the normal agent/tool loop. The throwaway client is + /// deliberately given exactly one tool and required to call a tool, so callers such as + /// /plan <goal> and failure replanning get a deterministic proposal rather than relying + /// on the normal agent's planning heuristic. + /// + public async Task GeneratePlanAsync( + string request, + string? revisionContext = null, + CancellationToken cancellationToken = default) + { + if (string.IsNullOrWhiteSpace(request)) + throw new ArgumentException("A planning goal is required.", nameof(request)); + + var planningPlugin = new PlanningPlugin(); + var proposePlan = NamedTool(planningPlugin.ProposePlan, "propose_plan"); + + using var httpClient = new HttpClient(new NumCtxHttpHandler(EffectiveNumCtx)) + { + BaseAddress = new Uri(_config.OllamaEndpoint), + Timeout = System.Threading.Timeout.InfiniteTimeSpan + }; + using IChatClient client = new OllamaApiClient(httpClient, _config.GetEffectiveModelName()); + + var system = revisionContext == null + ? "You are a software implementation planner. Break the user's goal into concrete, ordered, " + + "independently verifiable steps. You must call propose_plan exactly once. Do not perform work." + : "You are revising the unfinished portion of a software implementation plan because execution evidence " + + "or a user edit made the current remainder stale. Use the supplied context to replace only the requested " + + "remaining work with concrete, ordered, independently verifiable steps. Write every returned step as the " + + "actual work to execute. Never describe the act of revising the plan, say 'replace/update/revise step', " + + "or refer to old step numbers. You must call propose_plan exactly once. Do not perform work."; + + var messages = new List + { + new(ChatRole.System, system), + new(ChatRole.User, revisionContext == null + ? request + : $"Original request:\n{request}\n\nRevision context:\n{revisionContext}") + }; + + GeneratedPlanArguments? arguments = null; + try + { + var response = await client.GetResponseAsync(messages, new ChatOptions + { + Temperature = 0.2f, + MaxOutputTokens = _config.MaxTokens, + Tools = [proposePlan], + ToolMode = ChatToolMode.RequireSpecific("propose_plan") + }, cancellationToken); + + var call = response.Messages + .SelectMany(message => message.Contents) + .OfType() + .FirstOrDefault(content => content.Name == "propose_plan"); + if (call != null) + arguments = DeserializePlanArguments(JsonSerializer.Serialize(call.Arguments)); + } + catch (OperationCanceledException) { throw; } + catch + { + // Some Ollama-compatible providers reject named tool choice even though they support + // tools. Fall through to schema-constrained JSON rather than making /plan heuristic. + } + + if (TryMaterializePlan(arguments, out var generated)) return generated; + + // Provider ignored/rejected forced tool choice. Ask for the same typed payload without + // tools, constrained by MEAI's exported JSON schema. This is still proposal-only. + try + { + var jsonOptions = new JsonSerializerOptions(JsonSerializerDefaults.Web) + { + PropertyNameCaseInsensitive = true + }; + var jsonResponse = await client.GetResponseAsync( + [ + new ChatMessage(ChatRole.System, + system + " Return only JSON matching the requested schema. Every step requires " + + "a non-empty description and instruction."), + messages[1] + ], + new ChatOptions + { + Temperature = 0.2f, + MaxOutputTokens = _config.MaxTokens, + ResponseFormat = ChatResponseFormat.ForJsonSchema(jsonOptions) + }, + cancellationToken); + arguments = DeserializePlanArguments(jsonResponse.Text); + } + catch (OperationCanceledException) { throw; } + catch + { + // Last-resort host fallback below keeps the explicit command deterministic even for a + // provider that supports neither tool choice nor schema-constrained output. + } + + if (TryMaterializePlan(arguments, out generated)) return generated; + + var fallbackGoal = string.Join(" ", request + .Split(['\r', '\n'], StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)); + if (fallbackGoal.Length > 160) fallbackGoal = fallbackGoal[..157] + "..."; + return new GeneratedPlan( + fallbackGoal, + [new PlanStepProposal("Complete the requested goal", request.Trim())]); + } + + private sealed record GeneratedPlanArguments(string? Goal, PlanStepProposal[]? Steps); + + private static GeneratedPlanArguments? DeserializePlanArguments(string? json) + { + if (string.IsNullOrWhiteSpace(json)) return null; + var options = new JsonSerializerOptions { PropertyNameCaseInsensitive = true }; + try { return JsonSerializer.Deserialize(json, options); } + catch (JsonException) + { + // Tolerate providers that wrap otherwise-valid JSON in prose or a markdown fence. + var first = json.IndexOf('{'); + var last = json.LastIndexOf('}'); + if (first < 0 || last <= first) return null; + try { return JsonSerializer.Deserialize(json[first..(last + 1)], options); } + catch (JsonException) { return null; } + } + } + + private static bool TryMaterializePlan( + GeneratedPlanArguments? arguments, + [System.Diagnostics.CodeAnalysis.NotNullWhen(true)] out GeneratedPlan? generated) + { + var goal = arguments?.Goal?.Trim(); + var steps = arguments?.Steps? + .Where(step => !string.IsNullOrWhiteSpace(step.description) && + !string.IsNullOrWhiteSpace(step.instruction)) + .Select(step => new PlanStepProposal(step.description.Trim(), step.instruction.Trim())) + .ToArray() ?? []; + generated = string.IsNullOrWhiteSpace(goal) || steps.Length == 0 + ? null + : new GeneratedPlan(goal, steps); + return generated != null; + } + + /// + /// Classifies a completed model response when the step model omitted its terminal marker. This + /// is a separate proposal-only call with exactly one required tool: it cannot touch the project + /// or continue the task, and it must return a structured success decision. Forced planning has + /// already established that the configured Ollama tool path honors RequireAny. + /// + private async Task VerifyPlanStepAsync( + string stepInstruction, + string stepResponse, + CancellationToken cancellationToken) + { + Task ReportOutcome(bool success, string reason) => + Task.FromResult(success ? "verified" : reason); + + var reportTool = NamedTool( + (Func>)ReportOutcome, + "report_plan_step_outcome"); + + using var httpClient = new HttpClient(new NumCtxHttpHandler(EffectiveNumCtx)) + { + BaseAddress = new Uri(_config.OllamaEndpoint), + Timeout = System.Threading.Timeout.InfiniteTimeSpan + }; + using IChatClient client = new OllamaApiClient(httpClient, _config.GetEffectiveModelName()); + + const int maxEvidenceChars = 12_000; + var evidence = stepResponse.Length <= maxEvidenceChars + ? stepResponse + : stepResponse[..6_000] + "\n...[middle truncated]...\n" + stepResponse[^6_000..]; + + var messages = new List + { + new(ChatRole.System, + "You are a strict plan-step verifier. You must call report_plan_step_outcome exactly once. " + + "Set success=true only when the response proves the exact step instruction was satisfied. " + + "A missing requested path, wrong path, wrong content, failed command, contradictory claim, " + + "or required check that could not be performed is failure. A same-named file elsewhere does " + + "not satisfy an exact requested path. Do not perform work and do not offer remediation."), + new(ChatRole.User, + $"Step instruction:\n{stepInstruction}\n\nStep response/evidence:\n{evidence}") + }; + + var response = await client.GetResponseAsync(messages, new ChatOptions + { + Temperature = 0, + MaxOutputTokens = Math.Min(_config.MaxTokens, 1024), + Tools = [reportTool], + ToolMode = ChatToolMode.RequireAny + }, cancellationToken); + + var call = response.Messages + .SelectMany(message => message.Contents) + .OfType() + .FirstOrDefault(content => content.Name == "report_plan_step_outcome") + ?? throw new PlanStepReportedFailureException( + "The step finished, but its outcome could not be verified."); + + var arguments = JsonSerializer.Deserialize( + JsonSerializer.Serialize(call.Arguments), + new JsonSerializerOptions { PropertyNameCaseInsensitive = true }); + + if (arguments == null) + throw new PlanStepReportedFailureException( + "The step finished, but its structured verification result was invalid."); + + var reason = string.IsNullOrWhiteSpace(arguments.Reason) + ? arguments.Success + ? "The verifier confirmed the step requirements." + : "The verifier found that the step requirements were not satisfied." + : arguments.Reason.Trim(); + return new PlanStepVerification(arguments.Success, reason); + } + + private sealed record PlanStepVerificationArguments(bool Success, string? Reason); + private sealed record PlanStepVerification(bool Success, string Reason); + /// /// The num_ctx stamped on outgoing chat requests: the configured context length for /// local models, 0 (leave the request untouched) for cloud models — their context @@ -502,6 +728,7 @@ public async IAsyncEnumerable ChatStreamAsync(string userMessage, [Syste // of STarfox/. The verbatim message (with App.razor's @file/@folder expansions) // is the ground truth for target paths. _currentTurnUserMessage = userMessage; + _planHandoff.SetRequestContext(userMessage); // Add message under lock, then release before the long AI call await _historyLock.WaitAsync(cancellationToken); @@ -553,16 +780,11 @@ public async IAsyncEnumerable ChatStreamAsync(string userMessage, [Syste { using var scope = _agentFunctionMiddleware!.BeginScope(); - // pauseDuringPlan: this outer call can run a whole plan (propose_plan). Both outer - // timers (the stall watchdog and the request-timeout ceiling) pause for the plan's - // duration so neither can cancel a step and surface as a bogus "Cancelled by user." - // Each step has its own watchdog + request timeout, so steps stay bounded. var result = await ExecuteAgentModelCallAsync( _chatHistory, retryOperationName: "ChatStreamAsync", tokenLabel: "Chat", spinnerMessage: "Thinking… (Esc to cancel)", - pauseDuringPlan: true, cancellationToken); var rawResponse = string.IsNullOrEmpty(result.Text) ? "No response from AI." : result.Text; @@ -863,18 +1085,14 @@ private async Task ExecuteAgentModelCallAsync( string retryOperationName, string tokenLabel, string spinnerMessage, - bool pauseDuringPlan, - CancellationToken cancellationToken) + CancellationToken cancellationToken, + Action? onTextDelta = null) { using var requestCts = new CancellationTokenSource(TimeSpan.FromMinutes(_config.RequestTimeoutMinutes)); using var responseCts = new CancellationTokenSource(TimeSpan.FromSeconds(_config.ModelResponseTimeoutSeconds)); using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, requestCts.Token, responseCts.Token); - using var watchdog = AttachAgentStallWatchdog( - responseCts, - pauseDuringPlan, - requestCts: pauseDuringPlan ? requestCts : null, - requestTimeout: TimeSpan.FromMinutes(_config.RequestTimeoutMinutes)); + using var watchdog = AttachAgentStallWatchdog(responseCts); _spinner.Start(spinnerMessage); @@ -916,7 +1134,7 @@ void OnTraceCompleted(FunctionExecutionResult result) var messagesSnapshot = history.ToList(); var response = await RetryPolicy.ExecuteWithRetryAsync( - async () => await InvokeAgentChatAsync(messagesSnapshot, responseCts, linkedCts.Token), + async () => await InvokeAgentChatAsync(messagesSnapshot, responseCts, linkedCts.Token, onTextDelta), _config.MaxRetryAttempts, retryOperationName, linkedCts.Token @@ -984,7 +1202,8 @@ void OnTraceCompleted(FunctionExecutionResult result) private async Task InvokeAgentChatAsync( List messages, CancellationTokenSource responseCts, - CancellationToken linkedToken) + CancellationToken linkedToken, + Action? onTextDelta = null) { var useStreaming = _config.StreamingMode switch { @@ -1000,7 +1219,8 @@ private async Task InvokeAgentChatAsync( return await StreamBuffering.BufferAsync( _agent!.RunStreamingAsync(messages, session: null, cancellationToken: linkedToken), onChunk: () => { try { responseCts.CancelAfter(timeout); } catch (ObjectDisposedException) { } }, - linkedToken); + onText: onTextDelta, + cancellationToken: linkedToken); } /// @@ -1013,51 +1233,30 @@ private async Task InvokeAgentChatAsync( /// activity, via 's events. Dispose the returned handle /// once the model call completes to detach the hooks. /// - /// pauseDuringPlan: the outer chat turn's whole plan (propose_plan) executes inside this - /// single model call — pausing suppresses tool-event resumes so the plan's own per-step - /// watchdogs (which pass pauseDuringPlan=false) are the ones that fire on a stalled step, not - /// this outer one. The request-timeout ceiling (requestCts) pauses for the same reason: a - /// long-running plan crossing RequestTimeoutMinutes would otherwise get cancelled and - /// mislabeled "Cancelled by user." + /// There used to be a pauseDuringPlan mode here that suspended both this watchdog and the + /// request-timeout ceiling for the duration of a plan, because the whole plan ran inside the + /// propose_plan tool call and a slow step would otherwise trip a timer and surface as a bogus + /// "Cancelled by user." Plans now run after the turn unwinds, as a peer of the chat turn, so + /// there is no plan inside this call to protect against and each step keeps its own watchdog. /// - private IDisposable AttachAgentStallWatchdog( - CancellationTokenSource responseCts, - bool pauseDuringPlan = false, - CancellationTokenSource? requestCts = null, - TimeSpan requestTimeout = default) + private IDisposable AttachAgentStallWatchdog(CancellationTokenSource responseCts) { var middleware = _agentFunctionMiddleware!; var timeout = TimeSpan.FromSeconds(_config.ModelResponseTimeoutSeconds); - var planActive = false; - void Pause() { try { responseCts.CancelAfter(Timeout.InfiniteTimeSpan); } catch (ObjectDisposedException) { } } void Resume() { try { responseCts.CancelAfter(timeout); } catch (ObjectDisposedException) { } } void OnStarted() => Pause(); - void OnFinished() { if (!planActive && middleware.PendingFunctionCount == 0) Resume(); } + void OnFinished() { if (middleware.PendingFunctionCount == 0) Resume(); } middleware.OnFunctionStarted += OnStarted; middleware.OnFunctionFinished += OnFinished; - void PauseRequest() { try { requestCts?.CancelAfter(Timeout.InfiniteTimeSpan); } catch (ObjectDisposedException) { } } - void ResumeRequest() { try { requestCts?.CancelAfter(requestTimeout); } catch (ObjectDisposedException) { } } - - Action? onPlanStart = null, onPlanEnd = null; - if (pauseDuringPlan && _planHandoff != null) - { - onPlanStart = () => { planActive = true; Pause(); PauseRequest(); }; - onPlanEnd = () => { planActive = false; Resume(); ResumeRequest(); }; - _planHandoff.ExecutionStarted += onPlanStart; - _planHandoff.ExecutionFinished += onPlanEnd; - } - return new ActionDisposable(() => { middleware.OnFunctionStarted -= OnStarted; middleware.OnFunctionFinished -= OnFinished; - if (onPlanStart != null) _planHandoff!.ExecutionStarted -= onPlanStart; - if (onPlanEnd != null) _planHandoff!.ExecutionFinished -= onPlanEnd; }); } @@ -1241,9 +1440,37 @@ public static string BuildStepContext(string systemPrompt, string? originalUserR sb.AppendLine("--- End of Previous Steps ---\n"); } + sb.AppendLine("\n--- Required Step Outcome ---"); + sb.AppendLine(PlanStepReport.Contract); + sb.AppendLine("--- End Required Step Outcome ---"); + return sb.ToString(); } + /// + /// Supplies the request a plan is fulfilling, for a plan that did not originate in this + /// session's conversation. + /// + /// + /// Normally set when the user sends a message, and every plan step's context includes it as the + /// authority on WHERE work happens — target folders named in the request override unqualified + /// paths in a step instruction. A resumed plan has no such message: the process it was started + /// in is gone. Without this, resumed steps would run with no original request at all, which is + /// the exact shape of an observed failure where a plan lost its target folder and wrote every + /// file to the project root. + /// + public void SetRequestContext(string? request) + { + if (!string.IsNullOrWhiteSpace(request)) _currentTurnUserMessage = request; + } + + /// + /// Width budget for the narration shown beside the step label in the spinner. Conservative on + /// purpose: a spinner label that wraps corrupts the line the spinner keeps redrawing, and the + /// console may be narrower than expected. + /// + private const int SpinnerNarrationWidth = 60; + public async Task ExecutePlanStepAsync(string stepInstruction, List previousResults, CancellationToken cancellationToken = default) { var contextBuilder = new System.Text.StringBuilder( @@ -1272,13 +1499,29 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List + { + // A step's text only renders once the step finishes, so without this a + // long step is a spinner and nothing else — observed live sitting at + // "Working…" for four minutes while the model narrated throughout. + // Showing the newest line keeps the spinner honest without duplicating + // output that is about to be rendered as markdown anyway. + narration.Append(text); + var line = narration.Shortened(SpinnerNarrationWidth); + // The model's line alone — no step number. The harness already prints + // the authoritative "Step 2/3:" header above, and a second number + // beside it is the same confusion the model's own invented counts caused. + _spinner.UpdateActivity(line ?? baseSpinnerMessage); + }); var response = string.IsNullOrEmpty(result.Text) ? "Step completed (no response content)." : result.Text; @@ -1375,7 +1618,20 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List + /// Appends an assistant message to the conversation without calling the model. + /// + /// + /// Used to record a completed plan's manifest. Deliberately does NOT re-invoke the model: the + /// old design let the outer model keep its turn after a plan finished, and it reliably read the + /// summary as "not started yet" and redid the work — which is why three separate layers existed + /// to argue it out of that. Writing the outcome straight into history removes the opportunity + /// rather than guarding against it. + /// + public void AppendAssistantNote(string text) + { + if (string.IsNullOrWhiteSpace(text)) return; + _chatHistory.Add(new ChatMessage(ChatRole.Assistant, text)); + } + /// /// Serializes the conversation — everything except the system prompt — to JSON. /// Null when there is nothing beyond the system prompt or serialization fails; diff --git a/src/MandoCode/Services/Ai/AgentFunctionMiddleware.cs b/src/MandoCode/Services/Ai/AgentFunctionMiddleware.cs index edda475..05a085f 100644 --- a/src/MandoCode/Services/Ai/AgentFunctionMiddleware.cs +++ b/src/MandoCode/Services/Ai/AgentFunctionMiddleware.cs @@ -118,13 +118,7 @@ public InvocationScope BeginScope() { if (context.Function.Name == "propose_plan" && _planHandoff != null) { - if (_currentScope.Value?.PlanAlreadyProcessed == true) - { - return "A plan was already proposed and handled for this request. Do NOT propose another plan " + - "or start new work. Respond to the user now with a brief summary of what was accomplished, then stop."; - } - - return await HandleProposePlanAsync(context, cancellationToken); + return HandleProposePlan(context); } var scope = _currentScope.Value; @@ -136,12 +130,20 @@ public InvocationScope BeginScope() "Stop immediately — do not call tools, write files, or continue the work."; } - if (scope.PlanWorkCompleted && IsMutatingFunction(context.Function.Name)) + // A plan is queued for the moment this turn ends, so the model must not also do the + // work itself — it would race the plan and duplicate it. Mechanical, not a prose + // request: the refusal string is a courtesy, the refusal is the enforcement. + // + // This replaces the old post-plan mutation gate, which had to keep refusing mutations + // for the REST of the turn because the plan had already run inside the tool call and + // the model, never having seen the steps execute, would redo the work. With execution + // deferred there is no post-plan turn to guard, so the window shrinks to "between + // propose_plan and the end of this reply". + if (scope.ProposalPending && IsMutatingFunction(context.Function.Name)) { - return "A plan already completed this work during this turn — the files it " + - "created or modified exist on disk. Do NOT recreate or rewrite them. " + - "Respond to the user with a brief summary of the completed work; if " + - "further changes are needed, the user will ask in a follow-up message."; + return "A plan is queued and will run as soon as you finish this reply — it will " + + "make these changes for you. Do NOT make them yourself. Reply now with one " + + "short sentence telling the user their plan is ready to review."; } if (scope.BudgetExhausted) @@ -563,25 +565,42 @@ string CancelPlanFromWrite(string path) } } - private async Task HandleProposePlanAsync(FunctionInvocationContext context, CancellationToken cancellationToken) + /// + /// Records the proposal and returns immediately. The plan itself runs after this turn unwinds + /// (see ). + /// + /// + /// This used to await the entire plan — approval, every step, every nested tool call and diff + /// prompt — inside this one tool call, and almost every oddity in the planner descended from + /// that: the outer stall watchdog had to be paused or it killed the plan, the prompt gate had + /// to be released early or step 1 deadlocked, and because the outer model's turn was still open + /// it treated the returned summary as "not started yet" and redid the work. + /// + private string HandleProposePlan(FunctionInvocationContext context) { if (_planHandoff == null) return "Planning is not available in this context."; + // A step's own model call can reach propose_plan. Nested planning is always a runaway. + if (_planHandoff.IsExecuting) + return "A plan is already executing. Continue the current step instead of proposing a new plan."; + var goal = GetArg(context, "goal") ?? string.Empty; context.Arguments.TryGetValue("steps", out var stepsObj); var proposals = CoerceProposals(stepsObj); - var summary = await _planHandoff.ProcessAsync(goal, proposals, cancellationToken); + // Malformed or empty args are common from local models; fall through to direct work rather + // than queueing a plan of empty steps. + if (proposals.Length == 0) + return "Proposed plan had no steps. Proceed without a plan."; - if (proposals.Length > 0) - { - _currentScope.Value?.MarkPlanProcessed(); - if (_planHandoff.LastPlanExecutedWork) - _currentScope.Value?.MarkPlanWorkCompleted(); - } + _planHandoff.SetPendingProposal(goal, proposals); + _currentScope.Value?.MarkProposalPending(); - return summary; + return $"Plan received with {proposals.Length} step(s). It will be shown to the user for " + + "approval as soon as you finish this reply, and the approved steps will be executed " + + "for you. Do NOT start the work yourself and do NOT call propose_plan again. Reply " + + "now with one short sentence telling the user their plan is ready to review."; } private static PlanStepProposal[] CoerceProposals(object? raw) diff --git a/src/MandoCode/Services/Ai/InvocationScope.cs b/src/MandoCode/Services/Ai/InvocationScope.cs index fbcf029..3efdc33 100644 --- a/src/MandoCode/Services/Ai/InvocationScope.cs +++ b/src/MandoCode/Services/Ai/InvocationScope.cs @@ -280,36 +280,31 @@ public void RecordResultChars(int chars) } /// - /// Set once a propose_plan call with real steps has been fully processed this turn - /// (executed, rejected, or cancelled). A second proposal in the same turn is always - /// a runaway — observed live: a model completed a 5-step plan, immediately started - /// building an uninvited duplicate of the project, then proposed a THIRD round of - /// work. The filter short-circuits any repeat proposal with a stop directive. - /// One plan per user request; the user can always ask for more. + /// Set when the model has proposed a plan that will run as soon as this turn unwinds. + /// Mutating calls are refused while it holds, because the queued plan is about to make those + /// same changes and the model would otherwise race it and duplicate the work. Reads stay + /// allowed. A fresh scope (the next turn) resets this. /// - public bool PlanAlreadyProcessed { get; private set; } - - public void MarkPlanProcessed() - { - lock (_lock) PlanAlreadyProcessed = true; - } - - /// - /// Set when a plan executed at least one step to completion in this scope. The - /// filter's post-plan mutation gate then refuses filesystem-mutating calls for the - /// rest of the turn: the outer model never sees the steps run (each executes in its - /// own chat history), so it tends to treat the returned summary as "not started yet" - /// and redo the task — observed live overwriting a finished build under an - /// auto-approved session. Reads stay allowed; a fresh scope (next turn) resets this. - /// Distinct from , which is also set for rejected - /// plans — after a rejection the model must do the work directly, so mutations - /// must stay allowed there. - /// - public bool PlanWorkCompleted { get; private set; } - - public void MarkPlanWorkCompleted() + /// + /// Replaces two earlier circuits that both existed because the plan used to execute inside the + /// propose_plan tool call: + /// + /// PlanAlreadyProcessed, which refused a second proposal in the same turn — + /// observed live, a model finished a 5-step plan, immediately began an uninvited duplicate of + /// the project, then proposed a THIRD round of work. With execution deferred, a second proposal + /// is harmless: it simply replaces the first in the single-slot store. + /// PlanWorkCompleted, the post-plan mutation gate, which refused mutations for the + /// rest of the turn after a plan ran — observed live overwriting a finished build under an + /// auto-approved session, because the outer model never saw the steps execute and read the + /// summary as "not started yet". There is no post-plan turn any more, so the guard's window + /// shrinks to the gap between propose_plan and the end of the reply. + /// + /// + public bool ProposalPending { get; private set; } + + public void MarkProposalPending() { - lock (_lock) PlanWorkCompleted = true; + lock (_lock) ProposalPending = true; } /// diff --git a/src/MandoCode/Services/Ai/PlanHandoff.cs b/src/MandoCode/Services/Ai/PlanHandoff.cs index 42c06c4..19987e4 100644 --- a/src/MandoCode/Services/Ai/PlanHandoff.cs +++ b/src/MandoCode/Services/Ai/PlanHandoff.cs @@ -47,11 +47,16 @@ public bool IsExecuting /// /// Raised immediately before plan approval + execution begins, and again when it ends - /// (success, rejection, or throw). The whole plan runs inside a single outer model call - /// (the propose_plan tool), so the outer call's stall watchdog would otherwise fire mid-plan - /// on a slow step and surface as a bogus "Cancelled by user." AIService subscribes to pause - /// that outer watchdog for the plan's duration — the plan's own per-step watchdogs cover stalls. + /// (success, rejection, or throw). /// + /// + /// AIService used to subscribe to these to suspend the outer stall watchdog and the + /// request-timeout ceiling, because the whole plan ran inside the propose_plan tool call and a + /// slow step would otherwise trip a timer and surface as a bogus "Cancelled by user." Plans now + /// run after the turn unwinds, so no timer is running to suspend and that subscription is gone. + /// The events remain as a UI extension point — they're multicast, unlike + /// , so a host can observe plan activity without owning it. + /// public event Action? ExecutionStarted; public event Action? ExecutionFinished; @@ -68,6 +73,20 @@ public bool IsExecuting /// public bool LastPlanExecutedWork { get; private set; } + /// + /// Files written, edited or deleted so far by the plan currently executing, oldest first. + /// + /// + /// Evidence recorded at the middleware choke point — the call actually ran and succeeded — not + /// the model's self-report. Captured into the workflow's durable state so a resumed run knows + /// what already exists on disk; without it, resuming would have no way to distinguish work that + /// completed from work that never started. + /// + public IReadOnlyList<(string Operation, string Path)> FileOperations + { + get { lock (_lock) return [.. _fileOperations]; } + } + /// /// Called by AgentFunctionMiddleware after a successful filesystem-mutating call. /// No-ops outside plan execution so ordinary chat-turn writes don't pollute the @@ -82,15 +101,90 @@ public void RecordFileOperation(string operation, string relativePath) } } + // Single-slot holder for a plan the model proposed during the current turn. The plan is NOT + // run here: propose_plan returns a receipt immediately and the host runs the plan after the + // turn unwinds (see RunPendingPlanAsync). Last write wins — a model that proposes twice in one + // turn simply replaces its own proposal, which is strictly better than the old behavior of + // refusing the second one with a prose directive it could ignore anyway. + private (string Goal, string? OriginalRequest, PlanStepProposal[] Steps)? _pendingProposal; + private string? _currentRequest; + + /// True when the model proposed a plan this turn that hasn't been run yet. + public bool HasPendingProposal + { + get { lock (_lock) return _pendingProposal != null; } + } + /// - /// Called by AgentFunctionMiddleware when the model invokes propose_plan. - /// Guards against recursive planning (the model calling propose_plan while a - /// previous plan is still running) by returning a short-circuit message. + /// Supplies the request that opened the current model turn. A later proposal captures this + /// value so checkpoints retain the request's authoritative paths rather than only the model's + /// shortened goal. + /// + public void SetRequestContext(string? request) + { + lock (_lock) + { + _currentRequest = string.IsNullOrWhiteSpace(request) ? null : request; + if (_pendingProposal is { } pending) + _pendingProposal = (pending.Goal, _currentRequest, pending.Steps); + } + } + + /// Records a proposal for the host to run once the current turn finishes. + public void SetPendingProposal(string goal, PlanStepProposal[] steps) + { + lock (_lock) _pendingProposal = (goal, _currentRequest, steps); + } + + /// Drops any pending proposal — used when a turn ends without running one. + public void ClearPendingProposal() + { + lock (_lock) _pendingProposal = null; + } + + /// + /// Runs the proposal recorded during the turn that just ended, if there is one, and returns the + /// manifest the caller should place into chat history. Returns null when no plan was + /// proposed. + /// + /// + /// This is the entry point hosts call after their chat turn has fully drained. It exists so the + /// plan is a peer of the chat turn rather than a child of a tool call — the change that + /// removes the need for the outer watchdog pause, the prompt-gate release dance, and the + /// post-plan mutation gate. + /// + /// Hosts that previously relied on the plan running inside propose_plan must call this; + /// without it a proposed plan is simply never executed. + /// + /// + public async Task RunPendingPlanAsync(CancellationToken ct = default) + { + (string Goal, string? OriginalRequest, PlanStepProposal[] Steps)? pending; + lock (_lock) + { + pending = _pendingProposal; + _pendingProposal = null; + } + + if (pending == null) return null; + + return await ProcessAsync( + pending.Value.Goal, + pending.Value.Steps, + ct, + pending.Value.OriginalRequest); + } + + /// + /// Runs an approved plan end to end and returns the manifest describing what happened. + /// Guards against recursive planning (a plan step proposing another plan) by returning a + /// short-circuit message. /// public async Task ProcessAsync( string goal, PlanStepProposal[] proposals, - CancellationToken ct = default) + CancellationToken ct = default, + string? originalRequest = null) { lock (_lock) { @@ -112,7 +206,7 @@ public async Task ProcessAsync( var plan = new TaskPlan { - OriginalRequest = goal, + OriginalRequest = string.IsNullOrWhiteSpace(originalRequest) ? goal : originalRequest, Steps = steps, Status = TaskPlanStatus.Pending }; @@ -149,6 +243,59 @@ public async Task ProcessAsync( } } + /// + /// Marks a reconstructed plan as active and restores file-operation evidence from its saved + /// state. The returned scope must cover the whole resumed run so nested planning stays blocked + /// and new successful writes are appended to the restored evidence. + /// + public IDisposable BeginResumedExecution(IReadOnlyList savedFileOperations) + { + lock (_lock) + { + if (_isExecuting) + throw new InvalidOperationException("A plan is already executing."); + + _isExecuting = true; + LastPlanExecutedWork = false; + _fileOperations.Clear(); + foreach (var operation in savedFileOperations) + _fileOperations.Add((operation.Operation, operation.Path)); + } + + try + { + ExecutionStarted?.Invoke(); + return new ResumedExecutionScope(this); + } + catch + { + lock (_lock) _isExecuting = false; + throw; + } + } + + private void EndResumedExecution() + { + try + { + ExecutionFinished?.Invoke(); + } + finally + { + lock (_lock) + { + _isExecuting = false; + } + } + } + + private sealed class ResumedExecutionScope(PlanHandoff owner) : IDisposable + { + private PlanHandoff? _owner = owner; + + public void Dispose() => Interlocked.Exchange(ref _owner, null)?.EndResumedExecution(); + } + /// /// Builds the tool result the outer model sees after a plan executed: per-step /// statuses with capped result digests, the file operations recorded at the diff --git a/src/MandoCode/Services/Ai/Planning/IPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/IPlanRunner.cs new file mode 100644 index 0000000..4d6f8d6 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/IPlanRunner.cs @@ -0,0 +1,36 @@ +using MandoCode.Models; + +namespace MandoCode.Services; + +/// +/// Runs an approved to completion, reporting progress as it goes. +/// +/// +/// +/// The member signatures deliberately match what already exposes +/// and what both front-ends already consume, so the workflow engine can be swapped in behind this +/// interface without either UI changing. +/// +/// +/// Note the current consumer contract, which the workflow implementation must either honour or +/// deliberately replace: hands control to the consumer during +/// its yield return, and a consumer handling a failed step is expected to mutate +/// synchronously before returning. A consumer that doesn't (any +/// non-interactive caller) silently gets every failure downgraded to "skipped". Removing that +/// hazard — by making progress read-only and routing decisions back through a request port — is +/// the point of the workflow rebuild. +/// +/// +public interface IPlanRunner +{ + /// Executes the plan step by step, yielding progress as each step starts and settles. + IAsyncEnumerable ExecutePlanAsync( + TaskPlan plan, + CancellationToken cancellationToken = default); + + /// Marks a step skipped so execution moves on to the next one. + void SkipStep(TaskPlan plan, TaskStep step); + + /// Marks the whole plan cancelled; the runner stops at the next boundary. + void CancelPlan(TaskPlan plan); +} diff --git a/src/MandoCode/Services/Ai/Planning/IPlanStepExecutor.cs b/src/MandoCode/Services/Ai/Planning/IPlanStepExecutor.cs new file mode 100644 index 0000000..f5d197e --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/IPlanStepExecutor.cs @@ -0,0 +1,61 @@ +namespace MandoCode.Services; + +/// +/// Executes a single plan step. The seam that lets the plan runner be tested without a live model. +/// +/// +/// This exists before any workflow code is written, on purpose: with the runner bound directly to +/// , every test of step ordering, cancellation, retry or checkpoint/resume +/// would need a running Ollama, and the migration's go/no-go gates would be unenforceable. +/// +/// Implementations must route through AgentFunctionMiddleware — the ten guard circuits, +/// diff approvals and MCP gating all live there. A step executor that calls the model directly +/// would silently drop all of them. +/// +/// +public interface IPlanStepExecutor +{ + /// Runs one step to completion and returns the model's result text. + /// The step's detailed instruction (not its short UI description). + /// + /// Results of earlier steps, oldest first. Implementations may window this — the current one + /// keeps only the most recent few, which is what keeps 8k-context local models viable. + /// + Task ExecuteStepAsync( + string stepInstruction, + List previousResults, + CancellationToken cancellationToken = default); + + /// + /// Waits for tool calls still in flight from the step just finished to settle, so the next + /// step doesn't start while the previous one is still writing. + /// + /// + /// Part of the seam rather than left on , so the runner has no reason + /// to hold an reference at all and stays testable without one. + /// + Task WaitForQuiescenceAsync(TimeSpan timeout); +} + +/// +/// Adapts to . +/// Deliberately trivial: all step semantics stay in , so this migration +/// does not quietly fork them. +/// +public sealed class AiServicePlanStepExecutor(AIService aiService) : IPlanStepExecutor +{ + // Not null-guarded on purpose. TaskPlannerService has always accepted a null AIService, and + // several tests rely on it to exercise RequiresPlanning without standing up a model. Throwing + // here would turn that into a constructor failure — a behavior change this phase must not make. + // A null service still fails at the point of use, exactly as it did before. + private readonly AIService _aiService = aiService; + + public Task ExecuteStepAsync( + string stepInstruction, + List previousResults, + CancellationToken cancellationToken = default) + => _aiService.ExecutePlanStepAsync(stepInstruction, previousResults, cancellationToken); + + public Task WaitForQuiescenceAsync(TimeSpan timeout) + => _aiService.CompletionTracker.WaitForAllCompletionsAsync(timeout); +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs b/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs new file mode 100644 index 0000000..c39d9c3 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs @@ -0,0 +1,120 @@ +using System.Security.Cryptography; +using System.Text; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace MandoCode.Services; + +/// +/// Versioned wrapper around a durable plan-state snapshot. +/// +/// +/// +/// The application-owned payload is never stored bare. A schema or +/// workflow-topology change can alter what its cursor and statuses mean, and the dangerous failure +/// mode is re-running a step whose write_file already succeeded. Every field outside +/// exists to identify or validate the saved run — see +/// . +/// +/// +/// This lands before the first checkpoint is ever written. Adding the envelope later would leave +/// early checkpoints unversioned and indistinguishable, forcing a heuristic sniff. +/// +/// +public sealed record PlanCheckpointEnvelope +{ + /// Envelope format version. Bump when the fields below change shape. + public const int CurrentSchemaVersion = 1; + + [JsonPropertyName("schemaVersion")] + public int SchemaVersion { get; init; } = CurrentSchemaVersion; + + /// Graph shape that produced this checkpoint — . + [JsonPropertyName("topologyVersion")] + public string TopologyVersion { get; init; } = PlanExecutorIds.TopologyVersion; + + [JsonPropertyName("planId")] + public string PlanId { get; init; } = ""; + + /// + /// Hash of the project root, so a checkpoint is only ever offered for the project it belongs to. + /// A mismatch means "not ours" — invisible, not an error. + /// + [JsonPropertyName("projectRootHash")] + public string ProjectRootHash { get; init; } = ""; + + /// + /// Model that ran the completed steps. Resume across a model change is refused: a plan half-run + /// by one model and half by another is not a state anyone can reason about. + /// + [JsonPropertyName("modelName")] + public string ModelName { get; init; } = ""; + + [JsonPropertyName("mandoCodeVersion")] + public string MandoCodeVersion { get; init; } = ""; + + [JsonPropertyName("createdUtc")] + public DateTimeOffset CreatedUtc { get; init; } + + /// The serialized . Kept as JSON until compatibility passes. + [JsonPropertyName("payload")] + public JsonElement Payload { get; init; } + + /// + /// Stable hash of a project root, matching 's scheme + /// (full path, case-normalized, SHA-256, first 12 hex chars) so the two stores agree on identity. + /// + public static string HashProjectRoot(string projectRoot) + { + var full = Path.GetFullPath(projectRoot) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + return Convert.ToHexString( + SHA256.HashData(Encoding.UTF8.GetBytes(full.ToLowerInvariant())))[..12]; + } + + /// + /// Returns a human-readable reason this checkpoint must not be resumed, or null if it is + /// safe. Refusals are deliberately loud and specific: silently declining to resume looks + /// identical to losing the plan, and silently resuming a mismatch can redo completed writes. + /// + /// Hash of the project root being resumed into. + /// Model configured right now. + /// Optional Desktop agent/session owner for collision isolation. + public string? FindIncompatibility( + string projectRootHash, + string modelName, + string? expectedPlanId = null) + { + if (SchemaVersion != CurrentSchemaVersion) + { + return $"This plan was saved by a different version of MandoCode " + + $"(checkpoint format {SchemaVersion}, this build reads {CurrentSchemaVersion}). " + + "Start the plan again."; + } + + if (!string.Equals(TopologyVersion, PlanExecutorIds.TopologyVersion, StringComparison.Ordinal)) + { + return "This plan was saved by an older version of MandoCode and its steps no longer " + + "line up with how plans run now. Start it again."; + } + + if (!string.Equals(ProjectRootHash, projectRootHash, StringComparison.OrdinalIgnoreCase)) + { + return "This plan belongs to a different project folder."; + } + + if (expectedPlanId != null && + !string.Equals(PlanId, expectedPlanId, StringComparison.Ordinal)) + { + return "This plan belongs to a different agent session."; + } + + if (!string.Equals(ModelName, modelName, StringComparison.OrdinalIgnoreCase)) + { + return $"This plan was started with '{ModelName}' but the current model is " + + $"'{modelName}'. Switch back to resume it, or start the plan again."; + } + + return null; + } +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs new file mode 100644 index 0000000..ab21e39 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs @@ -0,0 +1,163 @@ +using System.Text.Json; +using MandoCode.Models; + +namespace MandoCode.Services; + +/// +/// On-disk record of the plan currently running for a project, so an interrupted plan can be +/// resumed after a crash, a Ctrl+C, or a restart. +/// +/// +/// +/// One file per project root and optional owner under ~/.mandocode/plans/, using readable +/// leaf names plus hashes so neither same-named projects nor Desktop agents sharing a project can +/// collide. Written whole-file with write-then-rename, and best-effort throughout: persistence +/// must never break a running plan. +/// +/// +/// Resume works by reconstructing the plan and running it again — completed and skipped steps are +/// stepped over, so only outstanding work re-runs. That is deliberately simpler than restoring a +/// framework checkpoint: it needs no byte-identical graph topology and is not coupled to the +/// workflow library's serialization format, both of which are silent-failure risks when the real +/// state being preserved is this small. +/// +/// +public static class PlanCheckpointStore +{ + /// Safety valve — a plan record is small; anything this large is corrupt. + private const int MaxBytes = 4 * 1024 * 1024; + + private static string Folder => Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".mandocode", "plans"); + + /// Stable path for a project and optional owner — readable leaf plus identity hashes. + public static string PathFor(string projectRoot, string? checkpointId = null) + { + var full = Path.GetFullPath(projectRoot) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + var hash = PlanCheckpointEnvelope.HashProjectRoot(full); + var leaf = new string(Path.GetFileName(full).Where(char.IsLetterOrDigit).Take(24).ToArray()); + var owner = string.IsNullOrWhiteSpace(checkpointId) + ? "" + : "-" + Convert.ToHexString(System.Security.Cryptography.SHA256.HashData( + System.Text.Encoding.UTF8.GetBytes(checkpointId)))[..12]; + return Path.Combine(Folder, leaf.Length > 0 + ? $"{leaf}-{hash}{owner}.json" + : $"{hash}{owner}.json"); + } + + /// + /// Records the current state of a running plan. Overwrites any previous record for this project. + /// + public static void Save( + string projectRoot, + PlanRunState state, + string modelName, + string planId, + string? checkpointId = null) + { + try + { + var envelope = new PlanCheckpointEnvelope + { + PlanId = planId, + ProjectRootHash = PlanCheckpointEnvelope.HashProjectRoot(projectRoot), + ModelName = modelName, + MandoCodeVersion = VersionLabel.ForAssembly(typeof(PlanCheckpointStore).Assembly), + CreatedUtc = DateTimeOffset.UtcNow, + Payload = JsonSerializer.SerializeToElement(state), + }; + + var json = JsonSerializer.Serialize(envelope); + if (json.Length > MaxBytes) return; + + Directory.CreateDirectory(Folder); + var path = PathFor(projectRoot, checkpointId); + var tmp = path + ".tmp"; + File.WriteAllText(tmp, json); + File.Move(tmp, path, overwrite: true); + } + catch { /* persistence must never break the plan */ } + } + + /// + /// Loads the recorded plan for a project, or null when there is none, it cannot be read, + /// or it is not safe to resume. explains a readable-but-unusable + /// record so the caller can say why rather than silently offering nothing. + /// + public static PlanRunState? Load( + string projectRoot, + string modelName, + out string? refusal, + string? checkpointId = null) + { + refusal = null; + try + { + var path = PathFor(projectRoot, checkpointId); + if (!File.Exists(path)) return null; + + var envelope = JsonSerializer.Deserialize(File.ReadAllText(path)); + if (envelope == null) return null; + + refusal = envelope.FindIncompatibility( + PlanCheckpointEnvelope.HashProjectRoot(projectRoot), + modelName, + checkpointId); + if (refusal != null) return null; + + return envelope.Payload.Deserialize(); + } + catch + { + // Truncated or corrupt: treat as absent rather than surfacing an error. A plan record is + // a convenience, and a half-written one is indistinguishable from no record at all. + return null; + } + } + + /// Removes the record — the plan finished, was cancelled, or the user discarded it. + public static void Delete(string projectRoot, string? checkpointId = null) + { + try + { + var path = PathFor(projectRoot, checkpointId); + if (File.Exists(path)) File.Delete(path); + } + catch { } + } + + /// True when a record exists for this project, without validating it. + public static bool Exists(string projectRoot, string? checkpointId = null) + { + try { return File.Exists(PathFor(projectRoot, checkpointId)); } + catch { return false; } + } + + /// + /// Rebuilds a runnable plan from a saved record. Completed and skipped steps keep their status, + /// so the runner steps over them and only outstanding work executes. + /// + public static TaskPlan ToPlan(PlanRunState state) => new() + { + OriginalRequest = state.Goal, + Status = TaskPlanStatus.Pending, + Steps = [.. state.Steps.Select(s => new TaskStep + { + StepNumber = s.Number, + Description = s.Description, + Instruction = s.Instruction, + // A step that was mid-flight when the process died is Pending again: it may have half + // run, but re-running it is safer than assuming it finished. + Status = s.Status is TaskStepStatus.Completed or TaskStepStatus.Skipped + ? s.Status + : TaskStepStatus.Pending, + Result = s.Result, + ErrorMessage = s.Error, + })], + }; + + /// Steps still to run in a saved record — what "resume" would actually do. + public static int OutstandingSteps(PlanRunState state) => state.Steps.Count( + s => s.Status is not (TaskStepStatus.Completed or TaskStepStatus.Skipped)); +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanExecutorIds.cs b/src/MandoCode/Services/Ai/Planning/PlanExecutorIds.cs new file mode 100644 index 0000000..6cedca3 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanExecutorIds.cs @@ -0,0 +1,92 @@ +namespace MandoCode.Services; + +/// +/// Stable identities for the plan workflow's agents and executors. +/// +/// +/// +/// MAF matches checkpointed state back to executors by identity, and for agent-backed executors +/// that identity derives from both ChatClientAgentOptions.Id and Name. A +/// checkpoint written under one identity cannot be resumed under another, and there is no repair +/// path — so every value here is a literal const string fixed before the first checkpoint +/// is ever written to disk. +/// +/// +/// Rules, enforced by PlanExecutorIdsTests: +/// +/// Literal constants only — no interpolation, no nameof, no Guid.NewGuid(). +/// Nothing volatile may appear in an id: not the model name, not the temperature, not the +/// project path, not the step number. re-runs on every MCP +/// reconcile and every KernelRebuild-scoped /config set; an id derived from any of +/// those would change mid-session and orphan a live plan's checkpoints with no error message. +/// The step agent must not share a name with the generalist agent — two differently-purposed +/// agents under one name is exactly the collision that routes restored state to the wrong executor. +/// Adding, removing or renaming a node is a topology change: bump +/// and refuse older checkpoints loudly rather than resuming onto a mismatched graph. +/// +/// +/// +public static class PlanExecutorIds +{ + /// + /// Graph shape version. Bump on ANY change to the executor set or the edges between them. + /// Checkpoints recording a different value are refused, never best-effort resumed — a + /// mismatched topology could re-run a step whose write_file already succeeded. + /// + public const string TopologyVersion = "1"; + + /// Logical id of the single generalist agent, pinned at . + public const string GeneralistAgentId = "mandocode.agent.generalist"; + + /// + /// Display name of the generalist agent. Deliberately unchanged from the pre-workflow value — + /// it is user-visible, and changing it would invalidate identity for no benefit. + /// + public const string GeneralistAgentName = "MandoCode"; + + /// Id of the agent that executes a single plan step. Distinct from the generalist pair. + public const string StepAgentId = "mandocode.plan.v1.step-agent"; + + /// Name of the step agent. Must differ from . + public const string StepAgentName = "mandocode-plan-step"; + + /// Normalizes the proposal into a plan and emits the approval request. + public const string Intake = "mandocode.plan.v1.intake"; + + /// Request port for plan sign-off; re-entered after a revision. + public const string ApprovalPort = "mandocode.plan.v1.approval"; + + /// Routes the approval verdict to execution, rejection or cancellation. + public const string Gate = "mandocode.plan.v1.gate"; + + /// Runs one step. Invoked once per step via the loop-back edge from triage. + public const string StepRunner = "mandocode.plan.v1.step-runner"; + + /// Sole owner and sole writer of plan state; decides what happens after each step. + public const string Triage = "mandocode.plan.v1.triage"; + + /// Request port for per-step recovery decisions (retry / skip / edit / replan / cancel). + public const string DecisionPort = "mandocode.plan.v1.step-decision"; + + /// Produces a revised plan, which then re-enters . + public const string Replanner = "mandocode.plan.v1.replanner"; + + /// Builds the closing manifest and yields the workflow output. + public const string Finalizer = "mandocode.plan.v1.finalizer"; + + /// + /// Every executor id in the graph. Exposed so a golden-list test can fail loudly on a + /// careless rename — that test is the regression net protecting checkpoint compatibility. + /// + public static IReadOnlyList All { get; } = + [ + Intake, + ApprovalPort, + Gate, + StepRunner, + Triage, + DecisionPort, + Replanner, + Finalizer, + ]; +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanRevision.cs b/src/MandoCode/Services/Ai/Planning/PlanRevision.cs new file mode 100644 index 0000000..bcae639 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanRevision.cs @@ -0,0 +1,95 @@ +using MandoCode.Models; + +namespace MandoCode.Services; + +/// Pure plan-suffix replacement shared by Desktop and CLI replan flows. +public static class PlanRevision +{ + public static TaskPlan CreateCandidate(TaskPlan current, int failedStepNumber, GeneratedPlan revision) + { + var failedIndex = current.Steps.FindIndex(step => step.StepNumber == failedStepNumber); + if (failedIndex < 0) throw new ArgumentOutOfRangeException(nameof(failedStepNumber)); + + var prefix = current.Steps.Take(failedIndex).Select(Clone).ToList(); + var replacement = TaskPlannerService.FromProposals(revision.Steps); + for (var i = 0; i < replacement.Count; i++) replacement[i].StepNumber = failedIndex + i + 1; + + return new TaskPlan + { + OriginalRequest = current.OriginalRequest, + Status = TaskPlanStatus.Pending, + Steps = [.. prefix, .. replacement] + }; + } + + /// Builds a full review candidate while preserving every step through the edited one. + public static TaskPlan CreateFollowingCandidate(TaskPlan current, int editedStepNumber, GeneratedPlan revision) + { + var editedIndex = current.Steps.FindIndex(step => step.StepNumber == editedStepNumber); + if (editedIndex < 0) throw new ArgumentOutOfRangeException(nameof(editedStepNumber)); + + var prefix = current.Steps.Take(editedIndex + 1).Select(Clone).ToList(); + var replacement = TaskPlannerService.FromProposals(revision.Steps); + for (var i = 0; i < replacement.Count; i++) replacement[i].StepNumber = editedIndex + i + 2; + + return new TaskPlan + { + OriginalRequest = current.OriginalRequest, + Status = TaskPlanStatus.Pending, + Steps = [.. prefix, .. replacement] + }; + } + + /// + /// Applies an approved candidate while preserving the failed step object's identity. The + /// workflow triage node still holds that object across the UI await and reads its Pending + /// status as the signal to dispatch the same cursor again. + /// + public static void ApplyApproved(TaskPlan current, int failedStepNumber, TaskPlan candidate) + { + var failedIndex = current.Steps.FindIndex(step => step.StepNumber == failedStepNumber); + if (failedIndex < 0 || candidate.Steps.Count <= failedIndex) + throw new InvalidOperationException("The revised plan has no replacement for the failed step."); + + var liveFailedStep = current.Steps[failedIndex]; + var firstReplacement = candidate.Steps[failedIndex]; + Copy(firstReplacement, liveFailedStep); + liveFailedStep.Status = TaskStepStatus.Pending; + liveFailedStep.Result = null; + liveFailedStep.ErrorMessage = null; + + current.Steps.RemoveRange(failedIndex + 1, current.Steps.Count - failedIndex - 1); + foreach (var step in candidate.Steps.Skip(failedIndex + 1)) + current.Steps.Add(Clone(step)); + current.Status = TaskPlanStatus.InProgress; + current.ExecutionSummary = null; + } + + /// Replaces only the steps after an edited, not-yet-executed step. + public static void ApplyFollowing(TaskPlan current, int editedStepNumber, TaskPlan candidate) + { + var editedIndex = current.Steps.FindIndex(step => step.StepNumber == editedStepNumber); + if (editedIndex < 0) throw new ArgumentOutOfRangeException(nameof(editedStepNumber)); + + current.Steps.RemoveRange(editedIndex + 1, current.Steps.Count - editedIndex - 1); + foreach (var step in candidate.Steps.Skip(editedIndex + 1)) + current.Steps.Add(Clone(step)); + } + + private static TaskStep Clone(TaskStep source) + { + var clone = new TaskStep(); + Copy(source, clone); + return clone; + } + + private static void Copy(TaskStep source, TaskStep target) + { + target.StepNumber = source.StepNumber; + target.Description = source.Description; + target.Instruction = source.Instruction; + target.Status = source.Status; + target.Result = source.Result; + target.ErrorMessage = source.ErrorMessage; + } +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanRunState.cs b/src/MandoCode/Services/Ai/Planning/PlanRunState.cs new file mode 100644 index 0000000..5c5e27d --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanRunState.cs @@ -0,0 +1,103 @@ +using System.Text.Json.Serialization; +using MandoCode.Models; + +namespace MandoCode.Services; + +/// +/// Everything needed to resume a plan, in a form that survives JSON round-tripping. +/// +/// +/// +/// Held in the workflow's own shared state rather than in , because the +/// context carries live delegates and a and can therefore never be +/// checkpointed. MAF captures shared state at each superstep boundary, so putting the run's facts +/// here is what makes resume possible at all. +/// +/// +/// Deliberately a snapshot of plain data — no behavior, no references to services. The live +/// still exists alongside it, because the current consumer contract requires +/// a mutable plan the UI can set on. That duplication is temporary: +/// once progress becomes read-only and the legacy runner is gone, this becomes the only +/// representation. +/// +/// +public sealed record PlanRunState +{ + /// The user's request, verbatim where available — authoritative for target paths. + [JsonPropertyName("goal")] + public string Goal { get; init; } = ""; + + /// Every step, in order. + [JsonPropertyName("steps")] + public IReadOnlyList Steps { get; init; } = []; + + /// Zero-based index of the next step to run; equals Steps.Count when finished. + [JsonPropertyName("cursor")] + public int Cursor { get; init; } + + /// + /// Results of completed steps, oldest first, in the form each step's context expects. + /// + [JsonPropertyName("previousResults")] + public IReadOnlyList PreviousResults { get; init; } = []; + + /// + /// Files this plan has created, edited or deleted — recorded at the middleware choke point, so + /// this is evidence a call actually succeeded rather than the model's account of it. + /// + [JsonPropertyName("fileOperations")] + public IReadOnlyList FileOperations { get; init; } = []; + + /// Captures the current shape of a live plan. + public static PlanRunState From( + TaskPlan plan, + int cursor, + IReadOnlyList previousResults, + IReadOnlyList<(string Operation, string Path)> fileOperations) => new() + { + Goal = plan.OriginalRequest ?? "", + Steps = [.. plan.Steps.Select(PlanStepState.From)], + Cursor = cursor, + PreviousResults = [.. previousResults], + FileOperations = [.. fileOperations.Select(f => new PlanFileOperation(f.Operation, f.Path))], + }; +} + +/// One step's durable state. +public sealed record PlanStepState +{ + [JsonPropertyName("number")] + public int Number { get; init; } + + /// Short label for display; never sent to the model. + [JsonPropertyName("description")] + public string Description { get; init; } = ""; + + /// The text actually executed. This is what a resumed run re-issues. + [JsonPropertyName("instruction")] + public string Instruction { get; init; } = ""; + + [JsonPropertyName("status")] + public TaskStepStatus Status { get; init; } + + [JsonPropertyName("result")] + public string? Result { get; init; } + + [JsonPropertyName("error")] + public string? Error { get; init; } + + public static PlanStepState From(TaskStep step) => new() + { + Number = step.StepNumber, + Description = step.Description, + Instruction = step.Instruction, + Status = step.Status, + Result = step.Result, + Error = step.ErrorMessage, + }; +} + +/// A filesystem change a plan made, as observed by the middleware. +public sealed record PlanFileOperation( + [property: JsonPropertyName("operation")] string Operation, + [property: JsonPropertyName("path")] string Path); diff --git a/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs new file mode 100644 index 0000000..48caa74 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs @@ -0,0 +1,87 @@ +using MandoCode.Models; + +namespace MandoCode.Services; + +/// +/// Resolves which plan engine to use, re-reading the planner config key on every access, and +/// records a running plan's progress so it can be resumed after an interruption. +/// +/// +/// Not a plain DI registration because planner is KernelRebuild-scoped: it is meant to +/// take effect on the next message without losing history, so a singleton captured at startup would +/// silently ignore /config set planner workflow. Re-reading also makes an A/B practical — +/// same session, same history, flip the key, re-run the same prompt. +/// +/// The workflow runner is cached once created; it holds no per-run state (each run builds its own +/// graph over its own context). +/// +/// +public sealed class PlanRunnerSelector( + MandoCodeConfig config, + TaskPlannerService legacyRunner, + IPlanStepExecutor stepExecutor, + PlanHandoff? planHandoff = null, + ProjectRootAccessor? projectRoot = null, + string? checkpointId = null) +{ + private WorkflowPlanRunner? _workflowRunner; + + /// True when the workflow engine is currently selected. + public bool UsingWorkflowEngine => string.Equals( + config.PlannerEngine, MandoCodeConfig.PlannerEngineWorkflow, StringComparison.OrdinalIgnoreCase); + + /// + /// True when progress is recorded for resume. Only the workflow engine reports its state, so + /// only it can be resumed — the legacy runner keeps the plan in a local variable. + /// + public bool SupportsResume => UsingWorkflowEngine && projectRoot != null; + + /// The engine to run the next plan with. + public IPlanRunner Current => UsingWorkflowEngine + ? _workflowRunner ??= new WorkflowPlanRunner(stepExecutor, planHandoff, RecordProgress) + : legacyRunner; + + /// The plan recorded for this project that could be resumed, or null. + /// + /// Set when a record exists but must not be resumed — a different model, a different build. + /// Worth showing: silently offering nothing looks identical to having lost the plan. + /// + public PlanRunState? FindResumable(out string? refusal) + { + refusal = null; + if (projectRoot == null) return null; + return PlanCheckpointStore.Load( + projectRoot.ProjectRoot, + config.GetEffectiveModelName(), + out refusal, + checkpointId); + } + + /// Forgets any recorded plan for this project. + public void DiscardResumable() + { + if (projectRoot != null) PlanCheckpointStore.Delete(projectRoot.ProjectRoot, checkpointId); + } + + /// + /// Called by the workflow runner each time the plan advances. Clears the record once nothing is + /// outstanding, so a finished plan is never offered for resume. + /// + private void RecordProgress(PlanRunState state) + { + if (projectRoot == null) return; + + if (PlanCheckpointStore.OutstandingSteps(state) == 0) + { + PlanCheckpointStore.Delete(projectRoot.ProjectRoot, checkpointId); + return; + } + + PlanCheckpointStore.Save( + projectRoot.ProjectRoot, + state, + config.GetEffectiveModelName(), + planId: checkpointId ?? PlanCheckpointEnvelope.HashProjectRoot(projectRoot.ProjectRoot), + checkpointId: checkpointId); + } +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanStepReport.cs b/src/MandoCode/Services/Ai/Planning/PlanStepReport.cs new file mode 100644 index 0000000..6a2ee14 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanStepReport.cs @@ -0,0 +1,54 @@ +using System.Text.RegularExpressions; + +namespace MandoCode.Services; + +/// +/// Parses the explicit terminal marker required from a plan-step model call. Tool execution and +/// model completion are not the same as task success: a model can successfully return prose that +/// says its verification failed. The marker turns that distinction into workflow state. +/// +public static partial class PlanStepReport +{ + public const string Contract = + "At the very end of your final response, report the step outcome on its own line. " + + "Use [PLAN_STEP_RESULT:SUCCESS] only when this step's instruction and verification are " + + "actually satisfied. If anything required is missing, incorrect, or unverifiable, use " + + "[PLAN_STEP_RESULT:FAILED] followed by a concise reason. Never call a failed verification success."; + + public static PlanStepReportResult Parse(string response) + { + response ??= ""; + var matches = MarkerRegex().Matches(response); + if (matches.Count == 0) + return new PlanStepReportResult(null, response.TrimEnd(), null); + + var marker = matches[^1]; + var succeeded = marker.Groups[1].Value.Equals("SUCCESS", StringComparison.OrdinalIgnoreCase); + var before = response[..marker.Index].TrimEnd(); + var after = response[(marker.Index + marker.Length)..].Trim(); + var display = string.IsNullOrEmpty(after) + ? before + : string.IsNullOrEmpty(before) ? after : before + Environment.NewLine + after; + + string? failure = null; + if (!succeeded) + { + failure = string.IsNullOrWhiteSpace(after) ? LastMeaningfulLine(before) : after; + if (string.IsNullOrWhiteSpace(failure)) failure = "The step reported that its requirements were not satisfied."; + } + + return new PlanStepReportResult(succeeded, display, failure); + } + + private static string? LastMeaningfulLine(string text) => text + .Split(['\r', '\n'], StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) + .LastOrDefault(); + + [GeneratedRegex(@"\[PLAN_STEP_RESULT\s*:\s*(SUCCESS|FAILED)\]", RegexOptions.IgnoreCase)] + private static partial Regex MarkerRegex(); +} + +public sealed record PlanStepReportResult(bool? Succeeded, string DisplayText, string? FailureReason); + +/// A model call completed normally but explicitly reported that the step did not. +public sealed class PlanStepReportedFailureException(string message) : Exception(message); diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs new file mode 100644 index 0000000..f29da20 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -0,0 +1,343 @@ +using MandoCode.Models; +using Microsoft.Agents.AI.Workflows; + +namespace MandoCode.Services; + +/// +/// Mutable state shared by the plan workflow's executors and the runner driving it. +/// +/// +/// +/// Holds the live because the current consumer contract requires it: the +/// runner yields a progress event and the consumer is expected to mutate plan.Status before +/// asking for the next one. preserves that exactly. +/// +/// +/// Phase 4 note: this object holds live delegates, so a workflow built around it is NOT +/// checkpointable. Moving the plan and the accumulated results into the workflow's own shared state +/// is a prerequisite for resume — the step cursor already lives there to establish the pattern. +/// +/// +internal sealed class PlanRunContext( + TaskPlan plan, + IPlanStepExecutor stepExecutor, + Func raise, + CancellationToken cancellationToken, + PlanHandoff? planHandoff = null, + Action? onStateSaved = null, + IReadOnlyList? seedResults = null) +{ + public TaskPlan Plan { get; } = plan; + public IPlanStepExecutor StepExecutor { get; } = stepExecutor; + public CancellationToken CancellationToken { get; } = cancellationToken; + + /// Source of the file-operation evidence recorded by the middleware, when available. + public PlanHandoff? PlanHandoff { get; } = planHandoff; + + /// + /// Writes the run's durable state into the workflow's shared scope. + /// + /// + /// Called at every point the run advances. MAF captures shared state at each superstep + /// boundary, so this — not , which lives on an unserializable context — is + /// what a checkpoint actually preserves. + /// + public ValueTask SaveStateAsync(IWorkflowContext context, int cursor, CancellationToken ct) + { + var state = PlanRunState.From( + Plan, + cursor, + PreviousResults, + PlanHandoff?.FileOperations ?? []); + + // Also handed to the host, which persists it so an interrupted plan can be resumed. + // Best-effort: a failure to record progress must not stop the plan making it. + try { onStateSaved?.Invoke(state); } catch { } + + return context.QueueStateUpdateAsync( + PlanWorkflowMessages.StateKey, state, PlanWorkflowMessages.StateScope, ct); + } + + /// + /// Step results accumulated so far, in the format each step's context expects. + /// + /// + /// Seeded on resume from the saved record. Without that, steps picked up after an interruption + /// would run blind to everything earlier steps produced, which is exactly the context the + /// remaining work usually depends on. + /// + public List PreviousResults { get; } = [.. seedResults ?? []]; + + /// How many times each step index has been retried after failing. + /// + /// Capped so a step that fails identically every time cannot loop forever. The user is asked + /// each time, so this is a backstop against a mistake rather than against the user. + /// + public Dictionary RetryCounts { get; } = []; + + /// Maximum retries of a single step within one run. + public const int MaxRetriesPerStep = 3; + + /// + /// Publishes a progress event and, by default, waits until the consumer has processed it and + /// asked for the next one. + /// + /// + /// Waiting is the default because it matches the legacy runner, where every progress event was + /// a yield return and therefore inherently blocked until the UI had handled it. Two + /// things depend on that ordering: the consumer's decision on a failed step (it mutates + /// plan.Status, only visible once it comes back for the next event), and rendering — + /// without the wait, a step's model call starts its own spinner while the step header is still + /// being drawn and the spinner frame bleeds into it. + /// + public Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer = true) + => raise(evt, waitForConsumer, CancellationToken); + + /// + /// Index of the next step that still needs running at or after , or -1. + /// Steps already Completed or Skipped are stepped over rather than re-run — a resumed or + /// partially-skipped plan must not redo work. + /// + public int NextRunnableIndex(int from) + { + for (var i = Math.Max(0, from); i < Plan.Steps.Count; i++) + { + var status = Plan.Steps[i].Status; + if (status != TaskStepStatus.Completed && status != TaskStepStatus.Skipped) + return i; + } + return -1; + } +} + +/// Seeds the run and dispatches the first runnable step. +[SendsMessage(typeof(RunPlanStep))] +[SendsMessage(typeof(PlanRunFinished))] +internal sealed class PlanIntakeExecutor(PlanRunContext ctx) + : Executor(PlanExecutorIds.Intake) +{ + public override async ValueTask HandleAsync( + StartPlanRun message, IWorkflowContext context, CancellationToken cancellationToken = default) + { + ctx.Plan.Status = TaskPlanStatus.InProgress; + await ctx.RaiseAsync(TaskProgressEvent.PlanCreated(ctx.Plan)); + + var first = ctx.NextRunnableIndex(0); + await context.QueueStateUpdateAsync( + PlanWorkflowMessages.CursorKey, Math.Max(first, 0), PlanWorkflowMessages.StateScope, cancellationToken); + await ctx.SaveStateAsync(context, Math.Max(first, 0), cancellationToken); + + if (first < 0) + { + await context.SendMessageAsync( + new PlanRunFinished("Nothing to run."), PlanExecutorIds.Finalizer, cancellationToken); + return; + } + + await context.SendMessageAsync(new RunPlanStep(first), PlanExecutorIds.StepRunner, cancellationToken); + } +} + +/// +/// Runs exactly one step and reports the outcome. Holds no plan state — triage owns all of it. +/// +[SendsMessage(typeof(PlanStepOutcome))] +internal sealed class PlanStepRunnerExecutor(PlanRunContext ctx) + : Executor(PlanExecutorIds.StepRunner) +{ + public override async ValueTask HandleAsync( + RunPlanStep message, IWorkflowContext context, CancellationToken cancellationToken = default) + { + var step = ctx.Plan.Steps[message.StepIndex]; + + // Two cancellation signals, same as the legacy runner: the token, and a plan the consumer + // cancelled between steps. + if (ctx.CancellationToken.IsCancellationRequested || ctx.Plan.Status == TaskPlanStatus.Cancelled) + { + await Report(context, new PlanStepOutcome( + message.StepIndex, PlanStepOutcomeKind.Cancelled, null, "Cancelled by user."), cancellationToken); + return; + } + + step.Status = TaskStepStatus.InProgress; + + // Raising waits for the consumer (see RaiseAsync), so the step header is on screen before + // the model call below starts its own spinner. Without that ordering the spinner frame + // bleeds into the header — "▫ Three-stepping... · 0sStep 3/3: ...", observed live. + await ctx.RaiseAsync(TaskProgressEvent.StepStarted(ctx.Plan, step)); + + PlanStepOutcome outcome; + try + { + var result = await ctx.StepExecutor.ExecuteStepAsync( + step.Instruction, ctx.PreviousResults, ctx.CancellationToken); + outcome = new PlanStepOutcome(message.StepIndex, PlanStepOutcomeKind.Completed, result, null); + } + catch (OperationCanceledException) when (ctx.CancellationToken.IsCancellationRequested) + { + outcome = new PlanStepOutcome(message.StepIndex, PlanStepOutcomeKind.Cancelled, null, "Cancelled by user."); + } + catch (PlanCancellationRequestedException) + { + // "Cancel plan" chosen at a diff-approval prompt mid-step. Unambiguous: stop everything. + outcome = new PlanStepOutcome( + message.StepIndex, PlanStepOutcomeKind.Cancelled, null, "Plan cancelled by user from diff approval."); + } + catch (Exception ex) + { + outcome = new PlanStepOutcome(message.StepIndex, PlanStepOutcomeKind.Failed, null, ex.Message); + } + + await ctx.StepExecutor.WaitForQuiescenceAsync(TimeSpan.FromSeconds(5)); + await Report(context, outcome, cancellationToken); + } + + private static ValueTask Report(IWorkflowContext context, PlanStepOutcome outcome, CancellationToken ct) + => context.SendMessageAsync(outcome, PlanExecutorIds.Triage, ct); +} + +/// +/// Sole owner and sole writer of plan state. Decides what happens after each step and advances the +/// cursor, so no other executor and no consumer needs to. +/// +[SendsMessage(typeof(RunPlanStep))] +[SendsMessage(typeof(PlanRunFinished))] +internal sealed class PlanTriageExecutor(PlanRunContext ctx) + : Executor(PlanExecutorIds.Triage) +{ + public override async ValueTask HandleAsync( + PlanStepOutcome message, IWorkflowContext context, CancellationToken cancellationToken = default) + { + var plan = ctx.Plan; + var step = plan.Steps[message.StepIndex]; + + switch (message.Kind) + { + case PlanStepOutcomeKind.Completed: + step.Result = message.Result; + step.Status = TaskStepStatus.Completed; + ctx.PreviousResults.Add($"Step {step.StepNumber} ({step.Description}): {message.Result}"); + await ctx.RaiseAsync(TaskProgressEvent.StepCompleted(plan, step, message.Result)); + break; + + case PlanStepOutcomeKind.Cancelled: + step.Status = TaskStepStatus.Failed; + step.ErrorMessage = message.Error; + plan.Status = TaskPlanStatus.Cancelled; + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); + await Finish(context, cancellationToken); + return; + + case PlanStepOutcomeKind.Failed: + step.Status = TaskStepStatus.Failed; + step.ErrorMessage = message.Error; + + // Defer skip-vs-cancel to the consumer, then reconcile — matching the legacy runner, + // where deciding before the yield silently downgraded "Cancel the plan" to "skip". + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Step failed.")); + + if (plan.Status == TaskPlanStatus.Cancelled) + { + await ctx.SaveStateAsync(context, message.StepIndex, cancellationToken); + await Finish(context, cancellationToken); + return; + } + + // A consumer that set the step back to Pending is asking for a retry. Re-dispatch + // the same index rather than advancing, which is what the cursor would otherwise do. + if (step.Status == TaskStepStatus.Pending) + { + var attempts = ctx.RetryCounts.TryGetValue(message.StepIndex, out var n) ? n : 0; + if (attempts < PlanRunContext.MaxRetriesPerStep) + { + ctx.RetryCounts[message.StepIndex] = attempts + 1; + step.ErrorMessage = null; + await ctx.SaveStateAsync(context, message.StepIndex, cancellationToken); + await context.SendMessageAsync( + new RunPlanStep(message.StepIndex), PlanExecutorIds.StepRunner, cancellationToken); + return; + } + + // Out of retries: fall through and treat it as skipped rather than looping. + step.Status = TaskStepStatus.Failed; + } + + // Either the consumer skipped it, or there was no interactive consumer at all. Both + // mean "move past it" — the step must not be re-run. + if (step.Status == TaskStepStatus.Failed) + step.Status = TaskStepStatus.Skipped; + break; + } + + if (plan.Status == TaskPlanStatus.Cancelled || ctx.CancellationToken.IsCancellationRequested) + { + plan.Status = TaskPlanStatus.Cancelled; + await ctx.SaveStateAsync(context, message.StepIndex, cancellationToken); + await Finish(context, cancellationToken); + return; + } + + var next = ctx.NextRunnableIndex(message.StepIndex + 1); + var cursor = next < 0 ? plan.Steps.Count : next; + await context.QueueStateUpdateAsync( + PlanWorkflowMessages.CursorKey, cursor, PlanWorkflowMessages.StateScope, cancellationToken); + + // The step just changed the plan — persist before dispatching the next one, so a checkpoint + // taken at this boundary reflects work that actually happened. + await ctx.SaveStateAsync(context, cursor, cancellationToken); + + if (next < 0) + { + await Finish(context, cancellationToken); + return; + } + + await context.SendMessageAsync(new RunPlanStep(next), PlanExecutorIds.StepRunner, cancellationToken); + } + + private static ValueTask Finish(IWorkflowContext context, CancellationToken ct) + => context.SendMessageAsync(new PlanRunFinished("done"), PlanExecutorIds.Finalizer, ct); +} + +/// Classifies the terminal state and emits the closing progress event. +[YieldsOutput(typeof(string))] +internal sealed class PlanFinalizerExecutor(PlanRunContext ctx) + : Executor(PlanExecutorIds.Finalizer) +{ + public override async ValueTask HandleAsync( + PlanRunFinished message, IWorkflowContext context, CancellationToken cancellationToken = default) + { + var plan = ctx.Plan; + + if (plan.Status == TaskPlanStatus.Cancelled) + { + await ctx.RaiseAsync(TaskProgressEvent.PlanCancelled(plan)); + await context.YieldOutputAsync(TaskPlanStatus.Cancelled.ToString(), cancellationToken); + return; + } + + var allSettled = plan.Steps.All(s => + s.Status == TaskStepStatus.Completed || s.Status == TaskStepStatus.Skipped); + var anySkipped = plan.Steps.Any(s => s.Status == TaskStepStatus.Skipped); + var anyFailed = plan.Steps.Any(s => s.Status == TaskStepStatus.Failed); + + if (allSettled && !anyFailed) + { + plan.Status = anySkipped + ? TaskPlanStatus.CompletedWithIssues + : TaskPlanStatus.Completed; + plan.ExecutionSummary = anySkipped + ? $"Completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps; " + + $"{plan.Steps.Count(s => s.Status == TaskStepStatus.Skipped)} step(s) were skipped after failure." + : $"Successfully completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps."; + await ctx.RaiseAsync(TaskProgressEvent.PlanCompleted(plan)); + } + else + { + plan.Status = TaskPlanStatus.Failed; + plan.ExecutionSummary = $"Completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps with some failures."; + } + + await context.YieldOutputAsync(plan.Status.ToString(), cancellationToken); + } +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs new file mode 100644 index 0000000..2707d27 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs @@ -0,0 +1,58 @@ +namespace MandoCode.Services; + +/// +/// Messages passed between the plan workflow's executors. +/// +/// +/// Records, and deliberately small: everything here has to survive JSON round-tripping once +/// checkpointing lands, so nothing may carry a delegate, a stream, or a live service reference. +/// The step cursor travels in these messages and in the workflow's shared state — never in the +/// graph's shape, because resume requires byte-identical topology and a per-step node layout would +/// differ between a 3-step and a 12-step plan. +/// +internal static class PlanWorkflowMessages +{ + /// Shared state scope. State written without a scope name is executor-private. + public const string StateScope = "mandocode.plan"; + + /// Key under holding the zero-based index of the next step. + public const string CursorKey = "cursor"; + + /// + /// Key under holding the whole . + /// + /// + /// This is what makes the run resumable: MAF captures shared state at every superstep boundary, + /// so anything here survives a checkpoint. Anything held only in + /// does not — that object carries live delegates and cannot be serialized. + /// + public const string StateKey = "state"; +} + +/// Kicks off a run. Carries nothing: the plan itself is owned by the run context. +internal sealed record StartPlanRun; + +/// Instructs the step runner to execute the step at . +internal sealed record RunPlanStep(int StepIndex); + +/// What happened to one step. Triage is the only thing that acts on this. +internal sealed record PlanStepOutcome( + int StepIndex, + PlanStepOutcomeKind Kind, + string? Result, + string? Error); + +internal enum PlanStepOutcomeKind +{ + /// Step finished and produced a result. + Completed, + + /// Step threw. Whether this skips the step or ends the plan is the consumer's call. + Failed, + + /// Cancellation token tripped, or the user cancelled the plan from a diff prompt. + Cancelled, +} + +/// Terminal message; the finalizer turns this into the workflow's output. +internal sealed record PlanRunFinished(string Summary); diff --git a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs new file mode 100644 index 0000000..0928eb2 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -0,0 +1,185 @@ +using System.Runtime.CompilerServices; +using System.Threading.Channels; +using MandoCode.Models; +using Microsoft.Agents.AI.Workflows; + +namespace MandoCode.Services; + +/// +/// Executes an approved plan as a Microsoft Agent Framework workflow. +/// +/// +/// +/// Drop-in alternative to behind the planner config key, so +/// both engines can be A/B'd against real local models from the same session. It emits the same +/// stream, so neither front-end changes. +/// +/// +/// Topology is fixed — intake, step runner, triage, finalizer — regardless of how many steps the +/// plan has. The cursor lives in the workflow's shared state and in the messages, never in the +/// graph's shape: resume requires byte-identical topology, and a node-per-step layout would differ +/// between a 3-step and a 12-step plan. +/// +/// +public sealed class WorkflowPlanRunner( + IPlanStepExecutor stepExecutor, + PlanHandoff? planHandoff = null, + Action? onStateSaved = null) : IPlanRunner +{ + private readonly IPlanStepExecutor _stepExecutor = stepExecutor + ?? throw new ArgumentNullException(nameof(stepExecutor)); + + // Optional: supplies the file-operation evidence recorded at the middleware choke point, which + // is part of what a resumed run needs in order to know what already happened on disk. + private readonly PlanHandoff? _planHandoff = planHandoff; + + // Invoked whenever the run advances, so the host can record progress for resume. + private readonly Action? _onStateSaved = onStateSaved; + + /// + /// One progress event, plus an optional handshake the producer waits on. + /// + /// + /// The handshake is what preserves the existing consumer contract: an interactive consumer + /// handles a failed step by mutating plan.Status, and that decision is only visible once + /// it comes back for the next event. Completing after the yield return + /// resumes gives the workflow exactly that ordering. + /// + private sealed record Signal(TaskProgressEvent Event, TaskCompletionSource? Ack); + + public IAsyncEnumerable ExecutePlanAsync( + TaskPlan plan, + CancellationToken cancellationToken = default) + => RunAsync(plan, seedResults: null, cancellationToken); + + /// + /// Continues a plan recorded before an interruption. + /// + /// + /// Seeds the results earlier steps produced. A resumed run that skipped this would execute its + /// remaining steps blind to everything already built — the context those steps usually depend + /// on — even though the record holds it. + /// + public IAsyncEnumerable ResumeAsync( + PlanRunState state, + CancellationToken cancellationToken = default) + => RunAsync(PlanCheckpointStore.ToPlan(state), state.PreviousResults, cancellationToken); + + /// + /// Continues a saved run using the same reconstructed plan instance the interactive host owns. + /// Failure decisions mutate that instance during progress-event handshakes, so constructing a + /// second plan inside the runner would make retry/cancel choices invisible to workflow triage. + /// + public IAsyncEnumerable ResumeAsync( + TaskPlan plan, + PlanRunState state, + CancellationToken cancellationToken = default) + => RunAsync(plan, state.PreviousResults, cancellationToken); + + private async IAsyncEnumerable RunAsync( + TaskPlan plan, + IReadOnlyList? seedResults, + [EnumeratorCancellation] CancellationToken cancellationToken = default) + { + var channel = Channel.CreateUnbounded(new UnboundedChannelOptions + { + SingleReader = true, + SingleWriter = true, + }); + + async Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer, CancellationToken ct) + { + var ack = waitForConsumer ? new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously) : null; + await channel.Writer.WriteAsync(new Signal(evt, ack), CancellationToken.None); + + if (ack == null) return; + + // Never let a consumer that stops enumerating wedge the workflow forever. + using var reg = ct.Register(() => ack.TrySetResult()); + await ack.Task; + } + + var ctx = new PlanRunContext( + plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff, _onStateSaved, seedResults); + var workflow = BuildWorkflow(ctx); + + var pump = Task.Run(async () => + { + try + { + // Named: the third positional parameter is sessionId, not the token. + await using var run = await InProcessExecution.RunStreamingAsync( + workflow, new StartPlanRun(), cancellationToken: cancellationToken); + + await foreach (var evt in run.WatchStreamAsync(cancellationToken)) + { + if (evt is WorkflowOutputEvent or WorkflowErrorEvent) break; + } + + // WatchStreamAsync can return before the run has actually quiesced — verified in the + // Phase 0 spike, where a tool call landed after the stream had ended. Declaring the + // plan finished here would report success while its steps were still writing files. + while (await run.GetStatusAsync(CancellationToken.None) == RunStatus.Running) + { + await Task.Delay(25, CancellationToken.None); + } + } + finally + { + channel.Writer.TryComplete(); + } + }, CancellationToken.None); + + try + { + await foreach (var signal in channel.Reader.ReadAllAsync(CancellationToken.None)) + { + yield return signal.Event; + + // Resumed: the consumer has handled the event and asked for the next one, so any + // status it set is now visible to triage. + signal.Ack?.TrySetResult(); + } + } + finally + { + // Surfaces executor faults rather than letting them vanish into the background task. + await pump; + } + } + + /// + /// Builds the graph. Kept here rather than in a factory type because the executors close over + /// per-run state and the graph is therefore built per run, not shared. + /// + /// Internal rather than private so the topology test can assert on the built graph. + internal static Workflow BuildWorkflow(PlanRunContext ctx) + { + var intake = new PlanIntakeExecutor(ctx); + var stepRunner = new PlanStepRunnerExecutor(ctx); + var triage = new PlanTriageExecutor(ctx); + var finalizer = new PlanFinalizerExecutor(ctx); + + // Plain edges: every message is typed and addressed to a specific executor id, so routing is + // already unambiguous. (The conditional AddEdge overloads are mutually ambiguous to C# + // overload resolution anyway.) + return new WorkflowBuilder(intake) + .AddEdge(intake, stepRunner) + .AddEdge(intake, finalizer) // empty plan + .AddEdge(stepRunner, triage) + .AddEdge(triage, stepRunner) // loop back for the next step + .AddEdge(triage, finalizer) + .WithOutputFrom(finalizer) + .Build(); + } + + /// + /// + /// Kept for contract parity with . Triage reads step status when + /// it reconciles a failure, so setting it here is all that is required. + /// + public void SkipStep(TaskPlan plan, TaskStep step) => step.Status = TaskStepStatus.Skipped; + + /// + public void CancelPlan(TaskPlan plan) => plan.Status = TaskPlanStatus.Cancelled; +} diff --git a/src/MandoCode/Services/Ai/StepNarration.cs b/src/MandoCode/Services/Ai/StepNarration.cs new file mode 100644 index 0000000..183d102 --- /dev/null +++ b/src/MandoCode/Services/Ai/StepNarration.cs @@ -0,0 +1,78 @@ +using System.Text; + +namespace MandoCode.Services; + +/// +/// Turns a model's streamed output into a single short line of "what it is doing right now", +/// suitable for a spinner label. +/// +/// +/// +/// A plan step's text is only rendered once the step finishes — streaming exists for the stall +/// watchdog's heartbeat, not for display. That leaves a long step looking silent: a spinner and +/// nothing else for minutes, which reads as a hang. Observed live, a step sat at "Working…" for +/// four minutes while the model was narrating the whole time. +/// +/// +/// This deliberately shows only the latest line rather than streaming everything to the console. +/// The full response still renders as markdown when the step completes, so printing it live too +/// would duplicate it — and a partially-arrived markdown document cannot be rendered sensibly. +/// +/// +public sealed class StepNarration +{ + private readonly StringBuilder _currentLine = new(); + private string _lastCompleteLine = ""; + + /// Feeds one streamed chunk in. Chunks may split mid-line or mid-word. + public void Append(string? chunk) + { + if (string.IsNullOrEmpty(chunk)) return; + + foreach (var c in chunk) + { + if (c == '\n') + { + var line = _currentLine.ToString().Trim(); + if (line.Length > 0) _lastCompleteLine = line; + _currentLine.Clear(); + } + else if (c != '\r') + { + _currentLine.Append(c); + } + } + } + + /// + /// The line to display, or null when nothing worth showing has arrived yet. + /// Prefers the line currently being written; falls back to the last completed one so the + /// display doesn't blank out between lines. + /// + public string? Latest + { + get + { + var partial = _currentLine.ToString().Trim(); + var line = partial.Length > 0 ? partial : _lastCompleteLine; + return line.Length > 0 ? line : null; + } + } + + /// + /// shortened to , or null. + /// + /// + /// A spinner label that wraps corrupts the line the spinner is redrawing, so the caller's + /// available width is a hard limit rather than a preference. + /// + public string? Shortened(int maxLength) + { + var line = Latest; + if (line == null) return null; + if (maxLength <= 1) return null; + if (line.Length <= maxLength) return line; + + return string.Concat(line.AsSpan(0, maxLength - 1), "…"); + } +} diff --git a/src/MandoCode/Services/Ai/StreamBuffering.cs b/src/MandoCode/Services/Ai/StreamBuffering.cs index 4f7f402..a716206 100644 --- a/src/MandoCode/Services/Ai/StreamBuffering.cs +++ b/src/MandoCode/Services/Ai/StreamBuffering.cs @@ -23,21 +23,38 @@ public static class StreamBuffering { /// The streamed chunks. /// Invoked once per chunk BEFORE it's appended — the watchdog heartbeat. + /// + /// Optional, invoked with each chunk's text as it arrives. Lets a caller show progress while a + /// long generation is still running; the assembled result is unaffected either way. + /// /// Cancels enumeration; an propagates. public static Task BufferAsync( IAsyncEnumerable stream, Action onChunk, + Action? onText = null, CancellationToken cancellationToken = default) => - WithHeartbeat(stream, onChunk, cancellationToken).ToAgentResponseAsync(cancellationToken); + WithHeartbeat(stream, onChunk, onText, cancellationToken).ToAgentResponseAsync(cancellationToken); private static async IAsyncEnumerable WithHeartbeat( IAsyncEnumerable stream, Action onChunk, + Action? onText, [EnumeratorCancellation] CancellationToken cancellationToken) { await foreach (var update in stream.WithCancellation(cancellationToken)) { onChunk(); + + if (onText != null) + { + var text = update.Text; + // Never let a progress display break the generation it is reporting on. + if (!string.IsNullOrEmpty(text)) + { + try { onText(text); } catch { } + } + } + yield return update; } } diff --git a/src/MandoCode/Services/Ai/TaskPlannerService.cs b/src/MandoCode/Services/Ai/TaskPlannerService.cs index 79d918b..4e52946 100644 --- a/src/MandoCode/Services/Ai/TaskPlannerService.cs +++ b/src/MandoCode/Services/Ai/TaskPlannerService.cs @@ -10,18 +10,31 @@ namespace MandoCode.Services; /// . A slim deterministic heuristic () /// only exists for local models that don't reliably self-invoke the tool. /// -public class TaskPlannerService +public class TaskPlannerService : IPlanRunner { - private readonly AIService _aiService; + private readonly IPlanStepExecutor _stepExecutor; private readonly MandoCodeConfig _config; private readonly object _planStatusLock = new(); - public TaskPlannerService(AIService aiService, MandoCodeConfig config) + /// + /// Preferred constructor. Taking the step executor rather than is what + /// lets plan sequencing, cancellation and skip/fail handling be tested without a live model. + /// + public TaskPlannerService(IPlanStepExecutor stepExecutor, MandoCodeConfig config) { - _aiService = aiService; + _stepExecutor = stepExecutor ?? throw new ArgumentNullException(nameof(stepExecutor)); _config = config; } + /// + /// Delegating overload kept so existing callers — including the Desktop app, which constructs + /// this by hand — compile unchanged against this commit. + /// + public TaskPlannerService(AIService aiService, MandoCodeConfig config) + : this(new AiServicePlanStepExecutor(aiService), config) + { + } + /// /// Deterministic planning signal for models that can't be trusted to self-invoke /// propose_plan. Only fires on near-zero-false-positive signals; everything else @@ -121,7 +134,7 @@ public async IAsyncEnumerable ExecutePlanAsync(TaskPlan plan, try { - var result = await _aiService.ExecutePlanStepAsync(step.Instruction, previousResults, cancellationToken); + var result = await _stepExecutor.ExecuteStepAsync(step.Instruction, previousResults, cancellationToken); step.Result = result; step.Status = TaskStepStatus.Completed; @@ -161,7 +174,7 @@ public async IAsyncEnumerable ExecutePlanAsync(TaskPlan plan, stepEvent = TaskProgressEvent.StepFailed(plan, step, ex.Message); } - await _aiService.CompletionTracker.WaitForAllCompletionsAsync(TimeSpan.FromSeconds(5)); + await _stepExecutor.WaitForQuiescenceAsync(TimeSpan.FromSeconds(5)); if (stepEvent != null) { @@ -195,15 +208,20 @@ public async IAsyncEnumerable ExecutePlanAsync(TaskPlan plan, } } - var allCompleted = plan.Steps.All(s => + var allSettled = plan.Steps.All(s => s.Status == TaskStepStatus.Completed || s.Status == TaskStepStatus.Skipped); - + var anySkipped = plan.Steps.Any(s => s.Status == TaskStepStatus.Skipped); var anyFailed = plan.Steps.Any(s => s.Status == TaskStepStatus.Failed); - if (allCompleted && !anyFailed) + if (allSettled && !anyFailed) { - plan.Status = TaskPlanStatus.Completed; - plan.ExecutionSummary = $"Successfully completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps."; + plan.Status = anySkipped + ? TaskPlanStatus.CompletedWithIssues + : TaskPlanStatus.Completed; + plan.ExecutionSummary = anySkipped + ? $"Completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps; " + + $"{plan.Steps.Count(s => s.Status == TaskStepStatus.Skipped)} step(s) were skipped after failure." + : $"Successfully completed {plan.CompletedStepsCount} of {plan.Steps.Count} steps."; yield return TaskProgressEvent.PlanCompleted(plan); } else if (plan.Status != TaskPlanStatus.Cancelled) diff --git a/src/MandoCode/Services/Input/InputStateMachine.cs b/src/MandoCode/Services/Input/InputStateMachine.cs index b652750..56c5d39 100644 --- a/src/MandoCode/Services/Input/InputStateMachine.cs +++ b/src/MandoCode/Services/Input/InputStateMachine.cs @@ -160,7 +160,7 @@ public static bool IsCommand(string input) public static string GetCommandName(string input) { if (!IsCommand(input)) return input; - return input.TrimStart().Substring(1).ToLower(); + return input.TrimStart().Substring(1).Trim().ToLowerInvariant(); } public IEnumerable GetAllCommands() => _commands.Keys; diff --git a/src/MandoCode/Services/Input/InstructionPromptCoordinator.cs b/src/MandoCode/Services/Input/InstructionPromptCoordinator.cs index 53f4b29..248a302 100644 --- a/src/MandoCode/Services/Input/InstructionPromptCoordinator.cs +++ b/src/MandoCode/Services/Input/InstructionPromptCoordinator.cs @@ -18,13 +18,14 @@ public sealed class InstructionPromptCoordinator public bool IsActive { get; private set; } public string Prompt { get; private set; } = string.Empty; + public string InitialValue { get; private set; } = string.Empty; /// /// Fires when changes so App.razor can re-render. /// public event Action? StateChanged; - public Task RequestAsync(string prompt) + public Task RequestAsync(string prompt, string? initialValue = null) { TaskCompletionSource tcs; lock (_gate) @@ -36,6 +37,7 @@ public Task RequestAsync(string prompt) tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _tcs = tcs; Prompt = prompt; + InitialValue = initialValue ?? string.Empty; IsActive = true; } @@ -52,6 +54,7 @@ public void Submit(string value) _tcs = null; IsActive = false; Prompt = string.Empty; + InitialValue = string.Empty; } StateChanged?.Invoke(); diff --git a/src/MandoCode/Services/VersionLabel.cs b/src/MandoCode/Services/VersionLabel.cs new file mode 100644 index 0000000..a9dcec9 --- /dev/null +++ b/src/MandoCode/Services/VersionLabel.cs @@ -0,0 +1,42 @@ +using System.Reflection; + +namespace MandoCode.Services; + +/// +/// Builds the version string shown under the startup banner. +/// +/// +/// Prefers the informational version so prerelease tags survive. Assembly.GetName().Version +/// is numeric-only and silently drops them, which made a test build indistinguishable from the +/// release it was cut from — the same confusion that makes a stale binary hard to spot. +/// +public static class VersionLabel +{ + /// Version label for the running assembly, e.g. "v0.15.0" or "v0.16.0-rc.1". + public static string ForAssembly(Assembly assembly) => Build( + assembly.GetCustomAttribute()?.InformationalVersion, + assembly.GetName().Version); + + /// + /// Pure formatting, split out so it can be tested without constructing an assembly. + /// + /// + /// e.g. 0.16.0-rc.1+a1a0df8. Build metadata after '+' (appended by SourceLink) is + /// dropped; the prerelease tag is kept, since telling builds apart at a glance is the point. + /// + /// Numeric fallback for when the attribute is missing or empty. + public static string Build(string? informationalVersion, Version? assemblyVersion) + { + if (!string.IsNullOrWhiteSpace(informationalVersion)) + { + var trimmed = informationalVersion.Trim(); + var plus = trimmed.IndexOf('+'); + if (plus >= 0) trimmed = trimmed[..plus]; + if (!string.IsNullOrWhiteSpace(trimmed)) return $"v{trimmed}"; + } + + return assemblyVersion != null + ? $"v{assemblyVersion.Major}.{assemblyVersion.Minor}.{assemblyVersion.Build}" + : ""; + } +} diff --git a/src/MandoCode/docs/TaskPlanner.md b/src/MandoCode/docs/TaskPlanner.md index 5d8944e..558eafb 100644 --- a/src/MandoCode/docs/TaskPlanner.md +++ b/src/MandoCode/docs/TaskPlanner.md @@ -309,10 +309,92 @@ See the main [README](../../../README.md#diff-approvals) for user-facing documen --- +## MAF workflow planner + +The optional workflow planner is built on `Microsoft.Agents.AI.Workflows`. Its first architectural +change was removing one root cause: the old planner executed the whole plan inside the +`propose_plan` tool call. Because the outer model turn was still open, it could not observe the work, +treated the returned summary as "not started yet," and repeated it. `propose_plan` now returns a +receipt, the turn drains, the host asks for approval, and only then does the selected runner execute. + +Decision: an **authored `WorkflowBuilder` graph**, not Magentic. Magentic's .NET manager is more +local-model-tolerant than expected (prompt-injected schema, tolerant JSON extraction, 3 retries), +but MandoCode has one agent — so `next_speaker` selection is degenerate — it throws and kills the +run after 3 ledger parse failures with no way to raise the limit, and **its manager cannot have +tools**, which would mean planning code changes against a repo it can't read. Handoff is rejected +too: it routes via `handoff_to_*` tool calls that cannot be forced, the worst possible dependency on +7B–30B instruction-following. + +| Phase | Scope | State | +|---|---|---| +| 0 | Spike the real 1.19.0 assembly | done | +| 1 | Freeze identity / checkpoint envelope / config key; extract `IPlanRunner` + `IPlanStepExecutor` | done | +| 2 | Un-nest: `propose_plan` returns a receipt, the host runs the plan after the turn drains | done | +| 3 | The graph behind `planner=workflow`: fixed topology, triage owns plan state | done | +| 3b | Show step instructions and support coherent pre-execution edits | done | +| 4 | Checkpointing, resume, discard, and native Desktop recovery actions | done | +| 5 | Retry, replan approval, deterministic `/plan `, and truthful partial status | done | +| 6 | Flip the `planner` default, then delete the legacy runner | deferred pending release soak | + +Spike findings worth keeping: + +- A middleware-decorated agent **keeps its middleware** when used as a workflow executor (verified on + net10.0 and net8.0). The guard circuits survive the boundary. +- `Microsoft.Agents.AI.Workflows` requires `Microsoft.Agents.AI` at the *same* version — a mismatch is + `NU1605`, which `TreatWarningsAsErrors` turns into a build error. Bump both together. +- No `` is needed: `WorkflowBuilder`, `RequestPort`, `InProcessExecution`, `CheckpointManager` + and `FileSystemJsonCheckpointStore` carry no `[Experimental]` attribute in 1.19.0. +- ⚠️ **`WatchStreamAsync` returns before the run quiesces.** Poll `GetStatusAsync()` until it leaves + `RunStatus.Running` before declaring a plan finished — measuring at stream-end reports a plan + complete while its steps are still writing files. + +Graph-authoring notes (all learned the hard way against the real assembly): + +- Every executor that sends must declare `[SendsMessage(typeof(T))]`; the finalizer needs + `[YieldsOutput(typeof(T))]`. Omitting one throws at **runtime**, not compile time. +- State written without a scope name is **executor-private**. Plan state uses the named-scope + overload — the first attempt silently reported "0 steps completed". +- The conditional `AddEdge` overloads are mutually ambiguous to C# overload resolution, and hand + the condition a nullable `T?`. Plain edges plus typed messages plus an explicit `targetId` route + unambiguously, so conditions are not needed. +- `Workflow.ToString()` and `EdgeInfo.ToString()` return type names only — a topology test built on + them passes vacuously. Use `ReflectExecutors()` / `ReflectEdges()` / `WorkflowVisualizer`. +- `InProcessExecution.RunStreamingAsync`'s third positional parameter is `sessionId`, not the + cancellation token. + +`planner` selects the engine (`legacy` | `workflow` | `default`) and is re-read per plan by +`PlanRunnerSelector`, so it can be flipped mid-session without losing history — which is what makes +an honest A/B against a local model practical. `PlanRunnerBehaviorTests` runs every behavioral case +against **both** engines: while both are selectable, any divergence would make that A/B +uninterpretable, since a behavior difference would be indistinguishable from a model difference. + +Executor and agent identities are fixed in `PlanExecutorIds` and must not drift: MAF derives +workflow-executor identity from both the agent's `Id` and `Name`, and a checkpoint written under one +identity can never be resumed under another. `PlanExecutorIdsTests` holds a golden list precisely so +a rename fails loudly. + +`/plan ` bypasses the heuristic and uses a stateless proposal-only call. That call receives +only `propose_plan`; it cannot read or change the project. If a provider ignores required tool +choice, proposal generation falls back to schema-constrained JSON and then to a safe one-step host +proposal. `/plan` without a goal inspects the current checkpoint, while `/plan-resume` and +`/plan-discard` act on it. + +The workflow checkpoint stores the original plan, settled step results, the next workflow cursor, +file-operation context, project root, model, planner engine, and fixed executor identities. Resume +validates that envelope before running and never replays settled steps. Desktop exposes the same +state through an Unfinished Plan card; the CLI exposes it through commands. + +On failure, the host offers retry, revise the remaining plan, skip, or cancel. A revision preserves +the settled prefix and returns the replacement suffix to the user for editing and approval. Reaching +the end with skipped work produces `CompletedWithIssues`, not an unqualified success. + +--- + ## Future Improvements - [ ] Step dependency graph for parallel execution of independent steps -- [ ] Plan persistence for resuming interrupted plans across sessions -- [ ] User-editable plan before approval +- [x] Plan persistence for resuming interrupted plans across sessions +- [x] User-editable plans with visible instructions and dependent-suffix regeneration +- [x] Retry, failure replanning with approval, skip/cancel decisions, and truthful partial status - [x] ~~Mid-generation progress signals~~ — shipped: `StreamingMode` config streams chunks as the model emits them (via `_agent.RunStreamingAsync`, buffered through `StreamBuffering` for the stall-watchdog heartbeat), with non-streaming still available as a fallback for models where streaming + auto-invoke proved unreliable. - [ ] Smarter `Required` vs `Auto` tool-choice on plan-step first turn (force a tool call on steps that the model drifts into prose on) diff --git a/tests/MandoCode.Tests/AgentFunctionMiddlewareLifecycleTests.cs b/tests/MandoCode.Tests/AgentFunctionMiddlewareLifecycleTests.cs index b021481..d6caab6 100644 --- a/tests/MandoCode.Tests/AgentFunctionMiddlewareLifecycleTests.cs +++ b/tests/MandoCode.Tests/AgentFunctionMiddlewareLifecycleTests.cs @@ -212,30 +212,72 @@ public async Task EditFailures_AcrossPathAliases_ShareOneCircuitCounter() } [Fact] - public async Task SecondProposePlan_InSameTurn_IsShortCircuited() + public async Task ProposePlan_QueuesThePlan_WithoutRunningIt() { + // The defining property of the deferred-proposal design: propose_plan returns a receipt and + // the plan runs only once the host drains the turn. Previously this single call awaited + // approval and every step inline, which is what forced the watchdog pause, the prompt-gate + // release dance, and the post-plan mutation gate into existence. + var ran = false; var handoff = new PlanHandoff { - OnPlanRequested = (_, _) => Task.FromResult("plan executed") + OnPlanRequested = (_, _) => { ran = true; return Task.FromResult("plan executed"); } }; var middleware = new AgentFunctionMiddleware(0, null, null, handoff); var fn = Fn((string goal, string steps) => "should never run — intercepted", "propose_plan"); using var scope = middleware.BeginScope(); - var args = new AIFunctionArguments + var receipt = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { ["goal"] = "build a game", ["steps"] = "[{\"description\":\"step one\",\"instruction\":\"do the thing\"}]" + }); + + Assert.False(ran); + Assert.True(handoff.HasPendingProposal); + Assert.True(scope.ProposalPending); + Assert.Contains("Plan received", receipt?.ToString()); + + // ...and it runs when the host asks for it, after the turn. + var manifest = await handoff.RunPendingPlanAsync(); + Assert.True(ran); + Assert.Equal("plan executed", manifest); + Assert.False(handoff.HasPendingProposal); + } + + [Fact] + public async Task SecondProposePlan_InSameTurn_ReplacesTheFirst() + { + // Previously refused outright, because a second proposal meant the first had already run + // and the model was starting uninvited extra work. With execution deferred nothing has run + // yet, so last-wins is both correct and safer than a prose refusal the model can ignore. + TaskPlan? executed = null; + var handoff = new PlanHandoff + { + OnPlanRequested = (plan, _) => { executed = plan; return Task.FromResult("done"); } }; + var middleware = new AgentFunctionMiddleware(0, null, null, handoff); + var fn = Fn((string goal, string steps) => "should never run — intercepted", "propose_plan"); + + using var scope = middleware.BeginScope(); + + await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + { + ["goal"] = "first goal", + ["steps"] = "[{\"description\":\"one\",\"instruction\":\"do one\"}]" + }); + await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + { + ["goal"] = "second goal", + ["steps"] = "[{\"description\":\"two\",\"instruction\":\"do two\"},{\"description\":\"three\",\"instruction\":\"do three\"}]" + }); - var first = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, args); - Assert.Contains("plan executed", first?.ToString()); - Assert.True(scope.PlanAlreadyProcessed); + await handoff.RunPendingPlanAsync(); - var second = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, args); - Assert.Contains("already proposed", second?.ToString()); - Assert.DoesNotContain("plan executed", second?.ToString()); + Assert.NotNull(executed); + Assert.Equal("second goal", executed!.OriginalRequest); + Assert.Equal(2, executed.Steps.Count); } [Fact] @@ -250,19 +292,51 @@ public async Task MalformedProposal_DoesNotConsumeThePlanSlot() using var scope = middleware.BeginScope(); - var malformed = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { ["goal"] = "build a game", ["steps"] = "not json at all" }); - Assert.False(scope.PlanAlreadyProcessed); + Assert.False(scope.ProposalPending); + Assert.False(handoff.HasPendingProposal); - var retry = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { ["goal"] = "build a game", ["steps"] = "[{\"description\":\"step one\",\"instruction\":\"do the thing\"}]" }); - Assert.Contains("plan executed", retry?.ToString()); - Assert.True(scope.PlanAlreadyProcessed); + Assert.True(scope.ProposalPending); + Assert.True(handoff.HasPendingProposal); + } + + [Fact] + public async Task ProposePlan_DuringPlanExecution_IsRefused() + { + // A step's own model call can reach propose_plan. Nested planning is always a runaway. + var handoff = new PlanHandoff(); + var middleware = new AgentFunctionMiddleware(0, null, null, handoff); + var fn = Fn((string goal, string steps) => "should never run — intercepted", "propose_plan"); + + string? nested = null; + handoff.OnPlanRequested = async (_, _) => + { + using var stepScope = middleware.BeginScope(); + nested = (await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + { + ["goal"] = "a plan within a plan", + ["steps"] = "[{\"description\":\"nested\",\"instruction\":\"nested\"}]" + }))?.ToString(); + return "outer done"; + }; + + using var scope = middleware.BeginScope(); + await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments + { + ["goal"] = "outer", + ["steps"] = "[{\"description\":\"one\",\"instruction\":\"do one\"}]" + }); + await handoff.RunPendingPlanAsync(); + + Assert.Contains("already executing", nested); } } diff --git a/tests/MandoCode.Tests/AgentFunctionMiddlewarePostPlanMutationGateTests.cs b/tests/MandoCode.Tests/AgentFunctionMiddlewarePendingPlanGateTests.cs similarity index 61% rename from tests/MandoCode.Tests/AgentFunctionMiddlewarePostPlanMutationGateTests.cs rename to tests/MandoCode.Tests/AgentFunctionMiddlewarePendingPlanGateTests.cs index 4dc5c4c..e718ea2 100644 --- a/tests/MandoCode.Tests/AgentFunctionMiddlewarePostPlanMutationGateTests.cs +++ b/tests/MandoCode.Tests/AgentFunctionMiddlewarePendingPlanGateTests.cs @@ -5,26 +5,33 @@ namespace MandoCode.Tests; /// -/// MAF-side sibling of the old PostPlanMutationGateTests' Kernel-driven cases (feat/agent-framework-migration, -/// Phase 6, since deleted along with the rest of SK). Its PlanHandoff-manifest tests weren't -/// duplicated here — PlanHandoff and TaskPlan are already framework-agnostic (confirmed during the -/// migration survey), so those tests already exercise the same code this middleware calls into; -/// there was nothing SK-specific in them to port. +/// Successor to AgentFunctionMiddlewarePostPlanMutationGateTests, retargeted onto the pending-plan +/// gate. +/// +/// The original gate refused mutations for the REST of the turn after a plan had run, because the +/// plan executed inside the propose_plan tool call: the outer model never saw the steps run, read +/// the returned summary as "not started yet", and redid the work — observed live overwriting a +/// finished build under an auto-approved session. +/// +/// Deferring execution removes the post-plan turn entirely, so the window that needs guarding +/// shrinks to "between propose_plan and the end of the reply" — the model must not race the plan +/// it just queued. The incident these tests were written for is still the reason they exist, which +/// is why they were retargeted rather than deleted. /// -public class AgentFunctionMiddlewarePostPlanMutationGateTests +public class AgentFunctionMiddlewarePendingPlanGateTests { private static AIFunction Fn(Delegate method, string name) => AIFunctionFactory.Create(method, new AIFunctionFactoryOptions { Name = name }); [Fact] - public async Task CompletedPlanScope_RefusesMutatingCall_WithoutInvokingIt() + public async Task PendingPlanScope_RefusesMutatingCall_WithoutInvokingIt() { var invoked = false; var middleware = new AgentFunctionMiddleware(5); var fn = Fn((string relativePath, string content) => { invoked = true; return "written"; }, "write_file"); using var scope = middleware.BeginScope(); - scope.MarkPlanWorkCompleted(); + scope.MarkProposalPending(); var result = await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { @@ -33,7 +40,7 @@ public async Task CompletedPlanScope_RefusesMutatingCall_WithoutInvokingIt() }); Assert.False(invoked); - Assert.Contains("already completed", result?.ToString(), StringComparison.OrdinalIgnoreCase); + Assert.Contains("plan is queued", result?.ToString(), StringComparison.OrdinalIgnoreCase); } [Theory] @@ -41,14 +48,14 @@ public async Task CompletedPlanScope_RefusesMutatingCall_WithoutInvokingIt() [InlineData("delete_file")] [InlineData("delete_folder")] [InlineData("create_folder")] - public async Task CompletedPlanScope_RefusesAllMutatingFunctions(string functionName) + public async Task PendingPlanScope_RefusesAllMutatingFunctions(string functionName) { var invoked = false; var middleware = new AgentFunctionMiddleware(5); var fn = Fn((string relativePath) => { invoked = true; return "ok"; }, functionName); using var scope = middleware.BeginScope(); - scope.MarkPlanWorkCompleted(); + scope.MarkProposalPending(); await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { ["relativePath"] = "Test" }); @@ -56,14 +63,15 @@ public async Task CompletedPlanScope_RefusesAllMutatingFunctions(string function } [Fact] - public async Task CompletedPlanScope_StillAllowsReads() + public async Task PendingPlanScope_StillAllowsReads() { + // The model may still want to look around before summarising; only writes would race the plan. var invoked = false; var middleware = new AgentFunctionMiddleware(5); var fn = Fn((string relativePath) => { invoked = true; return "file contents"; }, "read_file_contents"); using var scope = middleware.BeginScope(); - scope.MarkPlanWorkCompleted(); + scope.MarkProposalPending(); await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { ["relativePath"] = "Test/index.html" }); @@ -73,13 +81,14 @@ public async Task CompletedPlanScope_StillAllowsReads() [Fact] public async Task FreshScope_AllowsMutationsAgain() { + // The plan runs between turns; by the next scope it has finished and the model is free again. var invoked = false; var middleware = new AgentFunctionMiddleware(5); var fn = Fn((string relativePath, string content) => { invoked = true; return "written"; }, "write_file"); using (var planTurn = middleware.BeginScope()) { - planTurn.MarkPlanWorkCompleted(); + planTurn.MarkProposalPending(); } using var nextTurn = middleware.BeginScope(); @@ -93,14 +102,16 @@ public async Task FreshScope_AllowsMutationsAgain() } [Fact] - public async Task RejectedPlan_DoesNotEngageGate() + public async Task ScopeWithNoProposal_DoesNotEngageGate() { + // Successor to RejectedPlan_DoesNotEngageGate. A turn where nothing was proposed — including + // one where the user went on to reject the plan — must leave the model free to do the work + // directly. var invoked = false; var middleware = new AgentFunctionMiddleware(5); var fn = Fn((string relativePath, string content) => { invoked = true; return "written"; }, "write_file"); using var scope = middleware.BeginScope(); - scope.MarkPlanProcessed(); await AgentMiddlewareTestHelpers.InvokeAsync(middleware, fn, new AIFunctionArguments { diff --git a/tests/MandoCode.Tests/InputStateMachineTests.cs b/tests/MandoCode.Tests/InputStateMachineTests.cs index 974a510..be3573b 100644 --- a/tests/MandoCode.Tests/InputStateMachineTests.cs +++ b/tests/MandoCode.Tests/InputStateMachineTests.cs @@ -591,6 +591,7 @@ public void IsCommand_DetectsSlashPrefix(string input, bool expected) [InlineData("/help", "help")] [InlineData("/CLEAR", "clear")] // lowercased [InlineData(" /Config", "config")] // trimmed + lowered + [InlineData("/plan-resume ", "plan-resume")] // trailing input whitespace is not command syntax [InlineData("hello", "hello")] // not a command, returns as-is public void GetCommandName_ExtractsName(string input, string expected) { diff --git a/tests/MandoCode.Tests/InstructionPromptCoordinatorTests.cs b/tests/MandoCode.Tests/InstructionPromptCoordinatorTests.cs new file mode 100644 index 0000000..22440ff --- /dev/null +++ b/tests/MandoCode.Tests/InstructionPromptCoordinatorTests.cs @@ -0,0 +1,36 @@ +using MandoCode.Services; +using Xunit; + +namespace MandoCode.Tests; + +public sealed class InstructionPromptCoordinatorTests +{ + [Fact] + public async Task RequestAsync_ExposesInitialValueUntilSubmission() + { + var coordinator = new InstructionPromptCoordinator(); + + var request = coordinator.RequestAsync("Edit instruction:", "create beta.txt"); + + Assert.True(coordinator.IsActive); + Assert.Equal("Edit instruction:", coordinator.Prompt); + Assert.Equal("create beta.txt", coordinator.InitialValue); + + coordinator.Submit("create gamma.txt"); + + Assert.Equal("create gamma.txt", await request); + Assert.False(coordinator.IsActive); + Assert.Empty(coordinator.Prompt); + Assert.Empty(coordinator.InitialValue); + } + + [Fact] + public void RequestAsync_WithoutInitialValue_UsesEmptyText() + { + var coordinator = new InstructionPromptCoordinator(); + + _ = coordinator.RequestAsync("Enter instructions:"); + + Assert.Empty(coordinator.InitialValue); + } +} diff --git a/tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs b/tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs new file mode 100644 index 0000000..fdbf05d --- /dev/null +++ b/tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs @@ -0,0 +1,115 @@ +using System.Text.Json; +using Xunit; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Tests the versioned wrapper around the durable plan snapshot. Every field outside the payload is a +/// refusal criterion, because the failure mode of resuming a stale or foreign checkpoint is +/// re-running steps whose write_file already succeeded. +/// +public class PlanCheckpointEnvelopeTests +{ + private static PlanCheckpointEnvelope Make( + int? schemaVersion = null, + string? topologyVersion = null, + string projectRootHash = "abc123abc123", + string modelName = "qwen3:8b") + => new() + { + SchemaVersion = schemaVersion ?? PlanCheckpointEnvelope.CurrentSchemaVersion, + TopologyVersion = topologyVersion ?? PlanExecutorIds.TopologyVersion, + PlanId = "plan-1", + ProjectRootHash = projectRootHash, + ModelName = modelName, + MandoCodeVersion = "0.15.0", + CreatedUtc = new DateTimeOffset(2026, 8, 27, 12, 0, 0, TimeSpan.Zero), + Payload = JsonSerializer.SerializeToElement(new { opaque = true }), + }; + + [Fact] + public void RoundTrips_ThroughJson() + { + var json = JsonSerializer.Serialize(Make()); + var back = JsonSerializer.Deserialize(json)!; + + Assert.Equal(PlanCheckpointEnvelope.CurrentSchemaVersion, back.SchemaVersion); + Assert.Equal(PlanExecutorIds.TopologyVersion, back.TopologyVersion); + Assert.Equal("plan-1", back.PlanId); + Assert.Equal("qwen3:8b", back.ModelName); + Assert.True(back.Payload.GetProperty("opaque").GetBoolean()); + } + + [Fact] + public void MatchingEnvelope_IsResumable() + { + Assert.Null(Make().FindIncompatibility("abc123abc123", "qwen3:8b")); + } + + [Fact] + public void SchemaVersionMismatch_IsRefused() + { + var reason = Make(schemaVersion: 99).FindIncompatibility("abc123abc123", "qwen3:8b"); + Assert.NotNull(reason); + Assert.Contains("different version", reason); + } + + [Fact] + public void TopologyVersionMismatch_IsRefused() + { + // A graph-shape change means the checkpoint's step boundaries no longer mean what they did. + var reason = Make(topologyVersion: "0").FindIncompatibility("abc123abc123", "qwen3:8b"); + Assert.NotNull(reason); + Assert.Contains("Start it again", reason); + } + + [Fact] + public void DifferentProject_IsRefused() + { + var reason = Make().FindIncompatibility("ffffffffffff", "qwen3:8b"); + Assert.NotNull(reason); + Assert.Contains("different project", reason); + } + + [Fact] + public void DifferentModel_IsRefused_AndNamesBothModels() + { + // Half a plan run by one model and half by another is not a state anyone can reason about. + var reason = Make().FindIncompatibility("abc123abc123", "gemma3:12b"); + Assert.NotNull(reason); + Assert.Contains("qwen3:8b", reason); + Assert.Contains("gemma3:12b", reason); + } + + [Fact] + public void DifferentDesktopAgentSession_IsRefused() + { + var reason = Make().FindIncompatibility( + "abc123abc123", + "qwen3:8b", + expectedPlanId: "another-agent"); + + Assert.NotNull(reason); + Assert.Contains("different agent session", reason); + } + + [Fact] + public void ProjectRootHash_IsStable_AndCaseInsensitive() + { + var a = PlanCheckpointEnvelope.HashProjectRoot(@"C:\work\Api"); + var b = PlanCheckpointEnvelope.HashProjectRoot(@"c:\work\api\"); + + Assert.Equal(a, b); + Assert.Equal(12, a.Length); + } + + [Fact] + public void ProjectRootHash_DistinguishesSameLeafInDifferentPlaces() + { + // Two folders both named "api" must not collide — the reason the hash exists at all. + Assert.NotEqual( + PlanCheckpointEnvelope.HashProjectRoot(@"C:\one\api"), + PlanCheckpointEnvelope.HashProjectRoot(@"C:\two\api")); + } +} diff --git a/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs new file mode 100644 index 0000000..c1d25e8 --- /dev/null +++ b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs @@ -0,0 +1,120 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Reconstructing a plan from a saved record. +/// +/// These cover the pure logic — what resume would actually re-run, and how a half-finished step is +/// treated. The file I/O is deliberately not exercised: the store writes under the user's real +/// profile directory, and a test that reaches into ~/.mandocode would be writing to the machine it +/// runs on. Making the root injectable is the prerequisite for covering that, and is worth doing +/// before the store grows. +/// +public class PlanCheckpointStoreTests +{ + private static PlanRunState StateWith(params TaskStepStatus[] statuses) => new() + { + Goal = "build a game", + Cursor = statuses.Count(s => s is TaskStepStatus.Completed or TaskStepStatus.Skipped), + Steps = [.. statuses.Select((s, i) => new PlanStepState + { + Number = i + 1, + Description = $"step {i + 1}", + Instruction = $"do thing {i + 1}", + Status = s, + Result = s == TaskStepStatus.Completed ? $"result {i + 1}" : null, + })], + }; + + [Fact] + public void OutstandingStepsCountsOnlyUnfinishedWork() + { + var state = StateWith( + TaskStepStatus.Completed, + TaskStepStatus.Skipped, + TaskStepStatus.Pending, + TaskStepStatus.Failed); + + Assert.Equal(2, PlanCheckpointStore.OutstandingSteps(state)); + } + + [Fact] + public void AFinishedPlanHasNothingOutstanding() + { + // The selector uses this to delete the record, so a completed plan is never offered. + var state = StateWith(TaskStepStatus.Completed, TaskStepStatus.Skipped); + Assert.Equal(0, PlanCheckpointStore.OutstandingSteps(state)); + } + + [Fact] + public void RebuiltPlanKeepsFinishedStepsFinished() + { + // This is what stops resume redoing writes that already succeeded — the runner steps over + // anything Completed or Skipped. + var plan = PlanCheckpointStore.ToPlan( + StateWith(TaskStepStatus.Completed, TaskStepStatus.Skipped, TaskStepStatus.Pending)); + + Assert.Equal(TaskStepStatus.Completed, plan.Steps[0].Status); + Assert.Equal(TaskStepStatus.Skipped, plan.Steps[1].Status); + } + + [Fact] + public void AnInterruptedStepIsRunAgain() + { + // A step that was InProgress when the process died may have half run. Re-running is the + // safer assumption: the alternative is silently skipping work that never completed. + var plan = PlanCheckpointStore.ToPlan(StateWith(TaskStepStatus.InProgress)); + Assert.Equal(TaskStepStatus.Pending, plan.Steps[0].Status); + } + + [Fact] + public void AFailedStepIsRunAgain() + { + var plan = PlanCheckpointStore.ToPlan(StateWith(TaskStepStatus.Failed)); + Assert.Equal(TaskStepStatus.Pending, plan.Steps[0].Status); + } + + [Fact] + public void RebuiltPlanCarriesTheInstructions() + { + // Resume re-issues the instruction, not the short display description. + var plan = PlanCheckpointStore.ToPlan(StateWith(TaskStepStatus.Pending, TaskStepStatus.Pending)); + + Assert.Equal("build a game", plan.OriginalRequest); + Assert.Equal("do thing 2", plan.Steps[1].Instruction); + } + + [Fact] + public void RebuiltPlanStartsPending() + { + // Not Completed or Cancelled — the run is about to begin again. + Assert.Equal( + TaskPlanStatus.Pending, + PlanCheckpointStore.ToPlan(StateWith(TaskStepStatus.Pending)).Status); + } + + [Fact] + public void PathIsStablePerProject_AndDistinguishesSameLeafNames() + { + var a = PlanCheckpointStore.PathFor(@"C:\one\api"); + var b = PlanCheckpointStore.PathFor(@"C:\two\api"); + + Assert.Equal(a, PlanCheckpointStore.PathFor(@"C:\one\api\")); // trailing separator + Assert.NotEqual(a, b); // two folders called "api" + Assert.Contains("api-", Path.GetFileName(a)); // readable leaf retained + } + + [Fact] + public void DesktopAgentCheckpointPaths_DoNotCollideWithinOneProject() + { + var first = PlanCheckpointStore.PathFor(@"C:\work\api", "agent-one"); + var second = PlanCheckpointStore.PathFor(@"C:\work\api", "agent-two"); + + Assert.NotEqual(first, second); + Assert.Equal(first, PlanCheckpointStore.PathFor(@"C:\work\api\", "agent-one")); + Assert.NotEqual(first, PlanCheckpointStore.PathFor(@"C:\other\api", "agent-one")); + } +} diff --git a/tests/MandoCode.Tests/PlanExecutorIdsTests.cs b/tests/MandoCode.Tests/PlanExecutorIdsTests.cs new file mode 100644 index 0000000..d376c44 --- /dev/null +++ b/tests/MandoCode.Tests/PlanExecutorIdsTests.cs @@ -0,0 +1,86 @@ +using System.Text.RegularExpressions; +using Xunit; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Guards the plan workflow's identity scheme. MAF matches checkpointed state back to executors by +/// identity, and for agent-backed executors that identity comes from both the agent's Id and Name — +/// so a checkpoint written under one identity can never be resumed under another, and there is no +/// repair path for checkpoints already on disk. +/// +/// These are cheap assertions protecting an expensive mistake: a careless rename in review would +/// silently orphan every checkpoint in the field. +/// +public class PlanExecutorIdsTests +{ + [Fact] + public void AllExecutorIds_AreDistinct() + { + var all = PlanExecutorIds.All; + Assert.Equal(all.Count, all.Distinct(StringComparer.Ordinal).Count()); + } + + [Fact] + public void AllExecutorIds_FollowTheNamingScheme() + { + // Versioned and namespaced, so a topology bump is visible in every id. + var pattern = new Regex(@"^mandocode\.plan\.v\d+\.[a-z][a-z-]*$"); + + foreach (var id in PlanExecutorIds.All) + { + Assert.False(string.IsNullOrWhiteSpace(id)); + Assert.Matches(pattern, id); + } + } + + [Fact] + public void StepAgent_DoesNotShareIdentityWithGeneralistAgent() + { + // Two differently-purposed agents under one name is exactly the collision that routes + // restored state to the wrong executor. + Assert.NotEqual(PlanExecutorIds.GeneralistAgentId, PlanExecutorIds.StepAgentId); + Assert.NotEqual(PlanExecutorIds.GeneralistAgentName, PlanExecutorIds.StepAgentName); + } + + [Fact] + public void GeneralistAgentName_IsUnchanged() + { + // User-visible, and pinned as the agent's identity. Changing it would invalidate every + // existing checkpoint for no benefit. + Assert.Equal("MandoCode", PlanExecutorIds.GeneralistAgentName); + } + + [Fact] + public void TopologyVersion_MatchesTheVersionEmbeddedInTheIds() + { + // If the ids say v2 but TopologyVersion still says 1, resume would accept a checkpoint + // written against a different graph. + foreach (var id in PlanExecutorIds.All) + { + Assert.Contains($".v{PlanExecutorIds.TopologyVersion}.", id); + } + } + + [Fact] + public void GoldenList_MatchesExactly() + { + // Deliberately hard-coded rather than derived. This test SHOULD fail when the graph + // changes — that failure is the reminder to bump TopologyVersion and decide what happens + // to checkpoints already written. + string[] expected = + [ + "mandocode.plan.v1.intake", + "mandocode.plan.v1.approval", + "mandocode.plan.v1.gate", + "mandocode.plan.v1.step-runner", + "mandocode.plan.v1.triage", + "mandocode.plan.v1.step-decision", + "mandocode.plan.v1.replanner", + "mandocode.plan.v1.finalizer", + ]; + + Assert.Equal(expected, PlanExecutorIds.All); + } +} diff --git a/tests/MandoCode.Tests/PlanHandoffTests.cs b/tests/MandoCode.Tests/PlanHandoffTests.cs index d110fe8..d3fd633 100644 --- a/tests/MandoCode.Tests/PlanHandoffTests.cs +++ b/tests/MandoCode.Tests/PlanHandoffTests.cs @@ -106,4 +106,44 @@ public async Task ProcessAsync_ResetsFlagAfterCompletion() Assert.Equal("ok", first); Assert.Equal("ok", second); } + + [Fact] + public async Task PendingProposal_CapturesTheRequestThatProducedIt() + { + var handoff = new PlanHandoff(); + TaskPlan? captured = null; + handoff.OnPlanRequested = (plan, _) => + { + captured = plan; + return Task.FromResult("rejected"); + }; + + handoff.SetRequestContext("In @Games/Pacman, replace only the renderer."); + handoff.SetPendingProposal( + "replace renderer", + [new PlanStepProposal("renderer", "replace renderer")]); + + await handoff.RunPendingPlanAsync(); + + Assert.NotNull(captured); + Assert.Equal("In @Games/Pacman, replace only the renderer.", captured!.OriginalRequest); + } + + [Fact] + public void ResumedExecution_RestoresEvidence_AndRecordsNewOperations() + { + var handoff = new PlanHandoff(); + + using (handoff.BeginResumedExecution( + [new PlanFileOperation("write_file", "src/one.cs")])) + { + Assert.True(handoff.IsExecuting); + handoff.RecordFileOperation("edit_file", "src/two.cs"); + Assert.Equal(2, handoff.FileOperations.Count); + } + + Assert.False(handoff.IsExecuting); + Assert.Contains(handoff.FileOperations, op => op.Path == "src/one.cs"); + Assert.Contains(handoff.FileOperations, op => op.Path == "src/two.cs"); + } } diff --git a/tests/MandoCode.Tests/PlanProposalSlotTests.cs b/tests/MandoCode.Tests/PlanProposalSlotTests.cs new file mode 100644 index 0000000..260a464 --- /dev/null +++ b/tests/MandoCode.Tests/PlanProposalSlotTests.cs @@ -0,0 +1,84 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Plugins; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// The single-slot proposal store. A proposal belongs to the turn that produced it — anything else +/// means a plan the user walked away from runs later, attached to an unrelated request. +/// +/// Observed live: cancelling a turn after propose_plan left the proposal queued, and the cancelled +/// run also threw OperationCanceledException past the host's catch-all, which reported it a second +/// time as "Unexpected error: A task was canceled." on top of "Request cancelled." +/// +public class PlanProposalSlotTests +{ + private static PlanStepProposal[] Steps(params string[] descriptions) + => [.. descriptions.Select(d => new PlanStepProposal(d, $"do {d}"))]; + + [Fact] + public void SetThenClear_LeavesNothingPending() + { + var handoff = new PlanHandoff(); + handoff.SetPendingProposal("goal", Steps("one")); + Assert.True(handoff.HasPendingProposal); + + handoff.ClearPendingProposal(); + Assert.False(handoff.HasPendingProposal); + } + + [Fact] + public async Task ClearedProposal_DoesNotRunLater() + { + // The host clears at the start of every turn, so a proposal abandoned by a cancelled turn + // cannot execute at the end of the next one. + var ran = false; + var handoff = new PlanHandoff + { + OnPlanRequested = (_, _) => { ran = true; return Task.FromResult("executed"); } + }; + + handoff.SetPendingProposal("abandoned goal", Steps("one", "two")); + handoff.ClearPendingProposal(); + + var manifest = await handoff.RunPendingPlanAsync(); + + Assert.Null(manifest); + Assert.False(ran); + } + + [Fact] + public async Task RunningTakesTheSlot_SoAReplayIsANoOp() + { + var runs = 0; + var handoff = new PlanHandoff + { + OnPlanRequested = (_, _) => { runs++; return Task.FromResult("executed"); } + }; + + handoff.SetPendingProposal("goal", Steps("one")); + + Assert.Equal("executed", await handoff.RunPendingPlanAsync()); + Assert.Null(await handoff.RunPendingPlanAsync()); + Assert.Equal(1, runs); + } + + [Fact] + public async Task NoProposal_ReturnsNullRatherThanThrowing() + { + // Hosts call this unconditionally after every turn. + Assert.Null(await new PlanHandoff().RunPendingPlanAsync()); + } + + [Fact] + public void LastProposalWins() + { + var handoff = new PlanHandoff(); + handoff.SetPendingProposal("first", Steps("a")); + handoff.SetPendingProposal("second", Steps("b", "c")); + + Assert.True(handoff.HasPendingProposal); + } +} diff --git a/tests/MandoCode.Tests/PlanResumeContextTests.cs b/tests/MandoCode.Tests/PlanResumeContextTests.cs new file mode 100644 index 0000000..906ae15 --- /dev/null +++ b/tests/MandoCode.Tests/PlanResumeContextTests.cs @@ -0,0 +1,100 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// What a resumed plan carries forward. +/// +/// A resumed plan runs in a process that never saw the conversation it came from, so anything the +/// steps need has to come out of the saved record. Two things were initially lost, both of which +/// have already caused real failures in this codebase: the verbatim request (steps treat it as +/// authoritative for WHERE work happens — losing it made every file land in the project root), and +/// the results of earlier steps (the context the remaining work usually builds on). +/// +public class PlanResumeContextTests +{ + private static PlanRunState SavedMidPlan() => new() + { + Goal = "in @Games/ build a pacman game", + Cursor = 1, + PreviousResults = ["Step 1 (html): created Games/index.html"], + Steps = + [ + new PlanStepState + { + Number = 1, Description = "html", Instruction = "create index.html", + Status = TaskStepStatus.Completed, Result = "created Games/index.html", + }, + new PlanStepState + { + Number = 2, Description = "css", Instruction = "create style.css", + Status = TaskStepStatus.Pending, + }, + ], + }; + + [Fact] + public async Task ResumeRunsOnlyTheOutstandingSteps() + { + var exec = new ScriptedPlanStepExecutor(); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var _ in runner.ResumeAsync(SavedMidPlan())) { } + + Assert.Equal(["create style.css"], exec.Executed); + } + + [Fact] + public async Task ResumedStepsSeeWhatEarlierStepsProduced() + { + // The gap this test exists for: PreviousResults starts empty on a fresh run, so without + // seeding, step 2 would resume knowing nothing about the file step 1 wrote. + var exec = new ScriptedPlanStepExecutor(); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var _ in runner.ResumeAsync(SavedMidPlan())) { } + + var seen = exec.PreviousResultsSeen.Single(); + Assert.Contains("Games/index.html", seen.Single()); + } + + [Fact] + public async Task ResumeCompletesThePlan() + { + var plan = PlanCheckpointStore.ToPlan(SavedMidPlan()); + Assert.Equal(1, PlanCheckpointStore.OutstandingSteps(SavedMidPlan())); + + var exec = new ScriptedPlanStepExecutor(); + var runner = new WorkflowPlanRunner(exec); + + var events = new List(); + await foreach (var e in runner.ResumeAsync(SavedMidPlan())) events.Add(e); + + Assert.Contains(events, e => e.ProgressType == TaskProgressType.PlanCompleted); + } + + [Fact] + public async Task HostOwnedPlan_IsTheInstanceResumeMutates() + { + var saved = SavedMidPlan(); + var plan = PlanCheckpointStore.ToPlan(saved); + var runner = new WorkflowPlanRunner(new ScriptedPlanStepExecutor()); + + await foreach (var _ in runner.ResumeAsync(plan, saved)) { } + + Assert.Equal(TaskPlanStatus.Completed, plan.Status); + Assert.Equal(TaskStepStatus.Completed, plan.Steps[1].Status); + Assert.NotNull(plan.Steps[1].Result); + } + + [Fact] + public void TheSavedRecordCarriesTheVerbatimRequest() + { + // The CLI feeds this into AIService before resuming, because the process that received the + // original message is gone. Target folders named here override unqualified paths in a step + // instruction, so losing it is how a plan writes everything to the wrong place. + Assert.Contains("@Games/", SavedMidPlan().Goal); + } +} diff --git a/tests/MandoCode.Tests/PlanRevisionTests.cs b/tests/MandoCode.Tests/PlanRevisionTests.cs new file mode 100644 index 0000000..2305043 --- /dev/null +++ b/tests/MandoCode.Tests/PlanRevisionTests.cs @@ -0,0 +1,78 @@ +using MandoCode.Models; +using MandoCode.Plugins; +using MandoCode.Services; +using Xunit; + +namespace MandoCode.Tests; + +public sealed class PlanRevisionTests +{ + [Fact] + public void ApplyApproved_PreservesSettledPrefixAndFailedStepIdentity() + { + var plan = new TaskPlan + { + OriginalRequest = "ship it", + Status = TaskPlanStatus.InProgress, + Steps = + [ + Step(1, "done", TaskStepStatus.Completed, "result"), + Step(2, "failed", TaskStepStatus.Failed), + Step(3, "obsolete", TaskStepStatus.Pending) + ] + }; + var liveFailed = plan.Steps[1]; + var revision = new GeneratedPlan("revised", [ + new PlanStepProposal("repair", "repair carefully"), + new PlanStepProposal("verify", "run focused tests") + ]); + + var candidate = PlanRevision.CreateCandidate(plan, 2, revision); + PlanRevision.ApplyApproved(plan, 2, candidate); + + Assert.Same(liveFailed, plan.Steps[1]); + Assert.Equal(TaskStepStatus.Completed, plan.Steps[0].Status); + Assert.Equal("result", plan.Steps[0].Result); + Assert.Equal("repair", plan.Steps[1].Description); + Assert.Equal(TaskStepStatus.Pending, plan.Steps[1].Status); + Assert.Equal("verify", plan.Steps[2].Description); + Assert.Equal([1, 2, 3], plan.Steps.Select(step => step.StepNumber)); + } + + [Fact] + public void ApplyFollowing_PreservesEditedPrefixAndReplacesOnlyDependentSuffix() + { + var plan = new TaskPlan + { + OriginalRequest = "create then verify", + Steps = + [ + Step(1, "create beta", TaskStepStatus.Pending), + Step(2, "verify alpha", TaskStepStatus.Pending), + Step(3, "report alpha", TaskStepStatus.Pending) + ] + }; + var edited = plan.Steps[0]; + var revision = new GeneratedPlan("updated", [ + new PlanStepProposal("verify beta", "read and verify beta"), + new PlanStepProposal("report beta", "report the beta result") + ]); + + var candidate = PlanRevision.CreateFollowingCandidate(plan, 1, revision); + PlanRevision.ApplyFollowing(plan, 1, candidate); + + Assert.Same(edited, plan.Steps[0]); + Assert.Equal(["create beta", "verify beta", "report beta"], + plan.Steps.Select(step => step.Description)); + Assert.Equal([1, 2, 3], plan.Steps.Select(step => step.StepNumber)); + } + + private static TaskStep Step(int number, string description, TaskStepStatus status, string? result = null) => new() + { + StepNumber = number, + Description = description, + Instruction = description + " instruction", + Status = status, + Result = result + }; +} diff --git a/tests/MandoCode.Tests/PlanRunStateTests.cs b/tests/MandoCode.Tests/PlanRunStateTests.cs new file mode 100644 index 0000000..f924d34 --- /dev/null +++ b/tests/MandoCode.Tests/PlanRunStateTests.cs @@ -0,0 +1,118 @@ +using System.Text.Json; +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// The durable shape of a running plan. +/// +/// This has to survive JSON round-tripping and contain everything a resumed run needs, because it +/// is what MAF captures at each superstep boundary. Anything held only on PlanRunContext is lost — +/// that object carries live delegates and a cancellation token and cannot be serialized. +/// +public class PlanRunStateTests +{ + private static TaskPlan MakePlan() => new() + { + OriginalRequest = "build a pacman game in @Games/", + Steps = + [ + new TaskStep + { + StepNumber = 1, Description = "html", Instruction = "write index.html", + Status = TaskStepStatus.Completed, Result = "wrote index.html", + }, + new TaskStep + { + StepNumber = 2, Description = "css", Instruction = "write style.css", + Status = TaskStepStatus.Failed, ErrorMessage = "disk full", + }, + new TaskStep + { + StepNumber = 3, Description = "js", Instruction = "write game.js", + Status = TaskStepStatus.Pending, + }, + ], + }; + + private static PlanRunState Capture() => PlanRunState.From( + MakePlan(), + cursor: 2, + previousResults: ["Step 1 (html): wrote index.html"], + fileOperations: [("write_file", "index.html")]); + + [Fact] + public void CapturesEverythingNeededToResume() + { + var state = Capture(); + + Assert.Equal("build a pacman game in @Games/", state.Goal); + Assert.Equal(3, state.Steps.Count); + Assert.Equal(2, state.Cursor); + Assert.Single(state.PreviousResults); + Assert.Single(state.FileOperations); + } + + [Fact] + public void KeepsInstructionsVerbatim() + { + // A resumed run re-issues the instruction, not the short display description — losing it + // would leave the remaining steps unrunnable. + var state = Capture(); + Assert.Equal("write game.js", state.Steps[2].Instruction); + } + + [Fact] + public void PreservesPerStepOutcomes() + { + // Resume must be able to tell finished work from work that never ran, or it redoes writes + // that already succeeded. + var state = Capture(); + + Assert.Equal(TaskStepStatus.Completed, state.Steps[0].Status); + Assert.Equal("wrote index.html", state.Steps[0].Result); + Assert.Equal(TaskStepStatus.Failed, state.Steps[1].Status); + Assert.Equal("disk full", state.Steps[1].Error); + Assert.Equal(TaskStepStatus.Pending, state.Steps[2].Status); + } + + [Fact] + public void RecordsFilesAlreadyWritten() + { + // Evidence from the middleware choke point, not the model's account of what it did. + var op = Capture().FileOperations.Single(); + Assert.Equal("write_file", op.Operation); + Assert.Equal("index.html", op.Path); + } + + [Fact] + public void RoundTripsThroughJson() + { + // The whole point: MAF serializes shared state into the checkpoint. + var json = JsonSerializer.Serialize(Capture()); + var back = JsonSerializer.Deserialize(json)!; + + Assert.Equal("build a pacman game in @Games/", back.Goal); + Assert.Equal(2, back.Cursor); + Assert.Equal(3, back.Steps.Count); + Assert.Equal("write game.js", back.Steps[2].Instruction); + Assert.Equal(TaskStepStatus.Completed, back.Steps[0].Status); + Assert.Equal("index.html", back.FileOperations.Single().Path); + } + + [Fact] + public void CarriesNoLiveReferences() + { + // A guard against the failure this type exists to prevent: if someone adds a property + // holding a delegate, service or token, checkpointing silently stops working. + foreach (var prop in typeof(PlanRunState).GetProperties()) + { + var t = prop.PropertyType; + Assert.False(typeof(Delegate).IsAssignableFrom(t), $"{prop.Name} is a delegate"); + Assert.False(t == typeof(CancellationToken), $"{prop.Name} is a CancellationToken"); + Assert.NotEqual(typeof(TaskPlan), t); + } + } +} diff --git a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs new file mode 100644 index 0000000..f9cd7bc --- /dev/null +++ b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs @@ -0,0 +1,171 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Behavior shared by every plan runner, driven through with no +/// live model. +/// +/// Every case runs against BOTH engines. That is the whole point: while `planner` can select +/// either one, any divergence between them makes an A/B against real local models +/// uninterpretable — a behavior difference would be indistinguishable from a model difference. +/// +public class PlanRunnerBehaviorTests +{ + public static TheoryData Engines => new() { "legacy", "workflow" }; + + private static IPlanRunner MakeRunner(string engine, IPlanStepExecutor executor) => engine switch + { + "legacy" => new TaskPlannerService(executor, new MandoCodeConfig()), + "workflow" => new WorkflowPlanRunner(executor), + _ => throw new ArgumentOutOfRangeException(nameof(engine), engine, "unknown planner engine"), + }; + + private static TaskPlan MakePlan(params string[] instructions) => new() + { + OriginalRequest = "build the thing", + Steps = [.. instructions.Select((instr, i) => new TaskStep + { + StepNumber = i + 1, + Description = $"step {i + 1}", + Instruction = instr, + Status = TaskStepStatus.Pending, + })], + }; + + private static async Task> DrainAsync( + IPlanRunner runner, TaskPlan plan, CancellationToken ct = default) + { + var events = new List(); + await foreach (var e in runner.ExecutePlanAsync(plan, ct)) + { + events.Add(e); + } + return events; + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task RunsEveryStep_InOrder(string engine) + { + var exec = new ScriptedPlanStepExecutor(); + var plan = MakePlan("first", "second", "third"); + + await DrainAsync(MakeRunner(engine, exec), plan); + + Assert.Equal(["first", "second", "third"], exec.Executed); + Assert.Equal(TaskPlanStatus.Completed, plan.Status); + Assert.Equal(3, plan.CompletedStepsCount); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task CarriesEarlierResults_ForwardIntoLaterSteps(string engine) + { + var exec = new ScriptedPlanStepExecutor((_, i) => $"result-{i}"); + var plan = MakePlan("a", "b", "c"); + + await DrainAsync(MakeRunner(engine, exec), plan); + + Assert.Empty(exec.PreviousResultsSeen[0]); + Assert.Contains("result-0", exec.PreviousResultsSeen[1].Single()); + Assert.Equal(2, exec.PreviousResultsSeen[2].Count); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task WaitsForQuiescence_AfterEveryStep(string engine) + { + // Without this the next step can start while the previous one is still writing files. + var exec = new ScriptedPlanStepExecutor(); + await DrainAsync(MakeRunner(engine, exec), MakePlan("a", "b")); + + Assert.Equal(2, exec.QuiescenceWaits); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task EmitsPlanCreated_ThenAStepEventPerStep(string engine) + { + var exec = new ScriptedPlanStepExecutor(); + var events = await DrainAsync(MakeRunner(engine, exec), MakePlan("a", "b")); + + Assert.Equal(TaskProgressType.PlanCreated, events[0].ProgressType); + Assert.Equal(2, events.Count(e => e.ProgressType == TaskProgressType.StepCompleted)); + Assert.Contains(events, e => e.ProgressType == TaskProgressType.PlanCompleted); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task CancelledToken_StopsBeforeRunningAnyFurtherStep(string engine) + { + using var cts = new CancellationTokenSource(); + var exec = new ScriptedPlanStepExecutor((instr, _) => + { + if (instr == "second") cts.Cancel(); + return "ok"; + }); + var plan = MakePlan("first", "second", "third"); + + await DrainAsync(MakeRunner(engine, exec), plan, cts.Token); + + Assert.Equal(["first", "second"], exec.Executed); + Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task CancelPlan_MidFlight_StopsTheRun(string engine) + { + var runner = default(IPlanRunner); + var plan = MakePlan("first", "second", "third"); + var exec = new ScriptedPlanStepExecutor((instr, _) => + { + if (instr == "first") runner!.CancelPlan(plan); + return "ok"; + }); + runner = MakeRunner(engine, exec); + + await DrainAsync(runner, plan); + + Assert.Equal(["first"], exec.Executed); + Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task FailedStep_WithNoInteractiveConsumer_IsDowngradedToSkipped(string engine) + { + // Documents a real hazard rather than endorsing it. The runner defers the skip-vs-cancel + // decision to the consumer, which is expected to mutate plan.Status DURING the yield. A + // non-interactive caller does no such thing, so every failure silently becomes "skipped" + // and the plan still reports Completed. The workflow rebuild removes this by making + // progress read-only and routing the decision through a request port — when it does, this + // test should be rewritten, not deleted. + var exec = new ScriptedPlanStepExecutor((instr, _) => + instr == "boom" ? throw new InvalidOperationException("nope") : "ok"); + var plan = MakePlan("fine", "boom", "also fine"); + + await DrainAsync(MakeRunner(engine, exec), plan); + + Assert.Equal(["fine", "boom", "also fine"], exec.Executed); + Assert.Equal(TaskStepStatus.Skipped, plan.Steps[1].Status); + Assert.Equal(TaskPlanStatus.CompletedWithIssues, plan.Status); + Assert.Contains("1 step(s) were skipped", plan.ExecutionSummary); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task SkippedSteps_AreNotReExecuted(string engine) + { + var exec = new ScriptedPlanStepExecutor(); + var plan = MakePlan("a", "b", "c"); + plan.Steps[1].Status = TaskStepStatus.Skipped; + + await DrainAsync(MakeRunner(engine, exec), plan); + + Assert.Equal(["a", "c"], exec.Executed); + } +} diff --git a/tests/MandoCode.Tests/PlanStepReportTests.cs b/tests/MandoCode.Tests/PlanStepReportTests.cs new file mode 100644 index 0000000..fc45e94 --- /dev/null +++ b/tests/MandoCode.Tests/PlanStepReportTests.cs @@ -0,0 +1,46 @@ +using MandoCode.Services; +using Xunit; + +namespace MandoCode.Tests; + +public sealed class PlanStepReportTests +{ + [Fact] + public void SuccessMarker_IsRemovedFromDisplayedResult() + { + var result = PlanStepReport.Parse("Created and verified the file.\n[PLAN_STEP_RESULT:SUCCESS]"); + + Assert.True(result.Succeeded); + Assert.Equal("Created and verified the file.", result.DisplayText); + Assert.Null(result.FailureReason); + } + + [Fact] + public void FailedMarker_ReturnsExplicitReason() + { + var result = PlanStepReport.Parse( + "The requested file does not exist.\n[PLAN_STEP_RESULT:FAILED] Expected file was missing."); + + Assert.False(result.Succeeded); + Assert.Equal("Expected file was missing.", result.FailureReason); + Assert.DoesNotContain("PLAN_STEP_RESULT", result.DisplayText); + } + + [Fact] + public void MissingMarker_RemainsBackwardCompatible() + { + var result = PlanStepReport.Parse("Ordinary response from an older or noncompliant model."); + + Assert.Null(result.Succeeded); + Assert.Equal("Ordinary response from an older or noncompliant model.", result.DisplayText); + } + + [Fact] + public void StepContext_RequiresTruthfulTerminalOutcome() + { + var context = AIService.BuildStepContext("system", "goal", []); + + Assert.Contains("PLAN_STEP_RESULT:SUCCESS", context); + Assert.Contains("Never call a failed verification success", context); + } +} diff --git a/tests/MandoCode.Tests/PlanStepRetryTests.cs b/tests/MandoCode.Tests/PlanStepRetryTests.cs new file mode 100644 index 0000000..77efa4c --- /dev/null +++ b/tests/MandoCode.Tests/PlanStepRetryTests.cs @@ -0,0 +1,121 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Retrying a failed step. +/// +/// Until now a failed step offered only "skip" or "kill the plan", so a transient failure — a +/// momentary tool error, a model that fumbled one call — cost the whole step. Retry is signalled by +/// the consumer setting the step back to Pending while handling StepFailed; triage re-dispatches +/// the same index rather than advancing the cursor past it. +/// +/// Workflow engine only: the legacy runner walks its steps with a foreach and has no way back to +/// one it has already passed. +/// +public class PlanStepRetryTests +{ + private static TaskPlan MakePlan(params string[] instructions) => new() + { + OriginalRequest = "build the thing", + Steps = [.. instructions.Select((instr, i) => new TaskStep + { + StepNumber = i + 1, + Description = $"step {i + 1}", + Instruction = instr, + Status = TaskStepStatus.Pending, + })], + }; + + /// Fails the named instruction for the first attempts. + private static ScriptedPlanStepExecutor FailsThenSucceeds(string failing, int failures) + { + var seen = 0; + return new ScriptedPlanStepExecutor((instr, _) => + { + if (instr != failing) return "ok"; + if (seen++ < failures) throw new InvalidOperationException("transient"); + return "ok on retry"; + }); + } + + [Fact] + public async Task RetryRunsTheSameStepAgain() + { + var exec = FailsThenSucceeds("flaky", failures: 1); + var plan = MakePlan("first", "flaky", "third"); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var e in runner.ExecutePlanAsync(plan)) + { + if (e.ProgressType == TaskProgressType.StepFailed) + { + plan.Steps[e.CurrentStep - 1].Status = TaskStepStatus.Pending; + } + } + + // "flaky" ran twice — once failing, once succeeding — and the plan carried on. + Assert.Equal(["first", "flaky", "flaky", "third"], exec.Executed); + Assert.Equal(TaskStepStatus.Completed, plan.Steps[1].Status); + } + + [Fact] + public async Task RetryIsCappedSoAPermanentFailureCannotLoop() + { + // A step that fails identically every time must not spin forever just because the consumer + // keeps asking for a retry. + var exec = FailsThenSucceeds("doomed", failures: int.MaxValue); + var plan = MakePlan("doomed", "after"); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var e in runner.ExecutePlanAsync(plan)) + { + if (e.ProgressType == TaskProgressType.StepFailed) + { + plan.Steps[e.CurrentStep - 1].Status = TaskStepStatus.Pending; + } + } + + var attempts = exec.Executed.Count(i => i == "doomed"); + Assert.Equal(PlanRunContext.MaxRetriesPerStep + 1, attempts); // first try plus the retries + + // Once out of retries it is treated as skipped, and the plan moves on. + Assert.Equal(TaskStepStatus.Skipped, plan.Steps[0].Status); + Assert.Contains("after", exec.Executed); + } + + [Fact] + public async Task NotRetryingStillSkips() + { + // The existing behaviour has to survive: a consumer that does not ask for a retry gets the + // step skipped, exactly as before. + var exec = FailsThenSucceeds("boom", failures: int.MaxValue); + var plan = MakePlan("boom", "after"); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var _ in runner.ExecutePlanAsync(plan)) { } + + Assert.Equal(["boom", "after"], exec.Executed); + Assert.Equal(TaskStepStatus.Skipped, plan.Steps[0].Status); + } + + [Fact] + public async Task CancellingStillBeatsRetrying() + { + // Cancel must win: a consumer that cancels has made a stronger statement than one asking + // to try again. + var exec = FailsThenSucceeds("boom", failures: int.MaxValue); + var plan = MakePlan("boom", "never runs"); + var runner = new WorkflowPlanRunner(exec); + + await foreach (var e in runner.ExecutePlanAsync(plan)) + { + if (e.ProgressType == TaskProgressType.StepFailed) runner.CancelPlan(plan); + } + + Assert.Equal(["boom"], exec.Executed); + Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); + } +} diff --git a/tests/MandoCode.Tests/PlanWorkflowTopologyTests.cs b/tests/MandoCode.Tests/PlanWorkflowTopologyTests.cs new file mode 100644 index 0000000..18b8141 --- /dev/null +++ b/tests/MandoCode.Tests/PlanWorkflowTopologyTests.cs @@ -0,0 +1,114 @@ +using Xunit; +using MandoCode.Models; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// Guards the plan graph's shape. +/// +/// Resume matches checkpointed state to executors by identity and requires byte-identical topology, +/// so the graph must not depend on how many steps a plan has — otherwise a checkpoint from a 3-step +/// plan could never be restored into a 12-step one, and replanning would invalidate every +/// checkpoint in the field. +/// +/// Asserted against Workflow.ReflectExecutors()/ReflectEdges() rather than ToString(), which only +/// returns the type name and would make every assertion here pass vacuously. +/// +public class PlanWorkflowTopologyTests +{ + private static TaskPlan PlanWith(int stepCount) => new() + { + OriginalRequest = "goal", + Steps = [.. Enumerable.Range(1, stepCount).Select(i => new TaskStep + { + StepNumber = i, + Description = $"step {i}", + Instruction = $"do {i}", + Status = TaskStepStatus.Pending, + })], + }; + + private static Microsoft.Agents.AI.Workflows.Workflow Build(int stepCount) + { + var ctx = new PlanRunContext( + PlanWith(stepCount), + new ScriptedPlanStepExecutor(), + (_, _, _) => Task.CompletedTask, + CancellationToken.None); + + // Build() validates edge typing, connectivity from the start executor and executor binding, + // so a malformed graph fails here rather than at runtime mid-plan. + return WorkflowPlanRunner.BuildWorkflow(ctx); + } + + private static string Shape(int stepCount) + { + var wf = Build(stepCount); + var nodes = wf.ReflectExecutors().Keys.OrderBy(k => k, StringComparer.Ordinal); + var edges = wf.ReflectEdges() + .SelectMany(kv => kv.Value.Select(e => $"{kv.Key}->{e}")) + .OrderBy(s => s, StringComparer.Ordinal); + + return $"start={wf.StartExecutorId}\nnodes={string.Join(",", nodes)}\nedges={string.Join(",", edges)}"; + } + + [Theory] + [InlineData(1)] + [InlineData(3)] + [InlineData(12)] + public void GraphBuilds_ForAnyStepCount(int stepCount) + { + Assert.NotEmpty(Build(stepCount).ReflectExecutors()); + } + + [Fact] + public void Topology_IsIdentical_RegardlessOfStepCount() + { + // The step cursor lives in workflow state and in messages — never in the graph's shape. + var one = Shape(1); + Assert.Equal(one, Shape(3)); + Assert.Equal(one, Shape(12)); + } + + [Fact] + public void ExecutorSet_IsTheGoldenList() + { + // Deliberately hard-coded. This SHOULD fail when the graph gains a node — that failure is + // the reminder to bump PlanExecutorIds.TopologyVersion and decide what happens to any + // checkpoints already written. (Checkpointing is not live yet, so growing the graph before + // then costs nothing.) + string[] expected = + [ + PlanExecutorIds.Finalizer, + PlanExecutorIds.Intake, + PlanExecutorIds.StepRunner, + PlanExecutorIds.Triage, + ]; + + Assert.Equal( + expected.OrderBy(x => x, StringComparer.Ordinal), + Build(2).ReflectExecutors().Keys.OrderBy(x => x, StringComparer.Ordinal)); + } + + [Fact] + public void StartsAtIntake() + { + Assert.Equal(PlanExecutorIds.Intake, Build(2).StartExecutorId); + } + + [Fact] + public void TriageLoopsBackToTheStepRunner() + { + // The loop-back edge is what lets one step-runner node serve a plan of any length. + var edges = Build(2).ReflectEdges(); + Assert.True(edges.ContainsKey(PlanExecutorIds.Triage)); + + // Two ways out of triage: back to the step runner for the next step, or on to the finalizer. + Assert.Equal(2, edges[PlanExecutorIds.Triage].Count); + + var shape = Shape(2); + Assert.Contains(PlanExecutorIds.StepRunner, shape, StringComparison.Ordinal); + Assert.Contains(PlanExecutorIds.Triage, shape, StringComparison.Ordinal); + } +} diff --git a/tests/MandoCode.Tests/PlannerEngineConfigTests.cs b/tests/MandoCode.Tests/PlannerEngineConfigTests.cs new file mode 100644 index 0000000..6cd6dd0 --- /dev/null +++ b/tests/MandoCode.Tests/PlannerEngineConfigTests.cs @@ -0,0 +1,109 @@ +using System.Text.Json; +using Xunit; +using MandoCode.Models; + +namespace MandoCode.Tests; + +/// +/// Tests the `planner` config key. Its nullability is load-bearing, not incidental: when the +/// default eventually flips to the workflow engine, null must still mean "follow the build" while +/// an explicit "legacy" still means "the user chose this". With a non-nullable default those two +/// states are indistinguishable and the flip becomes a guess — the same guess Migrate() already +/// has to make for ModelResponseTimeoutSeconds. +/// +public class PlannerEngineConfigTests +{ + [Fact] + public void DefaultsToNull_MeaningBuildDefault() + { + Assert.Null(new MandoCodeConfig().PlannerEngine); + } + + [Fact] + public void IsNotWritten_WhenUnset() + { + // Older builds reserialize the whole config on save and have no UnmappedMemberHandling, + // so emitting a null key would be noise they'd drop anyway. + var json = JsonSerializer.Serialize(new MandoCodeConfig()); + Assert.DoesNotContain("\"planner\"", json); + } + + [Fact] + public void RoundTrips_WhenSet() + { + var json = JsonSerializer.Serialize( + new MandoCodeConfig { PlannerEngine = MandoCodeConfig.PlannerEngineLegacy }); + + Assert.Contains("\"planner\"", json); + Assert.Equal( + MandoCodeConfig.PlannerEngineLegacy, + JsonSerializer.Deserialize(json)!.PlannerEngine); + } + + [Fact] + public void AbsentKey_DeserializesToNull() + { + var config = JsonSerializer.Deserialize("""{"modelName":"qwen3:8b"}""")!; + Assert.Null(config.PlannerEngine); + } + + [Fact] + public void TrySet_AcceptsLegacy() + { + var config = new MandoCodeConfig(); + var result = ConfigKeySetter.TrySet(config, "planner", "legacy"); + + Assert.True(result.Ok); + Assert.Equal(MandoCodeConfig.PlannerEngineLegacy, config.PlannerEngine); + Assert.Equal(ConfigKeySetter.ApplyScope.KernelRebuild, result.Scope); + } + + [Theory] + [InlineData("default")] + [InlineData("auto")] + [InlineData("clear")] + public void TrySet_ResetsToBuildDefault(string value) + { + var config = new MandoCodeConfig { PlannerEngine = MandoCodeConfig.PlannerEngineLegacy }; + var result = ConfigKeySetter.TrySet(config, "planner", value); + + Assert.True(result.Ok); + Assert.Null(config.PlannerEngine); + } + + [Fact] + public void TrySet_AcceptsWorkflow() + { + var config = new MandoCodeConfig(); + var result = ConfigKeySetter.TrySet(config, "planner", "workflow"); + + Assert.True(result.Ok); + Assert.Equal(MandoCodeConfig.PlannerEngineWorkflow, config.PlannerEngine); + Assert.Equal(ConfigKeySetter.ApplyScope.KernelRebuild, result.Scope); + } + + [Fact] + public void TrySet_RejectsUnknownValues() + { + var config = new MandoCodeConfig(); + Assert.False(ConfigKeySetter.TrySet(config, "planner", "magentic").Ok); + Assert.Null(config.PlannerEngine); + } + + [Fact] + public void PlannerKey_IsIndependentOfEnableTaskPlanning() + { + // Overloading enableTaskPlanning as the engine switch would make "planning off" and + // "old engine" the same state, and render any A/B between engines uninterpretable. + var config = new MandoCodeConfig(); + ConfigKeySetter.TrySet(config, "planner", "legacy"); + + Assert.True(config.EnableTaskPlanning); + Assert.Equal(MandoCodeConfig.PlannerEngineLegacy, config.PlannerEngine); + + ConfigKeySetter.TrySet(config, "taskPlanning", "false"); + + Assert.False(config.EnableTaskPlanning); + Assert.Equal(MandoCodeConfig.PlannerEngineLegacy, config.PlannerEngine); + } +} diff --git a/tests/MandoCode.Tests/ScriptedPlanStepExecutor.cs b/tests/MandoCode.Tests/ScriptedPlanStepExecutor.cs new file mode 100644 index 0000000..5b81a22 --- /dev/null +++ b/tests/MandoCode.Tests/ScriptedPlanStepExecutor.cs @@ -0,0 +1,48 @@ +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// An that returns canned results and records what it was asked to +/// do. This is the seam that lets plan sequencing, cancellation, skip/fail handling and (later) +/// checkpoint/resume be tested deterministically, with no Ollama and no HTTP. +/// +public sealed class ScriptedPlanStepExecutor : IPlanStepExecutor +{ + private readonly Func _respond; + + /// + /// Maps (instruction, zero-based call index) to the step's result. Throw from here to simulate + /// a step failing. + /// + public ScriptedPlanStepExecutor(Func? respond = null) + => _respond = respond ?? ((instruction, i) => $"done:{i}:{instruction}"); + + /// Instructions received, in the order they were executed. + public List Executed { get; } = []; + + /// Snapshot of previousResults as each step saw it — proves context carry-forward. + public List> PreviousResultsSeen { get; } = []; + + public int QuiescenceWaits { get; private set; } + + public Task ExecuteStepAsync( + string stepInstruction, + List previousResults, + CancellationToken cancellationToken = default) + { + cancellationToken.ThrowIfCancellationRequested(); + + var index = Executed.Count; + Executed.Add(stepInstruction); + PreviousResultsSeen.Add([.. previousResults]); + + return Task.FromResult(_respond(stepInstruction, index)); + } + + public Task WaitForQuiescenceAsync(TimeSpan timeout) + { + QuiescenceWaits++; + return Task.CompletedTask; + } +} diff --git a/tests/MandoCode.Tests/StepNarrationTests.cs b/tests/MandoCode.Tests/StepNarrationTests.cs new file mode 100644 index 0000000..6cf6393 --- /dev/null +++ b/tests/MandoCode.Tests/StepNarrationTests.cs @@ -0,0 +1,113 @@ +using Xunit; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// The one-line "what it's doing right now" shown beside a running step. +/// +/// A plan step's text is only rendered once the step finishes — streaming exists for the stall +/// watchdog's heartbeat, not for display — so without this a long step is a spinner and nothing +/// else. Observed live: a step sat at "Working…" for four minutes while the model narrated +/// throughout, which read as a hang. +/// +public class StepNarrationTests +{ + [Fact] + public void ShowsTheLineCurrentlyBeingWritten() + { + var n = new StepNarration(); + n.Append("⚙️ Creating the maze"); + + Assert.Equal("⚙️ Creating the maze", n.Latest); + } + + [Fact] + public void ReassemblesChunksSplitMidWord() + { + // Streamed chunks have no relationship to word or line boundaries. + var n = new StepNarration(); + n.Append("⚙️ Crea"); + n.Append("ting the ma"); + n.Append("ze"); + + Assert.Equal("⚙️ Creating the maze", n.Latest); + } + + [Fact] + public void AdvancesToTheNewestLine() + { + var n = new StepNarration(); + n.Append("first thing\nsecond thing\nthird thing"); + + Assert.Equal("third thing", n.Latest); + } + + [Fact] + public void KeepsTheLastLine_WhileBetweenLines() + { + // A trailing newline must not blank the display until the next line starts arriving. + var n = new StepNarration(); + n.Append("doing the thing\n"); + + Assert.Equal("doing the thing", n.Latest); + } + + [Fact] + public void IgnoresBlankLines() + { + var n = new StepNarration(); + n.Append("real content\n\n\n"); + + Assert.Equal("real content", n.Latest); + } + + [Fact] + public void HandlesWindowsLineEndings() + { + var n = new StepNarration(); + n.Append("one\r\ntwo"); + + Assert.Equal("two", n.Latest); + } + + [Fact] + public void NullUntilSomethingArrives() + { + var n = new StepNarration(); + Assert.Null(n.Latest); + + n.Append(""); + n.Append(null); + n.Append(" \n "); + Assert.Null(n.Latest); + } + + [Fact] + public void ShortensWithAnEllipsis() + { + // A spinner label that wraps corrupts the line the spinner keeps redrawing, so the width + // budget is a hard limit. + var n = new StepNarration(); + n.Append(new string('x', 200)); + + var shortened = n.Shortened(20); + Assert.Equal(20, shortened!.Length); + Assert.EndsWith("…", shortened); + } + + [Fact] + public void LeavesShortLinesAlone() + { + var n = new StepNarration(); + n.Append("short"); + + Assert.Equal("short", n.Shortened(60)); + } + + [Fact] + public void ShortenedIsNull_WhenThereIsNothingToShow() + { + Assert.Null(new StepNarration().Shortened(60)); + } +} diff --git a/tests/MandoCode.Tests/StreamBufferingTests.cs b/tests/MandoCode.Tests/StreamBufferingTests.cs index 75b2e5f..c815f41 100644 --- a/tests/MandoCode.Tests/StreamBufferingTests.cs +++ b/tests/MandoCode.Tests/StreamBufferingTests.cs @@ -81,6 +81,6 @@ await Assert.ThrowsAnyAsync(async () => await StreamBuffering.BufferAsync( CancelAwareStream(), onChunk: () => cts.Cancel(), // cancel after the first chunk - cts.Token)); + cancellationToken: cts.Token)); } } diff --git a/tests/MandoCode.Tests/SystemPromptsTests.cs b/tests/MandoCode.Tests/SystemPromptsTests.cs index b27adfa..13c78a7 100644 --- a/tests/MandoCode.Tests/SystemPromptsTests.cs +++ b/tests/MandoCode.Tests/SystemPromptsTests.cs @@ -50,4 +50,28 @@ public void BothVariants_KeepTheCoreAssistantIdentity() Assert.Contains("LARGE FILES", prompt); } } + + [Fact] + public void ProgressLines_AreNotNumbered() + { + // The model cannot know how many pieces of work there will be, so it invents a total. + // Observed live: a 3-step plan rendered the harness's real "Step 2/3:" header directly above + // the model's own "(Step 2/5)" line, which reads as a broken progress display. + var prompt = SystemPrompts.BuildMandoCodeAssistant(webSearchEnabled: false); + + Assert.DoesNotContain("(Step 1/5)", prompt); + Assert.DoesNotContain("Always number your steps", prompt); + Assert.Contains("Do NOT number these lines", prompt); + } + + [Fact] + public void PlanningSection_DoesNotPromiseACompletionSummary() + { + // propose_plan returns a receipt as soon as the plan is queued; it no longer blocks until + // the plan has run, so telling the model to expect the outcome would be a lie. + var prompt = SystemPrompts.BuildMandoCodeAssistant(webSearchEnabled: false); + + Assert.Contains("returns as soon as the plan is queued", prompt); + Assert.DoesNotContain("You will receive a summary string when planning completes", prompt); + } } diff --git a/tests/MandoCode.Tests/VersionLabelTests.cs b/tests/MandoCode.Tests/VersionLabelTests.cs new file mode 100644 index 0000000..ea4119c --- /dev/null +++ b/tests/MandoCode.Tests/VersionLabelTests.cs @@ -0,0 +1,65 @@ +using Xunit; +using MandoCode.Services; + +namespace MandoCode.Tests; + +/// +/// The banner's version label. Getting this wrong is how a stale or mislabelled binary goes +/// unnoticed — the numeric assembly version alone cannot distinguish a prerelease test build from +/// the release it was cut from. +/// +public class VersionLabelTests +{ + [Fact] + public void KeepsPrereleaseTag_AndDropsBuildMetadata() + { + Assert.Equal( + "v0.16.0-rc.1", + VersionLabel.Build("0.16.0-rc.1+a1a0df8d15c1a5da", new Version(0, 16, 0, 0))); + } + + [Fact] + public void PlainVersion_RendersUnchanged() + { + Assert.Equal("v0.15.0", VersionLabel.Build("0.15.0+deadbeef", new Version(0, 15, 0, 0))); + } + + [Fact] + public void NoBuildMetadata_IsFine() + { + Assert.Equal("v1.2.3-rc.1", VersionLabel.Build("1.2.3-rc.1", new Version(1, 2, 3, 0))); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void FallsBackToAssemblyVersion_WhenInformationalVersionIsMissing(string? info) + { + // Three-part, matching what the banner showed before informational versions were read. + Assert.Equal("v2.4.6", VersionLabel.Build(info, new Version(2, 4, 6, 99))); + } + + [Fact] + public void FallsBackToAssemblyVersion_WhenInformationalVersionIsOnlyBuildMetadata() + { + Assert.Equal("v2.4.6", VersionLabel.Build("+abc123", new Version(2, 4, 6, 0))); + } + + [Fact] + public void EmptyWhenNothingIsAvailable() + { + Assert.Equal("", VersionLabel.Build(null, null)); + } + + [Fact] + public void RunningAssembly_ReportsAVersion() + { + // VersionLabel itself lives in the MandoCode assembly — a marker declared here would resolve + // to the test assembly and assert nothing. + var label = VersionLabel.ForAssembly(typeof(VersionLabel).Assembly); + + Assert.StartsWith("v", label); + Assert.DoesNotContain("+", label); // build metadata is stripped + } +}