mcp: record tool changesets as workspace patches - #14460

🤖 Handoff: benchmark results, revised plan, and next steps

Update (session 2), read this first. The original handoff below is still the reference. This update records decisions and what's left to do.

Decisions

1. Take over this PR and branch, and force-push the redesign.
2. Bundle the tooling (wcprof test capture and the benchmark harness) into this PR.
3. Parallelize with two staff workers, tooling and redesign.
4. Make no changes to engine/wcprof. This replaces section 3's phase-op idea:
   • no new op kind, and no wcprof.BeginOp phase markers;
   • every dagql call, including the internal srv.Select calls from core/mcp.go, is already recorded as a call op named after its field (Changeset.asPatch, Directory.withPatchFile, Workspace.withChanges, and later Workspace.__withPatch);
   • lazy work shows up where it's forced, as lazy/call_exec;
   • root reports (critpath/tree/breakdown) at the tool's call op;
   • drop the bench.* spans and the forced evaluation, and end each run with one read of the resulting workspace.

State

• Nothing is implemented or pushed yet. The head is still 592a560, 74 commits behind main.
• A trial rebase onto main (6dd48d4) had no conflicts. It was never pushed, so it needs to be redone.
• rebase needs a clean tree. Put any local dagger.toml/dagger.lock tool pins in a temporary commit, rebase, then soft-reset the temporary commit away. Never push those pins.

Staff breakage after reloading onto vito/agents#27

• Symptom: every staff call failed with convert field members: expected module type, got *hm.FunctionType.
• Cause: the old staff module stored the roster in a private map named members. Question about the UX for shell evaluation and strings #27 renames that map to agents and reuses members for the collection function. Reload carried the old key over, because overlayModuleObjectState in core/object_reload.go only warns about private keys, and the dang SDK then tried to decode it against the function.
• Workaround: start a fresh session.
• Fix for Question about the UX for shell evaluation and strings #27: in Staff.agent, use .withTools(tools, except: plumbing, version: 1).
• Possible engine follow-up: drop a carried private key whose name is now a function rather than a field.

Next steps

1. Check out vito/normalize-onto-workspace and rebase it onto main. Optionally, have a worker push the version: 1 fix to Address agent git history as workspace artifacts vito/agents#27.

2. Worker tooling:

   • engine-dev: add TestProfile next to Test. It starts the test engine with _DAGGER_WCPROF=1, runs go test, fetches http://daggerengine:6060/debug/wcprof/dump, and returns the dump even if the tests fail, along with the exit status and the tail of the output.
   • engine-lab: add engineTest(..., wcprofCapture: String! = ""), which saves the dump as a named capture so wcprofReport/compare work on test runs. Update the SKILL.md.
   • Harness: port TestBenchChangesetApply from 0149c9f, without core/mcp_bench.go, without core/mcp.go changes, and without forced evaluation. Give each scenario its own client where practical, keep the recipe stats, and gate the test out of CI.
   • Baselines: capture "pr" and "main". For "main", temporarily restore core/mcp.go from main.

3. Worker redesign: implement section 2, one commit per step, including the new tests:

   • a generator that copies the workspace with gitignore: true, run twice;
   • a workspace with a non-root cwd;
   • empty directories;
   • an error for paths under a mount.

   Then run TestLLM/, TestAgents/, TestAgentRestore/ and lint.

4. Harvest: cherry-pick both workers' commits from dag://staff/members/head?member=<name>. Capture "new" and compare it against "pr" and "main". Measure findRejectFiles on a larger tree, and settle the cwd question.

5. Publish: force-push with a lease on 592a560, unless the branch has moved since. Then rewrite the PR description.

From: https://dagger.cloud/dagger/traces/f307bb0061e85d1cff3f6b798016f9ef#bdcd2737767003df

This PR currently rebases a tool's changeset patch onto a sparse read of the workspace, checks the result against applying the raw changeset to that read, and falls back to patching onto Before. We benchmarked it and reworked the design in discussion. The plan below replaces most of what this PR does now. Nothing below is implemented on this branch yet.

1. Benchmark results

Setup: a value workspace of 5,000 files (22 MB) plus a 2.4 MB generated file. One tool call per run on a fresh workspace, with unique content per run so nothing hits the cache. Medians of 3 runs after a warmup, in ms per MCP.applyChangeset (normalization, no-op check and overlay, with each step forced to evaluate inside its span).

Where the time goes:

• Patch rendering (asPatch + blob): 55–85ms, or 460ms for vendor, in every variant. For small text changes the tool-result summary renders this same patch anyway (summarizePatch; the second render is a cache hit), so it isn't really normalization's cost.
• main: the overlay step (Workspace.withChanges of a changeset whose two sides are full trees) costs 215–306ms. Directory.withChanges walks both trees (ComputePaths) and builds a structural diff.
• This PR: the overlay drops to 5–12ms because both sides are sparse. The sparse read costs 11–17ms (45ms for vendor). But the check (base.withChanges(raw)) costs 100–180ms even for a one-file edit, because it needs a structural diff of the producer's full Before and After.
• Prototype: Changeset.PlanPatchRebase compares Before with the workspace at the changed paths straight from mounts, in 1–4ms.
• Recipe: both codegen scenarios on main keep the producer's container commands in the recorded recipe; this PR keeps only the workspace's own setup command.
• Recipe size: the vendor recipe is about 12 MB in every variant, because blob contents are stored inline in the call.

2. Revised plan (supersedes the current diff)

1. Host-backed workspaces: no normalization. The session isn't reproducible anyway, so normalizing only adds cost. Apply the raw changeset and keep only the cheap no-op check (PathCountExceeds(0)), so a successful no-op command still doesn't advance state. This drops the sparse host reads and every host special case.
2. Non-host workspaces: everything is already in the engine. No sparse reads, no extra layer just to select a subset of files.
3. Pure recipes: unwrap only. If Before/After contain only workspace reads and pure Directory/File operations (vito/editor's edit/mv/cp, withReplaced, withNewFile, …), record After.changes(from: Before). That's one call, just to drop the tool call (editor.edit(…)) from the recipe.
   • Decide this with a structural walk of the recipe, no evaluation. dagql/recipe_classification.go already does this kind of walk for non-replayable fields.
   • Use an allowlist: anything unrecognized (withExec, services, module functions, git/http fetches) takes the next path.
4. Expensive recipes (generators, commands): workspace + patch.
   • Render the patch against the workspace, not Before. A small core helper copies only the changed paths out of the in-engine workspace root and out of After, then runs git diff --binary (same approach as the staging in computeChangesetPathsDelta). Because the workspace's own content is the patch's starting point, applying it reproduces the raw changeset by construction, including overwrites where Before lacked the file. Example: a generator that copies the workspace with gitignore: true, run twice; a patch from Before would say "create file" and git apply hard-errors because the file exists.
   • Apply it with a new internal Workspace.__withPatch(patch: File!) that runs git apply directly on the workspace root. No changeset is built and no trees are compared. Empty-directory adds and removals go through the existing Workspace.withNewDirectory/withoutDirectory, or as arguments to the op. The recipe is then the prior workspace plus the blob.
   • No check and no fallbacks. Error only when a path is under a mount, as withChanges does today.
   • Show the model this patch in the tool result. It's what actually changed in its workspace, and it replaces the raw asPatch render, so there's no second render.
5. Remove from this PR: the sparse read, the target step, the extra-diff check, the Before fallback, and the PlanPatchRebase prototype.

Open questions and risks:

• git apply on the full root: with conflict markers enabled, applyGitPatch runs findRejectFiles, which walks the whole tree. It took 8–57ms on main's full Before. Measure it on a larger tree.
• Pure tools whose Before is the full root (e.g. ws.directory("/")) still pay the full-tree withChanges cost: about 215ms in the "edit" scenario on main. That's an engine issue: diffing a child snapshot against its parent shouldn't need a full walk. Fix it separately; don't route pure edits through the patch path.
• Recipe size from inline blob contents (12 MB for the vendor patch). Separate from this PR.
• cwd: vito/editor builds Before from source.directory("."), which is relative to the workspace cwd, while Workspace.withChanges applies at the workspace root. Check workspaces with a non-root cwd.
• Metadata: permission bits beyond the executable bit, and modes of new non-empty directories, don't survive a patch. Accepted as fine.

3. Next session, step 1: a wcprof feedback loop for tests and benchmarks

Today: engine-lab's wcprofEnable/wcprofCapture/wcprofReport only work against the engine started with start. engineTest (engine-dev's test, .dagger/modules/engine-dev/test.go, testContainer) starts its own engine. That engine exposes --debugaddr 0.0.0.0:6060, but nothing captures from it. Integration tests and benchmarks therefore can't feed wcprof, and the numbers above come from temporary OTel spans with forced evaluation, which distorts how lazy work is attributed.

Suggested shape:

• engine-dev: an opt-in to record wcprof on the test engine. Set _DAGGER_WCPROF=1 on the engine container, or POST on to /debug/wcprof/enabled before the tests. After go test, fetch http://<engine>:6060/debug/wcprof/dump from the test container and return it as a File, e.g. a testProfile(...) File next to test.
• engine-lab: something like engineTest(..., wcprofCapture: String = "") that adds the dump to wcprofCaptureNames/wcprofDumps under that name. wcprofReport (including view: "compare") then works on test runs, and captures survive across source edits, so a single session can capture main, this PR and the new design.
• Mark the code phases with wcprof ops (wcprof.BeginOp) instead of the bench.* OTel spans: applyChangeset, normalize, patch render, __withPatch, overlay. They cost nothing when profiling is off, so they can stay in the tree. A kind for code phases probably needs adding; the existing kinds are call/lazy/exec/session_phase/io/internal.
• Run benchmarks alone, since the recording is engine-wide; use the client filter or the phase ops to scope reports.

4. Reproducing the current benchmark

Each branch carries the same harness: core/mcp_bench.go (benchStep/benchEval helpers) and core/integration/llm_changeset_bench_test.go (TestLLM/TestBenchChangesetApply).

dagger api call engine-dev test --pkg ./core/integration \
  --run TestLLM/TestBenchChangesetApply --test-verbose

Then grep the test output for BENCH-MEDIAN (per-step medians) and BENCH scenario=… rep=3 (also prints recipe_bytes, withExec and withPatchFile counts). The full run takes about a minute after the first image pulls. Treat run-to-run noise as roughly ±10%. The withExec=1 in every recipe is the workspace's own setup command.