Skip to content

feat: turn stdio servers into a default-off function - #167

Open
nibalizer wants to merge 1 commit into
mainfrom
nibz/stdio_servers
Open

nibalizer wants to merge 1 commit into
mainfrom
nibz/stdio_servers

Conversation

@nibalizer

Copy link
Copy Markdown
Collaborator

Before this change, stdio servers were on by default. Now they must be manually enabled in the config file.

Stdio servers in front of an http api are largely going away except for edge cases, and atryum's capabilities with harnesses are the more exciting features anyway.

No need to default to this insecure (and unsecurable) function.

Also split docker images into 'minimal' and 'node' solving the issue of needing a node when trying to do stdio but not forcing it.

@even-steven

Copy link
Copy Markdown
Contributor

Review notes

The core gate looks sound. I traced all three exec.CommandContext sites in internal/mcp/client.go (invokeStdio, listToolsStdio, testStdio) and each is reachable only through Invoke / ListTools / TestConnection, all of which now check allowStdio — so there's no unguarded path to spawning a subprocess, including for legacy stdio rows already sitting in the DB. ForwardEnvelope already refused stdio, and Delete(?mode=disable) bypasses validateUpstream, so operators can still disable a legacy stdio server via the UI toggle. go test ./... passes on the branch.

Three things I'd want addressed:

1. atryum setup mcp can write a config that refuses to load

pkg/atryum/commands.go:283 — ensureStdioAllowed matches only the [mcp] header, never the value of allow_stdio.

If the config already contains:

[mcp]
allow_stdio = false

then addCalcUpstream appends the calc upstream (mode = "stdio", enabled = true), ensureStdioAllowed sees the [mcp] line and returns added=false, and the command prints added calc MCP upstream to ... and exits 0. The next atryum run then dies:

load config: upstream "calc" uses stdio mode, but stdio MCP servers are disabled by default; ...

Reproduced against this branch. This is exactly the failure the function's own doc comment says it exists to prevent. Checking for allow_stdio\s*=\s*true (and rewriting/erroring when it's explicitly false) would close it.

Secondary: the header-only match also misses [ mcp ], which is valid TOML — there it appends a second [mcp] table and the file stops parsing entirely.

2. Startup aborts over bootstrap-only stdio entries that are already inert

internal/config/config.go:251 — the new check fails Load for any enabled stdio [[upstreams]] entry, which takes down the whole process.

But those entries are bootstrap-only: once mcp_servers has rows, Resolver.BootstrapIfEmpty returns early and the config block is dead — it cannot spawn anything. So a deployment that bootstrapped months ago from atryum.example.toml (which ships shortcut, mode = "stdio", enabled = true) will, on upgrade, refuse to boot over a config line with no runtime effect. HTTP MCP proxying, the approval engine and the UI all go down until someone hand-edits the file.

The client-side gate already blocks the actual subprocess spawn, so a log.Printf warn-and-skip here would cover the real risk without the upgrade outage. If a hard failure is deliberate, it's worth calling out as a breaking change in the release notes.

3. atryum.example.toml ships with stdio re-enabled

atryum.example.toml:156 sets allow_stdio = true directly beneath a comment saying stdio is "DISABLED by default", and keeps the shortcut stdio upstream enabled. Both examples/opencode-plugin/README.md and examples/pi-extension/README.md tell users to run go run ./cmd/atryum run -config atryum.example.toml, and it's the natural template for a hand-written atryum.toml — so the shipped reference config undoes the posture this PR is establishing.

allow_stdio = false plus an enabled = false (or commented-out) shortcut block would document the knob without turning the feature back on.

Minor / non-blocking

  • The Servers UI still offers stdio in the mode dropdown with no indication it's disabled — saving returns a 400 with a good message, but nothing exposes allow_stdio to the frontend. Relatedly, with stdio off, PUT /api/v1/servers/{name} rejects any edit to an existing stdio row (even changing just its timeout).
  • Both build-push steps share the default type=gha cache scope, so the two targets' cache manifests overwrite each other run-to-run. Shared ui/builder stages still hit within a single run, so the practical cost is small — a distinct scope= on each would tidy it up.
  • Dockerfile stage order is correct: minimal is last, so a bare docker build -f Dockerfile.prod still yields the alpine image, and all three files referencing it pass an explicit target.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants