From 4aaa57fb98df975ad866e47bfbd4b04f1697a653 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 11:32:20 -0700 Subject: [PATCH 01/21] Planner: freeze identity/checkpoint/config decisions, extract IPlanRunner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for rebuilding the planner on Microsoft.Agents.AI.Workflows. No behavior change: this lands only the decisions that cannot be repaired later, while the diff is small enough to review in isolation. Pin the generalist agent's Id and Name. MAF derives workflow-executor identity from both, and BuildAgent re-runs 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 and unrepairably. This has to be true before the first checkpoint is ever written. Add PlanExecutorIds (literal consts + a topology version) and PlanCheckpointEnvelope (schema/topology/project/model fields, each a refusal criterion, wrapping the framework blob opaquely). Adding the envelope after checkpoints exist in the wild would leave the early ones unversioned and indistinguishable. Add the `planner` config key as string?, deliberately separate from enableTaskPlanning — which decides whether there is a planner at all, not which engine runs it. Nullable so that when the default eventually flips, null still means "follow the build" and an explicit "legacy" still means "the user chose this"; a non-nullable default makes those indistinguishable and turns the flip into the same guess Migrate() already has to make for ModelResponseTimeoutSeconds. "workflow" is rejected until the graph exists, so nobody selects a no-op. Extract IPlanRunner and IPlanStepExecutor. Without the step-executor seam every later test of ordering, cancellation, retry or resume would need a live Ollama. TaskPlannerService now takes IPlanStepExecutor; the (AIService, config) constructor is kept as a delegating overload so the Desktop app, which builds this by hand, compiles unmodified. Microsoft.Agents.AI 1.18.0 -> 1.19.0 alongside the new Workflows reference: Workflows requires core at the same version, and the mismatch is NU1605, which TreatWarningsAsErrors turns into a build error. No NoWarn needed — none of the types used carry [Experimental] in 1.19.0, verified by reflection against the real assembly rather than the docs. Tests: 506 -> 539, green on net10.0 and net8.0. Desktop builds unmodified. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/MandoCode.csproj | 10 +- src/MandoCode/Models/ConfigKeySetter.cs | 26 +++ src/MandoCode/Models/MandoCodeConfig.cs | 25 +++ src/MandoCode/Services/Ai/AIService.cs | 9 +- .../Services/Ai/Planning/IPlanRunner.cs | 36 ++++ .../Services/Ai/Planning/IPlanStepExecutor.cs | 61 +++++++ .../Ai/Planning/PlanCheckpointEnvelope.cs | 110 +++++++++++++ .../Services/Ai/Planning/PlanExecutorIds.cs | 92 +++++++++++ .../Services/Ai/TaskPlannerService.cs | 25 ++- src/MandoCode/docs/TaskPlanner.md | 51 +++++- .../PlanCheckpointEnvelopeTests.cs | 103 ++++++++++++ tests/MandoCode.Tests/PlanExecutorIdsTests.cs | 86 ++++++++++ .../PlanRunnerBehaviorTests.cs | 154 ++++++++++++++++++ .../PlannerEngineConfigTests.cs | 110 +++++++++++++ .../ScriptedPlanStepExecutor.cs | 48 ++++++ 15 files changed, 936 insertions(+), 10 deletions(-) create mode 100644 src/MandoCode/Services/Ai/Planning/IPlanRunner.cs create mode 100644 src/MandoCode/Services/Ai/Planning/IPlanStepExecutor.cs create mode 100644 src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs create mode 100644 src/MandoCode/Services/Ai/Planning/PlanExecutorIds.cs create mode 100644 tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs create mode 100644 tests/MandoCode.Tests/PlanExecutorIdsTests.cs create mode 100644 tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs create mode 100644 tests/MandoCode.Tests/PlannerEngineConfigTests.cs create mode 100644 tests/MandoCode.Tests/ScriptedPlanStepExecutor.cs diff --git a/src/MandoCode/MandoCode.csproj b/src/MandoCode/MandoCode.csproj index b92a213..c2b8bc3 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 90cc328..0c18366 100644 --- a/src/MandoCode/Models/ConfigKeySetter.cs +++ b/src/MandoCode/Models/ConfigKeySetter.cs @@ -161,11 +161,10 @@ public static SetResult TrySet(MandoCodeConfig config, string key, string value) } if (planner == MandoCodeConfig.PlannerEngineWorkflow) { - // The workflow engine lands in a later phase; accept the name only when the - // graph actually exists, so nobody selects a no-op and thinks it took. - return Fail("Error: The 'workflow' planner is not available in this build yet"); + config.PlannerEngine = planner; + return new(true, "✓ Planner engine set to: workflow (experimental)", ApplyScope.KernelRebuild); } - return Fail("Error: Value must be 'legacy' or 'default'"); + return Fail("Error: Value must be 'legacy', 'workflow', or 'default'"); case "streaming": case "responsestreaming": @@ -274,7 +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 | default) + 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/Program.cs b/src/MandoCode/Program.cs index 1dad660..144badf 100644 --- a/src/MandoCode/Program.cs +++ b/src/MandoCode/Program.cs @@ -164,6 +164,17 @@ 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())); + // Register MusicPlayerService as singleton services.AddSingleton(provider => { diff --git a/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs new file mode 100644 index 0000000..d6ac20b --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs @@ -0,0 +1,33 @@ +using MandoCode.Models; + +namespace MandoCode.Services; + +/// +/// Resolves which plan engine to use, re-reading the planner config key on every access. +/// +/// +/// 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) +{ + private WorkflowPlanRunner? _workflowRunner; + + /// True when the workflow engine is currently selected. + public bool UsingWorkflowEngine => string.Equals( + config.PlannerEngine, MandoCodeConfig.PlannerEngineWorkflow, StringComparison.OrdinalIgnoreCase); + + /// The engine to run the next plan with. + public IPlanRunner Current => UsingWorkflowEngine + ? _workflowRunner ??= new WorkflowPlanRunner(stepExecutor) + : legacyRunner; +} diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs new file mode 100644 index 0000000..7302248 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -0,0 +1,256 @@ +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) +{ + public TaskPlan Plan { get; } = plan; + public IPlanStepExecutor StepExecutor { get; } = stepExecutor; + public CancellationToken CancellationToken { get; } = cancellationToken; + + /// Step results accumulated so far, in the format each step's context expects. + public List PreviousResults { get; } = []; + + /// + /// Publishes a progress event. When is true this does not + /// return until the consumer has processed the event and asked for the next one — the point at + /// which any mutation it made to plan.Status is visible. + /// + public Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer = false) + => 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); + + 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; + 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), waitForConsumer: true); + 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."), waitForConsumer: true); + 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."), waitForConsumer: true); + + if (plan.Status == TaskPlanStatus.Cancelled) + { + await Finish(context, cancellationToken); + return; + } + + // 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 Finish(context, cancellationToken); + return; + } + + var next = ctx.NextRunnableIndex(message.StepIndex + 1); + await context.QueueStateUpdateAsync( + PlanWorkflowMessages.CursorKey, Math.Max(next, plan.Steps.Count), PlanWorkflowMessages.StateScope, 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; + } + + // Classification copied deliberately from the legacy runner, including its quirk that a + // plan whose steps were all skipped after failures still reports Completed. Changing it + // here would make the two engines disagree while both are selectable; it belongs in the + // phase that retires the legacy runner. + var allCompleted = plan.Steps.All(s => + s.Status == TaskStepStatus.Completed || s.Status == TaskStepStatus.Skipped); + var anyFailed = plan.Steps.Any(s => s.Status == TaskStepStatus.Failed); + + if (allCompleted && !anyFailed) + { + plan.Status = TaskPlanStatus.Completed; + plan.ExecutionSummary = $"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..64bfae5 --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs @@ -0,0 +1,48 @@ +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"; +} + +/// 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..633a76c --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -0,0 +1,144 @@ +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) : IPlanRunner +{ + private readonly IPlanStepExecutor _stepExecutor = stepExecutor + ?? throw new ArgumentNullException(nameof(stepExecutor)); + + /// + /// 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 async IAsyncEnumerable ExecutePlanAsync( + TaskPlan plan, + [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); + 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/docs/TaskPlanner.md b/src/MandoCode/docs/TaskPlanner.md index 28b3673..ec463c2 100644 --- a/src/MandoCode/docs/TaskPlanner.md +++ b/src/MandoCode/docs/TaskPlanner.md @@ -329,8 +329,9 @@ too: it routes via `handoff_to_*` tool calls that cannot be forced, the worst po |---|---|---| | 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 | next | -| 3 | The graph: 8 fixed executors, two `RequestPort`s, read-only progress | | +| 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 | Move approval and step decisions onto `RequestPort`s; make progress read-only | | | 4 | Checkpointing + resume | | | 5 | Retry / replan / forced-tool-use escalation | | | 6 | Flip the `planner` default, then delete the legacy runner | | @@ -347,6 +348,26 @@ Spike findings worth keeping: `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 diff --git a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs index 74ab38a..a28049b 100644 --- a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs +++ b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs @@ -5,13 +5,24 @@ namespace MandoCode.Tests; /// -/// Behavior of the current plan runner, driven through with no live -/// model. These assertions are the baseline the workflow engine must reproduce, so they are written -/// against rather than the concrete service — the same suite should run -/// unchanged against the new engine. +/// 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", @@ -24,9 +35,6 @@ public class PlanRunnerBehaviorTests })], }; - private static IPlanRunner MakeRunner(IPlanStepExecutor executor) - => new TaskPlannerService(executor, new MandoCodeConfig()); - private static async Task> DrainAsync( IPlanRunner runner, TaskPlan plan, CancellationToken ct = default) { @@ -38,55 +46,60 @@ private static async Task> DrainAsync( return events; } - [Fact] - public async Task RunsEveryStep_InOrder() + [Theory] + [MemberData(nameof(Engines))] + public async Task RunsEveryStep_InOrder(string engine) { var exec = new ScriptedPlanStepExecutor(); var plan = MakePlan("first", "second", "third"); - await DrainAsync(MakeRunner(exec), plan); + await DrainAsync(MakeRunner(engine, exec), plan); Assert.Equal(["first", "second", "third"], exec.Executed); Assert.Equal(TaskPlanStatus.Completed, plan.Status); Assert.Equal(3, plan.CompletedStepsCount); } - [Fact] - public async Task CarriesEarlierResults_ForwardIntoLaterSteps() + [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(exec), plan); + 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); } - [Fact] - public async Task WaitsForQuiescence_AfterEveryStep() + [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(exec), MakePlan("a", "b")); + await DrainAsync(MakeRunner(engine, exec), MakePlan("a", "b")); Assert.Equal(2, exec.QuiescenceWaits); } - [Fact] - public async Task EmitsPlanCreated_ThenAStepEventPerStep() + [Theory] + [MemberData(nameof(Engines))] + public async Task EmitsPlanCreated_ThenAStepEventPerStep(string engine) { var exec = new ScriptedPlanStepExecutor(); - var events = await DrainAsync(MakeRunner(exec), MakePlan("a", "b")); + 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); } - [Fact] - public async Task CancelledToken_StopsBeforeRunningAnyFurtherStep() + [Theory] + [MemberData(nameof(Engines))] + public async Task CancelledToken_StopsBeforeRunningAnyFurtherStep(string engine) { using var cts = new CancellationTokenSource(); var exec = new ScriptedPlanStepExecutor((instr, _) => @@ -96,14 +109,15 @@ public async Task CancelledToken_StopsBeforeRunningAnyFurtherStep() }); var plan = MakePlan("first", "second", "third"); - await DrainAsync(MakeRunner(exec), plan, cts.Token); + await DrainAsync(MakeRunner(engine, exec), plan, cts.Token); Assert.Equal(["first", "second"], exec.Executed); Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); } - [Fact] - public async Task CancelPlan_MidFlight_StopsTheRun() + [Theory] + [MemberData(nameof(Engines))] + public async Task CancelPlan_MidFlight_StopsTheRun(string engine) { var runner = default(IPlanRunner); var plan = MakePlan("first", "second", "third"); @@ -112,7 +126,7 @@ public async Task CancelPlan_MidFlight_StopsTheRun() if (instr == "first") runner!.CancelPlan(plan); return "ok"; }); - runner = MakeRunner(exec); + runner = MakeRunner(engine, exec); await DrainAsync(runner, plan); @@ -120,8 +134,9 @@ public async Task CancelPlan_MidFlight_StopsTheRun() Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); } - [Fact] - public async Task FailedStep_WithNoInteractiveConsumer_IsDowngradedToSkipped() + [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 @@ -133,21 +148,22 @@ public async Task FailedStep_WithNoInteractiveConsumer_IsDowngradedToSkipped() instr == "boom" ? throw new InvalidOperationException("nope") : "ok"); var plan = MakePlan("fine", "boom", "also fine"); - await DrainAsync(MakeRunner(exec), plan); + 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.Completed, plan.Status); } - [Fact] - public async Task SkippedSteps_AreNotReExecuted() + [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(exec), plan); + await DrainAsync(MakeRunner(engine, exec), plan); Assert.Equal(["a", "c"], exec.Executed); } 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 index 65007f0..6cd6dd0 100644 --- a/tests/MandoCode.Tests/PlannerEngineConfigTests.cs +++ b/tests/MandoCode.Tests/PlannerEngineConfigTests.cs @@ -72,15 +72,14 @@ public void TrySet_ResetsToBuildDefault(string value) } [Fact] - public void TrySet_RejectsWorkflow_UntilTheGraphExists() + public void TrySet_AcceptsWorkflow() { - // Accepting a name that does nothing would let someone select it and believe it took. var config = new MandoCodeConfig(); var result = ConfigKeySetter.TrySet(config, "planner", "workflow"); - Assert.False(result.Ok); - Assert.Contains("not available", result.Message); - Assert.Null(config.PlannerEngine); + Assert.True(result.Ok); + Assert.Equal(MandoCodeConfig.PlannerEngineWorkflow, config.PlannerEngine); + Assert.Equal(ConfigKeySetter.ApplyScope.KernelRebuild, result.Scope); } [Fact] From d8a0d62cc5f811c96ca09e0a704eddd5f9358709 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 17:50:18 -0700 Subject: [PATCH 04/21] Show the full version, including prerelease tag, in the banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stamps this branch as 0.15.0-plan-test and makes the startup banner display it, so it is obvious at a glance which binary is running while both planner engines are selectable. The banner read Assembly.GetName().Version, which is numeric-only and silently drops any prerelease tag — a test build looked identical to the release it was cut from. It now reads the informational version, dropping the "+commit" metadata SourceLink appends but keeping the tag. Parsing moved into VersionLabel so it is testable: a private helper inside a Razor component could not be, and the only other way to see the banner is to launch the interactive TUI. UpdateCheckService is unaffected — its ParseStable deliberately rejects prerelease tags and falls back to the numeric assembly version, so update checks still compare 0.15.0. Drop the -plan-test suffix, and the test pinning it, before release. Tests: 556 -> 565. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/Banner.razor | 8 ++- src/MandoCode/MandoCode.csproj | 6 +- src/MandoCode/Services/VersionLabel.cs | 42 ++++++++++++++ tests/MandoCode.Tests/VersionLabelTests.cs | 64 ++++++++++++++++++++++ 4 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 src/MandoCode/Services/VersionLabel.cs create mode 100644 tests/MandoCode.Tests/VersionLabelTests.cs 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/MandoCode.csproj b/src/MandoCode/MandoCode.csproj index f2e1f73..37b98da 100644 --- a/src/MandoCode/MandoCode.csproj +++ b/src/MandoCode/MandoCode.csproj @@ -13,7 +13,11 @@ MandoCode - 0.15.0 + + 0.15.0-plan-test Armando Fernandez (DevMando) Your AI coding assistant — run locally or in the cloud with Ollama. No API keys required. Just you and your code. https://github.com/DevMando/MandoCode diff --git a/src/MandoCode/Services/VersionLabel.cs b/src/MandoCode/Services/VersionLabel.cs new file mode 100644 index 0000000..7e63cea --- /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. +/// +internal static class VersionLabel +{ + /// Version label for the running assembly, e.g. "v0.15.0-plan-test". + 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.15.0-plan-test+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/tests/MandoCode.Tests/VersionLabelTests.cs b/tests/MandoCode.Tests/VersionLabelTests.cs new file mode 100644 index 0000000..952daaf --- /dev/null +++ b/tests/MandoCode.Tests/VersionLabelTests.cs @@ -0,0 +1,64 @@ +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.15.0-plan-test", + VersionLabel.Build("0.15.0-plan-test+a1a0df8d15c1a5da", new Version(0, 15, 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_CarriesThePlanTestTag() + { + // Guards the actual csproj stamp, so the marker can't silently go missing from a test build. + // Delete this alongside the -plan-test suffix when the branch is released. + // VersionLabel itself lives in the MandoCode assembly — a marker declared here would resolve + // to the test assembly and assert nothing. + Assert.Equal("v0.15.0-plan-test", VersionLabel.ForAssembly(typeof(VersionLabel).Assembly)); + } +} From 0be4545d19ce68c8cb2b65e3c97a1fdc610b0e7b Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 19:28:11 -0700 Subject: [PATCH 05/21] Make VersionLabel public so the Desktop app can share it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Desktop's window title had the same defect the CLI banner did — it read a numeric-only version, so a prerelease tag never showed and a tagged test build looked identical to the release it was cut from. Sharing the formatter rather than duplicating the parsing keeps both products labelling builds the same way. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Services/VersionLabel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/MandoCode/Services/VersionLabel.cs b/src/MandoCode/Services/VersionLabel.cs index 7e63cea..55edb6f 100644 --- a/src/MandoCode/Services/VersionLabel.cs +++ b/src/MandoCode/Services/VersionLabel.cs @@ -10,7 +10,7 @@ namespace MandoCode.Services; /// 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. /// -internal static class VersionLabel +public static class VersionLabel { /// Version label for the running assembly, e.g. "v0.15.0-plan-test". public static string ForAssembly(Assembly assembly) => Build( From b5f80782588025d56fb9f5f98738a4b6d8eef3e7 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 19:42:10 -0700 Subject: [PATCH 06/21] Planner: drop a pending proposal when its turn is cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by a live run. Cancelling a turn after propose_plan left the proposal in the single slot, so the plan the user had just walked away from would execute at the end of the next, unrelated turn. The proposal is now cleared at the start of every turn — it belongs to the turn that produced it — and again if the turn ends cancelled. Tests: 565 -> 570. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 13 +++ .../MandoCode.Tests/PlanProposalSlotTests.cs | 84 +++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 tests/MandoCode.Tests/PlanProposalSlotTests.cs diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index fdd4bd1..9f8f363 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -1644,6 +1644,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(); @@ -1991,6 +1996,14 @@ 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); 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); + } +} From 431f1c18e29787f30f753a889916b2d8bf7a9970 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 19:45:18 -0700 Subject: [PATCH 07/21] Stop the model inventing its own step numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Two step counters disagreeing on screen reads as a broken progress display. The model's count is the wrong one — it cannot know how many pieces of work there will be, so it guesses. Removing the numbering rather than relabelling the harness's header drops a falsehood instead of disambiguating two truths, and keeps the narration itself, which is useful during a long step. Also adds a regression test for the planning section, which was corrected earlier to stop promising a completion summary that propose_plan no longer returns. Tests: 570 -> 572. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Models/SystemPrompts.cs | 16 ++++++++------ tests/MandoCode.Tests/SystemPromptsTests.cs | 24 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/MandoCode/Models/SystemPrompts.cs b/src/MandoCode/Models/SystemPrompts.cs index c7f3aeb..7d9cb07 100644 --- a/src/MandoCode/Models/SystemPrompts.cs +++ b/src/MandoCode/Models/SystemPrompts.cs @@ -105,16 +105,18 @@ 1. Call the appropriate function 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 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); + } } From 72a8636f1c1a78abdbd1be808347af4a42c7986f Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 20:24:07 -0700 Subject: [PATCH 08/21] Planner: stop the spinner when a plan finishes, and order progress events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rendering bugs from deferring plan execution, both surfaced by running against a very fast model where the timing is obvious. The spinner kept animating after a plan completed. ExecuteAgentModelCallAsync starts a spinner per model call and never stops it — it has always relied on the caller. That used to be the enumerator drain's finally, which wrapped the plan back when plans ran inside the propose_plan tool call. Plans now run after that finally, so nothing owned the last step's spinner. The plan run gets its own finally. Progress events now wait for the consumer by default. In the legacy runner every event was a `yield return`, which inherently blocked until the UI had handled it; the workflow runner was firing them without waiting, so a step's model call started its own spinner while the step header was still being drawn and the frame bled into it — "▫ Three-stepping... · 0sStep 3/3: ..." observed live. Making the wait the default removes the whole class of ordering divergence between the two engines rather than patching the one case, and it is also what the failed-step contract already depended on. Tests: 572 green on net10.0 and net8.0. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 15 ++++++++++- .../Ai/Planning/PlanWorkflowExecutors.cs | 25 +++++++++++++------ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 9f8f363..5a55be7 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -1793,12 +1793,25 @@ 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. - await RunPendingPlanAsync(); + try + { + await RunPendingPlanAsync(); + } + finally + { + Spinner.Stop(); + } } catch (OperationCanceledException) { diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 7302248..76bfb42 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -32,11 +32,18 @@ internal sealed class PlanRunContext( public List PreviousResults { get; } = []; /// - /// Publishes a progress event. When is true this does not - /// return until the consumer has processed the event and asked for the next one — the point at - /// which any mutation it made to plan.Status is visible. + /// Publishes a progress event and, by default, waits until the consumer has processed it and + /// asked for the next one. /// - public Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer = false) + /// + /// 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); /// @@ -105,6 +112,10 @@ public override async ValueTask HandleAsync( } 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; @@ -158,14 +169,14 @@ public override async ValueTask HandleAsync( 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), waitForConsumer: true); + 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."), waitForConsumer: true); + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); await Finish(context, cancellationToken); return; @@ -175,7 +186,7 @@ public override async ValueTask HandleAsync( // 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."), waitForConsumer: true); + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Step failed.")); if (plan.Status == TaskPlanStatus.Cancelled) { From e1deff1859892ed99eec29e027330f7c7d0e6e71 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 20:35:25 -0700 Subject: [PATCH 09/21] Planner: give each step the plan's file manifest and its own boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two causes of the same live run: 1.1M tokens for a four-step game, where step 1 built the entire game and steps 3 and 4 each re-read a 750-line file five times. Steps only carried the last two prose summaries forward. Those describe work rather than naming files, so a step had two bad options: guess, or re-read whole files to find out. Both were observed — step 3 of an earlier run wrote getElementById('gameCanvas') against step 1's id="game-canvas", so the game never started; this run re-read instead, and paid for it in tokens. Each step now receives the list of files the plan has already created or modified, sourced from the evidence AgentFunctionMiddleware already records at the choke point rather than from the model's self-reports. Roughly one short line per file — the highest signal-per-token context available — deduplicated and capped at 40 so a sprawling plan cannot crowd out the step's own instruction. Step instructions now state their boundary. Without it a capable model treats the first step as the whole task: a step scoped to "create the game HTML shell" wrote the HTML, the CSS and all 612 lines of the engine, after which the remaining three steps each found the work done and added one small thing apiece. Extracted as BuildStepUserMessage so the wording is testable, matching BuildStepContext. This matters most on the local models the product targets, where the re-reading would exhaust the context window rather than just the budget. Tests: 572 -> 579. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Services/Ai/AIService.cs | 73 ++++++++++++++- src/MandoCode/Services/Ai/PlanHandoff.cs | 15 ++++ tests/MandoCode.Tests/PlanStepContextTests.cs | 89 +++++++++++++++++++ 3 files changed, 173 insertions(+), 4 deletions(-) diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index c761867..03b230f 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -1178,7 +1178,11 @@ private string FormatErrorMessage(Exception ex) /// project root). Previous-step results are limited to the last 2 and the original /// request is capped so step context stays small on local models. /// - public static string BuildStepContext(string systemPrompt, string? originalUserRequest, List previousResults) + public static string BuildStepContext( + string systemPrompt, + string? originalUserRequest, + List previousResults, + IReadOnlyList<(string Operation, string Path)>? fileOperations = null) { // Generous enough for a long message plus @folder listings; small enough that a // pasted @file of several thousand lines can't flood every step's context. Paths @@ -1217,20 +1221,81 @@ public static string BuildStepContext(string systemPrompt, string? originalUserR sb.AppendLine("--- End of Previous Steps ---\n"); } + AppendFileManifest(sb, fileOperations); + return sb.ToString(); } + /// + /// Lists the files earlier steps have already created or modified. + /// + /// + /// The highest signal-per-token context a step can get — roughly one short line per file. Only + /// the last two steps' prose summaries carry forward otherwise, and those describe work rather + /// than naming files, which leaves a step two bad options: invent names (observed live — step 3 + /// wrote getElementById('gameCanvas') against step 1's id="game-canvas", so the + /// game never started) or re-read whole files to find out (observed live — a 750-line file read + /// five times in one step, contributing to a 1.1M-token run). + /// + private static void AppendFileManifest( + System.Text.StringBuilder sb, + IReadOnlyList<(string Operation, string Path)>? fileOperations) + { + if (fileOperations is not { Count: > 0 }) return; + + // Distinct paths, first-touch order. A file edited ten times is still one line. + var paths = new List(); + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var (_, path) in fileOperations) + { + if (!string.IsNullOrWhiteSpace(path) && seen.Add(path)) paths.Add(path); + } + + if (paths.Count == 0) return; + + // Bounded: a sprawling plan must not crowd out the step's own instruction. + const int MaxListed = 40; + var listed = paths.Take(MaxListed).ToList(); + + sb.AppendLine("--- Files This Plan Has Already Created Or Modified ---"); + foreach (var path in listed) sb.AppendLine(path); + if (paths.Count > listed.Count) + sb.AppendLine($"...and {paths.Count - listed.Count} more."); + + sb.AppendLine( + "These exist on disk right now. Read one only if you actually need its current " + + "contents, and do not read the same file twice in this step — you already have it. " + + "Never guess at element ids, class names, or function names in these files: if your " + + "work has to line up with them, read the file once and match what is really there."); + sb.AppendLine("--- End of Files ---\n"); + } + + /// + /// The user-role message that drives one plan step. + /// + /// + /// States the step's boundary explicitly. Without it a capable model treats the first step as + /// the whole task — observed live, a step scoped to "create the game HTML shell" wrote the + /// HTML, the CSS and all 612 lines of the game engine, after which the remaining three steps + /// each found their work already done and contributed one small addition apiece. + /// + public static string BuildStepUserMessage(string stepInstruction) => + $"Execute this step now: {stepInstruction}\n\n" + + "Do ONLY this step. The plan's later steps run after you and will cover the rest — doing " + + "their work now makes them redundant and wastes the user's time and tokens. Stop as soon " + + "as this step's own work is done.\n\n" + + "Remember: Use the available functions to complete this task. Do not describe the function call - actually invoke it."; + public async Task ExecutePlanStepAsync(string stepInstruction, List previousResults, CancellationToken cancellationToken = default) { var contextBuilder = new System.Text.StringBuilder( - BuildStepContext(_systemPrompt, _currentTurnUserMessage, previousResults)); + BuildStepContext(_systemPrompt, _currentTurnUserMessage, previousResults, _planHandoff?.FileOperations)); // Create a temporary chat history for this step List stepHistory = [ new ChatMessage(ChatRole.System, contextBuilder.ToString()), - new ChatMessage(ChatRole.User, - $"Execute this step now: {stepInstruction}\n\nRemember: Use the available functions to complete this task. Do not describe the function call - actually invoke it.") + new ChatMessage(ChatRole.User, BuildStepUserMessage(stepInstruction)) ]; var stepLabel = $"Step {previousResults.Count + 1}"; diff --git a/src/MandoCode/Services/Ai/PlanHandoff.cs b/src/MandoCode/Services/Ai/PlanHandoff.cs index 3a88cc6..00bd286 100644 --- a/src/MandoCode/Services/Ai/PlanHandoff.cs +++ b/src/MandoCode/Services/Ai/PlanHandoff.cs @@ -73,6 +73,21 @@ 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. Fed into each step's context so a later step knows which files + /// earlier steps produced. Without it a step only sees the previous steps' prose summaries, and + /// either invents names that don't match what was written or re-reads whole files to find out; + /// one observed run re-read a 750-line file five times in a single step. + /// + 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 diff --git a/tests/MandoCode.Tests/PlanStepContextTests.cs b/tests/MandoCode.Tests/PlanStepContextTests.cs index 1c2d977..c3ae3f5 100644 --- a/tests/MandoCode.Tests/PlanStepContextTests.cs +++ b/tests/MandoCode.Tests/PlanStepContextTests.cs @@ -64,4 +64,93 @@ public void IncludesOnlyLastTwoPreviousStepResults() Assert.Contains("result two", context); Assert.Contains("result three", context); } + + // ---- File manifest ---- + // + // Steps only carry the last two prose summaries forward, which describe work rather than + // naming files. That left a step guessing: observed live, step 3 wrote + // getElementById('gameCanvas') against step 1's id="game-canvas" and the game never started; + // another run re-read a 750-line file five times in one step, contributing to 1.1M tokens. + + private static readonly (string Operation, string Path)[] Ops = + [ + ("write_file", "index.html"), + ("write_file", "style.css"), + ("edit_file", "index.html"), + ("write_file", "game.js"), + ]; + + [Fact] + public void ListsFilesEarlierStepsTouched() + { + var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); + + Assert.Contains("index.html", context); + Assert.Contains("style.css", context); + Assert.Contains("game.js", context); + } + + [Fact] + public void ListsEachFileOnce_EvenWhenTouchedRepeatedly() + { + var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); + + var section = context[context.IndexOf("--- Files This Plan", StringComparison.Ordinal)..]; + var occurrences = section.Split("index.html").Length - 1; + Assert.Equal(1, occurrences); + } + + [Fact] + public void TellsTheModelNotToGuessNamesOrReReadFiles() + { + // The two failure modes this section exists to prevent. + var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); + + Assert.Contains("do not read the same file twice", context, StringComparison.OrdinalIgnoreCase); + Assert.Contains("Never guess at element ids", context, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void OmitsTheSectionEntirely_WhenNothingHasBeenWritten() + { + // The first step of a plan has no manifest, and an empty header would be noise. + var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], []); + Assert.DoesNotContain("--- Files This Plan", context); + + Assert.DoesNotContain("--- Files This Plan", + AIService.BuildStepContext(SystemPrompt, "build a game", [])); + } + + [Fact] + public void CapsTheListSoOneStepCannotFloodTheContext() + { + var many = Enumerable.Range(1, 60).Select(i => ("write_file", $"file{i}.cs")).ToArray(); + var context = AIService.BuildStepContext(SystemPrompt, "big refactor", [], many); + + Assert.Contains("file1.cs", context); + Assert.DoesNotContain("file60.cs", context); + Assert.Contains("and 20 more", context); + } + + // ---- Step boundary ---- + + [Fact] + public void StepMessage_TellsTheModelToDoOnlyThisStep() + { + // Observed live: a step scoped to "create the game HTML shell" wrote the HTML, the CSS and + // all 612 lines of the engine, leaving the plan's other three steps with nothing to do. + var message = AIService.BuildStepUserMessage("Create the game HTML shell"); + + Assert.Contains("Create the game HTML shell", message); + Assert.Contains("ONLY this step", message); + Assert.Contains("later steps", message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void StepMessage_StillInsistsOnRealToolCalls() + { + // Long-standing local-model failure: describing a call instead of making one. + var message = AIService.BuildStepUserMessage("do the thing"); + Assert.Contains("actually invoke it", message); + } } From 5e7bc1515e35eca2c6140511d012a0c970390cda Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 20:37:43 -0700 Subject: [PATCH 10/21] Drop the -plan-test prerelease tag Both apps go back to a plain 0.15.0. The banner change that surfaced the tag stays: reading the informational version rather than the numeric one is a real fix, since GetName().Version silently drops any prerelease suffix and makes a tagged build indistinguishable from the release it was cut from. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/MandoCode.csproj | 6 +----- src/MandoCode/Services/VersionLabel.cs | 4 ++-- tests/MandoCode.Tests/VersionLabelTests.cs | 13 +++++++------ 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/MandoCode/MandoCode.csproj b/src/MandoCode/MandoCode.csproj index 37b98da..f2e1f73 100644 --- a/src/MandoCode/MandoCode.csproj +++ b/src/MandoCode/MandoCode.csproj @@ -13,11 +13,7 @@ MandoCode - - 0.15.0-plan-test + 0.15.0 Armando Fernandez (DevMando) Your AI coding assistant — run locally or in the cloud with Ollama. No API keys required. Just you and your code. https://github.com/DevMando/MandoCode diff --git a/src/MandoCode/Services/VersionLabel.cs b/src/MandoCode/Services/VersionLabel.cs index 55edb6f..a9dcec9 100644 --- a/src/MandoCode/Services/VersionLabel.cs +++ b/src/MandoCode/Services/VersionLabel.cs @@ -12,7 +12,7 @@ namespace MandoCode.Services; /// public static class VersionLabel { - /// Version label for the running assembly, e.g. "v0.15.0-plan-test". + /// 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); @@ -21,7 +21,7 @@ public static string ForAssembly(Assembly assembly) => Build( /// Pure formatting, split out so it can be tested without constructing an assembly. /// /// - /// e.g. 0.15.0-plan-test+a1a0df8. Build metadata after '+' (appended by SourceLink) is + /// 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. diff --git a/tests/MandoCode.Tests/VersionLabelTests.cs b/tests/MandoCode.Tests/VersionLabelTests.cs index 952daaf..ea4119c 100644 --- a/tests/MandoCode.Tests/VersionLabelTests.cs +++ b/tests/MandoCode.Tests/VersionLabelTests.cs @@ -14,8 +14,8 @@ public class VersionLabelTests public void KeepsPrereleaseTag_AndDropsBuildMetadata() { Assert.Equal( - "v0.15.0-plan-test", - VersionLabel.Build("0.15.0-plan-test+a1a0df8d15c1a5da", new Version(0, 15, 0, 0))); + "v0.16.0-rc.1", + VersionLabel.Build("0.16.0-rc.1+a1a0df8d15c1a5da", new Version(0, 16, 0, 0))); } [Fact] @@ -53,12 +53,13 @@ public void EmptyWhenNothingIsAvailable() } [Fact] - public void RunningAssembly_CarriesThePlanTestTag() + public void RunningAssembly_ReportsAVersion() { - // Guards the actual csproj stamp, so the marker can't silently go missing from a test build. - // Delete this alongside the -plan-test suffix when the branch is released. // VersionLabel itself lives in the MandoCode assembly — a marker declared here would resolve // to the test assembly and assert nothing. - Assert.Equal("v0.15.0-plan-test", VersionLabel.ForAssembly(typeof(VersionLabel).Assembly)); + var label = VersionLabel.ForAssembly(typeof(VersionLabel).Assembly); + + Assert.StartsWith("v", label); + Assert.DoesNotContain("+", label); // build metadata is stripped } } From 5fcdc319376b14b393964516d14c4fe368aa260b Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 20:58:39 -0700 Subject: [PATCH 11/21] Planner: stop the UI from holding up the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making every progress event wait for the consumer was too blunt and was visibly slower on a fast model: the workflow blocked on markdown rendering before it could start the next step. Only a failed step needs the wait. There the consumer decides skip-vs-cancel by mutating plan.Status, and reading that before the decision lands is the bug the legacy runner documents — "Cancel the plan" silently downgraded to "skip". That one raise is now explicit as RaiseAndAwaitDecisionAsync; everything else is fire-and-forget. Dropping the wait is safe because the channel is FIFO with a single reader: events cannot arrive out of order, the display simply trails the work. On a fast model the next step may already be running while the previous step's output is still being drawn. The spinner bleeding that originally motivated the blanket wait had a different cause — two spinners on one console. AIService starts one per model call and the host starts its own from the step-progress events. During a plan the host owns it, so the step's model call no longer starts a competing one. Tests: 579 -> 583, including cancel-on-failed-step and event ordering, both run against each engine. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Services/Ai/AIService.cs | 13 ++++- .../Ai/Planning/PlanWorkflowExecutors.cs | 52 ++++++++++++------- .../PlanRunnerBehaviorTests.cs | 44 ++++++++++++++++ 3 files changed, 89 insertions(+), 20 deletions(-) diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index 03b230f..324ddf0 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -873,7 +873,13 @@ private async Task ExecuteAgentModelCallAsync( using var watchdog = AttachAgentStallWatchdog(responseCts); - _spinner.Start(spinnerMessage); + // An empty message means the caller owns the spinner. Plan steps do: the host already + // starts and stops one from the step-progress events, and a second spinner writing to the + // console at the same time is what produced frames bleeding into step headers. + if (!string.IsNullOrEmpty(spinnerMessage)) + { + _spinner.Start(spinnerMessage); + } // Partial-trace accumulator: AgentFunctionMiddleware fires these events per tool call, // synchronously, regardless of whether the OUTER RunAsync call eventually succeeds or @@ -1317,7 +1323,10 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List /// /// -/// 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. +/// Holds the live because the current consumer contract requires it: on a +/// failed step the consumer decides skip-vs-cancel by mutating plan.Status, which is only +/// visible once it comes back for the next event. is what +/// preserves that; every other event is fire-and-forget so rendering never holds up the model. /// /// /// Phase 4 note: this object holds live delegates, so a workflow built around it is NOT @@ -32,19 +33,34 @@ internal sealed class PlanRunContext( public List PreviousResults { get; } = []; /// - /// Publishes a progress event and, by default, waits until the consumer has processed it and - /// asked for the next one. + /// Publishes a progress event without waiting for the consumer to render it. /// /// - /// 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. + /// Fire-and-forget is safe because the channel behind this is FIFO with a single reader: events + /// can never arrive out of order, the display simply trails the work. That matters — the legacy + /// runner blocked on every event because each was a yield return, and reproducing that + /// made the workflow wait on markdown rendering before it could start the next step, which was + /// visibly slower on a fast model. + /// + /// Use for the one event that genuinely needs the + /// consumer to answer. + /// /// - public Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer = true) - => raise(evt, waitForConsumer, CancellationToken); + public Task RaiseAsync(TaskProgressEvent evt) + => raise(evt, false, CancellationToken); + + /// + /// Publishes a progress event and waits until the consumer has handled it and come back for the + /// next one — the point at which any change it made to the plan is visible. + /// + /// + /// Only a failed step needs this. The consumer decides skip-vs-cancel by mutating + /// , and reading that before it has decided is precisely the bug + /// the legacy runner documents: deciding before the yield silently downgraded "Cancel the plan" + /// to "skip". + /// + public Task RaiseAndAwaitDecisionAsync(TaskProgressEvent evt) + => raise(evt, true, CancellationToken); /// /// Index of the next step that still needs running at or after , or -1. @@ -113,9 +129,9 @@ public override async ValueTask HandleAsync( 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. + // Does not wait for the UI: the model should not be held up by rendering. Ordering is + // still guaranteed by the FIFO channel, and the spinner no longer contends because the + // consumer owns it outright during a plan (see ExecutePlanStepAsync). await ctx.RaiseAsync(TaskProgressEvent.StepStarted(ctx.Plan, step)); PlanStepOutcome outcome; @@ -176,7 +192,7 @@ public override async ValueTask HandleAsync( step.Status = TaskStepStatus.Failed; step.ErrorMessage = message.Error; plan.Status = TaskPlanStatus.Cancelled; - await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); + await ctx.RaiseAndAwaitDecisionAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); await Finish(context, cancellationToken); return; @@ -186,7 +202,7 @@ public override async ValueTask HandleAsync( // 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.")); + await ctx.RaiseAndAwaitDecisionAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Step failed.")); if (plan.Status == TaskPlanStatus.Cancelled) { diff --git a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs index a28049b..fd68a65 100644 --- a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs +++ b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs @@ -167,4 +167,48 @@ public async Task SkippedSteps_AreNotReExecuted(string engine) Assert.Equal(["a", "c"], exec.Executed); } + + [Theory] + [MemberData(nameof(Engines))] + public async Task ConsumerCancellingOnAFailedStep_StopsThePlan(string engine) + { + // The one place a runner must wait for the consumer. The consumer decides skip-vs-cancel by + // mutating plan.Status while handling StepFailed; reading it before the decision lands is + // the bug the legacy runner documents — "Cancel the plan" silently downgraded to "skip". + // Every other progress event is fire-and-forget so rendering never holds up the model, so + // this asserts the exception is still honoured. + var exec = new ScriptedPlanStepExecutor((instr, _) => + instr == "boom" ? throw new InvalidOperationException("nope") : "ok"); + var plan = MakePlan("fine", "boom", "never runs"); + var runner = MakeRunner(engine, exec); + + await foreach (var e in runner.ExecutePlanAsync(plan)) + { + if (e.ProgressType == TaskProgressType.StepFailed) runner.CancelPlan(plan); + } + + Assert.Equal(["fine", "boom"], exec.Executed); + Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); + } + + [Theory] + [MemberData(nameof(Engines))] + public async Task ProgressEvents_ArriveInOrder(string engine) + { + // Fire-and-forget is only safe because the transport is FIFO with a single reader: the + // display may trail the work, but it can never show it out of sequence. + var exec = new ScriptedPlanStepExecutor(); + var events = await DrainAsync(MakeRunner(engine, exec), MakePlan("a", "b", "c")); + + var types = events.Select(e => e.ProgressType).ToList(); + Assert.Equal(TaskProgressType.PlanCreated, types.First()); + Assert.Equal(TaskProgressType.PlanCompleted, types.Last()); + + // Each step's completion follows every earlier step's completion. + var completedSteps = events + .Where(e => e.ProgressType == TaskProgressType.StepCompleted) + .Select(e => e.CurrentStep) + .ToList(); + Assert.Equal(completedSteps.OrderBy(n => n), completedSteps); + } } From a445fbe6d1c9e4bf2588cebc397745e825cfdbb3 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Thu, 27 Aug 2026 21:10:54 -0700 Subject: [PATCH 12/21] Restore the per-step spinner message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Working on Step 2 — press Esc to cancel" is back. Making the host the sole spinner owner had silently downgraded it to a generic "Working...", losing both which step was running and how to stop it. Ownership is simply inverted instead: the step's own model call keeps its named spinner, and the host no longer starts one on StepStarted. Still exactly one spinner per console, which is what stopped frames bleeding into step headers. Tests: 583 green. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 5 ++++- src/MandoCode/Services/Ai/AIService.cs | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 5a55be7..ad4f1bf 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -2179,7 +2179,10 @@ SpinnerService.SetTaskbarProgress((progressEvent.CurrentStep - 1) * 100 / progressEvent.TotalSteps); AnsiConsole.MarkupLine($"[deepskyblue1]Step {progressEvent.CurrentStep}/{progressEvent.TotalSteps}:[/] {Spectre.Console.Markup.Escape(progressEvent.StepDescription)}"); AnsiConsole.WriteLine(); - Spinner.Start("Working..."); + // No spinner started here on purpose. The step's own model call starts one that + // names the step ("Working on Step 2 — press Esc to cancel"); starting a second one + // here means two spinners writing to the same console, which is what bled spinner + // frames into step headers. break; case TaskProgressType.StepCompleted: diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index 324ddf0..d673954 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -1323,10 +1323,10 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List Date: Thu, 27 Aug 2026 22:29:24 -0700 Subject: [PATCH 13/21] Revert to the planner behavior from 72a8636 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rolls back three commits at the user's request, after live testing showed the combination had degraded the experience: a445fbe restore the per-step spinner message 5fcdc31 stop the UI from holding up the model e1deff1 file manifest and step boundary Back to: every progress event waits for the consumer, the step's model call owns a named spinner, and steps receive neither the file manifest nor a "do only this step" instruction. The step-boundary text is the likely reason the model's running narration went quiet, and the narration is worth more than what the boundary bought. Known cost of going back, both previously measured live: plans feel slower on a fast model, because the workflow waits on markdown rendering before starting the next step; and a step can re-read a large file repeatedly, which is what took one four-step run to 1.1M tokens. Both remain open — the durable fixes belong in phase 5 (triage reacting to a step that produced no file operations, and read-dedup that knows a step just wrote the file it is reading) rather than in prompt text. The -plan-test version tag stays removed; only planner behavior is reverted. Tests: 583 -> 572, the count from that commit. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 5 +- src/MandoCode/Services/Ai/AIService.cs | 84 ++--------------- src/MandoCode/Services/Ai/PlanHandoff.cs | 15 ---- .../Ai/Planning/PlanWorkflowExecutors.cs | 52 ++++------- .../PlanRunnerBehaviorTests.cs | 44 --------- tests/MandoCode.Tests/PlanStepContextTests.cs | 89 ------------------- 6 files changed, 24 insertions(+), 265 deletions(-) diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index ad4f1bf..5a55be7 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -2179,10 +2179,7 @@ SpinnerService.SetTaskbarProgress((progressEvent.CurrentStep - 1) * 100 / progressEvent.TotalSteps); AnsiConsole.MarkupLine($"[deepskyblue1]Step {progressEvent.CurrentStep}/{progressEvent.TotalSteps}:[/] {Spectre.Console.Markup.Escape(progressEvent.StepDescription)}"); AnsiConsole.WriteLine(); - // No spinner started here on purpose. The step's own model call starts one that - // names the step ("Working on Step 2 — press Esc to cancel"); starting a second one - // here means two spinners writing to the same console, which is what bled spinner - // frames into step headers. + Spinner.Start("Working..."); break; case TaskProgressType.StepCompleted: diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index d673954..c761867 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -873,13 +873,7 @@ private async Task ExecuteAgentModelCallAsync( using var watchdog = AttachAgentStallWatchdog(responseCts); - // An empty message means the caller owns the spinner. Plan steps do: the host already - // starts and stops one from the step-progress events, and a second spinner writing to the - // console at the same time is what produced frames bleeding into step headers. - if (!string.IsNullOrEmpty(spinnerMessage)) - { - _spinner.Start(spinnerMessage); - } + _spinner.Start(spinnerMessage); // Partial-trace accumulator: AgentFunctionMiddleware fires these events per tool call, // synchronously, regardless of whether the OUTER RunAsync call eventually succeeds or @@ -1184,11 +1178,7 @@ private string FormatErrorMessage(Exception ex) /// project root). Previous-step results are limited to the last 2 and the original /// request is capped so step context stays small on local models. /// - public static string BuildStepContext( - string systemPrompt, - string? originalUserRequest, - List previousResults, - IReadOnlyList<(string Operation, string Path)>? fileOperations = null) + public static string BuildStepContext(string systemPrompt, string? originalUserRequest, List previousResults) { // Generous enough for a long message plus @folder listings; small enough that a // pasted @file of several thousand lines can't flood every step's context. Paths @@ -1227,81 +1217,20 @@ public static string BuildStepContext( sb.AppendLine("--- End of Previous Steps ---\n"); } - AppendFileManifest(sb, fileOperations); - return sb.ToString(); } - /// - /// Lists the files earlier steps have already created or modified. - /// - /// - /// The highest signal-per-token context a step can get — roughly one short line per file. Only - /// the last two steps' prose summaries carry forward otherwise, and those describe work rather - /// than naming files, which leaves a step two bad options: invent names (observed live — step 3 - /// wrote getElementById('gameCanvas') against step 1's id="game-canvas", so the - /// game never started) or re-read whole files to find out (observed live — a 750-line file read - /// five times in one step, contributing to a 1.1M-token run). - /// - private static void AppendFileManifest( - System.Text.StringBuilder sb, - IReadOnlyList<(string Operation, string Path)>? fileOperations) - { - if (fileOperations is not { Count: > 0 }) return; - - // Distinct paths, first-touch order. A file edited ten times is still one line. - var paths = new List(); - var seen = new HashSet(StringComparer.OrdinalIgnoreCase); - foreach (var (_, path) in fileOperations) - { - if (!string.IsNullOrWhiteSpace(path) && seen.Add(path)) paths.Add(path); - } - - if (paths.Count == 0) return; - - // Bounded: a sprawling plan must not crowd out the step's own instruction. - const int MaxListed = 40; - var listed = paths.Take(MaxListed).ToList(); - - sb.AppendLine("--- Files This Plan Has Already Created Or Modified ---"); - foreach (var path in listed) sb.AppendLine(path); - if (paths.Count > listed.Count) - sb.AppendLine($"...and {paths.Count - listed.Count} more."); - - sb.AppendLine( - "These exist on disk right now. Read one only if you actually need its current " + - "contents, and do not read the same file twice in this step — you already have it. " + - "Never guess at element ids, class names, or function names in these files: if your " + - "work has to line up with them, read the file once and match what is really there."); - sb.AppendLine("--- End of Files ---\n"); - } - - /// - /// The user-role message that drives one plan step. - /// - /// - /// States the step's boundary explicitly. Without it a capable model treats the first step as - /// the whole task — observed live, a step scoped to "create the game HTML shell" wrote the - /// HTML, the CSS and all 612 lines of the game engine, after which the remaining three steps - /// each found their work already done and contributed one small addition apiece. - /// - public static string BuildStepUserMessage(string stepInstruction) => - $"Execute this step now: {stepInstruction}\n\n" + - "Do ONLY this step. The plan's later steps run after you and will cover the rest — doing " + - "their work now makes them redundant and wastes the user's time and tokens. Stop as soon " + - "as this step's own work is done.\n\n" + - "Remember: Use the available functions to complete this task. Do not describe the function call - actually invoke it."; - public async Task ExecutePlanStepAsync(string stepInstruction, List previousResults, CancellationToken cancellationToken = default) { var contextBuilder = new System.Text.StringBuilder( - BuildStepContext(_systemPrompt, _currentTurnUserMessage, previousResults, _planHandoff?.FileOperations)); + BuildStepContext(_systemPrompt, _currentTurnUserMessage, previousResults)); // Create a temporary chat history for this step List stepHistory = [ new ChatMessage(ChatRole.System, contextBuilder.ToString()), - new ChatMessage(ChatRole.User, BuildStepUserMessage(stepInstruction)) + new ChatMessage(ChatRole.User, + $"Execute this step now: {stepInstruction}\n\nRemember: Use the available functions to complete this task. Do not describe the function call - actually invoke it.") ]; var stepLabel = $"Step {previousResults.Count + 1}"; @@ -1323,9 +1252,6 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List 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. Fed into each step's context so a later step knows which files - /// earlier steps produced. Without it a step only sees the previous steps' prose summaries, and - /// either invents names that don't match what was written or re-reads whole files to find out; - /// one observed run re-read a 750-line file five times in a single step. - /// - 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 diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 0478006..76bfb42 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -8,10 +8,9 @@ namespace MandoCode.Services; /// /// /// -/// Holds the live because the current consumer contract requires it: on a -/// failed step the consumer decides skip-vs-cancel by mutating plan.Status, which is only -/// visible once it comes back for the next event. is what -/// preserves that; every other event is fire-and-forget so rendering never holds up the model. +/// 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 @@ -33,34 +32,19 @@ internal sealed class PlanRunContext( public List PreviousResults { get; } = []; /// - /// Publishes a progress event without waiting for the consumer to render it. + /// Publishes a progress event and, by default, waits until the consumer has processed it and + /// asked for the next one. /// /// - /// Fire-and-forget is safe because the channel behind this is FIFO with a single reader: events - /// can never arrive out of order, the display simply trails the work. That matters — the legacy - /// runner blocked on every event because each was a yield return, and reproducing that - /// made the workflow wait on markdown rendering before it could start the next step, which was - /// visibly slower on a fast model. - /// - /// Use for the one event that genuinely needs the - /// consumer to answer. - /// + /// 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) - => raise(evt, false, CancellationToken); - - /// - /// Publishes a progress event and waits until the consumer has handled it and come back for the - /// next one — the point at which any change it made to the plan is visible. - /// - /// - /// Only a failed step needs this. The consumer decides skip-vs-cancel by mutating - /// , and reading that before it has decided is precisely the bug - /// the legacy runner documents: deciding before the yield silently downgraded "Cancel the plan" - /// to "skip". - /// - public Task RaiseAndAwaitDecisionAsync(TaskProgressEvent evt) - => raise(evt, true, CancellationToken); + 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. @@ -129,9 +113,9 @@ public override async ValueTask HandleAsync( step.Status = TaskStepStatus.InProgress; - // Does not wait for the UI: the model should not be held up by rendering. Ordering is - // still guaranteed by the FIFO channel, and the spinner no longer contends because the - // consumer owns it outright during a plan (see ExecutePlanStepAsync). + // 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; @@ -192,7 +176,7 @@ public override async ValueTask HandleAsync( step.Status = TaskStepStatus.Failed; step.ErrorMessage = message.Error; plan.Status = TaskPlanStatus.Cancelled; - await ctx.RaiseAndAwaitDecisionAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Cancelled.")); await Finish(context, cancellationToken); return; @@ -202,7 +186,7 @@ public override async ValueTask HandleAsync( // 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.RaiseAndAwaitDecisionAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Step failed.")); + await ctx.RaiseAsync(TaskProgressEvent.StepFailed(plan, step, message.Error ?? "Step failed.")); if (plan.Status == TaskPlanStatus.Cancelled) { diff --git a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs index fd68a65..a28049b 100644 --- a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs +++ b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs @@ -167,48 +167,4 @@ public async Task SkippedSteps_AreNotReExecuted(string engine) Assert.Equal(["a", "c"], exec.Executed); } - - [Theory] - [MemberData(nameof(Engines))] - public async Task ConsumerCancellingOnAFailedStep_StopsThePlan(string engine) - { - // The one place a runner must wait for the consumer. The consumer decides skip-vs-cancel by - // mutating plan.Status while handling StepFailed; reading it before the decision lands is - // the bug the legacy runner documents — "Cancel the plan" silently downgraded to "skip". - // Every other progress event is fire-and-forget so rendering never holds up the model, so - // this asserts the exception is still honoured. - var exec = new ScriptedPlanStepExecutor((instr, _) => - instr == "boom" ? throw new InvalidOperationException("nope") : "ok"); - var plan = MakePlan("fine", "boom", "never runs"); - var runner = MakeRunner(engine, exec); - - await foreach (var e in runner.ExecutePlanAsync(plan)) - { - if (e.ProgressType == TaskProgressType.StepFailed) runner.CancelPlan(plan); - } - - Assert.Equal(["fine", "boom"], exec.Executed); - Assert.Equal(TaskPlanStatus.Cancelled, plan.Status); - } - - [Theory] - [MemberData(nameof(Engines))] - public async Task ProgressEvents_ArriveInOrder(string engine) - { - // Fire-and-forget is only safe because the transport is FIFO with a single reader: the - // display may trail the work, but it can never show it out of sequence. - var exec = new ScriptedPlanStepExecutor(); - var events = await DrainAsync(MakeRunner(engine, exec), MakePlan("a", "b", "c")); - - var types = events.Select(e => e.ProgressType).ToList(); - Assert.Equal(TaskProgressType.PlanCreated, types.First()); - Assert.Equal(TaskProgressType.PlanCompleted, types.Last()); - - // Each step's completion follows every earlier step's completion. - var completedSteps = events - .Where(e => e.ProgressType == TaskProgressType.StepCompleted) - .Select(e => e.CurrentStep) - .ToList(); - Assert.Equal(completedSteps.OrderBy(n => n), completedSteps); - } } diff --git a/tests/MandoCode.Tests/PlanStepContextTests.cs b/tests/MandoCode.Tests/PlanStepContextTests.cs index c3ae3f5..1c2d977 100644 --- a/tests/MandoCode.Tests/PlanStepContextTests.cs +++ b/tests/MandoCode.Tests/PlanStepContextTests.cs @@ -64,93 +64,4 @@ public void IncludesOnlyLastTwoPreviousStepResults() Assert.Contains("result two", context); Assert.Contains("result three", context); } - - // ---- File manifest ---- - // - // Steps only carry the last two prose summaries forward, which describe work rather than - // naming files. That left a step guessing: observed live, step 3 wrote - // getElementById('gameCanvas') against step 1's id="game-canvas" and the game never started; - // another run re-read a 750-line file five times in one step, contributing to 1.1M tokens. - - private static readonly (string Operation, string Path)[] Ops = - [ - ("write_file", "index.html"), - ("write_file", "style.css"), - ("edit_file", "index.html"), - ("write_file", "game.js"), - ]; - - [Fact] - public void ListsFilesEarlierStepsTouched() - { - var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); - - Assert.Contains("index.html", context); - Assert.Contains("style.css", context); - Assert.Contains("game.js", context); - } - - [Fact] - public void ListsEachFileOnce_EvenWhenTouchedRepeatedly() - { - var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); - - var section = context[context.IndexOf("--- Files This Plan", StringComparison.Ordinal)..]; - var occurrences = section.Split("index.html").Length - 1; - Assert.Equal(1, occurrences); - } - - [Fact] - public void TellsTheModelNotToGuessNamesOrReReadFiles() - { - // The two failure modes this section exists to prevent. - var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], Ops); - - Assert.Contains("do not read the same file twice", context, StringComparison.OrdinalIgnoreCase); - Assert.Contains("Never guess at element ids", context, StringComparison.OrdinalIgnoreCase); - } - - [Fact] - public void OmitsTheSectionEntirely_WhenNothingHasBeenWritten() - { - // The first step of a plan has no manifest, and an empty header would be noise. - var context = AIService.BuildStepContext(SystemPrompt, "build a game", [], []); - Assert.DoesNotContain("--- Files This Plan", context); - - Assert.DoesNotContain("--- Files This Plan", - AIService.BuildStepContext(SystemPrompt, "build a game", [])); - } - - [Fact] - public void CapsTheListSoOneStepCannotFloodTheContext() - { - var many = Enumerable.Range(1, 60).Select(i => ("write_file", $"file{i}.cs")).ToArray(); - var context = AIService.BuildStepContext(SystemPrompt, "big refactor", [], many); - - Assert.Contains("file1.cs", context); - Assert.DoesNotContain("file60.cs", context); - Assert.Contains("and 20 more", context); - } - - // ---- Step boundary ---- - - [Fact] - public void StepMessage_TellsTheModelToDoOnlyThisStep() - { - // Observed live: a step scoped to "create the game HTML shell" wrote the HTML, the CSS and - // all 612 lines of the engine, leaving the plan's other three steps with nothing to do. - var message = AIService.BuildStepUserMessage("Create the game HTML shell"); - - Assert.Contains("Create the game HTML shell", message); - Assert.Contains("ONLY this step", message); - Assert.Contains("later steps", message, StringComparison.OrdinalIgnoreCase); - } - - [Fact] - public void StepMessage_StillInsistsOnRealToolCalls() - { - // Long-standing local-model failure: describing a call instead of making one. - var message = AIService.BuildStepUserMessage("do the thing"); - Assert.Contains("actually invoke it", message); - } } From fe8191fac7864e4536ed531502fb26408b4f7f68 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 11:02:42 -0700 Subject: [PATCH 14/21] Show what a step is doing while it runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plan step's text only renders once the step finishes — streaming exists for the stall watchdog's heartbeat, not for display — so a long step was a spinner and nothing else. Observed live: four minutes at "Working…" while the model narrated the whole time, which read as a hang. The spinner label now carries the model's newest line as it arrives: Working on Step 2 — press Esc to cancel Step 2 — ⚙️ Creating world generation system... Only the latest line, not the whole stream. The full response still renders as markdown when the step completes, so echoing it live would duplicate it, and a half-arrived markdown document cannot be rendered sensibly anyway. StreamBuffering gains an optional onText callback beside the existing heartbeat; StepNarration reassembles chunks that split mid-word or mid-line and shortens the result, since a spinner label that wraps corrupts the line the spinner redraws. Exceptions from the callback are swallowed — a progress display must never break the generation it is reporting on. Tests: 572 -> 582. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Services/Ai/AIService.cs | 37 +++++- src/MandoCode/Services/Ai/StepNarration.cs | 78 ++++++++++++ src/MandoCode/Services/Ai/StreamBuffering.cs | 19 ++- tests/MandoCode.Tests/StepNarrationTests.cs | 113 ++++++++++++++++++ tests/MandoCode.Tests/StreamBufferingTests.cs | 2 +- 5 files changed, 241 insertions(+), 8 deletions(-) create mode 100644 src/MandoCode/Services/Ai/StepNarration.cs create mode 100644 tests/MandoCode.Tests/StepNarrationTests.cs diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index c761867..2654766 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -865,7 +865,8 @@ private async Task ExecuteAgentModelCallAsync( string retryOperationName, string tokenLabel, string spinnerMessage, - 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)); @@ -913,7 +914,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 @@ -981,7 +982,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 { @@ -997,7 +999,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); } /// @@ -1220,6 +1223,13 @@ public static string BuildStepContext(string systemPrompt, string? originalUserR return sb.ToString(); } + /// + /// 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( @@ -1248,12 +1258,27 @@ 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); + _spinner.UpdateActivity( + line == null ? baseSpinnerMessage : $"{stepLabel} — {line}"); + }); var response = string.IsNullOrEmpty(result.Text) ? "Step completed (no response content)." : result.Text; 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/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)); } } From 3433a717460d191f3df1cadccff4a6f16e9489d0 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 13:39:35 -0700 Subject: [PATCH 15/21] Planner: put the run's durable state where a checkpoint can see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for resume. MAF captures a workflow's shared state at each superstep boundary, but everything describing the run lived on PlanRunContext — an object carrying live delegates and a cancellation token, which can never be serialized. A checkpoint taken today would preserve nothing useful. PlanRunState is a plain-data snapshot: goal, per-step instruction and outcome, cursor, accumulated results, and the files the plan has already touched. Intake writes it, triage rewrites it as the plan advances, and both terminal paths write it before finishing so a cancelled run doesn't checkpoint stale state. Step instructions and per-step outcomes are both kept deliberately: a resumed run re-issues the instruction, not the short display description, and it has to be able to tell finished work from work that never started or it redoes writes that already succeeded. The file-operation list comes from the middleware choke point, so it is evidence a call ran rather than the model's account of it. The live TaskPlan stays alongside for now, because the consumer contract still requires a mutable plan the UI can set Status on. That duplication ends when progress becomes read-only and the legacy runner is retired. A test asserts the type carries no delegates, tokens or TaskPlan references — adding one would silently break checkpointing rather than fail a build. Tests: 582 -> 588. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/MandoCode.csproj | 6 +- src/MandoCode/Services/Ai/AIService.cs | 6 +- src/MandoCode/Services/Ai/PlanHandoff.cs | 14 +++ .../Services/Ai/Planning/PlanRunState.cs | 103 +++++++++++++++ .../Ai/Planning/PlanRunnerSelector.cs | 5 +- .../Ai/Planning/PlanWorkflowExecutors.cs | 36 +++++- .../Ai/Planning/PlanWorkflowMessages.cs | 10 ++ .../Ai/Planning/WorkflowPlanRunner.cs | 8 +- tests/MandoCode.Tests/PlanRunStateTests.cs | 118 ++++++++++++++++++ 9 files changed, 297 insertions(+), 9 deletions(-) create mode 100644 src/MandoCode/Services/Ai/Planning/PlanRunState.cs create mode 100644 tests/MandoCode.Tests/PlanRunStateTests.cs diff --git a/src/MandoCode/MandoCode.csproj b/src/MandoCode/MandoCode.csproj index f2e1f73..e57421d 100644 --- a/src/MandoCode/MandoCode.csproj +++ b/src/MandoCode/MandoCode.csproj @@ -13,7 +13,11 @@ MandoCode - 0.15.0 + + 0.15.0-stepdelta-test Armando Fernandez (DevMando) Your AI coding assistant — run locally or in the cloud with Ollama. No API keys required. Just you and your code. https://github.com/DevMando/MandoCode diff --git a/src/MandoCode/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index 2654766..cc2b5d5 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -1276,8 +1276,10 @@ public async Task ExecutePlanStepAsync(string stepInstruction, List 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 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 index d6ac20b..ccbeab1 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs @@ -18,7 +18,8 @@ namespace MandoCode.Services; public sealed class PlanRunnerSelector( MandoCodeConfig config, TaskPlannerService legacyRunner, - IPlanStepExecutor stepExecutor) + IPlanStepExecutor stepExecutor, + PlanHandoff? planHandoff = null) { private WorkflowPlanRunner? _workflowRunner; @@ -28,6 +29,6 @@ public sealed class PlanRunnerSelector( /// The engine to run the next plan with. public IPlanRunner Current => UsingWorkflowEngine - ? _workflowRunner ??= new WorkflowPlanRunner(stepExecutor) + ? _workflowRunner ??= new WorkflowPlanRunner(stepExecutor, planHandoff) : legacyRunner; } diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 76bfb42..8dee6cc 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -22,12 +22,36 @@ internal sealed class PlanRunContext( TaskPlan plan, IPlanStepExecutor stepExecutor, Func raise, - CancellationToken cancellationToken) + CancellationToken cancellationToken, + PlanHandoff? planHandoff = 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 ?? []); + + return context.QueueStateUpdateAsync( + PlanWorkflowMessages.StateKey, state, PlanWorkflowMessages.StateScope, ct); + } + /// Step results accumulated so far, in the format each step's context expects. public List PreviousResults { get; } = []; @@ -78,6 +102,7 @@ public override async ValueTask HandleAsync( 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) { @@ -190,6 +215,7 @@ public override async ValueTask HandleAsync( if (plan.Status == TaskPlanStatus.Cancelled) { + await ctx.SaveStateAsync(context, message.StepIndex, cancellationToken); await Finish(context, cancellationToken); return; } @@ -204,13 +230,19 @@ public override async ValueTask HandleAsync( 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, Math.Max(next, plan.Steps.Count), PlanWorkflowMessages.StateScope, cancellationToken); + 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) { diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs index 64bfae5..2707d27 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowMessages.cs @@ -17,6 +17,16 @@ internal static class PlanWorkflowMessages /// 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. diff --git a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs index 633a76c..f5db126 100644 --- a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -21,11 +21,15 @@ namespace MandoCode.Services; /// between a 3-step and a 12-step plan. /// /// -public sealed class WorkflowPlanRunner(IPlanStepExecutor stepExecutor) : IPlanRunner +public sealed class WorkflowPlanRunner(IPlanStepExecutor stepExecutor, PlanHandoff? planHandoff = 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; + /// /// One progress event, plus an optional handshake the producer waits on. /// @@ -59,7 +63,7 @@ async Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer, CancellationT await ack.Task; } - var ctx = new PlanRunContext(plan, _stepExecutor, RaiseAsync, cancellationToken); + var ctx = new PlanRunContext(plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff); var workflow = BuildWorkflow(ctx); var pump = Task.Run(async () => 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); + } + } +} From b9ec0d5c8fe62c8f8828999ebbc55186604027d3 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 13:48:21 -0700 Subject: [PATCH 16/21] Planner: resume an interrupted plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 4. A plan that died to a crash, a Ctrl+C, or a closed terminal can now be picked up where it left off. Progress is recorded to ~/.mandocode/plans/-.json as the run advances, using the same leaf+hash naming as SessionResumeStore so two folders both called "api" cannot collide, and the same write-then-rename discipline so a crash mid-save cannot tear a good record. The record is deleted the moment nothing is outstanding, so a finished plan is never offered. Resume rebuilds the plan and runs it again rather than restoring a framework checkpoint. Steps that completed or were skipped keep that status and the runner steps over them, so only outstanding work re-runs. That drops a dependency on byte-identical graph topology and on the workflow library's serialization format — both silent-failure risks — for state this small. A step that was mid-flight when the process died goes back to Pending: it may have half run, and re-running is safer than assuming it finished. /plan shows the saved plan, /plan resume continues it, /plan discard forgets it. A one-line notice at startup surfaces an unfinished plan, because someone whose work died has no reason to guess the command exists. Both are silent on the common path. Only the workflow engine records state, so only it can be resumed; the command says so rather than reporting "nothing to resume", which would look identical to having lost the work. A record that exists but cannot be safely resumed — different model, different build — explains itself instead of being silently ignored. Tests: 588 -> 596. The store's file I/O is not covered: it writes under the user's real profile directory, so testing it means making the root injectable first. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 165 ++++++++++++++++++ src/MandoCode/Models/SlashCommands.cs | 1 + src/MandoCode/Program.cs | 4 +- .../Ai/Planning/PlanCheckpointStore.cs | 146 ++++++++++++++++ .../Ai/Planning/PlanRunnerSelector.cs | 53 +++++- .../Ai/Planning/PlanWorkflowExecutors.cs | 7 +- .../Ai/Planning/WorkflowPlanRunner.cs | 10 +- .../PlanCheckpointStoreTests.cs | 109 ++++++++++++ 8 files changed, 488 insertions(+), 7 deletions(-) create mode 100644 src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs create mode 100644 tests/MandoCode.Tests/PlanCheckpointStoreTests.cs diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 5a55be7..0343f94 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -895,6 +895,8 @@ private async Task RunInteractiveLoopAsync() { + ShowUnfinishedPlanNotice(); + while (true) { // Show session token count right-aligned above prompt @@ -954,6 +956,12 @@ continue; } + if (command == "plan" || command.StartsWith("plan ")) + { + await HandlePlanCommandAsync(command.Length > 4 ? command[5..].Trim() : ""); + continue; + } + if (command == "help") { Console.WriteLine(); @@ -989,6 +997,7 @@ 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", "Show, resume, or discard an unfinished plan"); table.AddRow("/clear", "Clear conversation history"); table.AddRow("/exit", "Exit MandoCode"); AnsiConsole.Write(table); @@ -1972,6 +1981,162 @@ _planSelectTcs?.TrySetResult(choice); } + /// + /// One line at startup when this project has an unfinished plan. + /// + /// + /// Discoverability, not decoration: someone whose plan died to a crash or a Ctrl+C has no + /// reason to guess that /plan exists, and the work is only worth recording if they find + /// out it was. Silent on the common path — no saved plan, nothing printed. + /// + 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 (!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(); + + // Rebuild and run. Steps already Completed or Skipped keep that status, so the runner + // steps over them and only outstanding work executes. + var plan = PlanCheckpointStore.ToPlan(saved); + + _requestCts = new CancellationTokenSource(); + StartCancelKeyListener(); + try + { + await foreach (var progressEvent in PlanRunners.Current.ExecutePlanAsync(plan, _requestCts.Token)) + { + HandleProgressEvent(progressEvent, plan); + } + + var manifest = PlanHandoff.BuildManifest(plan, []); + 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 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; diff --git a/src/MandoCode/Models/SlashCommands.cs b/src/MandoCode/Models/SlashCommands.cs index 5706daa..b1e0a15 100644 --- a/src/MandoCode/Models/SlashCommands.cs +++ b/src/MandoCode/Models/SlashCommands.cs @@ -21,6 +21,7 @@ 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", "Show, resume, or discard an unfinished plan (/plan resume | /plan discard)" }, { "/clear", "Clear conversation history" }, { "/learn", "Learn about LLMs and local AI models" }, { "/retry", "Retry Ollama connection" }, diff --git a/src/MandoCode/Program.cs b/src/MandoCode/Program.cs index 144badf..a731b1a 100644 --- a/src/MandoCode/Program.cs +++ b/src/MandoCode/Program.cs @@ -173,7 +173,9 @@ static async Task Main(string[] args) services.AddSingleton(provider => new PlanRunnerSelector( provider.GetRequiredService(), provider.GetRequiredService(), - provider.GetRequiredService())); + provider.GetRequiredService(), + provider.GetRequiredService(), + provider.GetRequiredService())); // Register MusicPlayerService as singleton services.AddSingleton(provider => diff --git a/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs new file mode 100644 index 0000000..14a748e --- /dev/null +++ b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs @@ -0,0 +1,146 @@ +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 under ~/.mandocode/plans/, using the same leaf+hash naming as +/// so two folders both called "api" cannot 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 file path for a project root — readable leaf plus a hash of the full path. + public static string PathFor(string projectRoot) + { + 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()); + return Path.Combine(Folder, leaf.Length > 0 ? $"{leaf}-{hash}.json" : $"{hash}.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) + { + 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); + 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) + { + refusal = null; + try + { + var path = PathFor(projectRoot); + 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); + 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) + { + try + { + var path = PathFor(projectRoot); + 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) + { + try { return File.Exists(PathFor(projectRoot)); } + 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/PlanRunnerSelector.cs b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs index ccbeab1..878ed0f 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs @@ -3,7 +3,8 @@ namespace MandoCode.Services; /// -/// Resolves which plan engine to use, re-reading the planner config key on every access. +/// 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 @@ -19,7 +20,8 @@ public sealed class PlanRunnerSelector( MandoCodeConfig config, TaskPlannerService legacyRunner, IPlanStepExecutor stepExecutor, - PlanHandoff? planHandoff = null) + PlanHandoff? planHandoff = null, + ProjectRootAccessor? projectRoot = null) { private WorkflowPlanRunner? _workflowRunner; @@ -27,8 +29,53 @@ public sealed class PlanRunnerSelector( 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) + ? _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); + } + + /// Forgets any recorded plan for this project. + public void DiscardResumable() + { + if (projectRoot != null) PlanCheckpointStore.Delete(projectRoot.ProjectRoot); + } + + /// + /// 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); + return; + } + + PlanCheckpointStore.Save( + projectRoot.ProjectRoot, + state, + config.GetEffectiveModelName(), + planId: PlanCheckpointEnvelope.HashProjectRoot(projectRoot.ProjectRoot)); + } } diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 8dee6cc..4f58390 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -23,7 +23,8 @@ internal sealed class PlanRunContext( IPlanStepExecutor stepExecutor, Func raise, CancellationToken cancellationToken, - PlanHandoff? planHandoff = null) + PlanHandoff? planHandoff = null, + Action? onStateSaved = null) { public TaskPlan Plan { get; } = plan; public IPlanStepExecutor StepExecutor { get; } = stepExecutor; @@ -48,6 +49,10 @@ public ValueTask SaveStateAsync(IWorkflowContext context, int cursor, Cancellati 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); } diff --git a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs index f5db126..adcce53 100644 --- a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -21,7 +21,10 @@ namespace MandoCode.Services; /// between a 3-step and a 12-step plan. /// /// -public sealed class WorkflowPlanRunner(IPlanStepExecutor stepExecutor, PlanHandoff? planHandoff = null) : IPlanRunner +public sealed class WorkflowPlanRunner( + IPlanStepExecutor stepExecutor, + PlanHandoff? planHandoff = null, + Action? onStateSaved = null) : IPlanRunner { private readonly IPlanStepExecutor _stepExecutor = stepExecutor ?? throw new ArgumentNullException(nameof(stepExecutor)); @@ -30,6 +33,9 @@ public sealed class WorkflowPlanRunner(IPlanStepExecutor stepExecutor, PlanHando // 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. /// @@ -63,7 +69,7 @@ async Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer, CancellationT await ack.Task; } - var ctx = new PlanRunContext(plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff); + var ctx = new PlanRunContext(plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff, _onStateSaved); var workflow = BuildWorkflow(ctx); var pump = Task.Run(async () => diff --git a/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs new file mode 100644 index 0000000..c3275fe --- /dev/null +++ b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs @@ -0,0 +1,109 @@ +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 + } +} From 97264a2280fd70c3202a084b9af4a060597742cb Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 14:22:09 -0700 Subject: [PATCH 17/21] Planner: add /plan-resume and /plan-discard, and fix what resume carried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the two dedicated commands alongside /plan, matching the existing /music-stop style, and makes them the ones the startup notice points at. Writing them surfaced two gaps in resume as shipped, both of which would have made a resumed plan quietly worse than the run it continued: Resumed steps had no original request. Every step's context includes the user's verbatim request as the authority on WHERE work happens — target folders named there override unqualified paths in a step instruction. That message is captured when the user sends it, so a plan resumed in a new process had none at all. This is the exact shape of an observed failure where a plan lost its target folder and wrote every file to the project root. The saved record already carried the goal; the CLI now feeds it back in via AIService.SetRequestContext before resuming. Resumed steps had no earlier results. PlanRunContext.PreviousResults started empty, so remaining steps ran blind to everything already built, even though the record held it. WorkflowPlanRunner.ResumeAsync now seeds them. Neither was covered by the Phase 4 tests, which asserted which steps re-run but not what those steps could see when they did. Tests: 596 -> 600. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 38 ++++++-- src/MandoCode/Models/SlashCommands.cs | 4 +- src/MandoCode/Services/Ai/AIService.cs | 17 ++++ .../Ai/Planning/PlanWorkflowExecutors.cs | 14 ++- .../Ai/Planning/WorkflowPlanRunner.cs | 24 +++++- .../MandoCode.Tests/PlanResumeContextTests.cs | 86 +++++++++++++++++++ 6 files changed, 170 insertions(+), 13 deletions(-) create mode 100644 tests/MandoCode.Tests/PlanResumeContextTests.cs diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 0343f94..120691c 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -956,6 +956,18 @@ continue; } + if (command == "plan-resume") + { + await HandlePlanCommandAsync("resume"); + continue; + } + + if (command == "plan-discard") + { + await HandlePlanCommandAsync("discard"); + continue; + } + if (command == "plan" || command.StartsWith("plan ")) { await HandlePlanCommandAsync(command.Length > 4 ? command[5..].Trim() : ""); @@ -997,7 +1009,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", "Show, resume, or discard an unfinished plan"); + table.AddRow("/plan", "Show 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); @@ -2006,7 +2020,7 @@ 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.MarkupLine("[dim]/plan-resume[/] to continue, [dim]/plan-discard[/] to forget it."); AnsiConsole.WriteLine(); } catch { /* a notice must never stop the session starting */ } @@ -2048,7 +2062,7 @@ // 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.MarkupLine("[dim]Use[/] /plan-discard [dim]to forget it.[/]"); AnsiConsole.WriteLine(); return; } @@ -2070,7 +2084,7 @@ 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.MarkupLine("[dim]/plan-resume[/] to continue, [dim]/plan-discard[/] to forget it."); AnsiConsole.WriteLine(); return; } @@ -2080,15 +2094,25 @@ $"[dim]({outstanding} step(s) left)[/]"); AnsiConsole.WriteLine(); - // Rebuild and run. Steps already Completed or Skipped keep that status, so the runner - // steps over them and only outstanding work executes. + // 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 { - await foreach (var progressEvent in PlanRunners.Current.ExecutePlanAsync(plan, _requestCts.Token)) + var runner = PlanRunners.Current as WorkflowPlanRunner; + var progress = runner != null + ? runner.ResumeAsync(saved, _requestCts.Token) + : PlanRunners.Current.ExecutePlanAsync(plan, _requestCts.Token); + + await foreach (var progressEvent in progress) { HandleProgressEvent(progressEvent, plan); } diff --git a/src/MandoCode/Models/SlashCommands.cs b/src/MandoCode/Models/SlashCommands.cs index b1e0a15..4e476c1 100644 --- a/src/MandoCode/Models/SlashCommands.cs +++ b/src/MandoCode/Models/SlashCommands.cs @@ -21,7 +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", "Show, resume, or discard an unfinished plan (/plan resume | /plan discard)" }, + { "/plan", "Show an unfinished plan for this project" }, + { "/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/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index cc2b5d5..c4240e5 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -1223,6 +1223,23 @@ public static string BuildStepContext(string systemPrompt, string? originalUserR 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 diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 4f58390..737ffd8 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -24,7 +24,8 @@ internal sealed class PlanRunContext( Func raise, CancellationToken cancellationToken, PlanHandoff? planHandoff = null, - Action? onStateSaved = null) + Action? onStateSaved = null, + IReadOnlyList? seedResults = null) { public TaskPlan Plan { get; } = plan; public IPlanStepExecutor StepExecutor { get; } = stepExecutor; @@ -57,8 +58,15 @@ public ValueTask SaveStateAsync(IWorkflowContext context, int cursor, Cancellati PlanWorkflowMessages.StateKey, state, PlanWorkflowMessages.StateScope, ct); } - /// Step results accumulated so far, in the format each step's context expects. - public List PreviousResults { get; } = []; + /// + /// 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 ?? []]; /// /// Publishes a progress event and, by default, waits until the consumer has processed it and diff --git a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs index adcce53..864634b 100644 --- a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -47,8 +47,27 @@ public sealed class WorkflowPlanRunner( /// private sealed record Signal(TaskProgressEvent Event, TaskCompletionSource? Ack); - public async IAsyncEnumerable ExecutePlanAsync( + 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); + + private async IAsyncEnumerable RunAsync( + TaskPlan plan, + IReadOnlyList? seedResults, [EnumeratorCancellation] CancellationToken cancellationToken = default) { var channel = Channel.CreateUnbounded(new UnboundedChannelOptions @@ -69,7 +88,8 @@ async Task RaiseAsync(TaskProgressEvent evt, bool waitForConsumer, CancellationT await ack.Task; } - var ctx = new PlanRunContext(plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff, _onStateSaved); + var ctx = new PlanRunContext( + plan, _stepExecutor, RaiseAsync, cancellationToken, _planHandoff, _onStateSaved, seedResults); var workflow = BuildWorkflow(ctx); var pump = Task.Run(async () => diff --git a/tests/MandoCode.Tests/PlanResumeContextTests.cs b/tests/MandoCode.Tests/PlanResumeContextTests.cs new file mode 100644 index 0000000..bb5d68c --- /dev/null +++ b/tests/MandoCode.Tests/PlanResumeContextTests.cs @@ -0,0 +1,86 @@ +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 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); + } +} From d5989682a57f33611953156761b40b84d51792fc Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 14:55:04 -0700 Subject: [PATCH 18/21] Planner: only mention an unfinished plan on --continue --continue is this app's existing "pick up where I left off" gesture, and the only thing that reloads a previous session. Launching without it means a fresh start, where a plan from an earlier session is noise. Moving the notice there also keeps the two halves of "where I left off" together: a plan resumed alongside its restored conversation runs with the context it originally had, rather than only the goal and the step instructions. It notifies rather than resuming. Resuming writes files, so doing it automatically at launch would let a plan the user had walked away from start changing their project before they had read a line of output. /plan-resume stays explicit. Tests: 600 green. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 120691c..881a83d 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -383,6 +383,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 @@ -895,8 +899,6 @@ private async Task RunInteractiveLoopAsync() { - ShowUnfinishedPlanNotice(); - while (true) { // Show session token count right-aligned above prompt @@ -1996,12 +1998,20 @@ } /// - /// One line at startup when this project has an unfinished plan. + /// One line during --continue startup when this project has an unfinished plan. /// /// - /// Discoverability, not decoration: someone whose plan died to a crash or a Ctrl+C has no - /// reason to guess that /plan exists, and the work is only worth recording if they find - /// out it was. Silent on the common path — no saved plan, nothing printed. + /// + /// 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() { From ce754855361facb0d655d66c7d471f784ca65f0c Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 14:59:27 -0700 Subject: [PATCH 19/21] Planner: show what a step will actually do, and let it be edited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The approval table listed only each step's description — a <=60 character label the model writes for display. The instruction, which is the text actually sent to the model and the only thing with consequences, was never shown. Approving a plan meant approving work you had not read: two steps can read identically in summary and target completely different files. The instruction is now a third column, wrapped rather than truncated, since a clipped instruction is the same problem as showing none. Adds "Edit a step", which rewrites the instruction rather than the description — editing the label would change what the table claims without changing what happens, which is worse than not offering it. The label follows along when the new text is short enough to serve as one. The menu loops after an edit instead of forcing an immediate approve-or-reject, because reviewing a plan is iterative. Reuses InstructionPromptCoordinator, the existing bridge from an imperative handler to a VDOM text input, rather than adding a second input path. Tests: 600 green. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 102 +++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 12 deletions(-) diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 881a83d..3c02909 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -1937,6 +1937,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"; @@ -1951,6 +1952,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)), }; @@ -2257,17 +2259,26 @@ // 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. - using (KeyCoordinator.Suppress()) + // 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) { - choice = await PromptPlanChoiceAsync(ct); + 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. + using (KeyCoordinator.Suppress()) + { + choice = await PromptPlanChoiceAsync(ct); + } + + if (choice != EditPlanStepLabel) break; + + await EditPlanStepAsync(plan, ct); } } @@ -2345,18 +2356,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)}[/]" ); } @@ -2366,6 +2391,59 @@ AnsiConsole.WriteLine(); } + /// + /// 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) + { + AnsiConsole.WriteLine(); + + var raw = await InstructionCoordinator.RequestAsync( + $"Which step to edit? (1-{plan.Steps.Count}, blank to go back)"); + + if (!int.TryParse(raw?.Trim(), out var number) || number < 1 || number > plan.Steps.Count) + { + AnsiConsole.MarkupLine("[dim]No step selected.[/]"); + AnsiConsole.WriteLine(); + return false; + } + + var step = plan.Steps[number - 1]; + + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine($"[deepskyblue1]Step {step.StepNumber}[/] — current instruction:"); + AnsiConsole.MarkupLine($"[dim]{Spectre.Console.Markup.Escape(step.Instruction ?? "")}[/]"); + AnsiConsole.WriteLine(); + + var revised = await InstructionCoordinator.RequestAsync( + "New instruction (blank to leave unchanged):"); + + if (string.IsNullOrWhiteSpace(revised)) + { + AnsiConsole.MarkupLine("[dim]Left unchanged.[/]"); + AnsiConsole.WriteLine(); + return false; + } + + step.Instruction = revised.Trim(); + + // Keep the label in step with the text when the model's original label no longer describes + // what the step does. Truncated the same way FromProposals does. + if (step.Description.Length == 0 || revised.Length <= 60) + { + step.Description = revised.Length > 60 ? revised[..57] + "..." : revised.Trim(); + } + + AnsiConsole.MarkupLine($"[green]Step {step.StepNumber} updated.[/]"); + AnsiConsole.WriteLine(); + return true; + } + private void HandleProgressEvent(TaskProgressEvent progressEvent, TaskPlan plan) { switch (progressEvent.ProgressType) From 5b3c296c0982f5b9171931bc12a66421c5066230 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Fri, 28 Aug 2026 15:02:43 -0700 Subject: [PATCH 20/21] Planner: let a failed step be retried MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 and everything that depended on it. Retry is signalled by the consumer setting the step back to Pending while handling StepFailed. Triage then re-dispatches the same index instead of advancing the cursor past it, which is what the existing code would otherwise do. Capped at 3 attempts per step: a step failing identically every time must not spin forever because the consumer keeps asking, and once out of retries it is skipped so the plan still finishes. Cancel still beats retry — a consumer that cancels has made the stronger statement. Offered on the workflow engine only. The legacy runner walks its steps with a foreach and has no way back to one it has already passed, so the prompt omits the option there rather than showing something that would silently do nothing. Tests: 600 -> 604, covering the retry itself, the cap, and that not retrying still skips exactly as before. Co-Authored-By: Claude Opus 5 (1M context) --- src/MandoCode/Components/App.razor | 30 +++-- .../Ai/Planning/PlanWorkflowExecutors.cs | 29 +++++ tests/MandoCode.Tests/PlanStepRetryTests.cs | 121 ++++++++++++++++++ 3 files changed, 171 insertions(+), 9 deletions(-) create mode 100644 tests/MandoCode.Tests/PlanStepRetryTests.cs diff --git a/src/MandoCode/Components/App.razor b/src/MandoCode/Components/App.razor index 3c02909..67918a6 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -2480,10 +2480,19 @@ AnsiConsole.MarkupLine($"[red]Step {progressEvent.CurrentStep} failed:[/] {Spectre.Console.Markup.Escape(progressEvent.Message ?? "Unknown error")}"); // 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 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, skipStepLabel, cancelPlanLabel } + : new[] { skipStepLabel, cancelPlanLabel }; + string failChoice; using (KeyCoordinator.Suppress()) { @@ -2491,7 +2500,7 @@ new SelectionPrompt() .Title("[deepskyblue1]How would you like to proceed?[/]") .HighlightStyle(SelectionHighlight) - .AddChoices(new[] { skipStepLabel, cancelPlanLabel }) + .AddChoices(choices) ); } @@ -2499,14 +2508,17 @@ { PlanRunners.Current.CancelPlan(plan); } - else + else if (failChoice == retryStepLabel && stepToDecide != null) { - var failedStep = plan.Steps.FirstOrDefault(s => s.StepNumber == progressEvent.CurrentStep); - if (failedStep != null) - { - PlanRunners.Current.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 (stepToDecide != null) + { + PlanRunners.Current.SkipStep(plan, stepToDecide); + SpinnerService.SetTaskbarWarning(progressEvent.CurrentStep * 100 / progressEvent.TotalSteps); } break; diff --git a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs index 737ffd8..cacd377 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -68,6 +68,16 @@ public ValueTask SaveStateAsync(IWorkflowContext context, int cursor, Cancellati /// 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. @@ -233,6 +243,25 @@ public override async ValueTask HandleAsync( 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) 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); + } +} From e05839930a6645569eb18d5e6a2b79f4a84c04f9 Mon Sep 17 00:00:00 2001 From: Armando Fernandez Date: Sat, 29 Aug 2026 15:54:56 -0700 Subject: [PATCH 21/21] Planner: finish recovery, replanning, and explicit control --- docs/CHANGELOG.md | 50 ++- src/MandoCode/Components/App.razor | 369 +++++++++++++++--- src/MandoCode/Components/PromptInput.razor | 27 +- src/MandoCode/MandoCode.csproj | 6 +- src/MandoCode/Models/GeneratedPlan.cs | 9 + src/MandoCode/Models/SlashCommands.cs | 2 +- src/MandoCode/Models/TaskPlan.cs | 3 + src/MandoCode/Models/TaskProgressEvent.cs | 4 +- src/MandoCode/Services/Ai/AIService.cs | 239 +++++++++++- src/MandoCode/Services/Ai/PlanHandoff.cs | 86 +++- .../Ai/Planning/PlanCheckpointEnvelope.cs | 26 +- .../Ai/Planning/PlanCheckpointStore.cs | 49 ++- .../Services/Ai/Planning/PlanRevision.cs | 95 +++++ .../Ai/Planning/PlanRunnerSelector.cs | 16 +- .../Services/Ai/Planning/PlanStepReport.cs | 54 +++ .../Ai/Planning/PlanWorkflowExecutors.cs | 18 +- .../Ai/Planning/WorkflowPlanRunner.cs | 11 + .../Services/Ai/TaskPlannerService.cs | 15 +- .../Services/Input/InputStateMachine.cs | 2 +- .../Input/InstructionPromptCoordinator.cs | 5 +- src/MandoCode/docs/TaskPlanner.md | 42 +- .../MandoCode.Tests/InputStateMachineTests.cs | 1 + .../InstructionPromptCoordinatorTests.cs | 36 ++ .../PlanCheckpointEnvelopeTests.cs | 14 +- .../PlanCheckpointStoreTests.cs | 11 + tests/MandoCode.Tests/PlanHandoffTests.cs | 40 ++ .../MandoCode.Tests/PlanResumeContextTests.cs | 14 + tests/MandoCode.Tests/PlanRevisionTests.cs | 78 ++++ .../PlanRunnerBehaviorTests.cs | 3 +- tests/MandoCode.Tests/PlanStepReportTests.cs | 46 +++ 30 files changed, 1224 insertions(+), 147 deletions(-) create mode 100644 src/MandoCode/Models/GeneratedPlan.cs create mode 100644 src/MandoCode/Services/Ai/Planning/PlanRevision.cs create mode 100644 src/MandoCode/Services/Ai/Planning/PlanStepReport.cs create mode 100644 tests/MandoCode.Tests/InstructionPromptCoordinatorTests.cs create mode 100644 tests/MandoCode.Tests/PlanRevisionTests.cs create mode 100644 tests/MandoCode.Tests/PlanStepReportTests.cs 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 67918a6..3f200d9 100644 --- a/src/MandoCode/Components/App.razor +++ b/src/MandoCode/Components/App.razor @@ -63,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 @@ -183,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. @@ -972,7 +963,11 @@ if (command == "plan" || command.StartsWith("plan ")) { - await HandlePlanCommandAsync(command.Length > 4 ? command[5..].Trim() : ""); + // 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; } @@ -1011,7 +1006,7 @@ 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", "Show an unfinished plan"); + 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"); @@ -1266,7 +1261,6 @@ /// private void HandleInstructionSubmit(string value) { - _instructionValue = ""; InstructionCoordinator.Submit(value ?? string.Empty); } @@ -2050,6 +2044,12 @@ { AnsiConsole.WriteLine(); + if (!string.IsNullOrWhiteSpace(arg) && arg is not "resume" and not "discard") + { + await ForcePlanAsync(arg); + return; + } + if (!PlanRunners.SupportsResume) { AnsiConsole.MarkupLine( @@ -2119,17 +2119,18 @@ StartCancelKeyListener(); try { + using var execution = PlanHandoff.BeginResumedExecution(saved.FileOperations); var runner = PlanRunners.Current as WorkflowPlanRunner; var progress = runner != null - ? runner.ResumeAsync(saved, _requestCts.Token) + ? runner.ResumeAsync(plan, saved, _requestCts.Token) : PlanRunners.Current.ExecutePlanAsync(plan, _requestCts.Token); await foreach (var progressEvent in progress) { - HandleProgressEvent(progressEvent, plan); + await HandleProgressEventAsync(progressEvent, plan, _requestCts.Token); } - var manifest = PlanHandoff.BuildManifest(plan, []); + var manifest = PlanHandoff.BuildManifest(plan, PlanHandoff.FileOperations); AI.AppendAssistantNote(manifest); } catch (OperationCanceledException) @@ -2147,6 +2148,48 @@ 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 }; @@ -2259,26 +2302,27 @@ // Stop the outer "Thinking..." spinner so it doesn't fight the approval prompt. Spinner.Stop(); - // 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) + // 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()) { - 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. - using (KeyCoordinator.Suppress()) + // 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; + if (choice != EditPlanStepLabel) break; - await EditPlanStepAsync(plan, ct); + await EditPlanStepAsync(plan, ct); + } } } @@ -2316,7 +2360,7 @@ { await foreach (var progressEvent in PlanRunners.Current.ExecutePlanAsync(plan, ct)) { - HandleProgressEvent(progressEvent, plan); + await HandleProgressEventAsync(progressEvent, plan, ct); } } catch (OperationCanceledException) @@ -2330,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.[/]"); @@ -2399,52 +2447,141 @@ /// 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) + 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(); - var raw = await InstructionCoordinator.RequestAsync( - $"Which step to edit? (1-{plan.Steps.Count}, blank to go back)"); + 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; + } - if (!int.TryParse(raw?.Trim(), out var number) || number < 1 || number > plan.Steps.Count) + 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 step = plan.Steps[number - 1]; + var selectedIndex = Array.IndexOf(stepLabels, selectedLabel); + if (selectedIndex < 0 || selectedIndex >= selectableSteps.Length) + return false; - AnsiConsole.WriteLine(); - AnsiConsole.MarkupLine($"[deepskyblue1]Step {step.StepNumber}[/] — current instruction:"); - AnsiConsole.MarkupLine($"[dim]{Spectre.Console.Markup.Escape(step.Instruction ?? "")}[/]"); - AnsiConsole.WriteLine(); + var step = selectableSteps[selectedIndex]; var revised = await InstructionCoordinator.RequestAsync( - "New instruction (blank to leave unchanged):"); + $"Edit step {step.StepNumber} instruction (Enter to save):", + step.Instruction); - if (string.IsNullOrWhiteSpace(revised)) + if (string.IsNullOrWhiteSpace(revised) || + string.Equals(revised.Trim(), step.Instruction?.Trim(), StringComparison.Ordinal)) { AnsiConsole.MarkupLine("[dim]Left unchanged.[/]"); AnsiConsole.WriteLine(); return false; } - step.Instruction = revised.Trim(); - - // Keep the label in step with the text when the model's original label no longer describes - // what the step does. Truncated the same way FromProposals does. - if (step.Description.Length == 0 || revised.Length <= 60) - { - step.Description = revised.Length > 60 ? revised[..57] + "..." : revised.Trim(); - } + 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 void HandleProgressEvent(TaskProgressEvent progressEvent, TaskPlan plan) + 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) { @@ -2479,9 +2616,19 @@ 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: 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[/]"; @@ -2490,7 +2637,7 @@ // 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, skipStepLabel, cancelPlanLabel } + ? new[] { retryStepLabel, revisePlanLabel, skipStepLabel, cancelPlanLabel } : new[] { skipStepLabel, cancelPlanLabel }; string failChoice; @@ -2515,6 +2662,18 @@ 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); @@ -2534,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/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 e57421d..f2e1f73 100644 --- a/src/MandoCode/MandoCode.csproj +++ b/src/MandoCode/MandoCode.csproj @@ -13,11 +13,7 @@ MandoCode - - 0.15.0-stepdelta-test + 0.15.0 Armando Fernandez (DevMando) Your AI coding assistant — run locally or in the cloud with Ollama. No API keys required. Just you and your code. https://github.com/DevMando/MandoCode 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/SlashCommands.cs b/src/MandoCode/Models/SlashCommands.cs index 4e476c1..a52bb41 100644 --- a/src/MandoCode/Models/SlashCommands.cs +++ b/src/MandoCode/Models/SlashCommands.cs @@ -21,7 +21,7 @@ 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", "Show an unfinished plan for this project" }, + { "/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" }, 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/Services/Ai/AIService.cs b/src/MandoCode/Services/Ai/AIService.cs index c4240e5..752999e 100644 --- a/src/MandoCode/Services/Ai/AIService.cs +++ b/src/MandoCode/Services/Ai/AIService.cs @@ -453,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 @@ -509,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); @@ -1220,6 +1440,10 @@ 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(); } @@ -1394,7 +1618,20 @@ public async Task ExecutePlanStepAsync(string stepInstruction, ListTrue when the model proposed a plan this turn that hasn't been run yet. public bool HasPendingProposal @@ -114,10 +115,25 @@ public bool HasPendingProposal get { lock (_lock) return _pendingProposal != null; } } + /// + /// 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, steps); + lock (_lock) _pendingProposal = (goal, _currentRequest, steps); } /// Drops any pending proposal — used when a turn ends without running one. @@ -143,7 +159,7 @@ public void ClearPendingProposal() /// public async Task RunPendingPlanAsync(CancellationToken ct = default) { - (string Goal, PlanStepProposal[] Steps)? pending; + (string Goal, string? OriginalRequest, PlanStepProposal[] Steps)? pending; lock (_lock) { pending = _pendingProposal; @@ -152,7 +168,11 @@ public void ClearPendingProposal() if (pending == null) return null; - return await ProcessAsync(pending.Value.Goal, pending.Value.Steps, ct); + return await ProcessAsync( + pending.Value.Goal, + pending.Value.Steps, + ct, + pending.Value.OriginalRequest); } /// @@ -163,7 +183,8 @@ public void ClearPendingProposal() public async Task ProcessAsync( string goal, PlanStepProposal[] proposals, - CancellationToken ct = default) + CancellationToken ct = default, + string? originalRequest = null) { lock (_lock) { @@ -185,7 +206,7 @@ public async Task ProcessAsync( var plan = new TaskPlan { - OriginalRequest = goal, + OriginalRequest = string.IsNullOrWhiteSpace(originalRequest) ? goal : originalRequest, Steps = steps, Status = TaskPlanStatus.Pending }; @@ -222,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/PlanCheckpointEnvelope.cs b/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs index a8c56f8..c39d9c3 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanCheckpointEnvelope.cs @@ -6,15 +6,15 @@ namespace MandoCode.Services; /// -/// Versioned wrapper around a MAF workflow checkpoint. +/// Versioned wrapper around a durable plan-state snapshot. /// /// /// -/// The framework's own checkpoint payload is written opaque, inside . It is -/// never stored bare: a Microsoft.Agents.AI.Workflows version bump could change that format -/// with no detection, and the failure mode is deserializing a stale shape into a plan that then -/// runs write_file. Every field outside exists to be a refusal -/// criterion — see . +/// 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 @@ -56,7 +56,7 @@ public sealed record PlanCheckpointEnvelope [JsonPropertyName("createdUtc")] public DateTimeOffset CreatedUtc { get; init; } - /// The framework's checkpoint blob. Opaque here by design — never inspected. + /// The serialized . Kept as JSON until compatibility passes. [JsonPropertyName("payload")] public JsonElement Payload { get; init; } @@ -79,7 +79,11 @@ public static string HashProjectRoot(string projectRoot) /// /// Hash of the project root being resumed into. /// Model configured right now. - public string? FindIncompatibility(string projectRootHash, string modelName) + /// Optional Desktop agent/session owner for collision isolation. + public string? FindIncompatibility( + string projectRootHash, + string modelName, + string? expectedPlanId = null) { if (SchemaVersion != CurrentSchemaVersion) { @@ -99,6 +103,12 @@ public static string HashProjectRoot(string projectRoot) 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 " diff --git a/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs index 14a748e..ab21e39 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanCheckpointStore.cs @@ -9,10 +9,10 @@ namespace MandoCode.Services; /// /// /// -/// One file per project root under ~/.mandocode/plans/, using the same leaf+hash naming as -/// so two folders both called "api" cannot collide. Written -/// whole-file with write-then-rename, and best-effort throughout: persistence must never break a -/// running plan. +/// 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 @@ -30,20 +30,31 @@ public static class PlanCheckpointStore private static string Folder => Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".mandocode", "plans"); - /// Stable file path for a project root — readable leaf plus a hash of the full path. - public static string PathFor(string projectRoot) + /// 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()); - return Path.Combine(Folder, leaf.Length > 0 ? $"{leaf}-{hash}.json" : $"{hash}.json"); + 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) + public static void Save( + string projectRoot, + PlanRunState state, + string modelName, + string planId, + string? checkpointId = null) { try { @@ -61,7 +72,7 @@ public static void Save(string projectRoot, PlanRunState state, string modelName if (json.Length > MaxBytes) return; Directory.CreateDirectory(Folder); - var path = PathFor(projectRoot); + var path = PathFor(projectRoot, checkpointId); var tmp = path + ".tmp"; File.WriteAllText(tmp, json); File.Move(tmp, path, overwrite: true); @@ -74,19 +85,25 @@ public static void Save(string projectRoot, PlanRunState state, string modelName /// 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) + public static PlanRunState? Load( + string projectRoot, + string modelName, + out string? refusal, + string? checkpointId = null) { refusal = null; try { - var path = PathFor(projectRoot); + 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); + PlanCheckpointEnvelope.HashProjectRoot(projectRoot), + modelName, + checkpointId); if (refusal != null) return null; return envelope.Payload.Deserialize(); @@ -100,20 +117,20 @@ public static void Save(string projectRoot, PlanRunState state, string modelName } /// Removes the record — the plan finished, was cancelled, or the user discarded it. - public static void Delete(string projectRoot) + public static void Delete(string projectRoot, string? checkpointId = null) { try { - var path = PathFor(projectRoot); + 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) + public static bool Exists(string projectRoot, string? checkpointId = null) { - try { return File.Exists(PathFor(projectRoot)); } + try { return File.Exists(PathFor(projectRoot, checkpointId)); } catch { return false; } } 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/PlanRunnerSelector.cs b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs index 878ed0f..48caa74 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanRunnerSelector.cs @@ -21,7 +21,8 @@ public sealed class PlanRunnerSelector( TaskPlannerService legacyRunner, IPlanStepExecutor stepExecutor, PlanHandoff? planHandoff = null, - ProjectRootAccessor? projectRoot = null) + ProjectRootAccessor? projectRoot = null, + string? checkpointId = null) { private WorkflowPlanRunner? _workflowRunner; @@ -49,13 +50,17 @@ public sealed class PlanRunnerSelector( { refusal = null; if (projectRoot == null) return null; - return PlanCheckpointStore.Load(projectRoot.ProjectRoot, config.GetEffectiveModelName(), out refusal); + 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); + if (projectRoot != null) PlanCheckpointStore.Delete(projectRoot.ProjectRoot, checkpointId); } /// @@ -68,7 +73,7 @@ private void RecordProgress(PlanRunState state) if (PlanCheckpointStore.OutstandingSteps(state) == 0) { - PlanCheckpointStore.Delete(projectRoot.ProjectRoot); + PlanCheckpointStore.Delete(projectRoot.ProjectRoot, checkpointId); return; } @@ -76,6 +81,7 @@ private void RecordProgress(PlanRunState state) projectRoot.ProjectRoot, state, config.GetEffectiveModelName(), - planId: PlanCheckpointEnvelope.HashProjectRoot(projectRoot.ProjectRoot)); + 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 index cacd377..f29da20 100644 --- a/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs +++ b/src/MandoCode/Services/Ai/Planning/PlanWorkflowExecutors.cs @@ -316,18 +316,20 @@ public override async ValueTask HandleAsync( return; } - // Classification copied deliberately from the legacy runner, including its quirk that a - // plan whose steps were all skipped after failures still reports Completed. Changing it - // here would make the two engines disagree while both are selectable; it belongs in the - // phase that retires the legacy runner. - 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."; await ctx.RaiseAsync(TaskProgressEvent.PlanCompleted(plan)); } else diff --git a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs index 864634b..0928eb2 100644 --- a/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs +++ b/src/MandoCode/Services/Ai/Planning/WorkflowPlanRunner.cs @@ -65,6 +65,17 @@ public IAsyncEnumerable ResumeAsync( 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, diff --git a/src/MandoCode/Services/Ai/TaskPlannerService.cs b/src/MandoCode/Services/Ai/TaskPlannerService.cs index c693438..4e52946 100644 --- a/src/MandoCode/Services/Ai/TaskPlannerService.cs +++ b/src/MandoCode/Services/Ai/TaskPlannerService.cs @@ -208,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/docs/TaskPlanner.md b/src/MandoCode/docs/TaskPlanner.md index ec463c2..558eafb 100644 --- a/src/MandoCode/docs/TaskPlanner.md +++ b/src/MandoCode/docs/TaskPlanner.md @@ -309,13 +309,13 @@ See the main [README](../../../README.md#diff-approvals) for user-facing documen --- -## Migration to MAF Workflows (in progress) +## MAF workflow planner -The planner is being rebuilt on `Microsoft.Agents.AI.Workflows`. The driver is a single root cause: -**the whole plan currently executes inside one `propose_plan` tool call.** Because the outer model's -turn is still open, it never sees the steps run, treats the summary as "not started yet," and redoes -the work — so `BuildManifest`, the App.razor stop directive, and the `PlanWorkCompleted` gate all -exist to stop it. The watchdog pause and the prompt-gate release dance have the same origin. +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), @@ -331,10 +331,10 @@ too: it routes via `handoff_to_*` tool calls that cannot be forced, the worst po | 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 | Move approval and step decisions onto `RequestPort`s; make progress read-only | | -| 4 | Checkpointing + resume | | -| 5 | Retry / replan / forced-tool-use escalation | | -| 6 | Flip the `planner` default, then delete the legacy runner | | +| 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: @@ -373,14 +373,28 @@ workflow-executor identity from both the agent's `Id` and `Name`, and a checkpoi 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 — in progress, phase 4 above; - envelope and identity scheme already landed in phase 1 -- [~] User-editable plan before approval — in progress, phase 3 above; the approval view will also - show each step's `Instruction`, which today is never shown before the user approves it +- [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/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 index 452127a..fdbf05d 100644 --- a/tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs +++ b/tests/MandoCode.Tests/PlanCheckpointEnvelopeTests.cs @@ -5,7 +5,7 @@ namespace MandoCode.Tests; /// -/// Tests the versioned wrapper around MAF's checkpoint blob. Every field outside the payload is a +/// 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. /// @@ -82,6 +82,18 @@ public void DifferentModel_IsRefused_AndNamesBothModels() 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() { diff --git a/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs index c3275fe..c1d25e8 100644 --- a/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs +++ b/tests/MandoCode.Tests/PlanCheckpointStoreTests.cs @@ -106,4 +106,15 @@ public void PathIsStablePerProject_AndDistinguishesSameLeafNames() 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/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/PlanResumeContextTests.cs b/tests/MandoCode.Tests/PlanResumeContextTests.cs index bb5d68c..906ae15 100644 --- a/tests/MandoCode.Tests/PlanResumeContextTests.cs +++ b/tests/MandoCode.Tests/PlanResumeContextTests.cs @@ -75,6 +75,20 @@ public async Task ResumeCompletesThePlan() 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() { 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/PlanRunnerBehaviorTests.cs b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs index a28049b..f9cd7bc 100644 --- a/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs +++ b/tests/MandoCode.Tests/PlanRunnerBehaviorTests.cs @@ -152,7 +152,8 @@ public async Task FailedStep_WithNoInteractiveConsumer_IsDowngradedToSkipped(str Assert.Equal(["fine", "boom", "also fine"], exec.Executed); Assert.Equal(TaskStepStatus.Skipped, plan.Steps[1].Status); - Assert.Equal(TaskPlanStatus.Completed, plan.Status); + Assert.Equal(TaskPlanStatus.CompletedWithIssues, plan.Status); + Assert.Contains("1 step(s) were skipped", plan.ExecutionSummary); } [Theory] 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); + } +}