fix(execenv): reclaim managed skill dirs on reuse so stray agent files can't reopen the suffix bug (#3684)

Review follow-up on #3716. CleanupSidecars deletes the recorded SKILL.md but
preserves a managed skill directory once it is non-empty (the right call when
rolling a local_directory workdir back to pre-task state, where the agent may
have added content under a dir we created). On the reuse path that preservation
left the canonical slug occupied whenever a prior-run agent wrote a file into
.claude/skills/issue-review/, so the refresh still dodged to
issue-review-multica.

Add removeReusedManagedSkillDirs: before CleanupSidecars on the reuse path,
force-remove the skill directories the prior manifest recorded directly under
the provider's skills parent -- they are platform-owned by definition (the
manifest is proof we created them), so reclaiming them and re-creating each
skill at its natural slug is correct and matches the Codex path's
os.RemoveAll(skillsDir). Only immediate children of the skills parent are
removed, so sibling skills, checked-out repos, and the rest of the workdir are
untouched; the reuse path only runs on cloud workdirs (the daemon skips Reuse
for local_directory tasks) so there is no user-owned skills tree to protect.

Extract skillsDirPath (side-effect-free) from resolveSkillsDir so the rollback
can match the manifest's skill roots without re-creating directories.

Tests: TestReuseReclaimsManagedSkillDirWithStrayAgentFile reproduces the
review's exact repro through real Prepare+Reuse (without the fix:
[issue-review issue-review-multica]; with it: [issue-review]).
TestReuseSkillRefreshIsCanonicalAcrossProviders table-tests the rollback +
refresh sequence across claude/openclaw/copilot/default.

MUL-2963

Co-authored-by: multica-agent <github@multica.ai>

server/internal/daemon/execenv/context.go

@@ -152,21 +152,33 @@ func writeProjectResources(workDir string, ctx TaskContextForEnv, manifest *side
 }
 
 // resolveSkillsDir returns the directory where skills should be written
-// 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.
+// based on the agent provider, creating it. 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
+	skillsDir := skillsDirPath(workDir, provider)
+	if err := recordMkdirAll(skillsDir, 0o755, manifest); err != nil {
+		return "", err
+	}
+	return skillsDir, nil
+}
+
+// skillsDirPath returns the provider-native skills parent directory under
+// workDir WITHOUT creating it or recording anything. resolveSkillsDir wraps
+// this with the MkdirAll/manifest bookkeeping; the reuse-path skill rollback
+// (removeReusedManagedSkillDirs) needs the bare path with no side effects so
+// it can match the managed skill roots the prior manifest recorded.
+func skillsDirPath(workDir, provider string) string {
 	switch provider {
 	case "claude":
 		// Claude Code natively discovers skills from .claude/skills/ in the workdir.
-		skillsDir = filepath.Join(workDir, ".claude", "skills")
+		return filepath.Join(workDir, ".claude", "skills")
 	case "copilot":
 		// GitHub Copilot CLI natively discovers project-level skills from
 		// .github/skills/<name>/SKILL.md (takes precedence over user-level
 		// skills in ~/.copilot/skills/).
 		// See: https://docs.github.com/en/copilot/reference/copilot-cli-reference/cli-config-dir-reference
-		skillsDir = filepath.Join(workDir, ".github", "skills")
+		return filepath.Join(workDir, ".github", "skills")
 	case "opencode":
 		// OpenCode natively discovers project skills from .opencode/skills/ in
 		// the workdir. ConfigPaths.directories() walks up from the discovery
@@ -176,7 +188,7 @@ func resolveSkillsDir(workDir, provider string, manifest *sidecarManifest) (stri
 		// `opencode run --dir <workDir>` + PWD override in opencodeBackend —
 		// without those, OpenCode walks from the daemon's inherited PWD and
 		// misses .opencode/skills + AGENTS.md entirely (MUL-2416).
-		skillsDir = filepath.Join(workDir, ".opencode", "skills")
+		return filepath.Join(workDir, ".opencode", "skills")
 	case "openclaw":
 		// OpenClaw's native skill scanner reads <workspaceDir>/skills/. The
 		// daemon pairs this with a per-task synthesized openclaw-config.json
@@ -184,35 +196,31 @@ func resolveSkillsDir(workDir, provider string, manifest *sidecarManifest) (stri
 		// workDir, so writing here is what the CLI actually scans. Before
 		// MUL-2219 this used to fall back to .agent_context/skills/, which
 		// no openclaw scan path ever inspected.
-		skillsDir = filepath.Join(workDir, "skills")
+		return filepath.Join(workDir, "skills")
 	case "pi":
 		// Pi natively discovers skills from .pi/skills/ in the workdir.
-		skillsDir = filepath.Join(workDir, ".pi", "skills")
+		return filepath.Join(workDir, ".pi", "skills")
 	case "cursor":
 		// Cursor natively discovers skills from .cursor/skills/ in the workdir.
-		skillsDir = filepath.Join(workDir, ".cursor", "skills")
+		return filepath.Join(workDir, ".cursor", "skills")
 	case "kimi":
 		// Kimi Code CLI auto-discovers project-level skills from .kimi/skills/
 		// in the workdir. See https://moonshotai.github.io/kimi-cli/en/customization/skills.html
-		skillsDir = filepath.Join(workDir, ".kimi", "skills")
+		return filepath.Join(workDir, ".kimi", "skills")
 	case "kiro":
 		// Kiro CLI auto-discovers project-level skills from .kiro/skills/
 		// in the workdir.
-		skillsDir = filepath.Join(workDir, ".kiro", "skills")
+		return filepath.Join(workDir, ".kiro", "skills")
 	case "antigravity":
 		// Antigravity (`agy`) auto-discovers workspace-level skills from
 		// .agents/skills/ in the workdir. The CLI inherits Gemini CLI's
 		// workspace skill layout; see https://antigravity.google/docs/gcli-migration
 		// under "Workspace skills".
-		skillsDir = filepath.Join(workDir, ".agents", "skills")
+		return filepath.Join(workDir, ".agents", "skills")
 	default:
 		// Fallback: write to .agent_context/skills/ (referenced by meta config).
-		skillsDir = filepath.Join(workDir, ".agent_context", "skills")
+		return filepath.Join(workDir, ".agent_context", "skills")
 	}
-	if err := recordMkdirAll(skillsDir, 0o755, manifest); err != nil {
-		return "", err
-	}
-	return skillsDir, nil
 }
 
 var nonAlphaNum = regexp.MustCompile(`[^a-z0-9]+`)

server/internal/daemon/execenv/execenv.go

@@ -295,6 +295,40 @@ func Reuse(params ReuseParams, logger *slog.Logger) *Environment {
 		logger:         logger,
 	}
 
+	// Roll back the previous dispatch's sidecar writes before refreshing.
+	// On reuse the workdir still holds the prior run's issue_context.md and
+	// skill directories; without clearing them first, writeSkillFiles sees
+	// its own earlier output occupying the canonical slug and falls back to
+	// a collision-free sibling (issue-review, issue-review-multica,
+	// issue-review-multica-2, …), accumulating a fresh duplicate on every
+	// re-dispatch to the same issue. allocateCollisionFreeSkillDir exists to
+	// dodge *user*-owned skill dirs (the local_directory flow), not our own
+	// prior writes, so we undo them via the prior manifest first and let the
+	// refresh below re-create each skill at its natural slug. This also brings
+	// the standard providers in line with the Codex path, where
+	// hydrateCodexSkills already wipes its skills dir before re-hydrating.
+	//
+	// Two steps, in order:
+	//   1. removeReusedManagedSkillDirs reclaims the platform's own skill
+	//      directories even when a prior-run agent left a file inside one.
+	//      CleanupSidecars alone can't do this — it preserves any recorded dir
+	//      the agent populated (correct on the local_directory teardown path),
+	//      which would otherwise keep the canonical slug occupied and push the
+	//      refresh back to issue-review-multica.
+	//   2. CleanupSidecars rolls back the remaining sidecar files
+	//      (issue_context.md, project resources) and the manifest itself.
+	//
+	// No-op when RootDir is empty (legacy local_directory reuse, which the
+	// daemon skips anyway) or when no prior manifest exists (older build).
+	if env.RootDir != "" {
+		if err := removeReusedManagedSkillDirs(env.RootDir, skillsDirPath(params.WorkDir, params.Provider)); err != nil {
+			logger.Warn("execenv: reclaim managed skill dirs on reuse failed", "error", err)
+		}
+		if err := CleanupSidecars(env.RootDir); err != nil {
+			logger.Warn("execenv: roll back prior sidecars on reuse failed", "error", err)
+		}
+	}
+
 	// 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

server/internal/daemon/execenv/execenv_test.go

@@ -512,6 +512,222 @@ func TestWriteContextFilesClaudeNativeSkills(t *testing.T) {
 	}
 }
 
+// TestReuseRefreshesSkillsWithoutDuplicating is the regression guard for
+// GitHub #3684: re-dispatching the same agent on the same issue goes through
+// the Reuse path, which must refresh skills in place rather than pile up
+// collision-free duplicates (issue-review, issue-review-multica,
+// issue-review-multica-2, …). Reuse rolls back the prior dispatch's writes
+// via its sidecar manifest before re-writing, so each skill lands at its
+// natural slug on every dispatch instead of dodging its own prior output.
+func TestReuseRefreshesSkillsWithoutDuplicating(t *testing.T) {
+	t.Parallel()
+
+	workspacesRoot := t.TempDir()
+	task := TaskContextForEnv{
+		IssueID: "reuse-skill-dedup",
+		AgentSkills: []SkillContextForEnv{
+			{Name: "Issue Review", Content: "Review the issue."},
+		},
+	}
+
+	env, err := Prepare(PrepareParams{
+		WorkspacesRoot: workspacesRoot,
+		WorkspaceID:    "ws-reuse-dedup",
+		TaskID:         "11112222-3333-4444-5555-666677778888",
+		Provider:       "claude",
+		Task:           task,
+	}, testLogger())
+	if err != nil {
+		t.Fatalf("Prepare failed: %v", err)
+	}
+	defer env.Cleanup(true)
+
+	skillsDir := filepath.Join(env.WorkDir, ".claude", "skills")
+
+	// Re-dispatch twice on the same persistent workdir.
+	for i := 0; i < 2; i++ {
+		if reused := Reuse(ReuseParams{
+			WorkDir:  env.WorkDir,
+			Provider: "claude",
+			Task:     task,
+		}, testLogger()); reused == nil {
+			t.Fatalf("Reuse #%d returned nil", i+1)
+		}
+	}
+
+	entries, err := os.ReadDir(skillsDir)
+	if err != nil {
+		t.Fatalf("read skills dir: %v", err)
+	}
+	var names []string
+	for _, e := range entries {
+		names = append(names, e.Name())
+	}
+	if len(names) != 1 || names[0] != "issue-review" {
+		t.Fatalf("after re-dispatch the skills dir = %v, want exactly [issue-review] with no -multica duplicates", names)
+	}
+
+	// The surviving skill keeps its natural slug in frontmatter, so the agent
+	// invokes `issue-review` and not a suffixed copy.
+	body, err := os.ReadFile(filepath.Join(skillsDir, "issue-review", "SKILL.md"))
+	if err != nil {
+		t.Fatalf("read SKILL.md: %v", err)
+	}
+	if !strings.Contains(string(body), "name: issue-review") {
+		t.Errorf("SKILL.md frontmatter should pin name: issue-review; got:\n%s", body)
+	}
+}
+
+// TestReuseReclaimsManagedSkillDirWithStrayAgentFile covers the edge case the
+// #3716 review surfaced: a prior-dispatch agent writes a file into the
+// platform's managed skill directory. CleanupSidecars on its own would keep
+// that now-non-empty directory, leaving the canonical slug occupied so the
+// next refresh dodges to issue-review-multica. Reuse must reclaim the
+// platform-owned skill directory so the refreshed skill stays at its natural
+// slug.
+func TestReuseReclaimsManagedSkillDirWithStrayAgentFile(t *testing.T) {
+	t.Parallel()
+
+	workspacesRoot := t.TempDir()
+	task := TaskContextForEnv{
+		IssueID: "reuse-stray-file",
+		AgentSkills: []SkillContextForEnv{
+			{Name: "Issue Review", Content: "Review the issue."},
+		},
+	}
+
+	env, err := Prepare(PrepareParams{
+		WorkspacesRoot: workspacesRoot,
+		WorkspaceID:    "ws-reuse-stray",
+		TaskID:         "aaaabbbb-cccc-dddd-eeee-ffff00001111",
+		Provider:       "claude",
+		Task:           task,
+	}, testLogger())
+	if err != nil {
+		t.Fatalf("Prepare failed: %v", err)
+	}
+	defer env.Cleanup(true)
+
+	skillsDir := filepath.Join(env.WorkDir, ".claude", "skills")
+
+	// Prior-run agent drops scratch inside the managed skill directory.
+	stray := filepath.Join(skillsDir, "issue-review", "agent-notes.md")
+	if err := os.WriteFile(stray, []byte("agent scratch"), 0o644); err != nil {
+		t.Fatalf("seed stray agent file: %v", err)
+	}
+
+	if reused := Reuse(ReuseParams{
+		WorkDir:  env.WorkDir,
+		Provider: "claude",
+		Task:     task,
+	}, testLogger()); reused == nil {
+		t.Fatal("Reuse returned nil")
+	}
+
+	entries, err := os.ReadDir(skillsDir)
+	if err != nil {
+		t.Fatalf("read skills dir: %v", err)
+	}
+	var names []string
+	for _, e := range entries {
+		names = append(names, e.Name())
+	}
+	if len(names) != 1 || names[0] != "issue-review" {
+		t.Fatalf("after reuse with a stray agent file the skills dir = %v, want exactly [issue-review] with no -multica duplicate", names)
+	}
+
+	// The managed skill dir is platform-owned: reclaiming it drops the agent's
+	// stray scratch (matching the Codex path) and re-creates a clean SKILL.md.
+	if _, err := os.Stat(stray); !os.IsNotExist(err) {
+		t.Errorf("expected stray file under the managed skill dir to be reclaimed; stat err = %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(skillsDir, "issue-review", "SKILL.md")); err != nil {
+		t.Errorf("expected a refreshed SKILL.md at the canonical slug: %v", err)
+	}
+}
+
+// TestReuseSkillRefreshIsCanonicalAcrossProviders exercises the reuse skill
+// rollback (removeReusedManagedSkillDirs + CleanupSidecars + writeContextFiles
+// — the exact sequence Reuse runs) directly across the file-based providers,
+// including the stray-agent-file boundary. Driving the sequence rather than
+// full Reuse avoids the per-provider config setup (codex-home, openclaw
+// binary) while still covering each provider's skills-dir layout.
+func TestReuseSkillRefreshIsCanonicalAcrossProviders(t *testing.T) {
+	t.Parallel()
+
+	for _, provider := range []string{"claude", "openclaw", "copilot", ""} {
+		provider := provider
+		name := provider
+		if name == "" {
+			name = "default"
+		}
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			workDir := t.TempDir()
+			envRoot := t.TempDir()
+			task := TaskContextForEnv{
+				IssueID: "reuse-table",
+				AgentSkills: []SkillContextForEnv{
+					{Name: "Issue Review", Content: "v1"},
+				},
+			}
+
+			// First dispatch: write context + persist the manifest.
+			m1 := &sidecarManifest{}
+			if err := writeContextFiles(workDir, provider, task, m1); err != nil {
+				t.Fatalf("first writeContextFiles: %v", err)
+			}
+			if err := writeSidecarManifest(envRoot, m1); err != nil {
+				t.Fatalf("persist manifest: %v", err)
+			}
+
+			skillsDir := skillsDirPath(workDir, provider)
+			stray := filepath.Join(skillsDir, "issue-review", "agent-notes.md")
+			if err := os.WriteFile(stray, []byte("scratch"), 0o644); err != nil {
+				t.Fatalf("seed stray file: %v", err)
+			}
+
+			// Second dispatch: same rollback + refresh sequence Reuse runs.
+			task.AgentSkills[0].Content = "v2"
+			if err := removeReusedManagedSkillDirs(envRoot, skillsDirPath(workDir, provider)); err != nil {
+				t.Fatalf("removeReusedManagedSkillDirs: %v", err)
+			}
+			if err := CleanupSidecars(envRoot); err != nil {
+				t.Fatalf("CleanupSidecars: %v", err)
+			}
+			m2 := &sidecarManifest{}
+			if err := writeContextFiles(workDir, provider, task, m2); err != nil {
+				t.Fatalf("second writeContextFiles: %v", err)
+			}
+			if err := writeSidecarManifest(envRoot, m2); err != nil {
+				t.Fatalf("persist manifest #2: %v", err)
+			}
+
+			entries, err := os.ReadDir(skillsDir)
+			if err != nil {
+				t.Fatalf("read skills dir: %v", err)
+			}
+			var names []string
+			for _, e := range entries {
+				names = append(names, e.Name())
+			}
+			if len(names) != 1 || names[0] != "issue-review" {
+				t.Fatalf("skills dir = %v, want exactly [issue-review]", names)
+			}
+			if _, err := os.Stat(stray); !os.IsNotExist(err) {
+				t.Errorf("stray agent file should be reclaimed; stat err = %v", err)
+			}
+			body, err := os.ReadFile(filepath.Join(skillsDir, "issue-review", "SKILL.md"))
+			if err != nil {
+				t.Fatalf("read refreshed SKILL.md: %v", err)
+			}
+			if !strings.Contains(string(body), "v2") {
+				t.Errorf("SKILL.md should carry refreshed content v2; got:\n%s", body)
+			}
+		})
+	}
+}
+
 func TestCleanupPreservesLogs(t *testing.T) {
 	t.Parallel()
 	workspacesRoot := t.TempDir()

server/internal/daemon/execenv/sidecar_manifest.go

@@ -331,6 +331,63 @@ func CleanupSidecars(envRoot string) error {
 	return firstErr
 }
 
+// removeReusedManagedSkillDirs force-removes the skill directories the prior
+// dispatch recorded under skillsParent in its sidecar manifest at envRoot,
+// even when they are now non-empty. It is the reuse-path companion to
+// CleanupSidecars and runs just before it.
+//
+// CleanupSidecars deliberately preserves a recorded directory once it has
+// become non-empty — the agent may have dropped a file inside a dir we
+// created, and on the local_directory teardown path that content must
+// survive. But that same preservation reopens #3684 on the reuse path: if a
+// prior-run agent wrote into .claude/skills/issue-review/, CleanupSidecars
+// deletes the recorded SKILL.md yet keeps the directory, so the canonical
+// slug stays occupied and the refreshed skill dodges to
+// issue-review-multica. A managed skill directory is platform-owned — the
+// manifest is proof we created it — so on reuse we reclaim the whole
+// directory (dropping any scratch the agent left inside it, exactly as the
+// Codex path's os.RemoveAll(skillsDir) already does) and let the refresh
+// re-create it at its natural slug.
+//
+// Only directories whose immediate parent is skillsParent are removed, so
+// the blast radius is exactly the platform's own skill roots: sibling skills
+// the agent installed under the same parent, checked-out repos, and the rest
+// of the workdir are untouched. The reuse path only ever runs on cloud
+// workdirs (the daemon skips Reuse for local_directory tasks), so there is no
+// user-owned skills tree to protect here in the first place.
+//
+// envRoot or skillsParent empty, a missing manifest, or a parse failure are
+// all no-ops — the refresh simply proceeds. The manifest file is left in
+// place; CleanupSidecars, which runs next, owns deleting it.
+func removeReusedManagedSkillDirs(envRoot, skillsParent string) error {
+	if envRoot == "" || skillsParent == "" {
+		return nil
+	}
+	data, err := os.ReadFile(filepath.Join(envRoot, sidecarManifestFile))
+	if errors.Is(err, fs.ErrNotExist) {
+		return nil
+	}
+	if err != nil {
+		return fmt.Errorf("read sidecar manifest for reuse skill rollback: %w", err)
+	}
+	var m sidecarManifest
+	if err := json.Unmarshal(data, &m); err != nil {
+		return fmt.Errorf("parse sidecar manifest for reuse skill rollback: %w", err)
+	}
+
+	cleanParent := filepath.Clean(skillsParent)
+	var firstErr error
+	for _, d := range m.Dirs {
+		if filepath.Dir(filepath.Clean(d)) != cleanParent {
+			continue
+		}
+		if err := os.RemoveAll(d); err != nil && firstErr == nil {
+			firstErr = fmt.Errorf("remove managed skill dir %s: %w", d, 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: