diff --git a/server/internal/daemon/execenv/codex_home.go b/server/internal/daemon/execenv/codex_home.go index 014aad1bd87..0206be58e07 100644 --- a/server/internal/daemon/execenv/codex_home.go +++ b/server/internal/daemon/execenv/codex_home.go @@ -241,12 +241,15 @@ func prepareCodexHomeWithOpts(codexHome string, opts CodexHomeOptions, logger *s } } } - // Drop `[[skills.config]]` entries inherited from the user's + // Drop user-level skill config and marketplace update sources inherited from // ~/.codex/config.toml. Codex Desktop writes plugin-backed skills with a // `name` and no `path`, which the CLI's stricter TOML parser rejects with - // `missing field path` and bails out of `thread/start`. Multica writes the - // agent's active skills directly to `codex-home/skills/`, so the - // user-level registry is redundant here. See codex_skill_strip.go. + // `missing field path` and bails out of `thread/start`. Marketplace entries + // also trigger redundant git refreshes in every isolated task home. Multica + // writes the agent's active skills directly to `codex-home/skills/`. Keep + // `[plugins.*]` so installed plugins still load from the exposed shared + // plugin cache, but do not let tasks update marketplaces. See + // codex_skill_strip.go. if err := sanitizeCopiedCodexConfig(filepath.Join(codexHome, "config.toml")); err != nil { logger.Warn("execenv: codex-home sanitize config failed", "error", err) } diff --git a/server/internal/daemon/execenv/codex_skill_strip.go b/server/internal/daemon/execenv/codex_skill_strip.go index 6447a28fac1..3444f28eed0 100644 --- a/server/internal/daemon/execenv/codex_skill_strip.go +++ b/server/internal/daemon/execenv/codex_skill_strip.go @@ -1,13 +1,16 @@ package execenv import ( + "bytes" "fmt" "os" - "strings" + + "github.com/pelletier/go-toml/v2" + "github.com/pelletier/go-toml/v2/unstable" ) -// stripSkillsConfigEntries removes every `[[skills.config]]` array-of-tables -// block from the given config.toml content. +// stripTaskIrrelevantCodexConfigEntries removes user-level skill config and +// marketplace update sources from copied config.toml content. // // Background: Codex Desktop writes one `[[skills.config]]` entry per skill it // knows about — file-backed skills get a `path = "..."` field, while @@ -24,50 +27,219 @@ import ( // that directory. The user-level skill registry is irrelevant to a per-task // run, so dropping it is both safe and the right scope of isolation. // -// Lines outside `[[skills.config]]` blocks are preserved untouched. -func stripSkillsConfigEntries(content string) string { - if !strings.Contains(content, "[[skills.config]]") { - return content +// Multica materializes the agent's assigned skills directly under the per-task +// CODEX_HOME/skills. User-level `[marketplaces.*]` entries are also unsafe in a +// task copy because Codex probes or clones each git source before the first +// turn. `[plugins.*]` entries remain intact so installed plugins still load +// from the shared plugin cache exposed by prepareCodexHomeWithOpts. +// +// Source ranges preserve comments and every unrelated expression byte-for-byte +// while supporting all legal TOML key forms, including dotted keys, quoted +// tables, inline root values, and array tables. +func stripTaskIrrelevantCodexConfigEntries(content []byte) ([]byte, error) { + if len(bytes.TrimSpace(content)) == 0 { + return content, nil } - lines := strings.Split(content, "\n") - out := make([]string, 0, len(lines)) - inSkillsConfig := false - for _, line := range lines { - trimmed := strings.TrimSpace(line) + var decoded map[string]any + if err := toml.Unmarshal(content, &decoded); err != nil { + return nil, fmt.Errorf("parse config.toml before task sanitization: %w", err) + } - // A new TOML header always closes the current `[[skills.config]]` - // block, regardless of whether it's another entry of the same array - // or a different table. - if strings.HasPrefix(trimmed, "[") { - if trimmed == "[[skills.config]]" { - inSkillsConfig = true + var parser unstable.Parser + parser.Reset(content) + var currentTable []string + ranges := make([]tomlByteRange, 0, 16) + for parser.NextExpression() { + expr := parser.Expression() + keys := tomlNodeKeys(expr) + switch expr.Kind { + case unstable.Table, unstable.ArrayTable: + currentTable = keys + if isTaskIrrelevantCodexConfigPath(keys) { + r, err := tomlTableHeaderLineRange(content, expr) + if err != nil { + return nil, fmt.Errorf("locate task-irrelevant config table: %w", err) + } + r.start = precedingBlankLinesStart(content, r.start) + ranges = append(ranges, r) + } + case unstable.KeyValue: + fullKey := make([]string, 0, len(currentTable)+len(keys)) + fullKey = append(fullKey, currentTable...) + fullKey = append(fullKey, keys...) + if len(fullKey) == 1 && fullKey[0] == "skills" && expr.Value().Kind == unstable.InlineTable { + inlineRanges, removeExpression, err := taskIrrelevantInlineSkillsRanges(content, expr) + if err != nil { + return nil, fmt.Errorf("locate inline skills.config: %w", err) + } + if removeExpression { + ranges = append(ranges, tomlExpressionLineRange(content, expr)) + } else { + ranges = append(ranges, inlineRanges...) + } continue } - inSkillsConfig = false - out = append(out, line) - continue + if isTaskIrrelevantCodexConfigPath(fullKey) { + ranges = append(ranges, tomlExpressionLineRange(content, expr)) + } + } + } + if err := parser.Error(); err != nil { + return nil, fmt.Errorf("parse config.toml task sanitization expressions: %w", err) + } + stripped, err := removeTOMLByteRanges(content, ranges) + if err != nil { + return nil, fmt.Errorf("remove task-irrelevant config expressions: %w", err) + } + if err := toml.Unmarshal(stripped, &decoded); err != nil { + return nil, fmt.Errorf("validate config.toml after task sanitization: %w", err) + } + if len(ranges) > 0 { + stripped = bytes.TrimLeft(stripped, "\n") + stripped = bytes.TrimRight(stripped, "\n") + if len(stripped) > 0 { + stripped = append(stripped, '\n') + } + } + return stripped, nil +} + +// taskIrrelevantInlineSkillsRanges locates config entries inside a top-level +// inline skills table. Removing the whole expression is safe only when every +// entry belongs to skills.config; otherwise the returned ranges preserve the +// remaining inline table byte-for-byte. +func taskIrrelevantInlineSkillsRanges(content []byte, expression *unstable.Node) ([]tomlByteRange, bool, error) { + table := expression.Value() + entries := make([]*unstable.Node, 0, 4) + targets := make([]bool, 0, 4) + targetCount := 0 + for it := table.Children(); it.Next(); { + entry := it.Node() + if entry.Kind != unstable.KeyValue { + return nil, false, fmt.Errorf("inline skills table contains %s instead of key-value", entry.Kind) + } + keys := tomlNodeKeys(entry) + isTarget := len(keys) > 0 && keys[0] == "config" + entries = append(entries, entry) + targets = append(targets, isTarget) + if isTarget { + targetCount++ } + } + if targetCount == 0 { + return nil, false, nil + } + if targetCount == len(entries) { + return nil, true, nil + } - if inSkillsConfig { + closingBrace, err := tomlInlineTableClosingBrace(content, expression, table) + if err != nil { + return nil, false, err + } + ranges := make([]tomlByteRange, 0, targetCount) + for index, entry := range entries { + if !targets[index] { + continue + } + start, err := firstTOMLKeyOffset(content, entry) + if err != nil { + return nil, false, err + } + if index+1 < len(entries) { + end, err := firstTOMLKeyOffset(content, entries[index+1]) + if err != nil { + return nil, false, err + } + ranges = append(ranges, tomlByteRange{start: start, end: end}) continue } - out = append(out, line) + + // The last entry has no following key whose offset can delimit it. + // Include its preceding separator and stop immediately before the + // inline table's closing brace. + separator := start + for separator > 0 && (content[separator-1] == ' ' || content[separator-1] == '\t') { + separator-- + } + if separator == 0 || content[separator-1] != ',' { + return nil, false, fmt.Errorf("last inline skills.config entry has no preceding comma") + } + end := closingBrace + for end > start && (content[end-1] == ' ' || content[end-1] == '\t') { + end-- + } + ranges = append(ranges, tomlByteRange{start: separator - 1, end: end}) } + return ranges, false, nil +} - stripped := strings.Join(out, "\n") - // Collapse the trailing blank-line cluster that the removal can leave - // behind so repeated copies don't grow the file unboundedly. - stripped = strings.TrimRight(stripped, "\n") + "\n" - if strings.TrimSpace(stripped) == "" { - return "" - } - return stripped +func firstTOMLKeyOffset(content []byte, keyValue *unstable.Node) (int, error) { + it := keyValue.Key() + if !it.Next() { + return 0, fmt.Errorf("inline key-value has no key") + } + offset := int(it.Node().Raw.Offset) + if offset < 0 || offset > len(content) { + return 0, fmt.Errorf("inline key offset is outside config") + } + return offset, nil +} + +// tomlInlineTableClosingBrace derives the value boundary from go-toml's full +// KeyValue source range instead of duplicating TOML string grammar locally. +func tomlInlineTableClosingBrace(content []byte, expression, table *unstable.Node) (int, error) { + start := int(table.Raw.Offset) + if start < 0 || start >= len(content) || content[start] != '{' { + return 0, fmt.Errorf("inline table opening brace is outside config") + } + end := int(expression.Raw.Offset) + int(expression.Raw.Length) + if end <= start || end > len(content) || content[end-1] != '}' { + return 0, fmt.Errorf("inline table closing brace is outside key-value range") + } + return end - 1, nil +} + +func tomlExpressionLineRange(content []byte, expr *unstable.Node) tomlByteRange { + start := int(expr.Raw.Offset) + end := start + int(expr.Raw.Length) + start = bytes.LastIndexByte(content[:start], '\n') + 1 + if end < len(content) { + if newline := bytes.IndexByte(content[end:], '\n'); newline >= 0 { + end += newline + 1 + } + } + return tomlByteRange{start: start, end: end} +} + +func precedingBlankLinesStart(content []byte, start int) int { + for start > 0 { + lineEnd := start - 1 + lineStart := bytes.LastIndexByte(content[:lineEnd], '\n') + 1 + if len(bytes.TrimSpace(content[lineStart:lineEnd])) != 0 { + break + } + start = lineStart + } + return start +} + +func isTaskIrrelevantCodexConfigPath(keys []string) bool { + if len(keys) == 0 { + return false + } + if keys[0] == "marketplaces" { + return true + } + return len(keys) >= 2 && keys[0] == "skills" && keys[1] == "config" } // sanitizeCopiedCodexConfig rewrites the per-task config.toml in place, -// dropping `[[skills.config]]` entries inherited from the shared -// `~/.codex/config.toml`. No-op if the file doesn't exist or doesn't change. +// dropping user-level skill config and marketplace update sources inherited +// from the shared `~/.codex/config.toml`. Installed plugin entries remain so +// they can load from the shared plugin cache. No-op if the file doesn't exist +// or doesn't change. func sanitizeCopiedCodexConfig(configPath string) error { data, err := os.ReadFile(configPath) if err != nil { @@ -76,11 +248,14 @@ func sanitizeCopiedCodexConfig(configPath string) error { } return fmt.Errorf("read config.toml: %w", err) } - stripped := stripSkillsConfigEntries(string(data)) - if stripped == string(data) { + stripped, err := stripTaskIrrelevantCodexConfigEntries(data) + if err != nil { + return err + } + if bytes.Equal(stripped, data) { return nil } - if err := os.WriteFile(configPath, []byte(stripped), 0o644); err != nil { + if err := os.WriteFile(configPath, stripped, 0o644); err != nil { return fmt.Errorf("write config.toml: %w", err) } return nil diff --git a/server/internal/daemon/execenv/codex_skill_strip_test.go b/server/internal/daemon/execenv/codex_skill_strip_test.go index a1be1afbe25..ee9afdc13c6 100644 --- a/server/internal/daemon/execenv/codex_skill_strip_test.go +++ b/server/internal/daemon/execenv/codex_skill_strip_test.go @@ -1,13 +1,16 @@ package execenv import ( + "bytes" "os" "path/filepath" "strings" "testing" + + "github.com/pelletier/go-toml/v2" ) -func TestStripSkillsConfigEntries(t *testing.T) { +func TestStripTaskIrrelevantCodexConfigEntries(t *testing.T) { t.Parallel() tests := []struct { @@ -108,9 +111,12 @@ enabled = false for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := stripSkillsConfigEntries(tt.in) - if got != tt.want { - t.Errorf("stripSkillsConfigEntries result mismatch\n--- got ---\n%s\n--- want ---\n%s", got, tt.want) + got, err := stripTaskIrrelevantCodexConfigEntries([]byte(tt.in)) + if err != nil { + t.Fatalf("stripTaskIrrelevantCodexConfigEntries failed: %v", err) + } + if string(got) != tt.want { + t.Errorf("stripTaskIrrelevantCodexConfigEntries result mismatch\n--- got ---\n%s\n--- want ---\n%s", got, tt.want) } }) } @@ -123,6 +129,14 @@ func TestSanitizeCopiedCodexConfig(t *testing.T) { configPath := filepath.Join(dir, "config.toml") original := `model = "o3" +[marketplaces.claude-plugins-official] +last_updated = "2026-07-17T03:11:33Z" +source_type = "git" +source = "https://github.com/anthropics/claude-plugins-official.git" + +[plugins."superpowers@claude-plugins-official"] +enabled = true + [[skills.config]] name = "superpowers:brainstorming" enabled = false @@ -133,6 +147,9 @@ enabled = true [profiles.default] model = "o3" + +[mcp_servers.foo] +command = "foo" ` if err := os.WriteFile(configPath, []byte(original), 0o644); err != nil { t.Fatalf("write fixture: %v", err) @@ -150,14 +167,182 @@ model = "o3" if strings.Contains(got, "[[skills.config]]") { t.Errorf("expected all [[skills.config]] entries to be removed, got:\n%s", got) } + if strings.Contains(got, "[marketplaces.") { + t.Errorf("expected user-level marketplace entries to be removed, got:\n%s", got) + } + if !strings.Contains(got, `[plugins."superpowers@claude-plugins-official"]`) { + t.Errorf("expected installed plugin registry to be preserved, got:\n%s", got) + } if !strings.Contains(got, `[profiles.default]`) { t.Errorf("unrelated tables should be preserved, got:\n%s", got) } + if !strings.Contains(got, `[mcp_servers.foo]`) { + t.Errorf("unrelated MCP configuration should be preserved, got:\n%s", got) + } if !strings.Contains(got, `model = "o3"`) { t.Errorf("top-level keys should be preserved, got:\n%s", got) } } +func TestSanitizeCopiedCodexConfigHandlesSemanticRegistryForms(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + original string + want string + }{ + { + name: "dotted and inline root keys", + original: `model = "o3" +marketplaces.foo.source = "https://example.test/plugins.git" +plugins."demo@foo".enabled = true +skills.config = [{ path = "/tmp/SKILL.md", enabled = true }] +`, + want: `model = "o3" +plugins."demo@foo".enabled = true +`, + }, + { + name: "quoted and array table headers", + original: `model = "o3" + +[ "marketplaces" . foo ] +source = "https://example.test/plugins.git" + +[ "plugins" . "demo@foo" ] +enabled = true + +[[skills.config]] +path = "/tmp/SKILL.md" +enabled = true +`, + want: `model = "o3" + +[ "plugins" . "demo@foo" ] +enabled = true +`, + }, + { + name: "inline marketplace and plugin roots", + original: `marketplaces = { foo = { source_type = "git", source = "https://example.test/plugins.git" } } +plugins = { "demo@foo" = { enabled = true } } +model = "o3" +`, + want: `plugins = { "demo@foo" = { enabled = true } } +model = "o3" +`, + }, + { + name: "inline skills table removes only config", + original: `skills = { config = [{ name = "superpowers:brainstorming", enabled = false }], discovery_path = "skills" } +model = "o3" +`, + want: `skills = { discovery_path = "skills" } +model = "o3" +`, + }, + { + name: "inline skills table removes trailing config", + original: `skills = { discovery_path = "literal } remains", config = [{ path = "/tmp/SKILL.md" }] } +model = "o3" +`, + want: `skills = { discovery_path = "literal } remains" } +model = "o3" +`, + }, + { + name: "inline skills table preserves four-quote multiline basic string", + original: `skills = { discovery_path = """ +Closing with four quotes +"""", config = [{ path = "/tmp/SKILL.md" }] } +model = "o3" +`, + want: `skills = { discovery_path = """ +Closing with four quotes +"""" } +model = "o3" +`, + }, + { + name: "inline skills table preserves four-quote multiline literal string", + original: `skills = { discovery_path = ''' +Closing with four apostrophes +'''', config = [{ path = '/tmp/SKILL.md' }] } +model = "o3" +`, + want: `skills = { discovery_path = ''' +Closing with four apostrophes +'''' } +model = "o3" +`, + }, + { + name: "inline skills table with only config is dropped", + original: `model = "o3" +skills = { config = [{ path = "/tmp/SKILL.md" }] } +`, + want: `model = "o3" +`, + }, + { + name: "header-like text inside multiline string", + original: `developer_instructions = """ +[plugins.demo] +do not load this example +""" +model = "o3" +`, + want: `developer_instructions = """ +[plugins.demo] +do not load this example +""" +model = "o3" +`, + }, + { + name: "unrelated nested keys", + original: `[profiles.default] +plugins.demo = "profile-local-value" + +[skills] +discovery_path = "skills" +`, + want: `[profiles.default] +plugins.demo = "profile-local-value" + +[skills] +discovery_path = "skills" +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + configPath := filepath.Join(t.TempDir(), "config.toml") + if err := os.WriteFile(configPath, []byte(tt.original), 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + + if err := sanitizeCopiedCodexConfig(configPath); err != nil { + t.Fatalf("sanitizeCopiedCodexConfig failed: %v", err) + } + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read result: %v", err) + } + if string(data) != tt.want { + t.Errorf("sanitized config mismatch\n--- got ---\n%s\n--- want ---\n%s", data, tt.want) + } + var decoded map[string]any + if err := toml.Unmarshal(data, &decoded); err != nil { + t.Fatalf("sanitized config is invalid TOML: %v\n%s", err, data) + } + }) + } +} + func TestSanitizeCopiedCodexConfigNoop(t *testing.T) { t.Parallel() @@ -197,3 +382,24 @@ func TestSanitizeCopiedCodexConfigMissingFile(t *testing.T) { t.Errorf("missing file should be a no-op, got error: %v", err) } } + +func TestSanitizeCopiedCodexConfigRejectsInvalidInputWithoutWriting(t *testing.T) { + t.Parallel() + + configPath := filepath.Join(t.TempDir(), "config.toml") + original := []byte("plugins.demo = [\n") + if err := os.WriteFile(configPath, original, 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + + if err := sanitizeCopiedCodexConfig(configPath); err == nil { + t.Fatal("expected malformed config to fail") + } + got, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read result: %v", err) + } + if !bytes.Equal(got, original) { + t.Fatalf("invalid config was modified: got %q, want %q", got, original) + } +} diff --git a/server/internal/daemon/execenv/execenv_test.go b/server/internal/daemon/execenv/execenv_test.go index 825d6c17a85..f46a554d46a 100644 --- a/server/internal/daemon/execenv/execenv_test.go +++ b/server/internal/daemon/execenv/execenv_test.go @@ -2189,12 +2189,20 @@ func TestPrepareCodexHomeReportsMissingModelCatalogPath(t *testing.T) { // parser rejects them with `missing field path`. prepareCodexHome must drop // every `[[skills.config]]` entry while copying the user's config.toml so // the per-task home stays parseable. -func TestPrepareCodexHomeStripsSkillsConfigEntries(t *testing.T) { +func TestPrepareCodexHomeSanitizesMarketplaceAndSkillConfig(t *testing.T) { // Cannot use t.Parallel() with t.Setenv. sharedHome := t.TempDir() sharedConfig := `model = "o3" +[marketplaces.claude-plugins-official] +last_updated = "2026-07-17T03:11:33Z" +source_type = "git" +source = "https://github.com/anthropics/claude-plugins-official.git" + +[plugins."superpowers@claude-plugins-official"] +enabled = true + [[skills.config]] path = "/Users/x/SKILL.md" enabled = false @@ -2209,6 +2217,13 @@ model = "o3" if err := os.WriteFile(filepath.Join(sharedHome, "config.toml"), []byte(sharedConfig), 0o644); err != nil { t.Fatalf("write shared config.toml: %v", err) } + pluginSkill := filepath.Join(sharedHome, "plugins", "cache", "claude-plugins-official", "superpowers", "1.0.0", "skills", "brainstorming", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(pluginSkill), 0o755); err != nil { + t.Fatalf("create shared plugin cache: %v", err) + } + if err := os.WriteFile(pluginSkill, []byte("Use when brainstorming."), 0o644); err != nil { + t.Fatalf("write shared plugin skill: %v", err) + } t.Setenv("CODEX_HOME", sharedHome) codexHome := filepath.Join(t.TempDir(), "codex-home") @@ -2227,12 +2242,32 @@ model = "o3" if strings.Contains(tomlStr, "superpowers:brainstorming") { t.Errorf("per-task config.toml should not retain plugin skill names, got:\n%s", tomlStr) } + if strings.Contains(tomlStr, "[marketplaces.") { + t.Errorf("per-task config.toml should not inherit marketplace entries, got:\n%s", tomlStr) + } + if !strings.Contains(tomlStr, `[plugins."superpowers@claude-plugins-official"]`) { + t.Errorf("per-task config.toml should preserve installed plugin registry entries, got:\n%s", tomlStr) + } + pluginData, err := os.ReadFile(filepath.Join(codexHome, "plugins", "cache", "claude-plugins-official", "superpowers", "1.0.0", "skills", "brainstorming", "SKILL.md")) + if err != nil { + t.Fatalf("preserved plugin registry must retain access to shared plugin cache: %v", err) + } + if string(pluginData) != "Use when brainstorming." { + t.Errorf("shared plugin cache content = %q", pluginData) + } if !strings.Contains(tomlStr, `model = "o3"`) { t.Errorf("top-level keys should be preserved, got:\n%s", tomlStr) } if !strings.Contains(tomlStr, "[profiles.default]") { t.Errorf("unrelated tables should be preserved, got:\n%s", tomlStr) } + sharedData, err := os.ReadFile(filepath.Join(sharedHome, "config.toml")) + if err != nil { + t.Fatalf("read shared config.toml: %v", err) + } + if string(sharedData) != sharedConfig { + t.Errorf("shared config.toml must remain unchanged, got:\n%s", sharedData) + } } func TestPrepareCodexHomeSkipsMissingFiles(t *testing.T) {