diff --git a/server/internal/daemon/execenv/codex_user_skills.go b/server/internal/daemon/execenv/codex_user_skills.go new file mode 100644 index 000000000..5c1a5c448 --- /dev/null +++ b/server/internal/daemon/execenv/codex_user_skills.go @@ -0,0 +1,114 @@ +package execenv + +import ( + "fmt" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" +) + +// seedUserCodexSkills copies user-installed skill directories from the shared +// ~/.codex/skills/ into the per-task CODEX_HOME so the codex CLI discovers +// them natively. Codex is the only runtime whose HOME is redirected to a +// per-task directory (via the CODEX_HOME env var), so without this step the +// CLI never sees the user's `~/.codex/skills/` content. +// +// Workspace-assigned skills take precedence on name conflict: any user skill +// whose sanitized name matches a workspace skill's sanitized name is skipped +// here, and writeSkillFiles then writes the workspace version into a clean +// slot. +// +// Per-skill failures are logged and skipped — a single broken user skill +// must not prevent the task from running. Returning an error is reserved for +// failures that prevent listing the shared skills directory at all. +func seedUserCodexSkills(codexHome string, workspaceSkills []SkillContextForEnv, logger *slog.Logger) error { + sharedSkillsDir := filepath.Join(resolveSharedCodexHome(), "skills") + + info, err := os.Stat(sharedSkillsDir) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("stat shared skills dir: %w", err) + } + if !info.IsDir() { + return nil + } + + reserved := make(map[string]struct{}, len(workspaceSkills)) + for _, s := range workspaceSkills { + reserved[sanitizeSkillName(s.Name)] = struct{}{} + } + + entries, err := os.ReadDir(sharedSkillsDir) + if err != nil { + return fmt.Errorf("read shared skills dir: %w", err) + } + + targetSkillsDir := filepath.Join(codexHome, "skills") + for _, entry := range entries { + name := entry.Name() + if name == "" || strings.HasPrefix(name, ".") { + continue + } + if _, claimed := reserved[sanitizeSkillName(name)]; claimed { + logger.Info("execenv: codex user-skill yields to workspace skill", "name", name) + continue + } + src := filepath.Join(sharedSkillsDir, name) + // Installers like lark-cli ship each skill as a symlink into a + // shared ~/.agents/skills// directory. Resolve symlinks so we + // copy the real content into the per-task home. + resolved, err := filepath.EvalSymlinks(src) + if err != nil { + logger.Warn("execenv: codex user-skill resolve failed", "name", name, "error", err) + continue + } + fi, err := os.Stat(resolved) + if err != nil || !fi.IsDir() { + continue + } + dst := filepath.Join(targetSkillsDir, name) + if err := os.RemoveAll(dst); err != nil { + logger.Warn("execenv: codex user-skill clean dst failed", "name", name, "error", err) + continue + } + if err := copyDirTree(resolved, dst); err != nil { + logger.Warn("execenv: codex user-skill copy failed", "name", name, "error", err) + continue + } + } + return nil +} + +// copyDirTree walks src recursively and copies every regular file under it +// to the matching path under dst. Nested symlinks are ignored to keep the +// per-task home self-contained; the caller is expected to resolve the root +// before calling. +func copyDirTree(src, dst string) error { + return filepath.WalkDir(src, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + rel, err := filepath.Rel(src, path) + if err != nil { + return err + } + target := filepath.Join(dst, rel) + if d.IsDir() { + return os.MkdirAll(target, 0o755) + } + if d.Type()&os.ModeSymlink != 0 { + return nil + } + if !d.Type().IsRegular() { + return nil + } + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + return err + } + return copyFile(path, target) + }) +} diff --git a/server/internal/daemon/execenv/execenv.go b/server/internal/daemon/execenv/execenv.go index 4cf8364c9..e9d495d9e 100644 --- a/server/internal/daemon/execenv/execenv.go +++ b/server/internal/daemon/execenv/execenv.go @@ -145,8 +145,8 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { if err := prepareCodexHomeWithOpts(codexHome, CodexHomeOptions{CodexVersion: params.CodexVersion}, logger); err != nil { return nil, fmt.Errorf("execenv: prepare codex-home: %w", err) } - if err := writeCodexWorkspaceSkills(codexHome, params.Task.AgentSkills); err != nil { - return nil, fmt.Errorf("execenv: write codex skills: %w", err) + if err := hydrateCodexSkills(codexHome, params.Task.AgentSkills, logger); err != nil { + return nil, fmt.Errorf("execenv: hydrate codex skills: %w", err) } env.CodexHome = codexHome } @@ -186,7 +186,7 @@ func Reuse(workDir, provider, codexVersion string, task TaskContextForEnv, logge logger.Warn("execenv: refresh codex-home failed", "error", err) } else { env.CodexHome = codexHome - if err := writeCodexWorkspaceSkills(codexHome, task.AgentSkills); err != nil { + if err := hydrateCodexSkills(codexHome, task.AgentSkills, logger); err != nil { logger.Warn("execenv: refresh codex skills failed", "error", err) } } @@ -196,11 +196,24 @@ func Reuse(workDir, provider, codexVersion string, task TaskContextForEnv, logge return env } -func writeCodexWorkspaceSkills(codexHome string, skills []SkillContextForEnv) error { - if len(skills) == 0 { +// hydrateCodexSkills populates the per-task CODEX_HOME/skills directory with +// both user-installed skills (from the shared ~/.codex/skills/) and +// workspace-assigned skills. Workspace skills win on name conflict — they are +// written last and seedUserCodexSkills already pre-filters their names. +// +// Codex is the only runtime that needs this two-stage hydration because the +// daemon sets CODEX_HOME to a per-task directory, isolating the CLI from the +// user's real ~/.codex/. Other runtimes leave HOME untouched and discover +// user-level skills natively (see context.go for the workdir-local paths +// they use for workspace skills). +func hydrateCodexSkills(codexHome string, workspaceSkills []SkillContextForEnv, logger *slog.Logger) error { + if err := seedUserCodexSkills(codexHome, workspaceSkills, logger); err != nil { + logger.Warn("execenv: seed user codex skills failed", "error", err) + } + if len(workspaceSkills) == 0 { return nil } - return writeSkillFiles(filepath.Join(codexHome, "skills"), skills) + return writeSkillFiles(filepath.Join(codexHome, "skills"), workspaceSkills) } // GCMetaKind identifies which kind of parent record a task workdir belongs to. diff --git a/server/internal/daemon/execenv/execenv_test.go b/server/internal/daemon/execenv/execenv_test.go index 34799976a..0bc278146 100644 --- a/server/internal/daemon/execenv/execenv_test.go +++ b/server/internal/daemon/execenv/execenv_test.go @@ -2043,6 +2043,258 @@ func TestReuseUpdatesCodexWorkspaceSkills(t *testing.T) { } } +// TestPrepareCodexSeedsUserSkills covers the fix for #1922: skills the user +// installs under ~/.codex/skills/ must be discoverable by the codex CLI +// inside a Multica task, despite the daemon redirecting CODEX_HOME to a +// per-task directory. +func TestPrepareCodexSeedsUserSkills(t *testing.T) { + // Cannot use t.Parallel() with t.Setenv. + + sharedHome := t.TempDir() + t.Setenv("CODEX_HOME", sharedHome) + + // Lay out two user-installed skills with both a SKILL.md and a + // supporting file, plus an ignored dotfile that must not be copied. + userSkills := filepath.Join(sharedHome, "skills") + if err := os.MkdirAll(filepath.Join(userSkills, "summarize", "examples"), 0o755); err != nil { + t.Fatalf("seed user skill dir: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkills, "summarize", "SKILL.md"), []byte("summarize"), 0o644); err != nil { + t.Fatalf("seed user SKILL.md: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkills, "summarize", "examples", "ex.md"), []byte("example"), 0o644); err != nil { + t.Fatalf("seed user support file: %v", err) + } + if err := os.MkdirAll(filepath.Join(userSkills, "translate"), 0o755); err != nil { + t.Fatalf("seed second user skill: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkills, "translate", "SKILL.md"), []byte("translate"), 0o644); err != nil { + t.Fatalf("seed second user SKILL.md: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkills, ".DS_Store"), []byte("noise"), 0o644); err != nil { + t.Fatalf("seed ignored dotfile: %v", err) + } + + env, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-user-skills", + TaskID: "d6f7a8b9-c0d1-2345-efab-678901234567", + AgentName: "Codex Agent", + Provider: "codex", + Task: TaskContextForEnv{IssueID: "user-skills-test"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + if data, err := os.ReadFile(filepath.Join(env.CodexHome, "skills", "summarize", "SKILL.md")); err != nil { + t.Fatalf("user skill SKILL.md not seeded: %v", err) + } else if string(data) != "summarize" { + t.Errorf("summarize SKILL.md = %q, want %q", data, "summarize") + } + if data, err := os.ReadFile(filepath.Join(env.CodexHome, "skills", "summarize", "examples", "ex.md")); err != nil { + t.Fatalf("user skill support file not seeded: %v", err) + } else if string(data) != "example" { + t.Errorf("ex.md = %q, want %q", data, "example") + } + if data, err := os.ReadFile(filepath.Join(env.CodexHome, "skills", "translate", "SKILL.md")); err != nil { + t.Fatalf("second user skill not seeded: %v", err) + } else if string(data) != "translate" { + t.Errorf("translate SKILL.md = %q, want %q", data, "translate") + } + if _, err := os.Stat(filepath.Join(env.CodexHome, "skills", ".DS_Store")); !os.IsNotExist(err) { + t.Errorf("ignored dotfile leaked into codex-home/skills: err=%v", err) + } +} + +// TestPrepareCodexWorkspaceSkillBeatsUserSkillOnConflict checks that when a +// workspace-assigned skill shares a sanitized name with a user-installed +// skill, the workspace version fully replaces the user version (rather than +// leaving stale user files lingering). +func TestPrepareCodexWorkspaceSkillBeatsUserSkillOnConflict(t *testing.T) { + // Cannot use t.Parallel() with t.Setenv. + + sharedHome := t.TempDir() + t.Setenv("CODEX_HOME", sharedHome) + + userSkillDir := filepath.Join(sharedHome, "skills", "writing") + if err := os.MkdirAll(filepath.Join(userSkillDir, "drafts"), 0o755); err != nil { + t.Fatalf("seed user writing skill: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkillDir, "SKILL.md"), []byte("user writing"), 0o644); err != nil { + t.Fatalf("seed user SKILL.md: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkillDir, "drafts", "stale.md"), []byte("stale"), 0o644); err != nil { + t.Fatalf("seed user stale file: %v", err) + } + + env, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-skill-conflict", + TaskID: "e7f8a9b0-c1d2-3456-efab-789012345678", + AgentName: "Codex Agent", + Provider: "codex", + Task: TaskContextForEnv{ + IssueID: "skill-conflict-test", + AgentSkills: []SkillContextForEnv{ + {Name: "Writing", Content: "workspace writing"}, + }, + }, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + data, err := os.ReadFile(filepath.Join(env.CodexHome, "skills", "writing", "SKILL.md")) + if err != nil { + t.Fatalf("workspace skill not written: %v", err) + } + if string(data) != "workspace writing" { + t.Errorf("SKILL.md = %q, want workspace content", data) + } + // The user's stale support file must not leak through — seeding is + // skipped entirely for names that workspace skills claim. + if _, err := os.Stat(filepath.Join(env.CodexHome, "skills", "writing", "drafts", "stale.md")); !os.IsNotExist(err) { + t.Errorf("user-skill stale file leaked despite workspace conflict: err=%v", err) + } +} + +// TestPrepareCodexNoUserSkillsDir is a regression guard for the empty case — +// when ~/.codex/skills doesn't exist, the seed step is a no-op and Prepare +// still succeeds. +func TestPrepareCodexNoUserSkillsDir(t *testing.T) { + // Cannot use t.Parallel() with t.Setenv. + + sharedHome := t.TempDir() + t.Setenv("CODEX_HOME", sharedHome) + + env, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-no-user-skills", + TaskID: "f8a9b0c1-d2e3-4567-fabc-890123456789", + AgentName: "Codex Agent", + Provider: "codex", + Task: TaskContextForEnv{IssueID: "no-user-skills-test"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + if _, err := os.Stat(filepath.Join(env.CodexHome, "skills")); !os.IsNotExist(err) { + t.Errorf("skills dir should not exist when neither user nor workspace skills are present, err=%v", err) + } +} + +// TestPrepareCodexResolvesUserSkillSymlinks covers the lark-cli / +// shared-installer case: each user skill is a symlink into a separate +// installer directory. The per-task home must end up with a real copy, not +// a dangling symlink that points outside the task root. +func TestPrepareCodexResolvesUserSkillSymlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink semantics differ on Windows; covered by Unix path") + } + // Cannot use t.Parallel() with t.Setenv. + + sharedHome := t.TempDir() + t.Setenv("CODEX_HOME", sharedHome) + + installerRoot := filepath.Join(t.TempDir(), "installer", "lark-mail") + if err := os.MkdirAll(installerRoot, 0o755); err != nil { + t.Fatalf("seed installer dir: %v", err) + } + if err := os.WriteFile(filepath.Join(installerRoot, "SKILL.md"), []byte("lark"), 0o644); err != nil { + t.Fatalf("seed installer SKILL.md: %v", err) + } + + userSkills := filepath.Join(sharedHome, "skills") + if err := os.MkdirAll(userSkills, 0o755); err != nil { + t.Fatalf("seed user skills dir: %v", err) + } + if err := os.Symlink(installerRoot, filepath.Join(userSkills, "lark-mail")); err != nil { + t.Fatalf("seed user skill symlink: %v", err) + } + + env, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-symlinked-skills", + TaskID: "a9b0c1d2-e3f4-5678-abcd-901234567890", + AgentName: "Codex Agent", + Provider: "codex", + Task: TaskContextForEnv{IssueID: "symlinked-skills-test"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + dst := filepath.Join(env.CodexHome, "skills", "lark-mail") + fi, err := os.Lstat(dst) + if err != nil { + t.Fatalf("seeded skill missing: %v", err) + } + if fi.Mode()&os.ModeSymlink != 0 { + t.Errorf("seeded skill should be a real directory, got a symlink") + } + data, err := os.ReadFile(filepath.Join(dst, "SKILL.md")) + if err != nil { + t.Fatalf("seeded SKILL.md missing: %v", err) + } + if string(data) != "lark" { + t.Errorf("seeded SKILL.md = %q, want %q", data, "lark") + } +} + +// TestReuseSeedsUserSkillUpdates ensures that user-skill edits between two +// runs of the same task (the Reuse path) propagate into the per-task home. +func TestReuseSeedsUserSkillUpdates(t *testing.T) { + // Cannot use t.Parallel() with t.Setenv. + + sharedHome := t.TempDir() + t.Setenv("CODEX_HOME", sharedHome) + + userSkill := filepath.Join(sharedHome, "skills", "summarize") + if err := os.MkdirAll(userSkill, 0o755); err != nil { + t.Fatalf("seed user skill: %v", err) + } + if err := os.WriteFile(filepath.Join(userSkill, "SKILL.md"), []byte("v1"), 0o644); err != nil { + t.Fatalf("seed v1 SKILL.md: %v", err) + } + + workspacesRoot := t.TempDir() + env, err := Prepare(PrepareParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: "ws-user-skill-reuse", + TaskID: "b0c1d2e3-f4a5-6789-abcd-012345678901", + AgentName: "Codex Agent", + Provider: "codex", + Task: TaskContextForEnv{IssueID: "user-skill-reuse-test"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + defer env.Cleanup(true) + + if err := os.WriteFile(filepath.Join(userSkill, "SKILL.md"), []byte("v2"), 0o644); err != nil { + t.Fatalf("update user SKILL.md: %v", err) + } + + reused := Reuse(env.WorkDir, "codex", "", TaskContextForEnv{ + IssueID: "user-skill-reuse-test", + }, testLogger()) + if reused == nil { + t.Fatal("Reuse returned nil") + } + data, err := os.ReadFile(filepath.Join(reused.CodexHome, "skills", "summarize", "SKILL.md")) + if err != nil { + t.Fatalf("user skill not refreshed on reuse: %v", err) + } + if string(data) != "v2" { + t.Errorf("after Reuse, user skill content = %q, want %q", data, "v2") + } +} + func TestEnsureSymlinkRepairsBrokenLink(t *testing.T) { t.Parallel() dir := t.TempDir()