feat(server): mcpServers — declare an MCP server to Paddock itself (#691) - #701
Merged
Conversation
) Step 6 of #691, the last of the sequence. A top-level `mcpServers:` block in paddock.config.yaml, a SIBLING of `claude:` rather than a key inside it: `claude.mcpServers` asks whose servers this instance uses, this one says what it should have whether or not the machine has ever heard of it. Every keeper gets every declared server, with its `mcp__<name>__*` pattern added to that keeper's allow-list — without which it attaches and has every call auto-denied with no prompt and nothing in the logs. Precedence is host < declared < paddock's own. `paddock`/`paddock_manage` are reserved (an injected server owns that namespace); the browser server wins a `playwright` clash but warns by name. Secrets: `env:VAR_NAME` resolves from the environment anywhere a string is expected, the same indirection managementApi uses for client tokens. An unset reference DROPS the server rather than starting it unauthenticated. Nothing logged or served ever contains a value from the block — one renderer, `describeServer`, and no row in the Settings FIELDS table. Unlike `claude.mcpServers: host`, a declaration paddock cannot carry faithfully is an ERROR and is not attached (`headers`, `type: sse`, an unrecognised key, both/neither of command+url) — the user typed this file at us and can fix it. Only that server drops; the instance still boots. Also renames the step-5 types that said "Host" for values now used by two sources, and adds the `claude.mcpServers` row the Config screen was missing. Co-Authored-By: Claude <noreply@anthropic.com>
Deploying paddock with
|
| Latest commit: |
524789d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4fd5f41d.paddock-7u2.pages.dev |
| Branch Preview URL: | https://feat-691-user-mcp-servers.paddock-7u2.pages.dev |
Merged
edspencer
added a commit
that referenced
this pull request
Aug 6, 2026
… command line (#691) (#702) Follow-up to #691 step 6 (#701). `mcp-servers.ts` keeps a resolved credential out of every surface paddock owns — the boot log, an error, the Settings API — and that is complete. It is not the whole story, because of what happens downstream: // @herdctl/core, runner/runtime/cli-runtime.js const mcpServers = transformMcpServers(options.agent.mcp_servers); args.push("--mcp-config", JSON.stringify({ mcpServers })); // env and all A process argument is not private on Linux — /proc/<pid>/cmdline is world-readable with no hidepid, and ps prints it — so on the CLI runtime every declared server's `env` is legible to any local user for the lifetime of each `claude` invocation. The SDK runtime does not do this: it hands the same record to the SDK in-process and the spawned stdio server gets the value in its ENVIRONMENT, where /proc/<pid>/environ is owner-only, which is what Claude Code itself does. Which one runs is driveMode: `session` (default) is the SDK, `batch` is the CLI. Paddock cannot close this from here — the fix is upstream, where the Claude CLI's --mcp-config also accepts a file path — and refusing to attach the server would break the feature for the deployments most likely to need it. So it does what step 5 does about the fields the engine's schema cannot carry: refuses to be silent. An instance on `batch` with a credential-carrying declared server gets a WARNING naming the server; one on `session` gets the same as an info note, because one project pinning `driveMode: batch` brings the exposure back. Observed, not inferred: `test/integration/declared-mcp-argv.test.ts` drives a real turn through the CLI runtime and reads the token back out of the spawned process's argv (the fake `claude` now records `--mcp-config` and `--allowedTools` alongside the flags it already recorded). It is a CHARACTERISATION test — if that assertion ever fails, the engine has stopped doing this and the test and the warning should both be deleted rather than "fixed". The same test pins the other half in the opposite direction: `mcp__notion__*` reaches the same argv, which is what makes an attached server callable at all. Co-authored-by: HomeLab Agent <homelab-infra@valfenda.net> Co-authored-by: Claude <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Step 6 of #691 — the last item in the sequence, and the first one that is not a
claude:lever.Until now there was nowhere to say this.
claude.mcpServers: hostborrows the servers the machine already has, which is no help to the motivating case — someone running Paddock in a container who wants Notion in it has nothing to borrow. Every project's keeper gets every server declared here, with itsmcp__<name>__*pattern added to that keeper's allow-list.The design decisions, and the arguments for them
Sibling of
claude:, not a key inside it.claude.*answers whose X does this instance use? — every key has ahostside that borrows. This one answers what should this instance have?, which is a declaration, not an inheritance. Nesting it would have madeclaude.mcpServersmean two different things.Precedence:
host< declared < Paddock's own. A name declared here beats the same name inherited from~/.claude.json— including the host's directory scope, which itself beats the host's user scope — because this file is a statement about this instance while~/.claude.jsonis ambient machine state that happens to be readable. The narrower answer wins.Paddock's own still win the other side, and for two different reasons.
paddock/paddock_manage(the injectedsend_fileand self-management servers) are reserved names: they are materialised as an in-process HTTP bridge under the samemcp__<name>__*namespace, and two servers claiming one namespace has no defined winner, so declaring one is an error. The browser server merely wins — its flags are box-specific (Ansible's chromium engine,--no-sandboxfor an unprivileged LXC) andbrowserMcp: falseis right there in the same file — but it warns by name at boot rather than shadowing your declaration silently, which is what thehostpath does today.Instance-wide only; no
project.yamlkey. Three reasons, in increasing order of weight:projects: [a, b]filter on a server entry — with no migration and no change to anything downstream of the parse. Aproject.yamlkey is not: it is a second file, a second resolution order, and a second place to look for a token.project.yamllives inside the keeper's own working directory. Declaring MCP servers there would mean an agent can grant itself an arbitrary stdio server and the allow-list entry to call it, by editing a file it edits routinely. That is not a boundary Paddock enforces anywhere today (a keeper hasBash), so this is not a security claim — but there is a real difference between "possible with effort" and "the documented way to do it", and I would rather not make it the latter without someone asking.Shape:
{command?, args?, env?, url?}, matching herdctl'sMcpServerSchema— plustypeandheadersrecognised only in order to refuse them.Validation is strict here, and that is a deliberate inversion of step 5
#699 passes a host server it cannot carry faithfully, with a warning, on the grounds that a user who declared a server elsewhere should get it plus a warning rather than nothing plus a warning. The brief asks whether a hand-written declaration deserves better than a boot warning. It does, and the reasoning genuinely inverts: this file the user typed at Paddock, so an unusable declaration is a mistake they can fix, and a
headers:block that is silently dropped is an authentication failure that looks like a broken server.So these are errors, and the server is not attached:
headers:type: sseurlis mapped tohttpdownstream; same hash problemarg:forargs:would otherwise start the server with the wrong argv and no tracecommand,urlNever a boot failure: one bad server drops itself and the rest attach, matching
resolveManagementApiConfig. A typo in a capability must not take an instance down.Secrets
This is the first place in #691 where a user types a credential into Paddock's own config file, and that file is git-tracked and writable from the Config screen. So it borrows the rule
management-config.tsalready sets for management tokens, generalised:env:VAR_NAMEis a reference, resolved from the environment at boot — anywhere a string is expected (command, eachargsentry, eachenvvalue,url). One rule to learn, rather than a list of blessed fields. An unset variable drops that server with a warning naming the variable, rather than starting it without its credential.An inline value is a warning, not the hard error
managementApiuses. That is a considered difference:auth.tokenis unambiguously a credential, while an MCPenventry often is not (NOTION_VERSION: "2022-06-28"), so Paddock is guessing from the key's name — and guessing wrong must not refuse a working config. The warning fires for a credential-shaped key, or aurlcarrying a query string oruser:pass@.Nothing Paddock emits about this block ever contains a value from it. Every notice, warning and error routes through one function,
describeServer, which countsargsandenventries rather than printing them and strips a URL's query, fragment and userinfo. That function reducescommandto its basename, which the leak test is what forced — a full path is more useful in a log, but a rule with one remembered exception is not a rule.On the Settings surface, which the brief flagged:
instance-config.tsserialises eachFIELDSrow's value verbatim into the GET response, so a naive row for this block would publish tokens to any authenticated UI user. There is deliberately no such row, there is a comment saying why, and there is a test asserting it. What I did add there isclaude.mcpServers— read-only,own|host, no secret — which #699 shipped without: the Config screen showed four of the five levers.Two things that contradict #691, and one that contradicts #699
1. Both workarounds #691 offers in place of this feature are broken by the allow-list. #691 says a per-project
.mcp.jsonneeds "zero Paddock code", and thatCLAUDE_CONFIG_DIR=<paddock home> claude mcp add …"works today with no feature at all". Neither does. Paddock's fleet defaults carry an explicitallowed_tools, both runtimes deny any tool missing from it with no prompt, and herdctl auto-addsmcp__<name>__*patterns for injected servers only. A server Paddock did not attach itself therefore connects and has every call refused, with nothing in the logs — the same trap #699 hit, from the other direction. This is the mechanism #699 verified, applied to a case it did not consider; I have not observed it live (no MCP server exists on this box). It makes the workarounds worse than "clunky": they look like they worked. Docs updated to say so, and widening the allow-list from a project's own.mcp.jsonis a plausible follow-up nobody has filed.2.
.mcp.json's headless approval — #691's open caveat — is answered, and it is not the blocker. #691 flags that "approval lives in.claude.json(enabledMcpjsonServers), so headless approval needs checking". Checked, from the bundled CLI binary: apending.mcp.jsonserver is auto-approved when the process is non-interactive andprojectSettingsis an enabled setting source (if (dn() && ug("projectSettings")) return "approved"), and--setting-sources=projectmaps exactly toprojectSettings. Paddock is both. So approval was never the problem — finding 1 was.3. The step-5 types were named for one source and are now used by two.
HostMcpSource,HostMcpServer(s)andEMPTY_HOST_MCPall said "Host" for values that step 5 deliberately built source-agnostic, so they are renamed toMcpSources,McpServerDef(s)andEMPTY_MCP_SOURCES. That is most of the line count inclaude-mcp.tsandherdctl.ts; the behaviour there is unchanged.Otherwise #699's prediction held exactly:
mcpServersFor,mcpToolPatternand the allow-list widening inbuildAgentConfigneeded no logic change, andMcpSourcesgrew one field.Tests
Server 1813 → 1846 (+33, +2 files), web 941 unchanged. Every fixture synthetic;
SECRETis a made-up string that exists only in the two new test files, which is what makes the leak assertions exact rather than approximate.The one that matters is
test/integration/declared-mcp-servers.test.ts→ "puts a declared server on every keeper, with its allowlist pattern": through a realbuildApp()boot, it asserts both that the server lands onmcp_servers(the record both runtimes read) and thatmcp__notion__*lands onallowed_tools. Either half alone ships something that looks configured and does nothing.Next to it, "keeps the secret out of every API response and every log line" sweeps seven GET endpoints for the token, asserts the Settings table has no row for the block, and scans the complete set of strings this feature can log —
mcpServersDiagnostics.errors,.warnings, anddeclaredMcpNotices(...), which is exactly whatapp.tspasses to the logger — while also asserting those strings did mention both servers, so it cannot pass vacuously. And "survives a write from the Settings screen untouched" pins that a Config-screen PUT does not clobber the block, which would silently unconfigure every declared server on the next boot.Deliberately not done
PADDOCK_MCP_SERVERSenv var. A map of servers does not express as a scalar, and a JSON blob in a variable is worse to author than a file plusenv:references. File-only, exactly likemanagementApi.mcpServers— argued above; additive later if anyone wants it.claude.hooksmaterialisingsettings.json. Restart to pick up a new server; the docs say so..mcp.jsonallow-list gap (finding 1). It needs reading a file in each project's working directory and widening that keeper's allow-list from it — a different lever with its own trust question, and out of scope here.FieldSpecso the Config screen could list declared servers. It would be new leak surface to display something the boot log already says, secret-free.One thing that cannot be verified locally:
npm run typecheckfails onreapChatSessioninherdctl.ts—@herdctl/core@5.29.1installed against a declared^5.31.0. It is present onmainuntouched, and it also blocks local E2E. CI installs fresh.Completes the sequence in #691 (step 6 of 6). Deliberately not using a closing keyword: two things from that design are still open and live there — the plugins arm, blocked upstream per #699, and finding 1 above.
🤖 Generated with Claude Code