Make workflow plans resumable, reviewable, and able to replan - #79
Merged
Conversation
…nner 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) <noreply@anthropic.com>
The whole plan used to execute inside the propose_plan tool call — approval, every step, every nested tool call and diff prompt, all awaited inside that one call. Nearly every oddity in the planner descended from that nesting, and this removes the cause rather than adding another guard. propose_plan now records the proposal and returns a receipt immediately. The host runs the plan once its chat turn has fully drained, making the plan a peer of the turn rather than a child of a tool call. What that lets us delete: - pauseDuringPlan, in both the stall watchdog and the request-timeout ceiling. Both existed because a slow step inside the tool call would trip a timer and surface as a bogus "Cancelled by user." There is no plan inside the model call any more, and each step still has its own watchdog. - PlanAlreadyProcessed. It refused a second proposal in the same turn because the first had already run and the model was starting uninvited extra work. Nothing has run yet at proposal time, so last-wins is both simpler and safer than a prose refusal the model could ignore. - PlanWorkCompleted, the post-plan mutation gate. It refused mutations for the rest of the turn because the outer model never saw the steps execute and read the summary as "not started yet" — observed live overwriting a finished build under auto-approve. With no post-plan turn, the window shrinks to the gap between propose_plan and the end of the reply, which is what ProposalPending now covers. Its tests were retargeted, not deleted: the incident is still the reason they exist. A completed plan's manifest is appended to history via AppendAssistantNote rather than fed back for a closing turn. Removing the model's opportunity to redo the work is the point; three layers of asking it not to never reliably worked. Rejection is the one case that does need another turn — the user asked for a direct answer instead of stepwise execution, and previously got it for free from the still-open turn. The host now starts one explicitly, bounded to a single follow-up so a re-proposed plan cannot ping-pong. Also corrects the system prompt, which promised a completion summary that no longer arrives, and the "communicate your plan in natural language" instruction that competed directly with propose_plan. Tests: 541 green on net10.0 and net8.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second engine for running an approved plan, built on Microsoft.Agents.AI.Workflows and selectable per plan. The legacy runner stays the default and stays intact; this is a strangler step, not a replacement. Topology is fixed — intake, step-runner, triage, finalizer — no matter how many steps a plan has. The cursor lives in the workflow's shared state and in the messages, never in the graph's shape: resume demands byte-identical topology, so a node-per-step layout could never restore a 3-step checkpoint into a replanned 12-step one. PlanWorkflowTopologyTests pins that with a golden executor list. Triage is the sole writer of plan state. That removes the sharpest hazard in the legacy runner, where the CONSUMER had to mutate plan.Status during the yield and an earlier version silently downgraded "Cancel the plan" to "skip". The consumer contract is preserved for now via a handshake: the runner completes the ack only after `yield return` resumes, which is precisely when the consumer's decision is visible. When the decision moves onto a RequestPort the handshake goes away. PlanRunnerBehaviorTests now runs every case against BOTH engines. While both are selectable, any divergence makes an A/B against a local model uninterpretable — a behavior difference would be indistinguishable from a model difference. The workflow engine therefore reproduces the legacy terminal-status quirk (a plan whose steps were all skipped after failures still reports Completed) rather than quietly fixing it; that belongs in the phase that retires the legacy runner. PlanRunnerSelector re-reads the config key per plan rather than resolving once, so `/config set planner workflow` takes effect on the next message without losing history — same session, same history, flip, re-run the same prompt. Graph-authoring facts verified against the real 1.19.0 assembly, since the docs were not sufficient: [SendsMessage] is mandatory and fails at runtime; unscoped state is executor-private (the first attempt reported "0 steps completed"); conditional AddEdge<T> overloads are mutually ambiguous; and Workflow.ToString() returns only the type name, so the first version of the topology test passed vacuously and was rewritten onto ReflectExecutors/ReflectEdges. Desktop still constructs TaskPlannerService directly, so it remains on the legacy engine until it opts in. Tests: 541 -> 556, green on net10.0 and net8.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…ents 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
"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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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/<leaf>-<hash>.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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
--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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
This finishes the first complete workflow-planner loop behind the existing
planner=workflowsetting./plan <goal>path, while keeping the current automatic planning behavior./plan-resumeand/plan-discard, including support for the Desktop resume card.v0.15.0.Why
The planner could propose and execute useful work, but it was too easy to lose progress, continue with a stale plan, or report success after only part of the work finished. These changes make the plan a durable workflow that users can inspect and steer before and during execution.
Rollout
The workflow planner remains opt-in. The legacy planner stays as the default until we finish the longer soak and token-usage checks.
The companion Desktop PR will pin this branch for review, then move to this PR's merge commit before the Desktop PR is merged.
Validation
MandoCode 0.15.0locally and ran the installed CLI's--doctorcheck on .NET 10No package, tag, or release is published by this PR.