feat(core): plugins passthrough and full MCP server config (#444, #445) - #446
Merged
Conversation
Both issues are the same defect: herdctl narrowing what the Agent SDK supports underneath it, silently. #445 — `McpServerSchema` was `{command, args, env, url}`, a plain `z.object`, so every other key was stripped at `addAgent`; and `transformMcpServer` rewrote every `url` to `type: "http"`. Authenticated remote servers lost their bearer token and SSE servers were misconfigured as HTTP. Worse than a dropped field: Claude Code keys a remote server's stored OAuth token on `${name}|sha256({type,url,headers}).slice(0,16)`, so a stripped key also made a previously-authorised server unrecognisable. The schema now also accepts `headers`, an explicit `type` ("stdio" | "sse" | "http"), `timeout` and `alwaysLoad`, mirroring the SDK's own `McpServerConfig`. An explicit `type` wins; a bare `url` still infers `http`, so existing configs are unchanged. #444 — there was no channel at all for an embedder to name Claude Code plugins. New optional `plugins` field on an agent and on fleet `defaults`, plumbed through `toSDKOptions` to the SDK's `plugins` option. Entries are a bare path string or `{type: "local", path, skipMcpDiscovery?}`; the shorthand normalises to the object form. `mergeAgentConfig` is a per-field allowlist rather than a deep merge, so the defaults path is wired in explicitly and follows the `tools` convention (agent array replaces). Deliberately not widening the default `setting_sources`. The SDK's plugin auto-discovery is gated on `enabledPlugins`, a user-source settings key, so adding "user" to the default would enable it — but would also inherit user `CLAUDE.md`, `agents/`, `commands/` and `settings.json` hooks, which is a security boundary embedders (Paddock) deliberately hold closed. The per-agent `setting_sources` lever already exists for operators who want CLI parity; the explicit `plugins` list needs no such grant. Tests drive the whole trip — `addAgent` (real schema parse) → `getAgents` → `toSDKOptions` → the options object handed to the SDK's `query()` — because the stripping happened at the config boundary, one layer above where the existing adapter unit tests all passed. A compile-time contract test pins the hand-written SDK mirrors against the SDK's own types; `sdk-runtime.ts` widens them with `as Record<string, unknown>`, so nothing else in the build would catch the SDK drifting again. Co-Authored-By: Claude <noreply@anthropic.com>
`runtime: "cli"` agents would have silently ignored the new `plugins` field — the same silent-narrowing failure the field exists to fix, just one runtime over. Emit `--plugin-dir` per entry (`--plugin-dir-no-mcp` when `skipMcpDiscovery` is set), which is exactly what the Agent SDK does with its own `plugins` option, so a config behaves identically on either runtime. `mcp_servers` needed no equivalent change: the CLI runtime already builds `--mcp-config` from the shared `transformMcpServers`, so #445's `headers` and `type` flow through it for free — covered by a test rather than left to inference. Also annotate the string branch of `PluginSchema`'s union: without an explicit return type the two branches infer different object types and the union's output drops `skipMcpDiscovery`. Co-Authored-By: Claude <noreply@anthropic.com>
Documents the new `plugins` field (agent + fleet defaults, both entry forms, the replace-not-merge rule, and the Docker path caveat) and the `type`/`headers`/`timeout`/`alwaysLoad` MCP server fields. Two things worth their own callouts: why a remote server's `type` and `headers` matter beyond the wire (Claude Code keys the stored OAuth token on them), and that auto-discovered plugins need `setting_sources: ["user", …]` to be enabled — along with what else that grant pulls in, so the trade-off against naming plugins explicitly is visible. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR adds plugin configuration for agents and fleet defaults. It normalizes and forwards plugins through SDK and CLI runtimes. MCP configuration now supports explicit transports, headers, timeouts, and ChangesPlugin and MCP passthrough
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying herdctl with
|
| Latest commit: |
7b7dbd1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f47368f.herdctl.pages.dev |
| Branch Preview URL: | https://fix-444-445-plugins-and-mcp.herdctl.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/config/schema.ts`:
- Around line 522-535: Update McpServerSchema to enforce transport-specific
fields: require command for type "stdio" and url for types "sse" and "http",
while preserving inferred transport behavior when type is omitted. Adjust the
explicit transport cases in the agent configuration tests so stdio supplies
command and SSE/http supply url.
- Around line 528-530: The McpServerSchema currently permits authentication
headers on cleartext MCP URLs. Update McpServerSchema with validation or
transformMcpServer() so non-empty headers are accepted only when url uses
https://, while preserving header passthrough for secure URLs; also update the
MCP documentation to document this restriction.
In `@packages/core/src/runner/runtime/__tests__/cli-runtime.test.ts`:
- Around line 436-447: Update the test named “emits one --plugin-dir per plugin,
in order” to extract each --plugin-dir together with its immediately following
path and assert the complete ordered sequence of plugin flag/path pairs. Replace
the standalone jira containment check while preserving validation of both
plugins and their declared order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d1e3b839-04b2-44fd-811c-e71675cb15e6
📒 Files selected for processing (17)
.changeset/plugins-and-mcp-server-passthrough.mddocs/src/content/docs/architecture/runner.mddocs/src/content/docs/configuration/agent-config.mddocs/src/content/docs/configuration/fleet-config.mddocs/src/content/docs/configuration/mcp-servers.mdpackages/core/src/config/__tests__/agent.test.tspackages/core/src/config/__tests__/merge.test.tspackages/core/src/config/index.tspackages/core/src/config/merge.tspackages/core/src/config/schema.tspackages/core/src/runner/__tests__/mcp-and-plugin-passthrough.test.tspackages/core/src/runner/__tests__/sdk-type-contract.test.tspackages/core/src/runner/index.tspackages/core/src/runner/runtime/__tests__/cli-runtime.test.tspackages/core/src/runner/runtime/cli-runtime.tspackages/core/src/runner/sdk-adapter.tspackages/core/src/runner/types.ts
Comment on lines
522
to
535
| export const McpServerSchema = z.object({ | ||
| /** Transport. Inferred when omitted: `http` if `url` is set, else stdio. */ | ||
| type: z.enum(["stdio", "sse", "http"]).optional(), | ||
| command: z.string().optional(), | ||
| args: z.array(z.string()).optional(), | ||
| env: z.record(z.string(), z.string()).optional(), | ||
| url: z.string().optional(), | ||
| /** Request headers for `sse`/`http` servers — carries bearer / API-key auth. */ | ||
| headers: z.record(z.string(), z.string()).optional(), | ||
| /** Per-server tool-call timeout in ms. Values below 1000 are ignored by the SDK. */ | ||
| timeout: z.number().int().positive().optional(), | ||
| /** Always include this server's tools in the prompt instead of deferring them. */ | ||
| alwaysLoad: z.boolean().optional(), | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
version="0.3.215"
metadata="$(curl -fsSL "https://registry.npmjs.org/@anthropic-ai%2fclaude-agent-sdk/${version}")"
tarball="$(printf '%s' "$metadata" | jq -r '.dist.tarball')"
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL "$tarball" -o "$tmp/sdk.tgz"
tar -xzf "$tmp/sdk.tgz" -C "$tmp"
# Verify the SDK definitions for stdio, SSE, and HTTP MCP server configs.
rg -n -C 6 'Mcp(Stdio|SSE|Http)ServerConfig|McpServerConfig' "$tmp/package"Repository: edspencer/herdctl
Length of output: 14417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Schema definitions around McpServerSchema:"
sed -n '500,550p' packages/core/src/config/schema.ts
echo
echo "Relevant package references:"
if [ -f package.json ]; then cat package.json | sed -n '1,140p'; fi
if [ -f packages/core/package.json ]; then cat packages/core/package.json | sed -n '1,180p'; fi
echo
echo "Agent schema/test MCP server examples:"
rg -n -C 8 'mcp_servers|McpServer|mcpServers|stdio|http|sse|alwaysLoad' packages/core/src packages/core/src/config/__tests__/agent.test.ts 2>/dev/null || true
echo
echo "Check whether schema is validated from YAML/external config path:"
rg -n -C 4 'parse|McpServerSchema|agent|fleet|yaml|read.*config|mcp' packages/core/src/config packages/core/src | head -n 240Repository: edspencer/herdctl
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Schema definitions around McpServerSchema:"
sed -n '500,550p' packages/core/src/config/schema.ts
echo
echo "Agent test around MCP server examples:"
if [ -f packages/core/src/config/__tests__/agent.test.ts ]; then
sed -n '400,450p' packages/core/src/config/__tests__/agent.test.ts
else
fd -a 'agent.test.ts' . | head -5
fi
echo
echo "Focused MCP-related schema/test references:"
rg -n -C 4 'McpServerSchema|mcp_servers|mcpServers|McpServerConfig|type:\s*"stdio"|type:\s*"sse"|type:\s*"http"|command\s*[:=]|url\s*[:=]|headers' packages/core/src/config packages/core/src/runner packages/core/src/config/__tests__/agent.test.ts 2>/dev/null || true
echo
echo "Behavioral model of McpServerSchema accepts current shapes:"
node - <<'JS'
const schemas = process.argv.slice(2);
// We can only inspect static source text because dependency/runtime artifacts are unavailable.
const fs = require('fs');
const source = fs.readFileSync('packages/core/src/config/schema.ts','utf8');
const model = {
stdioWithOnlyUrl: { command: undefined, url: 'http://example.test' },
httpWithoutUrl: { type: 'http', url: undefined },
sseWithoutUrl: { type: 'sse', url: undefined },
stdioWithCommand: { command: 'node', args: ['./server.js'] },
};
for (const [name, value] of Object.entries(model)) {
const hasCommand = Boolean(value.command != null);
const hasUrl = Boolean(value.url != null);
const hasType = Boolean(value.type);
let ok = false, reason = '';
if (value.type === 'stdio') {
ok = hasCommand;
reason = hasCommand ? '' : 'stdio requires command';
} else if (value.type === 'http' || value.type === 'sse') {
ok = hasUrl;
reason = !hasUrl ? (`${value.type} requires url` || '') : '';
} else {
ok = true;
}
console.log(`${name}: ${ok ? 'consistent with SDK shapes' : 'rejected: ' + reason}`);
}
JSRepository: edspencer/herdctl
Length of output: 50374
Enforce the required MCP transport fields.
McpServerSchema accepts invalid SDK transport shapes, so invalid config can pass to the SDK. Require command when type: "stdio" is set, and require url when type: "sse" or type: "http" is set. Update packages/core/src/config/__tests__/agent.test.ts around the explicit type tests so stdio includes command while SSE/http include url.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/config/schema.ts` around lines 522 - 535, Update
McpServerSchema to enforce transport-specific fields: require command for type
"stdio" and url for types "sse" and "http", while preserving inferred transport
behavior when type is omitted. Adjust the explicit transport cases in the agent
configuration tests so stdio supplies command and SSE/http supply url.
Source: Coding guidelines
Comment on lines
528
to
+530
| url: z.string().optional(), | ||
| /** Request headers for `sse`/`http` servers — carries bearer / API-key auth. */ | ||
| headers: z.record(z.string(), z.string()).optional(), |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'sdk-adapter.ts' packages/core/src |
while IFS= read -r file; do
rg -n -C 8 'mcp_servers|mcpServers|headers|url' "$file"
doneRepository: edspencer/herdctl
Length of output: 2947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema relevant section =="
nl -ba packages/core/src/config/schema.ts | sed -n '500,545p'
echo
echo "== sdk-adapter transform section =="
nl -ba packages/core/src/SDKTransform.ts | sed -n '40,115p'
echo
echo "== references to transformMcpServers / SDKTransform =="
rg -n -C 4 'transformMcpServers|SDKTransform|McpServer' packages/core/src --glob '*.ts' --glob '*.tsx'Repository: edspencer/herdctl
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'schema.ts|sdk-adapter.ts|SDKTransform.ts' packages/core/src
echo
echo "== schema relevant section =="
awk 'NR>=500 && NR<=545 { printf "%6d %s\n", NR, $0 }' packages/core/src/config/schema.ts
echo
echo "== sdk adapter relevant section by candidate path =="
while IFS= read -r file; do
if awk 'NR==1{exit}' "$file"; then
echo "-- $file --"
awk 'NR>=40 && NR<=115 { printf "%6d %s\n", NR, $0 }' "$file"
fi
done < <(fd 'sdk-adapter.ts|SDKTransform.ts' packages/core/src)
echo
echo "== references =="
rg -n -C 3 'transformMcpServers|SDKTransform|McpServer|headers|url' packages/core/src --glob '*.ts' --glob '*.tsx' | awk 'NR<=220'Repository: edspencer/herdctl
Length of output: 2942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runner/sdk-adapter.ts transform section =="
awk 'NR>=40 && NR<=130 { printf "%6d %s\n", NR, $0 }' packages/core/src/runner/sdk-adapter.ts
echo
echo "== references to transformMcpServers / agent.mcp_servers =="
rg -n -C 4 'transformMcpServers|agent\.mcp_servers|\.mcp_servers|SDKTransform' packages/core/src --glob '*.ts' --glob '*.tsx'
echo
echo "== schema validation imports and agents config schemas =="
awk 'NR>=1 && NR<=90 { printf "%6d %s\n", NR, $0 }' packages/core/src/config/schema.tsRepository: edspencer/herdctl
Length of output: 22449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate docs mentioning MCP headers/authentication =="
rg -n -i -C 4 'mcp|headers|Authorization|bearer|sse|https://.*mcp|url' README.md docs packages --glob '*.md' --glob '*.mdx' | awk 'NR<=260'
echo
echo "== deterministic schema behavior probe =="
python3 - <<'PY'
from urllib.parse import urlparse
def current_transform(server):
result = {}
if server.get("type"):
result["type"] = server["type"]
elif server.get("url"):
result["type"] = "http"
if server.get("url"):
result["url"] = server["url"]
if server.get("headers") and len(server["headers"]) > 0:
result["headers"] = server["headers"]
return result
cases = [
{"name": "https auth", "url": "https://example.example/sse", "headers": {"Authorization": "Bearer secret"}},
{"name": "http auth no type", "url": "http://example.example/sse", "headers": {"Authorization": "Bearer secret"}},
{"name": "http auth explicit sse", "url": "http://example.example/sse", "headers": {"Authorization": "Bearer secret"}, "type": "sse"},
]
for case in cases:
server = dict(case)
cfg = current_transform(server)
print(cfg["name"], "url=", urlparse(cfg.get("url", "")).scheme, "type=", cfg.get("type"), "headers=", cfg.get("headers"))
PYRepository: edspencer/herdctl
Length of output: 24351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs content tree candidates =="
fd -a '\.md$|\.mdx$' docs/src/content/docs packages | awk 'NR<=120'
echo
echo "== focused MCP/header docs search =="
rg -n -i -C 5 'mcp|headers|Authorization|bearer|authenticated|authorization|url' docs/src/content docs packages --glob '*.md' --glob '*.mdx' --glob '*.yaml' | awk 'NR<=260'
echo
echo "== deterministic schema behavior probe =="
python3 - <<'PY'
from urllib.parse import urlparse
def current_transform(server):
result = {}
if server.get("type"):
result["type"] = server["type"]
elif server.get("url"):
result["type"] = "http"
if server.get("url"):
result["url"] = server["url"]
if server.get("headers") and len(server["headers"]) > 0:
result["headers"] = server["headers"]
return result
for name, case in {"http auth no type": {"url":"http://example.example/sse","headers":{"Authorization":"Bearer secret"}},
"http auth explicit sse": {"url":"http://example.example/sse","headers":{"Authorization":"Bearer secret"},"type":"sse"}}.items():
cfg = current_transform(case)
print(name, "url_scheme=", urlparse(cfg.get("url", "")).scheme, "type=", cfg.get("type"), "headers=", cfg.get("headers"))
PYRepository: edspencer/herdctl
Length of output: 25312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs MCP doc =="
awk 'NR>=1 && NR<=260 { printf "%6d %s\n", NR, $0 }' docs/src/content/docs/configuration/mcp-servers.md
echo
echo "== docs config header docs =="
rg -n -i -C 6 'mcp|headers|Authorization|bearer|sse|https|http|authenticated' docs/src/content/docs/configuration docs/src/content/docs/security docs/src/content/docs/reference --glob '*.md' --glob '*.mdx'Repository: edspencer/herdctl
Length of output: 50376
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External
Reachability path
● Entry
packages/core/src/config/__tests__/merge.test.ts
│
▼
● Hop
packages/core/src/config/merge.ts:132
Plugin
│
▼
● Sink
packages/core/src/config/schema.ts
Reject headers on cleartext MCP URLs.
McpServerSchema currently accepts headers with any url, and transformMcpServer() passes non-empty headers through before Claude Code makes the request. Add a Zod refine that requires https:// when headers is non-empty, or withhold auth headers from HTTP/sse servers before passthrough. Update the MCP documentation to state this restriction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/config/schema.ts` around lines 528 - 530, The
McpServerSchema currently permits authentication headers on cleartext MCP URLs.
Update McpServerSchema with validation or transformMcpServer() so non-empty
headers are accepted only when url uses https://, while preserving header
passthrough for secure URLs; also update the MCP documentation to document this
restriction.
Source: Coding guidelines
Comment on lines
+436
to
+447
| it("emits one --plugin-dir per plugin, in order", async () => { | ||
| const args = await captureArgs({ | ||
| plugins: [ | ||
| { type: "local", path: "/opt/plugins/slack" }, | ||
| { type: "local", path: "/opt/plugins/jira" }, | ||
| ], | ||
| }); | ||
|
|
||
| expect(args.filter((a) => a === "--plugin-dir")).toHaveLength(2); | ||
| expect(args[args.indexOf("--plugin-dir") + 1]).toBe("/opt/plugins/slack"); | ||
| expect(args).toContain("/opt/plugins/jira"); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert each plugin flag and path as an ordered pair.
The test can pass if the second --plugin-dir has the wrong adjacent path, because it only checks that /opt/plugins/jira occurs somewhere in args. Extract the plugin flag/path pairs and compare the complete ordered sequence.
Proposed test update
- expect(args.filter((a) => a === "--plugin-dir")).toHaveLength(2);
- expect(args[args.indexOf("--plugin-dir") + 1]).toBe("/opt/plugins/slack");
- expect(args).toContain("/opt/plugins/jira");
+ const pluginArgs = args.flatMap((arg, index) =>
+ arg === "--plugin-dir" ? [arg, args[index + 1]] : [],
+ );
+ expect(pluginArgs).toEqual([
+ "--plugin-dir",
+ "/opt/plugins/slack",
+ "--plugin-dir",
+ "/opt/plugins/jira",
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("emits one --plugin-dir per plugin, in order", async () => { | |
| const args = await captureArgs({ | |
| plugins: [ | |
| { type: "local", path: "/opt/plugins/slack" }, | |
| { type: "local", path: "/opt/plugins/jira" }, | |
| ], | |
| }); | |
| expect(args.filter((a) => a === "--plugin-dir")).toHaveLength(2); | |
| expect(args[args.indexOf("--plugin-dir") + 1]).toBe("/opt/plugins/slack"); | |
| expect(args).toContain("/opt/plugins/jira"); | |
| }); | |
| it("emits one --plugin-dir per plugin, in order", async () => { | |
| const args = await captureArgs({ | |
| plugins: [ | |
| { type: "local", path: "/opt/plugins/slack" }, | |
| { type: "local", path: "/opt/plugins/jira" }, | |
| ], | |
| }); | |
| const pluginArgs = args.flatMap((arg, index) => | |
| arg === "--plugin-dir" ? [arg, args[index + 1]] : [], | |
| ); | |
| expect(pluginArgs).toEqual([ | |
| "--plugin-dir", | |
| "/opt/plugins/slack", | |
| "--plugin-dir", | |
| "/opt/plugins/jira", | |
| ]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/runner/runtime/__tests__/cli-runtime.test.ts` around lines
436 - 447, Update the test named “emits one --plugin-dir per plugin, in order”
to extract each --plugin-dir together with its immediately following path and
assert the complete ordered sequence of plugin flag/path pairs. Replace the
standalone jira containment check while preserving validation of both plugins
and their declared order.
Owner
Author
|
Thanks — both CodeRabbit findings are declined, with reasons. Summarising here so the decision is on the record rather than buried in a resolved thread. 1. "Enforce the required MCP transport fields" — declinedThe suggestion is to The problem is what layer that schema validates. I checked this rather than assuming it. Both of these work today and would start throwing at parse time under the proposed refine: // Agent overrides only the transport; url comes from fleet defaults.
AgentConfigSchema.parse({ name: "a", mcp_servers: { linear: { type: "sse" } } })
// → merged with defaults {type: "http", url: "https://mcp.linear.app/sse"}
// → {type: "sse", url: "https://mcp.linear.app/sse"} ✅ valid only after merge// Agent adds only auth to a server the fleet defines.
AgentConfigSchema.parse({ name: "a", mcp_servers: { linear: { headers: { Authorization: "Bearer x" } } } })A cross-field requirement is meaningful on the merged config, not on a fragment. Enforcing it here would reject valid fleet configurations, so if this belongs anywhere it is a post-merge validation pass — out of scope for this PR, and worth its own issue. The PR body already lists the permissive schema as a deliberate non-goal; this is the concrete reason. 2. "Reject
|
Merged
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
1 participant
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.
Fixes #444. Fixes #445.
Both issues are the same defect class — herdctl narrowing what the Agent SDK supports underneath it, silently — and they touch adjacent code, so they ship together.
What changed
#445 —
McpServerSchemadroppedheadersandtype. The schema was{command, args, env, url}, a plainz.object, so every other key was stripped ataddAgent;transformMcpServerthen rewrote everyurltotype: "http". It now also acceptsheaders, an explicittype("stdio" | "sse" | "http"),timeoutandalwaysLoad, mirroring the SDK's ownMcpServerConfig. An explicittypewins; a bareurlstill infershttp, so existing configs are untouched.#444 — no plugins passthrough. New optional
pluginsfield on an agent config and on fleetdefaults, plumbed throughtoSDKOptionsto the SDK'spluginsoption. Entries are a bare path string or{type: "local", path, skipMcpDiscovery?}; the shorthand normalises to the object form.Both runtimes honour
plugins: the SDK runtime sets the option, and the CLI runtime emits--plugin-dir/--plugin-dir-no-mcpper entry — which is exactly what the SDK does with its own option, verified by reading the SDK's bundle. Leaving the CLI runtime out would have reproduced the same silent-narrowing bug one runtime over.Design notes for the downstream consumer
Paddock reaches both levers from ordinary agent config, so paddock#700 is unblocked as written:
agent.mcp_servers[name]now survives verbatim, so theclaude.mcpServers: hostmerge can stop stripping and stop warning.agent.pluginsis a plain array on the same agent config objectaddAgentalready takes.MCP server field names deliberately mirror the SDK's
McpServerConfigrather than herdctl's usual snake_case (hence camelCasealwaysLoad). An entry is passed through verbatim, so an operator — or Paddock reading~/.claude.json— can hand over a block unchanged. A translation layer is exactly where #445's fields got lost.I did not widen the default
setting_sources, and I'd argue against it. It would fix plugin enablement with no passthrough at all, but it pulls in every other user-source key — userCLAUDE.md,agents/,commands/,settings.jsonhooks — which is the boundary Paddock'sclaude.instructions/claude.hookslevers exist to hold closed. A blanket widening would silently undo that. The explicitpluginslist needs no such grant.One correction to #444
This already exists and already accepts
"user"—setting_sourcesonAgentConfigSchema, honoured bytoSDKOptionsahead of the["project"]default. #444's second blocker therefore already had an operator-facing lever before this PR; what was missing was only the passthrough and the documentation of the trade-off. There is a test in this PR asserting that behaviour, and it is one of the three (of twelve) that already passed onmain.Everything else in both issues held up. In particular the non-obvious claim in #444 — that the SDK discovers plugins and never enables them, rather than ignoring them — is consistent with what the SDK bundle does:
pluginsbecomes a--plugin-dirCLI flag directly, independent ofenabledPlugins, which is why the passthrough works without any settings-source change.Tests
The important one is
packages/core/src/runner/__tests__/mcp-and-plugin-passthrough.test.ts. It drives the whole trip —FleetManager.addAgent()(the realAgentConfigSchema.parse) →getAgents()→toSDKOptions()→ the options object actually handed to the SDK'squery(), withquerymocked to capture it. That framing is the point: the stripping happened at the config boundary, one layer above where the existingtransformMcpServerunit tests all sat happily passing. 9 of its 12 cases fail onmain; the 3 that pass are the deliberate no-regression guards (legacyurl→httpinference, the no-plugins case, and thesetting_sourcesbehaviour noted above).Also added:
sdk-type-contract.test.ts— compile-time assertions pinning the hand-writtenSDKQueryOptions/SDKMcpServerConfig/SDKPluginConfigmirrors against the SDK's own types.sdk-runtime.tswidens them withas Record<string, unknown>before callingquery(), so nothing else in the build would notice the SDK drifting — which is howtype?: "http"stayed wrong after the SDK grewsse. Reverting the union to"http"reproduceserror TS2344: Type '"sse"' does not satisfy the constraint '"http"'.--plugin-dir/--plugin-dir-no-mcpand forheaders+type: ssereaching--mcp-config.mergeAgentConfigtest fordefaults.plugins(that function is a per-field allowlist, not a deep merge, so a new field is silently dropped from defaults unless wired in explicitly — it is).What the tests do not prove
They prove the config survives to the SDK/CLI invocation boundary. They do not prove a planted plugin directory actually loads — that needs a real
clauderun against the API, which I did not do. The strongest evidence short of that is from reading the SDK bundle directly:pluginsis translated to--plugin-dir <path>(or--plugin-dir-no-mcpwhenskipMcpDiscoveryis set), and a non-localtypethrows. So the shape is right and reaches the CLI; whether the CLI then loads it is the SDK's contract, not herdctl's.Verification
pnpm typecheck(11/11),pnpm buildandpnpm lintall pass.pnpm testpasses except one pre-existing failure unrelated to this branch —state-manager"throws StateDirectoryCreateError when parent directory is not writable", which fails on my box because it runs as uid 0, wherechmod 0555does not prevent writes. It fails identically on unmodifiedmain; CI runs non-root, so it should be green here.Not done
plugins(the dashboard does not exposemcp_serverseither).McpServerSchemastays permissive — it does not enforce "urlxorcommand", or thatheadersis only meaningful for a remote server. Adding refinements could reject configs that parse today, and that is a separate call.pathis used as given, not resolved relative to the agent config file. For a dockerized agent it must resolve inside the container; documented rather than solved.tools(McpServerToolPolicy[]) is still not inMcpServerSchema— a nested union, not asked for, and no consumer for it yet.Release
Yes — Paddock needs a release to consume this. There is a changeset (
@herdctl/core,minor), so merging produces a Version Packages PR, and@herdctl/corehas to be published and Paddock's dependency bumped before paddock#700 can be built against it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation