diff --git a/docs/subsystems/library.md b/docs/subsystems/library.md index 0c0f7e82..9accc088 100644 --- a/docs/subsystems/library.md +++ b/docs/subsystems/library.md @@ -36,7 +36,7 @@ agents RTK, a CLI that cuts shell output down before the model reads it. - An edit never silently loses. When an agent's copy of a skill and the library both changed, the newest edit wins, and the losing one is kept with the backups. - A failure for one agent is a `Problem` in the result. It doesn't stop the others. - A server an agent can't reach is not written there, and `mcpFile.supports` says why. For example, Claude Desktop takes no remote server, and Codex, Goose, dsh, Grok, Command Code and Zed take no SSE. -- A server's headers (remote) or environment (command) can name a variable as `${NAME}` (#1250). `library.json` keeps the placeholder, and `refsOf` gives each MCP format the agent's own syntax, with the agent's source cited beside it: `${NAME}` for most, `{env:NAME}` for OpenCode and MiMo, `${env:NAME}` for Cursor, and Codex's own keys (`bearer_token_env_var`, `env_http_headers`, `env_vars`). Goose gets `${NAME}` plus `env_keys`. An agent with no syntax (Claude Desktop, Antigravity, Kimi, Cline, ZCode, Zed, dsh, Devin, Alma, Pi with pi-mcp-extension) is never written the value: `refsProblem` makes `supports` refuse the server, a magpie entry already there is taken out, and `AgentView.NoEnvRefs` greys its chip with the reason. Codex and Goose's extra keys are magpie's only while the server names a variable there (`refKeys`), so the user's own `env_vars` or `env_keys` are kept. `CheckServers` expands the variable from magpie's own environment, and one that isn't set is `novar`, naming it. +- A server's headers (remote) or environment (command) can name a variable as `${NAME}` (#1250). `library.json` keeps the placeholder, and `refsOf` gives each MCP format the agent's own syntax, with the agent's source cited beside it: `${NAME}` for most, `{env:NAME}` for OpenCode and MiMo, `${env:NAME}` for Cursor, and Codex's own keys (`bearer_token_env_var`, `env_http_headers`, `env_vars`). Goose gets `${NAME}` plus `env_keys`. dsh (DeepSeek Harness) gets a `!!js` expression it evaluates as it loads the server (#1435): a whole `${NAME}` is `!!js process.env.NAME`, and a value mixing text and variables is the text as JSON string literals joined with `(process.env.NAME ?? "")`, so the user's text is never run as code and an unset variable inside it is empty. A whole variable that isn't set makes dsh leave the server out as invalid. magpie reads only those forms back as `${NAME}`; any other `!!js` keeps the entry the user's. An agent with no syntax (Claude Desktop, Antigravity, Kimi, Cline, ZCode, Zed, Devin, Alma, Pi with pi-mcp-extension) is never written the value: `refsProblem` makes `supports` refuse the server, a magpie entry already there is taken out, and `AgentView.NoEnvRefs` greys its chip with the reason. Codex and Goose's extra keys are magpie's only while the server names a variable there (`refKeys`), so the user's own `env_vars` or `env_keys` are kept. `CheckServers` expands the variable from magpie's own environment, and one that isn't set is `novar`, naming it. - WorkBuddy's servers go in `$WORKBUDDY_CONFIG_DIR/mcp.json` (default `~/.workbuddy`), and CodeBuddy Code's go in the first of `.mcp.json`, `mcp.json` and `~/.codebuddy.json` that exists, as `resolveMcpFilePath` reads them (#1266). WorkBuddy connects a server only after the user has switched it on once in its own MCP settings. It keeps that approval in `mcp-approvals.json`, keyed by the command, args and env names or the URL's origin, so changing those asks again. magpie never writes the approval, and the servers page says so when a server is given to WorkBuddy. - Alma's servers go in `~/.config/alma/mcp.json` on every system (its home, not its data folder), as its own MCP settings write them: `command`/`args`/`env`, or `url`/`headers` with `transport: "sse"` only for SSE (#1292). Alma reads the file as it starts and when its MCP settings refresh, so a server given to it while it runs reaches it after one of those. - Zed's servers go in `context_servers` of the settings.json magpie sets its model in (`agent.zed`: `~/.config/zed` on macOS, `$XDG_CONFIG_HOME/zed` on Linux, `%APPDATA%\Zed` on Windows, or `$MAGPIE_ZED_CONFIG_DIR`), as `command`/`args`/`env` or `url`/`headers` (`ContextServerSettingsContent`, zed-industries/zed 2c99f547). The file is JSONC: it is edited in place, so the user's comments, other settings and own servers stay, and `enabled`, `remote`, `timeout` and `oauth` in magpie's entry are the user's. Zed speaks streamable HTTP only (its `http.rs` only POSTs), so no SSE. An entry Zed's extensions configure (`settings` and no command or url) is never written over: a server by that name is refused for Zed (`mcpFile.foreign`), and asks for another name. Zed reloads the file as it changes. @@ -48,7 +48,7 @@ agents RTK, a CLI that cuts shell output down before the model reads it. ## Verification ```sh -go test -tags nogui ./internal/library -run 'TestCheckSkillsFindsNewOnes|TestNewBase|TestTargets|TestServer|TestImportServer|TestCheck|TestInstructions|TestInstructionSets|TestSkill|TestRemoveFoundSkill|TestInstallSet|TestInstallLists|TestEverySkillAgents|TestEveryServerAgents|TestSomeServersAgents|TestRemoveServers|TestRTK|TestEnvRefs|TestAgentMCPFiles|TestCodeBuddyMCPFile|TestZedMCP' +go test -tags nogui ./internal/library -run 'TestCheckSkillsFindsNewOnes|TestNewBase|TestTargets|TestServer|TestImportServer|TestCheck|TestInstructions|TestInstructionSets|TestSkill|TestRemoveFoundSkill|TestInstallSet|TestInstallLists|TestEverySkillAgents|TestEveryServerAgents|TestSomeServersAgents|TestRemoveServers|TestRTK|TestEnvRefs|TestDshRefRoundTrip|TestDshImportKeepsJS|TestAgentMCPFiles|TestCodeBuddyMCPFile|TestZedMCP' go test -tags nogui ./internal/library node --test internal/gui/tests/library-items.test.cjs internal/gui/tests/library-rtk-gain.test.cjs internal/gui/tests/library-rtk-nohook.test.cjs internal/gui/tests/library-servers-bulk.test.cjs internal/gui/tests/library-env-refs.test.cjs internal/gui/tests/library-zed-mcp.test.cjs internal/gui/tests/library-workbuddy-trust.test.cjs internal/gui/tests/skill-new-in-repo.test.cjs internal/gui/tests/remove-found-skill.test.cjs ``` diff --git a/internal/library/dsh.go b/internal/library/dsh.go index 41548bcb..47818107 100644 --- a/internal/library/dsh.go +++ b/internal/library/dsh.go @@ -304,6 +304,11 @@ func dshYAML(indent, k string, v any) []string { key := indent + dshKey(k) + ":" switch x := v.(type) { case dshJS: + // a variable whole bare, as dsh-mcp-client's README has it; the + // rest quoted, which dsh's YAML reads as the same expression + if dshEnvRef.MatchString(string(x)) { + return []string{key + " !!js " + string(x)} + } return []string{key + " !!js " + dshScalar(string(x))} case string: return []string{key + " " + dshScalar(x)} diff --git a/internal/library/envref.go b/internal/library/envref.go index 20fb5352..9ef25817 100644 --- a/internal/library/envref.go +++ b/internal/library/envref.go @@ -9,6 +9,7 @@ package library // written with the value, the token would be in its file after all. import ( + "encoding/json" "errors" "fmt" "maps" @@ -39,6 +40,8 @@ const ( // refGoose: ${NAME} in a header, each name listed in env_keys; a // command's variable only through env_keys, under its own name refGoose + // refDsh: a !!js expression reading process.env (dshRef) + refDsh ) // envSyntax is how the agent reads a reference in a remote server's @@ -156,6 +159,18 @@ func (f *mcpFile) refsOf() envSyntax { // headers and env, and refuses one unset (dist/cli.mjs // resolveEnvPlaceholders) return envSyntax{refDollar, refDollar} + case fmtDsh: + // DeepSeek Harness evaluates a YAML !!js value of a plugin's config + // as it activates the row, with its own process in reach (dsh + // 0.2.0-rc.2: cordis-plugin-include's entryListSchema reads the + // tag, quoted or not; cordis-plugin-loader's interpolate evaluates + // it all through the config). dsh-mcp-client's README gives a + // command GITHUB_TOKEN: !!js process.env.GITHUB_TOKEN, and a command + // needs it so: it inherits dsh's environment without the + // credential-shaped names (dsh-subprocess scrubbedParentEnv). Tried + // (#1435): the server got the token, and with the variable unset + // dsh left the server out, saying its config is invalid. + return envSyntax{refDsh, refDsh} } // None: Claude Desktop gives a command its env as written and only // HOME, LOGNAME, PATH, SHELL, TERM and USER of its own (2.7032's app.asar @@ -172,9 +187,7 @@ func (f *mcpFile) refsOf() envSyntax { // Alma // 0.4.164 JSON.parses mcp.json and hands a command its env and a url // its headers as written (out/main/index.js createStdioTransport, - // connectRemoteServer). DeepSeek - // Harness takes only a YAML !!js expression (dsh-mcp-client README), - // which magpie doesn't write. Devin's docs name ${env:NAME} for OAuth + // connectRemoteServer). Devin's docs name ${env:NAME} for OAuth // fields only (extensibility/mcp/configuration.mdx), so its headers // and env aren't known to read one. return envSyntax{} @@ -324,6 +337,123 @@ var ( cursorRef = regexp.MustCompile(`\$\{env:([A-Za-z_][A-Za-z0-9_]*)\}`) ) +// dshEnvRef is the expression dsh reads a variable whole with. +var dshEnvRef = regexp.MustCompile(`^process\.env\.([A-Za-z_][A-Za-z0-9_]*)$`) + +// dshRef is a value with references as the !!js expression dsh evaluates +// to it, or false for one with none. A variable whole is process.env.NAME, +// as dsh-mcp-client's README writes one, so dsh leaves the server out +// while it's unset. In a longer value each variable is +// (process.env.NAME ?? "") and the text around it JSON strings, which +// JavaScript reads the same: what the user wrote is never code, and an +// unset variable is empty rather than "undefined". +func dshRef(v string) (dshJS, bool) { + if n := whole(v); n != "" { + return dshJS("process.env." + n), true + } + ms := envRef.FindAllStringSubmatchIndex(v, -1) + if len(ms) == 0 { + return "", false + } + var parts []string + lit := func(s string) { + if s != "" { + b, _ := json.Marshal(s) + parts = append(parts, string(b)) + } + } + at := 0 + for _, m := range ms { + lit(v[at:m[0]]) + parts = append(parts, `(process.env.`+v[m[2]:m[3]]+` ?? "")`) + at = m[1] + } + lit(v[at:]) + return dshJS(strings.Join(parts, " + ")), true +} + +// dshPart is one operand of a longer value dshRef writes: a JSON string, +// or a variable with "" in its place while unset. +var dshPart = regexp.MustCompile(`^(?:"(?:[^"\\]|\\.)*"|\(process\.env\.([A-Za-z_][A-Za-z0-9_]*) \?\? ""\))`) + +// dshUnref is the value an expression dshRef writes stands for, its +// references as the library writes them; false for any other expression, +// which is dsh's to work out. +func dshUnref(e dshJS) (string, bool) { + s := string(e) + if m := dshEnvRef.FindStringSubmatch(s); m != nil { + return "${" + m[1] + "}", true + } + var b strings.Builder + refs := 0 + for i := 0; i == 0 || s != ""; i++ { + if i > 0 { + rest, ok := strings.CutPrefix(s, " + ") + if !ok { + return "", false + } + s = rest + } + m := dshPart.FindStringSubmatchIndex(s) + if m == nil { + return "", false + } + if m[2] >= 0 { + b.WriteString("${" + s[m[2]:m[3]] + "}") + refs++ + } else { + var lit string + if json.Unmarshal([]byte(s[:m[1]]), &lit) != nil { + return "", false + } + b.WriteString(lit) + } + s = s[m[1]:] + } + if refs == 0 { + return "", false + } + return b.String(), true +} + +// dshRefs is m as dsh's entry has it, a value with a reference as its +// expression; nil stays nil. +func dshRefs(m map[string]string) map[string]any { + if m == nil { + return nil + } + out := make(map[string]any, len(m)) + for k, v := range m { + if e, ok := dshRef(v); ok { + out[k] = e + } else { + out[k] = v + } + } + return out +} + +// dshUnrefs is a dsh entry's headers or env with each expression dshRef +// writes read back as the value it stands for; any other value, and +// anything but a map, as it is. +func dshUnrefs(v any) any { + m, ok := v.(map[string]any) + if !ok { + return v + } + out := make(map[string]any, len(m)) + for k, x := range m { + if e, ok := x.(dshJS); ok { + if s, ok := dshUnref(e); ok { + out[k] = s + continue + } + } + out[k] = x + } + return out +} + // fromAgent is a value the agent has, with its references as the library // writes them. func fromAgent(syntax refSyntax, v string) string { diff --git a/internal/library/envref_test.go b/internal/library/envref_test.go index 651fbf10..0bacc27a 100644 --- a/internal/library/envref_test.go +++ b/internal/library/envref_test.go @@ -110,7 +110,7 @@ func TestEnvRefsLeftOut(t *testing.T) { cmd := Server{Name: "fs", Transport: "stdio", Command: "npx", Env: map[string]string{"MY_TOKEN": "${MY_TOKEN}"}} plain := Server{Name: "lit", Transport: "stdio", Command: "npx", Env: map[string]string{"K": "V"}} for _, f := range []mcpFile{{Format: fmtAntigravity}, {Format: fmtKimi}, {Format: fmtCline}, {Format: fmtZCode}, - {Format: fmtDevin}, {Format: fmtDesktop}, {Format: fmtDsh}, {Format: fmtPi, Literal: true}} { + {Format: fmtDevin}, {Format: fmtDesktop}, {Format: fmtPi, Literal: true}} { if err := f.supports(&cmd); !errors.Is(err, errNoEnvRef) { t.Errorf("%d %v: command: %v", f.Format, f.Literal, err) } @@ -274,3 +274,117 @@ func TestEnvRefsExpand(t *testing.T) { t.Errorf("remote: %+v", h) } } + +// #1435: DeepSeek Harness is given a reference as the !!js expression it +// evaluates, beside the user's own rows and comments; the entry reads back +// as the library has it, so a sync leaves it, and none of the text around +// a reference is ever written as code. +func TestEnvRefsDsh(t *testing.T) { + h := sandbox(t) + d := filepath.Join(h, "dsh-home") + t.Setenv("DSH_HOME", d) + p := filepath.Join(d, "profiles/web/cordis.patch.yml") + // the header dsh 0.2.0-rc.2 writes a new profile, and a row as + // dsh-mcp-client's README gives one + head := "# Your patch layer for this dsh profile, applied after every bundle layer:\n# a top-level YAML array of loader patch entries (id-targeted config\n# overrides, disables, and insert lists; `!!js` expressions allowed).\n" + user := head + "- id: llm-deepseek\n config:\n thinking: enabled\n" + + "- insert:\n - id: mcp-github\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: github\n transport: stdio\n command: npx\n args: ['-y', '@modelcontextprotocol/server-github']\n env:\n GITHUB_TOKEN: !!js process.env.GITHUB_TOKEN\n cwd: !!js process.cwd()\n" + write(t, p, user) + + odd := "a\"b`c` ${ \\ ${1} $${X_1}\n
'); process.exit(1); ('" + web := Server{Name: "web", Transport: "http", URL: "https://example.com/mcp", Agents: []string{"dsh"}, + Headers: map[string]string{"Authorization": "Bearer ${MY_TOKEN}", "X-Static": "literal", "X-Odd": odd, "X-Two": "${A_1}${B_2}"}} + fs := Server{Name: "fs", Transport: "stdio", Command: "npx", Args: []string{"-y", "fs"}, Agents: []string{"dsh"}, + Env: map[string]string{"MY_TOKEN": "${MY_TOKEN}", "PLAIN": "x", "MIX": "pre-${MY_TOKEN}-post"}} + for _, s := range []Server{web, fs} { + if r, err := SaveServer("", s); err != nil || len(r.Problems) != 0 { + t.Fatalf("%s: %+v %v", s.Name, r, err) + } + } + got := read(t, p) + for _, w := range []string{ + "MY_TOKEN: !!js process.env.MY_TOKEN\n", + `Authorization: !!js "\"Bearer \" + (process.env.MY_TOKEN ?? \"\")"`, + `MIX: !!js "\"pre-\" + (process.env.MY_TOKEN ?? \"\") + \"-post\""`, + `X-Two: !!js "(process.env.A_1 ?? \"\") + (process.env.B_2 ?? \"\")"`, + `PLAIN: "x"`, `X-Static: "literal"`, + "- id: llm-deepseek\n config:\n thinking: enabled\n", + "GITHUB_TOKEN: !!js process.env.GITHUB_TOKEN\n cwd: !!js process.cwd()\n", + } { + if !strings.Contains(got, w) { + t.Errorf("no %s in:\n%s", w, got) + } + } + if !strings.HasPrefix(got, head) || strings.Contains(got, "${MY_TOKEN}") { + t.Errorf("patch list:\n%s", got) + } + + f := targetByID("dsh").MCP + es, err := f.entries() + if err != nil { + t.Fatal(err) + } + // each expression is JSON strings and variables, nothing else, and + // stands for the library's value + hs, _ := es["web"]["headers"].(map[string]any) + for k, v := range hs { + e, ok := v.(dshJS) + if !ok { + continue + } + if back, ok := dshUnref(e); !ok || back != web.Headers[k] { + t.Errorf("%s: %s reads back as %q", k, e, back) + } + } + back, err := f.read() + if err != nil { + t.Fatal(err) + } + for _, s := range []Server{web, fs} { + if b := back[s.Name]; b == nil || !b.same(&s) { + t.Errorf("%s read back as %+v", s.Name, b) + } + } + // the user's row is the library's ${NAME} too + if g := back["github"]; g == nil || g.Env["GITHUB_TOKEN"] != "${GITHUB_TOKEN}" { + t.Errorf("github read as %+v", g) + } + v, err := Read(nil) + if err != nil { + t.Fatal(err) + } + for _, a := range v.Agents { + if a.ID == "dsh" && a.NoEnvRefs { + t.Error("dsh: noEnvRefs") + } + } + if r, err := Sync(); err != nil || len(r.Problems) != 0 || len(r.Changed) != 0 { + t.Errorf("sync: %+v %v", r, err) + } + if after := read(t, p); after != got { + t.Errorf("a sync rewrote it:\n%s", after) + } +} + +// dshRef's expression for any value reads back as that value, and one +// that isn't dshRef's is dsh's own. +func TestDshRefRoundTrip(t *testing.T) { + for _, v := range []string{"${A}", "x${A}", "${A}x", "${A}${B}", "\"${A}\"", "`${A}`", "\\${A}\\", "${A} ?? \"\"", "$${A}", "${1}${A}", "<&>${A}", "
${A}\x00"} { + e, ok := dshRef(v) + if !ok { + t.Fatalf("%q: no expression", v) + } + if back, ok := dshUnref(e); !ok || back != v { + t.Errorf("%q: %s reads back as %q", v, e, back) + } + } + if _, ok := dshRef("no ${ref here"); ok { + t.Error("a value with no reference has an expression") + } + for _, e := range []dshJS{"process.cwd()", `"lit"`, `process.env.A + "x"`, `"a" + (process.env.A ?? ""); process.exit()`, + `(process.env.A ?? "") + "x"`, `process.env["A"]`, `require('os').homedir()`, ``} { + if s, ok := dshUnref(e); ok { + t.Errorf("%s read as %q", e, s) + } + } +} diff --git a/internal/library/library_test.go b/internal/library/library_test.go index 09316e7e..35a40df1 100644 --- a/internal/library/library_test.go +++ b/internal/library/library_test.go @@ -1184,11 +1184,17 @@ func TestDshServerName(t *testing.T) { func TestDshImportKeepsJS(t *testing.T) { h := sandbox(t) p := filepath.Join(h, ".dsh/profiles/web/cordis.patch.yml") - write(t, p, "- insert:\n - id: mcp-engram\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: engram\n transport: stdio\n command: engram\n cwd: !!js process.cwd()\n env:\n HOME: !!js process.env.HOME\n") + write(t, p, "- insert:\n - id: mcp-engram\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: engram\n transport: stdio\n command: engram\n cwd: !!js process.cwd()\n env:\n HOME: !!js require('os').homedir()\n") v, _ := Read(nil) if len(v.FoundServers) != 0 { t.Errorf("an env dsh works out read as a server: %+v", v.FoundServers) } + // a variable read whole is the library's reference (#1435) + write(t, p, "- insert:\n - id: mcp-engram\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: engram\n transport: stdio\n command: engram\n cwd: !!js process.cwd()\n env:\n HOME: !!js process.env.HOME\n") + v, _ = Read(nil) + if len(v.FoundServers) != 1 || v.FoundServers[0].Server.Env["HOME"] != "${HOME}" { + t.Errorf("found: %+v", v.FoundServers) + } write(t, p, "- insert:\n - id: mcp-engram\n name: '@deepseek-ai/dsh-mcp-client'\n config:\n serverName: engram\n transport: stdio\n command: engram\n cwd: !!js process.cwd()\n") ok(t)(ImportServer("engram")) ok(t)(SaveServer("engram", Server{Name: "engram", Transport: "stdio", Command: "engram", Args: []string{"mcp"}, Agents: []string{"dsh"}})) diff --git a/internal/library/mcp.go b/internal/library/mcp.go index 4c29b7fa..1e93ccb8 100644 --- a/internal/library/mcp.go +++ b/internal/library/mcp.go @@ -591,16 +591,21 @@ func (f *mcpFile) encode(s *Server) ordered { add("tool_timeout_sec", t) } case fmtDsh: + // a reference is a !!js expression dsh evaluates (dshRef) add("serverName", s.Name) if s.Remote() { add("transport", "streamable-http") add("url", s.URL) - optional("headers", s.Headers) + if len(s.Headers) > 0 { + add("headers", dshRefs(s.Headers)) + } } else { add("transport", "stdio") add("command", s.Command) add("args", list(s.Args)) - optional("env", s.Env) + if len(s.Env) > 0 { + add("env", dshRefs(s.Env)) + } } } return o @@ -705,17 +710,26 @@ func (f *mcpFile) decode(name string, m map[string]any) (*Server, bool) { local(str(m, "command"), m["args"], m["env"]) } case fmtDsh: - // a value dsh works out itself (!!js) isn't one magpie can hold + // a variable read as dshRef writes it is a reference; any other + // value dsh works out itself (!!js) isn't one magpie can hold + headers, env := dshUnrefs(m["headers"]), dshUnrefs(m["env"]) for _, k := range owned[fmtDsh] { - if hasJS(m[k]) { + v := m[k] + switch k { + case "headers": + v = headers + case "env": + v = env + } + if hasJS(v) { return nil, false } } switch str(m, "transport") { case "stdio": - local(str(m, "command"), m["args"], m["env"]) + local(str(m, "command"), m["args"], env) case "streamable-http": - remote("http", str(m, "url"), m["headers"]) + remote("http", str(m, "url"), headers) } case fmtHermes, fmtKimi, fmtDevin, fmtAlma: // each takes a url over a command when an entry has both