You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: turn stdio servers into a default-off function - #167
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.