MUL-2784 fix(daemon): cleanup sidecar tree (.agent_context / .multica / provider skills) after local_directory tasks (#3444)

* fix(daemon): cleanup .agent_context / .multica / provider skill sidecars after local_directory tasks (MUL-2784) PR #3438 (MUL-2753) only restored CLAUDE.md / AGENTS.md / GEMINI.md to their pre-task bytes; the sidecar tree writeContextFiles seeds (.agent_context/, .multica/, .claude/skills/, .github/skills/, .opencode/skills/, skills/, .pi/skills/, .cursor/skills/, .kimi/skills/, .kiro/skills/, .agents/skills/, fallback .agent_context/skills/) was explicitly deferred to this follow-up. In local_directory mode the agent's workdir is the user's repo, so each task accumulates one more layer of those directories in the user's tree. Plan A: track every file/dir Prepare creates inside workDir in a sidecarManifest written to envRoot/.multica_sidecar_manifest.json (daemon scratch — never in the user's workdir). On local_directory teardown CleanupSidecars walks the manifest, removes the recorded files, then rmdir-iterates the recorded directories in reverse. Pre-existing files and directories are deliberately NOT recorded, so a user-installed .claude/skills/my-own-skill/ sibling — or any unrelated file the user keeps under .claude/, .github/, etc. — is preserved bit-for-bit. Non-empty rmdir fails ENOTEMPTY and is silently skipped, which is the signal that the user owns the directory. Daemon wiring lives next to the existing CleanupRuntimeConfig defer in runTask: runtime brief first, sidecars second. Cloud-mode runs still write a manifest for symmetry but never trigger the cleanup (the GC loop wipes envRoot wholesale). Tests (sidecar_manifest_test.go) cover the round-trip invariant per the issue's acceptance criteria: - empty workdir → Prepare → Cleanup → empty workdir, byte-exact, for every file-based provider (claude, codex, copilot, opencode, openclaw, hermes, pi, cursor, kimi, kiro, antigravity, gemini), - user's .claude/skills/my-own-skill/ (and equivalents per provider) survives Cleanup intact, - unrelated user files under .claude/, .github/, etc. survive, - three repeated cycles do not accumulate any orphan state, - project_resources branch (.multica/project/resources.json) is also reversible, - recordWriteFile refuses to record pre-existing files, - recordMkdirAll refuses to record pre-existing dirs, - Cleanup is a no-op when the manifest file is missing. Co-authored-by: multica-agent <github@multica.ai> * fix(daemon): refuse to overwrite pre-existing sidecar paths; pick collision-free skill slugs (MUL-2784 review) Addresses PR #3444 review (Elon): **Must-fix #1**: recordWriteFile used to overwrite pre-existing target files unconditionally and only skip the manifest record. That destroys user bytes at write time AND leaves the corrupted contents in place at cleanup time — the byte-exact contract the issue requires is violated on both halves. Fixed by making recordWriteFile detect any pre-existing entry (regular file, symlink, directory) via Lstat and return a sentinel errPathPreExists without touching the path. The user's bytes are preserved verbatim. For per-skill collisions (user's .claude/skills/issue-review/ vs Multica's "Issue Review"), writeSkillFiles now allocates a collision-free sibling slug via allocateCollisionFreeSkillDir: first attempt is the natural slug, then `<base>-multica`, `<base>-multica-2`, …, bounded at 64 attempts. Provider-native discovery still picks the skill up (every subdir under skillsParent is a distinct skill) and the user's path stays bit-for-bit intact. For Multica-only namespace files (.agent_context/issue_context.md, .multica/project/resources.json), the writer swallows errPathPreExists and continues — the runtime brief already carries every fact those files would, so a collision degrades to brief-only mode rather than destroying user content. **Must-fix #2**: Added byte-exact collision matrix tests covering every file-based provider (claude / codex / copilot / opencode / openclaw / hermes / pi / cursor / kimi / kiro / antigravity / gemini): - TestPrepareThenCleanupSidecarsSameSlugCollisionPerProvider: seeds user's `<provider>/skills/issue-review/SKILL.md` plus a private notes.md sibling, runs Prepare → Inject → Cleanup, asserts workdir snapshot is byte-identical to seed. - TestPrepareThenCleanupSidecarsIssueContextCollisionPerProvider: seeds user's `.agent_context/issue_context.md`, asserts round-trip preserves it. - TestPrepareThenCleanupSidecarsProjectResourcesCollisionPerProvider: same for `.multica/project/resources.json`. - TestPrepareThenCleanupSidecarsMultiSkillCollisionFreeAllocation: end-to-end check that the Multica skill lands at the collision-free sibling and Cleanup removes only the Multica side. - TestAllocateCollisionFreeSkillDir: directed unit test pinning the slug-bumping sequence. - TestRecordWriteFileRefusesToOverwritePreExistingFile (was TestRecordWriteFileSkipsPreExistingFile): flipped to assert the user's bytes survive and errPathPreExists is returned. - TestRecordWriteFileRefusesToOverwriteSymlinkOrDir: covers the Lstat path for non-file entries. **Should-fix**: CleanupSidecars used to swallow ANY non-ENOENT rmdir error as "user content present," silently dropping real I/O failures (EACCES, EPERM, EBUSY). Now it re-reads the directory after a failed rmdir via the new dirHasEntries helper — non-empty → silently skip (ENOTEMPTY, the intended branch); empty → genuine error, captured into firstErr and surfaced. Plus directed tests: - TestCleanupSidecarsSurfacesRealRmdirErrors - TestDirHasEntries Local verification: - go test ./internal/daemon/execenv/... — all green - go test ./internal/daemon/... — all green - go vet ./... — clean Co-authored-by: multica-agent <github@multica.ai> * fix(daemon): surface original rmdir error when post-rmdir ReadDir also fails (MUL-2784 review) Addresses remaining PR #3444 review blocker (Elon): dirHasEntries used to return true when ReadDir failed with anything other than ENOENT, which made CleanupSidecars treat every locked / faulted directory as ENOTEMPTY and silently drop the original rmdir error. The v1 fix from the previous round closed the EACCES-on-empty-dir branch but missed the case where the chmod also blocks ReadDir — exactly the failure mode the review called out. Helper change: dirHasEntries now returns (hasEntries, ok bool): - (false, true) — dir exists and is empty (or missing, race-safe) - (true, true) — dir has user content (the ENOTEMPTY branch) - (_, false) — ReadDir failed (EACCES, ENOTDIR, EIO, …); the caller cannot tell ENOTEMPTY from a real error and MUST surface the original rmdir error CleanupSidecars switches on (ok, hasEntries): - !ok → surface the ORIGINAL rmdir error (not the ReadDir failure — that's diagnostic plumbing and would distract from the root cause) - ok && hasEntries → swallow silently (intended ENOTEMPTY branch; preserve user content) - ok && !hasEntries → surface the rmdir error (empty dir + EACCES / EPERM / EBUSY → genuine cleanup failure) Tests: - TestDirHasEntries: extended with a regular-file sub-case (ReadDir returns ENOTDIR) asserting (false, false). The v1 helper returned (true) here, hiding the bug. - TestCleanupSidecarsSwallowsMissingAndNonEmptyDirs: renamed from TestCleanupSidecarsSurfacesRealRmdirErrors. The old name claimed to test the surfacing path but never actually exercised it. - TestCleanupSidecarsSurfacesEACCESOnEmptyRecordedDir: chmod parent to 0o555 so rmdir(recorded) fails EACCES while ReadDir(recorded) still succeeds (empty). Asserts firstErr is non-nil and references both the recorded path and the rmdir branch. Skipped when running as root (chmod is bypassed for uid 0). - TestCleanupSidecarsSurfacesEACCESWhenReadDirFailsToo: the must-fix case — chmod parent 0o555 AND chmod recorded 0o000 so BOTH rmdir and ReadDir fail. The surfaced error must be the ORIGINAL rmdir failure, not the ReadDir one. Skipped on uid 0. Local verification: - go test ./internal/daemon/execenv/... — all green - go test ./internal/daemon/... — all green - go vet ./... — clean Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: J <j@multica.ai> Co-authored-by: multica-agent <github@multica.ai>
2026-06-17 03:38:32 +02:00 · 2026-05-28 17:22:47 +08:00
parent 9b280bc631
commit ee4ec3b76d
7 changed files with 1631 additions and 43 deletions
--- a/server/internal/daemon/daemon.go
+++ b/server/internal/daemon/daemon.go
@@ -2600,6 +2600,17 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i
 			if cerr := execenv.CleanupRuntimeConfig(env.WorkDir, provider); cerr != nil {
 				d.logger.Warn("execenv: cleanup runtime config failed (non-fatal)", "error", cerr)
 			}
+			// Excise the sidecar tree (.agent_context/, .multica/,
+			// provider-specific .claude/skills/ etc.) that Prepare wrote
+			// into the user's repo. Without this pass the user's tree
+			// accumulates one directory layer per task — see MUL-2784.
+			// CleanupRuntimeConfig handles the runtime brief inside
+			// CLAUDE.md / AGENTS.md / GEMINI.md; CleanupSidecars handles
+			// every other file Prepare placed under WorkDir. Together
+			// they round-trip the workdir to its exact pre-task bytes.
+			if cerr := execenv.CleanupSidecars(env.RootDir); cerr != nil {
+				d.logger.Warn("execenv: cleanup sidecars failed (non-fatal)", "error", cerr)
+			}
 		}()
 	}

--- a/server/internal/daemon/execenv/context.go
+++ b/server/internal/daemon/execenv/context.go
@@ -2,8 +2,8 @@ package execenv

 import (
 	"encoding/json"
+	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -23,26 +23,43 @@ import (
 // Kiro:        skills → {workDir}/.kiro/skills/{name}/SKILL.md  (native discovery)
 // Antigravity: skills → {workDir}/.agents/skills/{name}/SKILL.md  (native discovery — see https://antigravity.google/docs/gcli-migration "Workspace skills")
 // Default:     skills → {workDir}/.agent_context/skills/{name}/SKILL.md
-func writeContextFiles(workDir, provider string, ctx TaskContextForEnv) error {
+//
+// manifest, when non-nil, is populated with every file we created and every
+// intermediate directory we had to MkdirAll (skipping any that pre-existed).
+// CleanupSidecars uses it to roll the workdir back to its pre-Prepare
+// state for local_directory tasks. Callers that don't need cleanup —
+// cloud-mode tasks whose envRoot is wiped wholesale by the GC loop — may
+// pass nil to skip the bookkeeping entirely.
+func writeContextFiles(workDir, provider string, ctx TaskContextForEnv, manifest *sidecarManifest) error {
 	contextDir := filepath.Join(workDir, ".agent_context")
-	if err := os.MkdirAll(contextDir, 0o755); err != nil {
+	if err := recordMkdirAll(contextDir, 0o755, manifest); err != nil {
 		return fmt.Errorf("create .agent_context dir: %w", err)
 	}

 	content := renderIssueContext(provider, ctx)
 	path := filepath.Join(contextDir, "issue_context.md")
-	if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
-		return fmt.Errorf("write issue_context.md: %w", err)
+	if err := recordWriteFile(path, []byte(content), 0o644, manifest); err != nil {
+		// A pre-existing path means the user already owns
+		// .agent_context/issue_context.md — either they created it
+		// themselves or it survived from a crashed prior run we can't
+		// safely distinguish from intentional content. Refusing the
+		// write is the correct call: the runtime brief (CLAUDE.md /
+		// AGENTS.md / GEMINI.md) already carries every fact this file
+		// would, so the agent runs fine without the sidecar copy.
+		// Anything else is a real failure.
+		if !errors.Is(err, errPathPreExists) {
+			return fmt.Errorf("write issue_context.md: %w", err)
+		}
 	}

 	if len(ctx.AgentSkills) > 0 {
-		skillsDir, err := resolveSkillsDir(workDir, provider)
+		skillsDir, err := resolveSkillsDir(workDir, provider, manifest)
 		if err != nil {
 			return fmt.Errorf("resolve skills dir: %w", err)
 		}
 		// Codex skills are written to codex-home in Prepare; skip here.
 		if provider != "codex" {
-			if err := writeSkillFiles(skillsDir, ctx.AgentSkills); err != nil {
+			if err := writeSkillFiles(skillsDir, ctx.AgentSkills, manifest); err != nil {
 				return fmt.Errorf("write skill files: %w", err)
 			}
 		}
@@ -52,7 +69,7 @@ func writeContextFiles(workDir, provider string, ctx TaskContextForEnv) error {
 	// block task startup. Missing resources surface as the agent simply not
 	// seeing the file, which matches the "scoped, not dumped" design (the
 	// meta skill content always lists what the agent should expect).
-	if err := writeProjectResources(workDir, ctx); err != nil {
+	if err := writeProjectResources(workDir, ctx, manifest); err != nil {
 		// Caller logs warnings; avoid noisy returns for non-fatal context.
 		return fmt.Errorf("write project resources: %w", err)
 	}
@@ -94,12 +111,16 @@ func (p ProjectResourceForEnv) MarshalJSON() ([]byte, error) {
 // working directory when the task carries project context. The file is
 // always written when a project is attached (even with zero resources) so
 // agents can rely on its presence as a signal that a project exists.
-func writeProjectResources(workDir string, ctx TaskContextForEnv) error {
+//
+// manifest, when non-nil, is populated with the .multica/project chain
+// of created directories and the resources.json file so CleanupSidecars
+// can undo them on local_directory teardown.
+func writeProjectResources(workDir string, ctx TaskContextForEnv, manifest *sidecarManifest) error {
 	if ctx.ProjectID == "" && len(ctx.ProjectResources) == 0 {
 		return nil
 	}
 	dir := filepath.Join(workDir, ".multica", "project")
-	if err := os.MkdirAll(dir, 0o755); err != nil {
+	if err := recordMkdirAll(dir, 0o755, manifest); err != nil {
 		return err
 	}
 	resources := ctx.ProjectResources
@@ -115,12 +136,24 @@ func writeProjectResources(workDir string, ctx TaskContextForEnv) error {
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(filepath.Join(dir, "resources.json"), data, 0o644)
+	if err := recordWriteFile(filepath.Join(dir, "resources.json"), data, 0o644, manifest); err != nil {
+		// .multica/project/resources.json is Multica-owned and a
+		// pre-existing path is almost certainly user content the
+		// manifest must not destroy. The runtime brief already lists
+		// every project resource so the agent runs fine without the
+		// JSON sidecar — collision degrades to brief-only mode.
+		if !errors.Is(err, errPathPreExists) {
+			return err
+		}
+	}
+	return nil
 }

 // resolveSkillsDir returns the directory where skills should be written
-// based on the agent provider.
-func resolveSkillsDir(workDir, provider string) (string, error) {
+// based on the agent provider. manifest, when non-nil, is populated with
+// every intermediate directory we had to MkdirAll so CleanupSidecars can
+// rmdir them on local_directory teardown.
+func resolveSkillsDir(workDir, provider string, manifest *sidecarManifest) (string, error) {
 	var skillsDir string
 	switch provider {
 	case "claude":
@@ -174,7 +207,7 @@ func resolveSkillsDir(workDir, provider string) (string, error) {
 		// Fallback: write to .agent_context/skills/ (referenced by meta config).
 		skillsDir = filepath.Join(workDir, ".agent_context", "skills")
 	}
-	if err := os.MkdirAll(skillsDir, 0o755); err != nil {
+	if err := recordMkdirAll(skillsDir, 0o755, manifest); err != nil {
 		return "", err
 	}
 	return skillsDir, nil
@@ -287,32 +320,57 @@ func sanitizeSkillName(name string) string {
 }

 // writeSkillFiles writes skill directories into the given parent directory.
-// Each skill gets its own subdirectory containing SKILL.md and supporting files.
-func writeSkillFiles(skillsDir string, skills []SkillContextForEnv) error {
-	if err := os.MkdirAll(skillsDir, 0o755); err != nil {
+// Each skill gets its own subdirectory containing SKILL.md and supporting
+// files. manifest, when non-nil, is populated with every newly-created
+// directory and file so CleanupSidecars can remove them on
+// local_directory teardown without touching user-owned skill directories
+// that happen to live alongside ours under the same skills/ parent.
+//
+// When a Multica skill's natural slug collides with a user-installed
+// skill at the same path, we allocate a collision-free sibling slug
+// (e.g. `issue-review-multica`) and write there instead. Provider-native
+// discovery still picks it up because every subdir under skillsDir is a
+// distinct skill; the user's original directory stays bit-for-bit
+// intact. Without this fallback writeSkillFiles would have to either
+// overwrite user bytes (the bug PR #3444 review caught) or skip the
+// skill entirely (which would silently drop a Multica skill the agent
+// expects to see).
+func writeSkillFiles(skillsDir string, skills []SkillContextForEnv, manifest *sidecarManifest) error {
+	if err := recordMkdirAll(skillsDir, 0o755, manifest); err != nil {
 		return fmt.Errorf("create skills dir: %w", err)
 	}

 	for _, skill := range skills {
-		slug := sanitizeSkillName(skill.Name)
-		dir := filepath.Join(skillsDir, slug)
-		if err := os.MkdirAll(dir, 0o755); err != nil {
+		baseSlug := sanitizeSkillName(skill.Name)
+		slug, dir, err := allocateCollisionFreeSkillDir(skillsDir, baseSlug)
+		if err != nil {
+			return fmt.Errorf("allocate skill dir for %q: %w", skill.Name, err)
+		}
+		if err := recordMkdirAll(dir, 0o755, manifest); err != nil {
 			return err
 		}

-		// Write main SKILL.md
+		// ensureSkillFrontmatter synthesises a `name:` value when the
+		// upstream skill is missing one. Use the chosen slug (which
+		// may differ from baseSlug on collision) so the YAML name
+		// matches the directory name; runtimes that key on either
+		// stay consistent.
 		body := ensureSkillFrontmatter(skill.Content, slug, skill.Description)
-		if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(body), 0o644); err != nil {
+		if err := recordWriteFile(filepath.Join(dir, "SKILL.md"), []byte(body), 0o644, manifest); err != nil {
 			return err
 		}

-		// Write supporting files
+		// Write supporting files. The skill directory is collision-
+		// free by construction, so a recordWriteFile collision under
+		// it would mean the skill's bundled files list two entries
+		// at the same path — that's an upstream data bug, not a
+		// user-content collision, and we surface it.
 		for _, f := range skill.Files {
 			fpath := filepath.Join(dir, f.Path)
-			if err := os.MkdirAll(filepath.Dir(fpath), 0o755); err != nil {
+			if err := recordMkdirAll(filepath.Dir(fpath), 0o755, manifest); err != nil {
 				return err
 			}
-			if err := os.WriteFile(fpath, []byte(f.Content), 0o644); err != nil {
+			if err := recordWriteFile(fpath, []byte(f.Content), 0o644, manifest); err != nil {
 				return err
 			}
 		}
--- a/server/internal/daemon/execenv/execenv.go
+++ b/server/internal/daemon/execenv/execenv.go
@@ -192,9 +192,19 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) {
 	}

 	// Write context files into workdir (skills go to provider-native paths).
-	if err := writeContextFiles(workDir, params.Provider, params.Task); err != nil {
+	// Track every file/dir we create in a manifest so CleanupSidecars can
+	// roll a local_directory workdir back to its pre-Prepare state. Cloud
+	// tasks don't need the manifest (the GC loop wipes envRoot wholesale),
+	// but we always write one — it's cheap, keeps Prepare/Reuse symmetric,
+	// and avoids a conditional that would silently disable cleanup if the
+	// local_directory detection logic ever drifts.
+	manifest := &sidecarManifest{}
+	if err := writeContextFiles(workDir, params.Provider, params.Task, manifest); err != nil {
 		return nil, fmt.Errorf("execenv: write context files: %w", err)
 	}
+	if err := writeSidecarManifest(envRoot, manifest); err != nil {
+		logger.Warn("execenv: write sidecar manifest failed (non-fatal)", "error", err)
+	}

 	// For Codex, set up a per-task CODEX_HOME seeded from ~/.codex/ with skills.
 	if params.Provider == "codex" {
@@ -271,10 +281,25 @@ func Reuse(params ReuseParams, logger *slog.Logger) *Environment {
 		logger:         logger,
 	}

-	// Refresh context files (issue_context.md, skills).
-	if err := writeContextFiles(params.WorkDir, params.Provider, params.Task); err != nil {
+	// Refresh context files (issue_context.md, skills). Reuse tracks a
+	// fresh manifest under env.RootDir so a later CleanupSidecars sees
+	// the up-to-date list of writes (an old manifest from a prior run
+	// would otherwise reference files this Reuse no longer creates). For
+	// local_directory tasks the daemon skips Reuse entirely (see
+	// daemon.runTask), but writing the manifest unconditionally keeps
+	// Prepare/Reuse symmetric so a future caller can rely on the
+	// manifest being current after either path. RootDir is empty on the
+	// legacy local_directory Reuse fallback — skip the persist in that
+	// case to avoid creating a stray manifest at the filesystem root.
+	manifest := &sidecarManifest{}
+	if err := writeContextFiles(params.WorkDir, params.Provider, params.Task, manifest); err != nil {
 		logger.Warn("execenv: refresh context files failed", "error", err)
 	}
+	if env.RootDir != "" {
+		if err := writeSidecarManifest(env.RootDir, manifest); err != nil {
+			logger.Warn("execenv: refresh sidecar manifest failed", "error", err)
+		}
+	}

 	// Restore CodexHome for Codex provider — the per-task codex-home directory
 	// lives alongside the workdir. Re-run prepareCodexHomeWithOpts to ensure
@@ -342,7 +367,11 @@ func hydrateCodexSkills(codexHome string, workspaceSkills []SkillContextForEnv,
 	if len(workspaceSkills) == 0 {
 		return nil
 	}
-	return writeSkillFiles(skillsDir, workspaceSkills)
+	// Codex skills live under env.RootDir/codex-home, which the GC loop
+	// (cloud) or env teardown (local_directory) wipes wholesale — they
+	// don't sit inside the user's workdir and don't need sidecar manifest
+	// tracking.
+	return writeSkillFiles(skillsDir, workspaceSkills, nil)
 }

 // GCMetaKind identifies which kind of parent record a task workdir belongs to.
--- a/server/internal/daemon/execenv/execenv_test.go
+++ b/server/internal/daemon/execenv/execenv_test.go
@@ -269,7 +269,7 @@ func TestProjectReposReplaceWorkspaceReposInMetaSkill(t *testing.T) {
 func TestWriteProjectResourcesSkippedWhenNone(t *testing.T) {
 	t.Parallel()
 	dir := t.TempDir()
-	if err := writeProjectResources(dir, TaskContextForEnv{}); err != nil {
+	if err := writeProjectResources(dir, TaskContextForEnv{}, nil); err != nil {
 		t.Fatalf("writeProjectResources: %v", err)
 	}
 	if _, err := os.Stat(filepath.Join(dir, ".multica", "project", "resources.json")); !os.IsNotExist(err) {
@@ -352,7 +352,7 @@ func TestWriteContextFiles(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "", ctx); err != nil {
+	if err := writeContextFiles(dir, "", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -405,7 +405,7 @@ func TestWriteContextFilesOmitsSkillsWhenEmpty(t *testing.T) {
 		IssueID: "minimal-issue-id",
 	}

-	if err := writeContextFiles(dir, "", ctx); err != nil {
+	if err := writeContextFiles(dir, "", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -435,7 +435,7 @@ func TestWriteContextFilesAutopilotRunOnly(t *testing.T) {
 		AutopilotSource:      "manual",
 	}

-	if err := writeContextFiles(dir, "", ctx); err != nil {
+	if err := writeContextFiles(dir, "", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -479,7 +479,7 @@ func TestWriteContextFilesClaudeNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "claude", ctx); err != nil {
+	if err := writeContextFiles(dir, "claude", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -757,7 +757,7 @@ func TestWriteContextFilesCopilotNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "copilot", ctx); err != nil {
+	if err := writeContextFiles(dir, "copilot", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -808,7 +808,7 @@ func TestWriteContextFilesOpencodeNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "opencode", ctx); err != nil {
+	if err := writeContextFiles(dir, "opencode", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -880,7 +880,7 @@ func TestWriteContextFilesPreservesExistingSkillFrontmatter(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "opencode", ctx); err != nil {
+	if err := writeContextFiles(dir, "opencode", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -915,7 +915,7 @@ func TestWriteContextFilesInjectsNameIntoNamelessFrontmatter(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "opencode", ctx); err != nil {
+	if err := writeContextFiles(dir, "opencode", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -953,7 +953,7 @@ func TestWriteContextFilesOpenclawNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "openclaw", ctx); err != nil {
+	if err := writeContextFiles(dir, "openclaw", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -993,7 +993,7 @@ func TestWriteContextFilesKiroNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "kiro", ctx); err != nil {
+	if err := writeContextFiles(dir, "kiro", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -1125,7 +1125,7 @@ func TestWriteContextFilesAntigravityNativeSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "antigravity", ctx); err != nil {
+	if err := writeContextFiles(dir, "antigravity", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

@@ -1550,7 +1550,7 @@ func TestWriteContextFilesHermesFallbackSkills(t *testing.T) {
 		},
 	}

-	if err := writeContextFiles(dir, "hermes", ctx); err != nil {
+	if err := writeContextFiles(dir, "hermes", ctx, nil); err != nil {
 		t.Fatalf("writeContextFiles failed: %v", err)
 	}

--- a/server/internal/daemon/execenv/openclaw_config_test.go
+++ b/server/internal/daemon/execenv/openclaw_config_test.go
@@ -464,7 +464,7 @@ func TestPrepareOpenclawSkillWriteMatchesScanPath(t *testing.T) {
 	if err := writeContextFiles(workDir, "openclaw", TaskContextForEnv{
 		IssueID:     "issue-1",
 		AgentSkills: skills,
-	}); err != nil {
+	}, nil); err != nil {
 		t.Fatalf("writeContextFiles: %v", err)
 	}

--- a/server/internal/daemon/execenv/sidecar_manifest.go
+++ b/server/internal/daemon/execenv/sidecar_manifest.go
@@ -0,0 +1,364 @@
+package execenv
+
+import (
+	"encoding/json"
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+)
+
+// sidecarManifestFile is the on-disk JSON Prepare writes into envRoot to
+// record every file and intermediate directory it created inside WorkDir.
+// CleanupSidecars reads it back to roll the workdir to its pre-Prepare
+// state. The file lives in envRoot (daemon scratch), never in WorkDir,
+// so a local_directory run does not litter the user's repo with the
+// bookkeeping file used to undo the litter.
+const sidecarManifestFile = ".multica_sidecar_manifest.json"
+
+// errPathPreExists is the sentinel recordWriteFile returns when the
+// target path already exists. The manifest contract is that we never
+// mutate paths we don't own: a pre-existing file belongs to the user
+// (or to stale state from a crashed prior run we cannot safely
+// distinguish from intentional user content) and the write must be
+// refused so cleanup can be a pure deletion of paths we created.
+//
+// Callers handle this in one of two ways:
+//
+//   - For per-skill directories the caller allocates a collision-free
+//     alternative slug (see allocateCollisionFreeSkillDir) and retries
+//     so the agent still discovers the Multica skill, just under a
+//     different directory name.
+//   - For Multica-only namespaces (.agent_context/issue_context.md,
+//     .multica/project/resources.json) the caller swallows the error
+//     and proceeds — the agent's runtime brief already carries every
+//     fact that would have appeared in those files, so missing-from-
+//     disk is degraded behavior, not failure.
+var errPathPreExists = errors.New("execenv: refuse to overwrite pre-existing path")
+
+// sidecarManifest records the filesystem mutations writeContextFiles and
+// its callees make inside the agent's WorkDir for a single task. The
+// manifest is the second half of the contract that makes local_directory
+// runs byte-exactly reversible:
+//
+//   - Files lists absolute paths of regular files we created. Files are
+//     recorded only after recordWriteFile has verified the target did
+//     NOT pre-exist; recordWriteFile refuses to overwrite a pre-existing
+//     path, so the manifest's existence rule and the write side's
+//     refuse-to-clobber rule are the same invariant viewed from two
+//     sides.
+//   - Dirs lists absolute paths of directories we created, in root-first
+//     creation order. Cleanup walks the list in reverse so deepest dirs
+//     get tried first; rmdir of a directory the user has populated since
+//     (e.g. .claude/skills/my-own-skill alongside our .claude/skills/
+//     issue-review) fails ENOTEMPTY and is skipped silently — the
+//     user's content is preserved without any per-dir bookkeeping. A
+//     directory is recorded only when it did NOT pre-exist for the same
+//     reason files are conditional.
+//
+// The manifest is intentionally minimal: it carries the paths needed to
+// reverse our writes and nothing else. It is not a log of every operation
+// and is not a substitute for the runtime config marker block, which has
+// its own dedicated round-trip mechanism in runtime_config.go (the brief
+// is appended to user-owned content rather than written into a new sidecar
+// directory).
+type sidecarManifest struct {
+	Files []string `json:"files,omitempty"`
+	Dirs  []string `json:"dirs,omitempty"`
+}
+
+// recordMkdirAll behaves like os.MkdirAll(path, perm) but additionally
+// records every parent directory it had to create (skipping any that
+// already existed) into m so CleanupSidecars can rmdir them later. The
+// recorded paths are appended in root-first order; Cleanup iterates in
+// reverse so the deepest directory is removed first.
+//
+// When m is nil this is identical to os.MkdirAll — the Reuse path uses
+// the nil mode because Reuse runs on cloud workdirs that the GC loop
+// wipes wholesale, so per-file cleanup is irrelevant and tracking the
+// dirs would just leave stale manifest bytes around.
+func recordMkdirAll(path string, perm os.FileMode, m *sidecarManifest) error {
+	if path == "" {
+		return os.MkdirAll(path, perm)
+	}
+	if m == nil {
+		return os.MkdirAll(path, perm)
+	}
+	// Walk leaf-first, collecting ancestors that don't currently exist.
+	// We stop at the first existing ancestor (or the filesystem root) so
+	// pre-existing user directories are never recorded — Cleanup must
+	// not rmdir a path the user owned before this task started.
+	var toCreate []string
+	cur := filepath.Clean(path)
+	for {
+		if _, err := os.Lstat(cur); err == nil {
+			break
+		} else if !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("stat ancestor %s: %w", cur, err)
+		}
+		toCreate = append(toCreate, cur)
+		parent := filepath.Dir(cur)
+		if parent == cur || parent == "." {
+			break
+		}
+		cur = parent
+	}
+	if err := os.MkdirAll(path, perm); err != nil {
+		return err
+	}
+	// Reverse leaf-first → root-first so Cleanup can reverse-iterate
+	// to peel directories from the leaves upward.
+	for i, j := 0, len(toCreate)-1; i < j; i, j = i+1, j-1 {
+		toCreate[i], toCreate[j] = toCreate[j], toCreate[i]
+	}
+	m.Dirs = append(m.Dirs, toCreate...)
+	return nil
+}
+
+// recordWriteFile writes data to path with perm and records the path in
+// m for later cleanup, but ONLY when path does not already exist. When
+// path is occupied — by a regular file, a symlink, a directory, or any
+// other filesystem entry — the function returns errPathPreExists
+// without touching the path. The user's bytes (or pre-existing entry
+// type) are preserved exactly.
+//
+// This is the invariant the manifest design rests on: cleanup is a
+// pure deletion of paths we created, never a restore. Overwriting a
+// pre-existing path and then refusing to delete it on cleanup (the
+// pre-fix behavior) destroys user data twice — once at write time and
+// once by leaving the corrupted bytes in place at exit. Refusing to
+// overwrite removes both halves of that failure mode.
+//
+// When m is nil this collapses to a plain os.WriteFile — the Reuse
+// path uses the nil mode because Reuse runs on cloud workdirs that
+// the GC loop wipes wholesale, so per-file collision avoidance is
+// irrelevant.
+func recordWriteFile(path string, data []byte, perm os.FileMode, m *sidecarManifest) error {
+	if m == nil {
+		return os.WriteFile(path, data, perm)
+	}
+	_, statErr := os.Lstat(path)
+	if statErr == nil {
+		// Any existing entry — regular file, symlink, directory —
+		// is a collision. Refuse to touch it.
+		return fmt.Errorf("%w: %s", errPathPreExists, path)
+	}
+	if !errors.Is(statErr, fs.ErrNotExist) {
+		return fmt.Errorf("stat target %s: %w", path, statErr)
+	}
+	if err := os.WriteFile(path, data, perm); err != nil {
+		return err
+	}
+	m.Files = append(m.Files, path)
+	return nil
+}
+
+// allocateCollisionFreeSkillDir picks a directory under skillsParent
+// whose path does NOT currently exist, so writeSkillFiles can lay
+// down a Multica skill without colliding with a user-installed skill
+// of the same slug. The first attempt is always the natural baseSlug
+// — that's the path provider-native discovery already knows. On
+// collision we append `-multica`, then `-multica-2`, `-multica-3`,
+// … until a free slot is found. The chosen slug is returned alongside
+// the absolute path so callers can use it in frontmatter and brief
+// listings.
+//
+// The collision-free fallback name is still a sibling under the same
+// skillsParent, so provider-native discovery still picks the skill up
+// (each subdir under .claude/skills/ etc. is scanned independently).
+// The user's directory at baseSlug is left bit-for-bit intact.
+//
+// The probe is bounded to a small ceiling — a user with thousands of
+// collisions on the same slug indicates an upstream bug, not a
+// realistic state. Returning an error in that case forces the caller
+// to surface the problem instead of looping forever.
+func allocateCollisionFreeSkillDir(skillsParent, baseSlug string) (slug, dir string, err error) {
+	const maxAttempts = 64
+	for i := 0; i < maxAttempts; i++ {
+		var candidate string
+		switch {
+		case i == 0:
+			candidate = baseSlug
+		case i == 1:
+			candidate = baseSlug + "-multica"
+		default:
+			candidate = fmt.Sprintf("%s-multica-%d", baseSlug, i)
+		}
+		path := filepath.Join(skillsParent, candidate)
+		if _, statErr := os.Lstat(path); statErr != nil {
+			if errors.Is(statErr, fs.ErrNotExist) {
+				return candidate, path, nil
+			}
+			return "", "", fmt.Errorf("stat candidate %s: %w", path, statErr)
+		}
+	}
+	return "", "", fmt.Errorf("allocate collision-free skill dir under %s: exhausted %d attempts for base %q", skillsParent, maxAttempts, baseSlug)
+}
+
+// writeSidecarManifest persists m to {envRoot}/{sidecarManifestFile}.
+// Empty manifests are still written so a later Cleanup that finds the
+// file knows tracking was attempted (vs. an old build that predates this
+// mechanism, where the file is absent and Cleanup must no-op). Failures
+// are returned to the caller; the caller treats them as non-fatal because
+// a missed manifest only degrades local_directory cleanup, not task
+// execution.
+func writeSidecarManifest(envRoot string, m *sidecarManifest) error {
+	if envRoot == "" {
+		return nil
+	}
+	if m == nil {
+		m = &sidecarManifest{}
+	}
+	data, err := json.Marshal(m)
+	if err != nil {
+		return fmt.Errorf("marshal sidecar manifest: %w", err)
+	}
+	return os.WriteFile(filepath.Join(envRoot, sidecarManifestFile), data, 0o644)
+}
+
+// CleanupSidecars rolls the user's workdir back to its pre-Prepare
+// state by removing every file the manifest at envRoot records and
+// then rmdir-ing every directory it records, deepest first.
+//
+// Two failure modes the function deliberately swallows:
+//
+//   - ENOENT on a recorded path. The file or directory was already
+//     gone — either the user removed it during the task, or a prior
+//     Cleanup run on the same envRoot already cleared it. Either
+//     way there is nothing left for this call to do.
+//   - Non-empty directory on rmdir. The user has populated a
+//     directory we created (added a sibling file under .claude/
+//     skills/, for example) and rmdir-ing would destroy that
+//     content. We detect this by re-reading the directory after
+//     rmdir fails: a non-empty listing means "user owns this — stop
+//     here." This is the must-fix from PR #3444 review — the
+//     previous version swallowed ANY non-ENOENT rmdir error as
+//     "non-empty," which silently dropped real I/O failures
+//     (EACCES, EPERM, EBUSY) and made cleanup look successful when
+//     it wasn't.
+//
+// All other errors — ReadFile failure, JSON parse failure, real
+// EACCES/EPERM/EIO during file deletion, real EACCES/EPERM/EIO
+// during dir removal — are captured into firstErr and surfaced to
+// the caller. Cleanup still continues for the remaining manifest
+// entries so a single bad path does not strand the rest of the
+// rollback.
+//
+// The function is a no-op when:
+//   - envRoot is empty (no daemon scratch for this task),
+//   - the manifest file is missing (older build, or Prepare did not run).
+//
+// Pair this with CleanupRuntimeConfig on the local_directory cleanup
+// path: that function handles the runtime brief inside CLAUDE.md /
+// AGENTS.md / GEMINI.md, this one handles the sidecar tree
+// (.agent_context/, .multica/, .claude/skills/, .github/skills/,
+// .opencode/skills/, skills/, .pi/skills/, .cursor/skills/,
+// .kimi/skills/, .kiro/skills/, .agents/skills/, fallback
+// .agent_context/skills/). The two together restore the workdir to
+// byte-exact pre-task state.
+func CleanupSidecars(envRoot string) error {
+	if envRoot == "" {
+		return nil
+	}
+	manifestPath := filepath.Join(envRoot, sidecarManifestFile)
+	data, err := os.ReadFile(manifestPath)
+	if errors.Is(err, fs.ErrNotExist) {
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("read sidecar manifest %s: %w", manifestPath, err)
+	}
+	var m sidecarManifest
+	if err := json.Unmarshal(data, &m); err != nil {
+		return fmt.Errorf("parse sidecar manifest %s: %w", manifestPath, err)
+	}
+
+	var firstErr error
+	captureErr := func(err error) {
+		if firstErr == nil {
+			firstErr = err
+		}
+	}
+
+	for _, f := range m.Files {
+		if err := os.Remove(f); err != nil && !errors.Is(err, fs.ErrNotExist) {
+			captureErr(fmt.Errorf("remove %s: %w", f, err))
+		}
+	}
+
+	// Reverse iterate so the deepest directory is tried first. When
+	// rmdir fails we re-read the directory to tell ENOTEMPTY (user
+	// content present — skip silently) apart from real I/O errors
+	// (permission denied, busy, etc. — capture and surface).
+	for i := len(m.Dirs) - 1; i >= 0; i-- {
+		d := m.Dirs[i]
+		err := os.Remove(d)
+		if err == nil || errors.Is(err, fs.ErrNotExist) {
+			continue
+		}
+		hasEntries, ok := dirHasEntries(d)
+		switch {
+		case !ok:
+			// ReadDir also failed — we can't tell ENOTEMPTY apart
+			// from a real I/O error. Surface the ORIGINAL rmdir
+			// error (not the ReadDir failure) so the operator sees
+			// the actual cleanup blocker; the ReadDir branch is
+			// just diagnostic plumbing and would distract from the
+			// root cause. Silently skipping here was the v1 bug:
+			// it hid EACCES on locked directories behind a phantom
+			// "directory non-empty" assumption.
+			captureErr(fmt.Errorf("rmdir %s: %w", d, err))
+		case hasEntries:
+			// User has populated this dir since Prepare ran. Leave
+			// it in place without surfacing the rmdir error — the
+			// whole point of the manifest design is to preserve
+			// user content under directories we created.
+		default:
+			// Empty directory but rmdir still failed → real I/O
+			// error (EACCES, EPERM, EBUSY, EIO, or a directory we
+			// mistakenly recorded that we don't actually own).
+			// Surface it so the caller can log a warning and an
+			// operator can investigate.
+			captureErr(fmt.Errorf("rmdir %s: %w", d, err))
+		}
+	}
+
+	if err := os.Remove(manifestPath); err != nil && !errors.Is(err, fs.ErrNotExist) {
+		captureErr(fmt.Errorf("remove manifest %s: %w", manifestPath, err))
+	}
+
+	return firstErr
+}
+
+// dirHasEntries inspects dir and reports whether it currently contains
+// any entries. The second return value distinguishes three states
+// CleanupSidecars must handle separately:
+//
+//   - (false, true) — dir exists and is empty, OR dir disappeared
+//     between the failed rmdir and our readdir (the race collapses
+//     into "empty" so cleanup keeps moving). When paired with a
+//     non-ENOENT rmdir failure in CleanupSidecars this is the
+//     "empty + rmdir refused" branch — a real I/O error that gets
+//     surfaced.
+//   - (true, true) — dir has user content. When paired with a rmdir
+//     failure this is the intended ENOTEMPTY branch — skip silently
+//     so the user's content is preserved.
+//   - (_, false) — readdir failed with a real I/O error (EACCES on a
+//     chmod'd dir, ENOTDIR on a recorded path that isn't actually a
+//     dir, EIO on a hardware fault, etc.). The caller cannot tell
+//     ENOTEMPTY from a real failure and MUST surface the original
+//     rmdir error instead of silently skipping. The v1 of this
+//     helper returned `true` here, which made CleanupSidecars treat
+//     every readdir failure as "user content present" and hid the
+//     underlying rmdir error.
+func dirHasEntries(dir string) (hasEntries bool, ok bool) {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return false, true
+		}
+		return false, false
+	}
+	return len(entries) > 0, true
+}
--- a/server/internal/daemon/execenv/sidecar_manifest_test.go
+++ b/server/internal/daemon/execenv/sidecar_manifest_test.go