env understands ${VAR}, because headers two methods away already did

CHANGELOG.md

@@ -6,6 +6,7 @@ All notable changes to smith. The format follows [Keep a Changelog](https://keep
 
 ### Added
 
+- **`${VAR}` works in a stdio server's `env`, as it already did in an HTTP server's `headers`**: `{"env": {"GITHUB_TOKEN": "${GITHUB_TOKEN}"}}` in `mcp.json` now hands the child what smith's own environment holds, instead of the fifteen literal characters `${GITHUB_TOKEN}` — which a server receives as a token and fails on somewhere far from the cause. The two halves of an entry had drifted: `headers` was given the expansion when HTTP servers arrived, `env` two methods away kept taking values verbatim, and the only ways left to give a subprocess a secret were to write it into a file that gets committed or to rely on the child inheriting smith's entire environment. The second is what #109 is about stopping, and it cannot be stopped while there is no deliberate way to pass one — so this comes first. One implementation serves both now, so a `${VAR}` cannot come to mean two things depending on which half of an entry it was written in, and the warning for an unset variable names the entry it was written in — `env 'GITHUB_TOKEN'` where before there was no warning at all, `header 'Authorization'` exactly as before, its closing words now reading *using an empty value* rather than *sending it empty*, which is the one thing about this that an existing HTTP user will notice. Only the exact `${NAME}` shape is a reference: a bare `$`, a `$5` and a `${not-a-name}` are left as written, and numbers and booleans keep arriving as the strings they obviously mean, so `"PORT": 8080` still needs no quotes (#122).
 - **Plugins & marketplaces, in Claude Code's own format**: `smith plugin marketplace add owner/repo` registers a marketplace — a repository or a directory with `.claude-plugin/marketplace.json` — and `smith plugin install <plugin>@<marketplace>` copies one of its plugins into `~/.smith/plugins/installed/`, from where its `skills/<name>/SKILL.md`, its root `SKILL.md` and its `agents/*.md` load like any other. `marketplace list|remove|update` and `plugin list|uninstall|update` complete the set; removing a marketplace removes the plugins that came from it, and a version that has not moved reports "up to date" instead of copying again. A plugin skill is `<plugin>:<skill>` and a plugin agent `<plugin>:<agent>` — the namespaced name is the guaranteed address, so installing a plugin can never change what an existing `/deploy` means. The bare name works too, but only where no other source claims it; where one does, the clash is reported once and only the full name resolves. `smith skills list` and `smith agents list` name the origin of every plugin entry. Deliberately not loaded, and said out loud rather than dropped: `hooks` — code execution outside the approval gate, which needs the trust store's digest model first — plus `.mcp.json`/`mcpServers`, `lspServers`, `commands`, themes and monitors, each named in a warning and in the install summary. `npm` and `command` sources are refused permanently, `git-subdir` and `archive` are Phase 2, and a marketplace source may only be `https://`, `owner/repo` or a local path: `ext::` is remote code execution and the rest cannot be checked. A source leaving the marketplace root, a name that could not be a directory and a symlink inside a plugin tree are all refused — a marketplace is third-party data. Startup pays one directory read and nothing else, because the `installed/<marketplace>/<plugin>/` path segments *are* the provenance — and nothing a plugin brought with it warns on that path either: a marketplace ships definitions written for another harness by the dozen, so what smith could not read in one is said by `smith plugin install`, and again on demand by `smith skills list` and `smith agents list`, while a definition you wrote yourself keeps warning at startup exactly as before. `smith doctor` draws the same line by volume: your own broken files in full, a plugin's counted by the reason they share (#98).
 - **`smith update` replaces the binary from the newest release**: it reads the latest tag from the GitHub API, compares it against `Smith::VERSION`, downloads the archive for this platform over HTTPS, verifies its SHA-256 against the release's new `SHA256SUMS`, and renames the new file over the old one in the same directory, which is the only replacement a running executable survives. `--check` reports and changes nothing. The download runs through the same post-DNS SSRF guard `web_fetch` uses, plus three rules of its own: a non-https URL is refused rather than upgraded, since it came from an API answer and not from a human; the redirect off `github.com` to `objects.githubusercontent.com` **is** followed, where `web_fetch` refuses a cross-host hop, so an allow-list of GitHub's own hosts takes over that job and is applied afresh on every one of at most five hops; and a `smith` member that is not a plain regular file is refused, because a symlinked one would be chmod'ed and renamed straight through the link. Releases now carry a `SHA256SUMS`, and one that does not is refused unless it predates this feature or `--allow-unverified` is passed. Release builds are stamped as such at compile time, so a `make build` from a working tree refuses to overwrite itself instead of destroying a build nothing can reproduce — as do a Homebrew or Nix install, a distribution directory, and a directory the current user cannot write, each naming the command to run instead. A build newer than the newest release is never downgraded (#96).
 - **`/model` switches the model mid-session**: bare it reports the model and provider in use, with a name it switches the model from the next request onward — the provider client, its API key and its connection stay as they are, which is all `-m` ever decided at startup. The new model is written to the session straight away, so `smith resume` comes back on it, and subagents spawned afterwards follow it. `--max-budget-usd` now adds each turn up at the rates in force when that turn was billed, so a switch cannot re-price money already spent. No allow-list stands in the way of a model released next week — a name must be a single word that is not a flag, a path or a provider, and anything else the provider rejects at the next request as an ordinary turn error. One session still records one model, so a session that switched is reported under the model it ended on by `smith stats` and the `COST` column (#94).

README.md

@@ -1184,7 +1184,7 @@ Servers go in `.smith/mcp.json` (project) or `~/.smith/mcp.json` (global), delib
     "filesystem": {
       "command": "npx",
       "args": ["-y", "@modelcontextprotocol/server-filesystem", "/path/to/project"],
-      "env": { "FOO": "bar" }
+      "env": { "FOO": "bar", "GITHUB_TOKEN": "${GITHUB_TOKEN}" }
     },
     "everything": {
       "type": "http",
@@ -1195,7 +1195,7 @@ Servers go in `.smith/mcp.json` (project) or `~/.smith/mcp.json` (global), delib
 }
 ```
 
-An entry with a `url` is an HTTP server — `type: "http"` may say so, and `sse` is accepted as an alias; everything else is a subprocess. `${VARIABLE}` in a header value is looked up in smith's environment at startup, so a token does not have to stand literally in the file; an unset one warns rather than being sent as `${...}`.
+An entry with a `url` is an HTTP server — `type: "http"` may say so, and `sse` is accepted as an alias; everything else is a subprocess. `${VARIABLE}` in a header value **or in an `env` value** is looked up in smith's environment at startup, so a token does not have to stand literally in a file you commit; an unset one warns and becomes empty rather than being passed on as `${...}`. Only that exact shape is a reference — a bare `$`, or `${not-a-name}`, is left as written. There is no way to escape one: a value that needs a literal `${NAME}` cannot have it, which is worth knowing for an `env` value meant as a template for the server to expand itself. An unset variable becomes empty rather than absent, and to a program reading it those are not the same thing — an empty `PYTHONPATH` is not no `PYTHONPATH` — so the startup warning is worth reading. Numbers and booleans in `env` are handed over as the strings they obviously mean, so `"PORT": 8080` needs no quoting.
 
 Global first, then project — a project entry of the same name replaces the global one. Entries with `"disabled": true`, or with a transport smith does not speak, are skipped with a word about why.
 

spec/mcp/server_config_spec.cr

@@ -37,6 +37,50 @@ describe Smith::MCP::ServerConfig do
       spec.stdio?.should be_true
     end
 
+    it "expands environment variables in env values, as headers already did" do
+      ENV["SMITH_MCP_TEST_ENV_TOKEN"] = "env-secret-42"
+
+      begin
+        spec = Smith::MCP::ServerConfig.parse(%({"mcpServers": {"x": {"command": "c", "env": {"GITHUB_TOKEN": "${SMITH_MCP_TEST_ENV_TOKEN}"}}}}), "mcp.json", IO::Memory.new).first
+        spec.env.should eq({"GITHUB_TOKEN" => "env-secret-42"})
+      ensure
+        ENV.delete("SMITH_MCP_TEST_ENV_TOKEN")
+      end
+    end
+
+    it "expands a variable in the middle of an env value, not only alone" do
+      ENV["SMITH_MCP_TEST_HOST"] = "example.com"
+
+      begin
+        spec = Smith::MCP::ServerConfig.parse(%({"mcpServers": {"x": {"command": "c", "env": {"ENDPOINT": "https://${SMITH_MCP_TEST_HOST}/v1"}}}}), "mcp.json", IO::Memory.new).first
+        spec.env.should eq({"ENDPOINT" => "https://example.com/v1"})
+      ensure
+        ENV.delete("SMITH_MCP_TEST_HOST")
+      end
+    end
+
+    it "warns about an unset env variable rather than passing the literal ${...}" do
+      # A server handed `${GITHUB_TOKEN}` as its token fails somewhere far from
+      # the cause. Empty is not better, but it is honest, and the warning says
+      # which entry to look at.
+      specs = [] of Smith::MCP::ServerSpec
+      warnings = capture do |io|
+        specs = Smith::MCP::ServerConfig.parse(%({"mcpServers": {"x": {"command": "c", "env": {"GITHUB_TOKEN": "${SMITH_MCP_MISSING_ENV}"}}}}), "mcp.json", io)
+      end
+
+      specs.first.env["GITHUB_TOKEN"].should eq("")
+      warnings.should contain("SMITH_MCP_MISSING_ENV")
+      warnings.should contain("not set")
+      warnings.should contain("env 'GITHUB_TOKEN'")
+    end
+
+    it "leaves an env value with no ${...} exactly as written" do
+      # The dollar sign is legal in a value and common in one. Only the full
+      # `${NAME}` shape is a reference; nothing else is touched.
+      spec = Smith::MCP::ServerConfig.parse(%({"mcpServers": {"x": {"command": "c", "env": {"PS1": "$ ", "COST": "$5", "RAW": "${not-a-name}"}}}}), "mcp.json", IO::Memory.new).first
+      spec.env.should eq({"PS1" => "$ ", "COST" => "$5", "RAW" => "${not-a-name}"})
+    end
+
     it "stringifies non-string env values, which real configs contain" do
       spec = Smith::MCP::ServerConfig.parse(%({"mcpServers": {"x": {"command": "c", "env": {"PORT": 8080, "DEBUG": true}}}}), "mcp.json", IO::Memory.new).first
       spec.env.should eq({"PORT" => "8080", "DEBUG" => "true"})
@@ -107,6 +151,10 @@ describe Smith::MCP::ServerConfig do
         specs.first.headers["Authorization"].should eq("Bearer ")
         warnings.should contain("SMITH_MCP_MISSING_TOKEN")
         warnings.should contain("not set")
+        # Which entry, not just which variable — the two sides share one
+        # implementation now, and nothing else pins that the header call site
+        # still names its own.
+        warnings.should contain("header 'Authorization'")
       end
 
       it "skips an http entry without a url" do

src/smith/mcp/server_config.cr

@@ -229,7 +229,7 @@ module Smith::MCP
         name: name,
         command: command,
         args: fields["args"]?.try(&.as_a?).try(&.compact_map(&.as_s?)) || Array(String).new,
-        env: string_map(fields["env"]?),
+        env: expand_env(fields["env"]?, name, warn_io),
         source: source
       )
     end
@@ -269,33 +269,87 @@ module Smith::MCP
         text = entry.as_s?
         next if text.nil?
 
-        result[key] = text.gsub(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/) do |match|
-          found = ENV[$1]?
-          if found.nil?
-            warn_io.puts "⚠️  MCP server '#{server}': header '#{key}' references #{match}, which is not set in the environment — sending it empty."
-            ""
-          else
-            found
-          end
-        end
+        result[key] = expand_vars(text, server, "header '#{key}'", warn_io)
       end
 
       result
     end
 
-    private def self.string_map(value : JSON::Any?) : Hash(String, String)
+    # The same for a stdio server's `env`, because the reason is the same one:
+    # a secret belongs in the environment and not in a file that gets
+    # committed. Until this, `headers` understood `${VAR}` and `env` two
+    # methods away did not, so the only ways to give a child process a token
+    # were to write it in plainly or to leave it to inherit smith's entire
+    # environment — the second of which is what #109 exists to stop, and
+    # cannot be stopped while there is no other way to pass one deliberately.
+    private def self.expand_env(value : JSON::Any?, server : String, warn_io : IO) : Hash(String, String)
       result = Hash(String, String).new
       table = value.try(&.as_h?)
       return result if table.nil?
 
       table.each do |key, entry|
         # Numbers and booleans appear in real configs (ports, flags); they mean
-        # the obvious thing as an environment variable.
-        text = entry.as_s? || entry.raw.try(&.to_s)
-        result[key] = text if text
+        # the obvious thing as an environment variable. Only a string is
+        # expanded, because only a string can hold a `${VAR}` to begin with:
+        # `8080` is a port, not a reference to anything.
+        if text = entry.as_s?
+          result[key] = expand_vars(text, server, "env '#{key}'", warn_io)
+        elsif raw = entry.raw.try(&.to_s)
+          result[key] = raw
+        end
       end
 
       result
     end
+
+    # One implementation for both, so a `${VAR}` cannot come to mean two
+    # different things depending on which half of an entry it was written in.
+    #
+    # `what` names the place rather than the kind, because a header and an env
+    # entry are both `key: value` and which one it was is the first thing
+    # somebody reading the warning needs to know.
+    #
+    # An unset variable becomes empty rather than being dropped, which the
+    # issue asked for and which is worth a note, because the two halves are
+    # not equally harmless. An empty header is inert. An empty *environment
+    # variable* is not the same thing as an absent one to the program reading
+    # it: an empty `PYTHONPATH` puts the working directory on the import path,
+    # an empty `PATH` or `HOME` is a different program than no `PATH` or
+    # `HOME`.
+    #
+    # The warning is the whole defence, and there is nothing behind it: an
+    # explicit entry *overrides* what the child would have inherited, so an
+    # empty expansion does not fall back to the inherited value, it replaces
+    # it. `clear_env` will not change that — what `clear_env` takes away is
+    # the fallback for a variable no entry names at all, which is a different
+    # hazard. This one is already as sharp as it is going to get.
+    #
+    # If it is ever traded the other way, the shape of the alternative is
+    # worth not rediscovering. `Process` reads a nil value as "leave this
+    # variable unset", and does it properly: a nil drops an *inherited* value
+    # too, so all three states — set, set-empty, absent — are reachable.
+    # Dropping the key costs widening `ServerSpec#env` and `spawn_server`'s
+    # signature to `Hash(String, String?)`, its body already passing the map
+    # straight through — plus the part that is not typing: `expand_vars`
+    # returns a `String` and so cannot say "this whole value was one unset
+    # reference". Only a value that is nothing else could become nil;
+    # `"a${UNSET}b"` has to stay a string. That decision is the work, not the
+    # signatures.
+    #
+    # There is no escape: a value that wants a literal `${NAME}` cannot have
+    # one, `$$` and a backslash included. Inherited from headers rather than
+    # decided here, and it matters more for `env`, where a value is likelier
+    # to be a template some other program means to expand itself.
+    private def self.expand_vars(text : String, server : String, what : String, warn_io : IO) : String
+      text.gsub(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/) do |match|
+        found = ENV[$1]?
+        if found.nil?
+          warn_io.puts "⚠️  MCP server '#{server}': #{what} references #{match}, which is not set in the environment — using an empty value."
+          ""
+        else
+          found
+        end
+      end
+    end
   end
 end