From c0d35505e1505f615a0ea2afd044407917f46eaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:03:57 +0200 Subject: [PATCH 1/4] feat: env understands ${VAR}, because headers two methods away already did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{"env": {"GITHUB_TOKEN": "${GITHUB_TOKEN}"}}` handed the child the eleven literal characters, which a server takes for a token and fails on somewhere far from the cause. `headers` got the expansion when HTTP servers arrived; `env` sat two methods away and kept taking values verbatim. That left two ways to give a subprocess a secret: write it into a file that gets committed, or let the child inherit smith's whole environment. The second is what #109 exists to stop, and it cannot be stopped while there is no deliberate way to pass one — which is why this is its own change and goes first. One implementation serves both, rather than the same regex written twice, so a `${VAR}` cannot come to mean two things depending on which half of an entry it was written in. The warning names the entry now — `env 'GITHUB_TOKEN'`, `header 'Authorization'` — since both are `key: value` and which one it was is the first thing a reader needs. Its closing words changed from "sending it empty" to "using an empty value", which reads right for both. Only the exact `${NAME}` shape is a reference. A bare `$`, a `$5` and a `${not-a-name}` are left as written, and only strings are expanded at all, because only a string can hold one: `8080` is a port. Numbers and booleans still arrive as the strings they obviously mean. Three of the four specs fail without the change. The fourth is the guard against over-eager expansion, so it passes either way by design. Closes #122 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 ++ README.md | 4 +-- spec/mcp/server_config_spec.cr | 44 ++++++++++++++++++++++++++++++ src/smith/mcp/server_config.cr | 50 ++++++++++++++++++++++++---------- 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9071ad..65895fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ 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 eleven 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 (`env 'GITHUB_TOKEN'`, `header 'Authorization'`) rather than only the variable. 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 @` copies one of its plugins into `~/.smith/plugins/installed/`, from where its `skills//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 `:` and a 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///` 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). diff --git a/README.md b/README.md index 233f680..c804ce1 100644 --- a/README.md +++ b/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. 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. diff --git a/spec/mcp/server_config_spec.cr b/spec/mcp/server_config_spec.cr index 8453cc8..edd68e4 100644 --- a/spec/mcp/server_config_spec.cr +++ b/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"}) diff --git a/src/smith/mcp/server_config.cr b/src/smith/mcp/server_config.cr index 91ae905..733b414 100644 --- a/src/smith/mcp/server_config.cr +++ b/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,55 @@ 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. + 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 From 863b0d84b34c4db2c837667f4c269feff5c08a3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:14:26 +0200 Subject: [PATCH 2/4] docs: name the missing escape and the empty-vs-absent hazard, and drop a claim that never happened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review, four corrections. Three are prose, one is a hazard worth writing down rather than fixing here. `${GITHUB_TOKEN}` is fifteen characters, not eleven. "The warning now names the entry rather than only the variable" never happened: main's header warning already read `header 'Authorization' references ${TOKEN}`. The only thing that changed for headers is the closing clause. `env` simply had no warning before. Said that way now. Two hazards named rather than papered over, both inherited from headers and both sharper in `env`: There is no escape for a literal `${NAME}` — not `$$`, not a backslash. The one input where old and new behaviour differ is `"a\${B}"`, which used to arrive verbatim and now loses its reference. An `env` value is far likelier than a header to be a template that some other program means to expand itself, so the README says there is no opt-out instead of only listing what is not a reference. An unset variable becomes empty rather than absent, which the issue asked for and which is not equally harmless on both sides. An empty header is inert; an empty environment variable is a different thing from a missing one to the program reading it — an empty PYTHONPATH puts the working directory on the import path. Today the inherited value is still there behind the warning. Under #109's clear_env it will not be, and the comment says the choice deserves revisiting there. The header spec now pins `header 'Authorization'` too. Both sides share one implementation, and nothing else stopped a later change from dropping the `what` argument at that call site with the suite still green. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 +-- README.md | 2 +- spec/mcp/server_config_spec.cr | 4 ++++ src/smith/mcp/server_config.cr | 14 ++++++++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65895fd..87eb2e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +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 eleven 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 (`env 'GITHUB_TOKEN'`, `header 'Authorization'`) rather than only the variable. 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). - +- **`${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. 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 @` copies one of its plugins into `~/.smith/plugins/installed/`, from where its `skills//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 `:` and a 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///` 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). diff --git a/README.md b/README.md index c804ce1..4e90c94 100644 --- a/README.md +++ b/README.md @@ -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 **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. Numbers and booleans in `env` are handed over as the strings they obviously mean, so `"PORT": 8080` needs no quoting. +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. diff --git a/spec/mcp/server_config_spec.cr b/spec/mcp/server_config_spec.cr index edd68e4..aac5a86 100644 --- a/spec/mcp/server_config_spec.cr +++ b/spec/mcp/server_config_spec.cr @@ -151,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 diff --git a/src/smith/mcp/server_config.cr b/src/smith/mcp/server_config.cr index 733b414..5fa4b28 100644 --- a/src/smith/mcp/server_config.cr +++ b/src/smith/mcp/server_config.cr @@ -308,6 +308,20 @@ module Smith::MCP # `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`. Today the warning is the whole defence, and the value the child + # would otherwise have inherited is still there behind it. Under #109's + # `clear_env` it will not be, and the choice deserves revisiting there. + # + # 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]? From b03c76102f5695f7e8f145546afecaec635aa3a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:20:20 +0200 Subject: [PATCH 3/4] docs: an explicit env entry overrides what the child would have inherited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught the handoff note to #109 stating the opposite of the truth, in the direction that makes today's build look safer than it is. Verified before fixing, parent `PYTHONPATH=/opt/inherited-libs`: explicit {"PYTHONPATH" => ""} -> child sees [] set-but-empty: 1 same, with clear_env: true -> child sees [] {"PYTHONPATH" => nil} -> absent: So an explicit entry *overrides* the inherited value rather than sitting in front of it: an empty expansion does not fall back, it replaces. And `clear_env` gives the identical result, so it takes away nothing that was softening this. What `clear_env` removes is the fallback for a variable no entry names at all — a different hazard, conflated with this one. The consequence is the opposite of what the comment implied: this is not a risk that arrives with #109, it is live now, on the current release, for anyone who writes an entry whose variable is unset. The third line is why the alternative is named too: `Process` reads a nil value as "leave it unset", so dropping the key costs widening `ServerSpec#env` to `Hash(String, String?)` and nothing else. The changelog also says what actually changed for an existing HTTP user — the closing words of the warning — since that is the only behaviour change in this PR for anyone not using `env`, and a runbook grepping for the old string deserves the pointer. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- src/smith/mcp/server_config.cr | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87eb2e4..56918fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +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. 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). +- **`${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 @` copies one of its plugins into `~/.smith/plugins/installed/`, from where its `skills//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 `:` and a 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///` 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). diff --git a/src/smith/mcp/server_config.cr b/src/smith/mcp/server_config.cr index 5fa4b28..a9ad2a4 100644 --- a/src/smith/mcp/server_config.cr +++ b/src/smith/mcp/server_config.cr @@ -314,9 +314,20 @@ module Smith::MCP # 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`. Today the warning is the whole defence, and the value the child - # would otherwise have inherited is still there behind it. Under #109's - # `clear_env` it will not be, and the choice deserves revisiting there. + # `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 alternative is cheap and worth + # not rediscovering: `Process` reads a nil value as "leave this variable + # unset", so dropping the key costs widening `ServerSpec#env` to + # `Hash(String, String?)` and nothing else — `spawn_server` passes the map + # straight through. # # There is no escape: a value that wants a literal `${NAME}` cannot have # one, `$$` and a backslash included. Inherited from headers rather than From ccb12fcfc8794c8e303c48be9a277ee79f927cc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Fri, 11 Sep 2026 01:25:44 +0200 Subject: [PATCH 4/4] docs: the alternative costs a decision, not two signatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from the final review round. "Costs widening `ServerSpec#env` and nothing else" was wrong twice over. `spawn_server`'s signature is annotated `Hash(String, String)` one file away and would have to widen with it — a reader taking "nothing else" literally meets a compile error in exactly the place this change's own premise warns about, a rule that drifted because its two halves live two methods apart. And the typing is the easy half. `expand_vars` returns a `String`, so it cannot report "this whole value was one unset reference" — only a value that is nothing but that could become nil, while `"a${UNSET}b"` has to stay a string. Deciding what a partial reference means is the work. A paragraph that exists so #109 need not rediscover the option should be right about what it would cost. The separator I reported as added last round went in one paragraph too late, so the hazard block still opened as a continuation of a sentence about argument naming. Co-Authored-By: Claude Opus 5 --- src/smith/mcp/server_config.cr | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/smith/mcp/server_config.cr b/src/smith/mcp/server_config.cr index a9ad2a4..d289ab2 100644 --- a/src/smith/mcp/server_config.cr +++ b/src/smith/mcp/server_config.cr @@ -308,6 +308,7 @@ module Smith::MCP # `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 @@ -323,11 +324,17 @@ module Smith::MCP # 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 alternative is cheap and worth - # not rediscovering: `Process` reads a nil value as "leave this variable - # unset", so dropping the key costs widening `ServerSpec#env` to - # `Hash(String, String?)` and nothing else — `spawn_server` passes the map - # straight through. + # 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