feat(acp): support remote HTTP MCP servers - #4271
Conversation
|
@custard-pirate, I had already built #4164 before I came across this PR. It covers named stdio MCP servers, isolated per-server environments, compatibility with the existing command setting and secret-safe launch configuration. This PR covers remote HTTP transport, so the work is complementary, but both currently define the configuration shape that reaches ACP. The practical win is one reviewable configuration for both local and hosted tools, with credentials scoped to the individual server. I think the clean merge is a single versioned typed document with I opened #4301 with the broader product shape and proposed PR split. I would like to align these schemas rather than leave Buzz with two permanent configuration surfaces. |
|
Thanks — agreed. A single versioned, typed MCP configuration with I’m happy to align #4271 with #4164. I’ll review its configuration model and #4301 in more detail, then we can coordinate the shared types and PR boundary so the two changes remain complementary. In particular, I’d like to preserve per-server credential isolation for HTTP servers without putting concrete endpoints or secrets into portable agent templates. |
|
Thanks — I’ve reviewed the rebased #4164 and I’m aligned with this boundary. The tagged Assuming the maintainers are happy with that division of work, I’ll adapt #4271 to add HTTP as another transport in the same versioned document, reusing the existing config path, ordering, validation, and redaction behavior rather than retaining a separate HTTP configuration surface. Concrete URLs, headers, and credentials will remain Project-owned and outside portable agent templates. I’ll also ensure HTTP credential values receive the same no-log/no-diagnostics treatment as stdio environment values. |
c571bb2 to
981d39a
Compare
|
@wolfyy970 I have updated #4271 on top of the current #4164 head and removed the separate HTTP configuration surface. The HTTP transport now extends the same versioned ConfiguredMcpServer document and reuses its ordering, validation, compatibility, and redaction behavior. The HTTP-specific commit is 981d39a; the PR remains draft while the stack evolves. The local validation is green: 703 buzz-acp library tests, Clippy with warnings denied, formatting, and diff checks. Please flag any place where this has drifted from the schema or boundary you intend for #4164. |
|
@wpfleger96 @tlongwell-block an open question here is if I think it'd be great to configure tools once in Buzz and bind them to a team |
|
Not harness-only. I’m retiring #4735 as the implementation branch and restacking the binding work around the relay-canonical agent path in #4940. The useful seam remains: Project setup resolves into the same MCP document for any runtime. If your client work is ready, please put it up against #4271’s HTTP transport. |
|
Thanks for aligning this. I have replaced #4164 with the reviewed four-commit stack on current The stack now includes a real two-server, two-session agent proof. Please rebase your HTTP commit onto the new #4164 head and I will review it against the shared validation and redaction cases. |
981d39a to
ddce720
Compare
|
Rebased onto the replacement #4164 four-commit stack at 780561c. HTTP is now an additional variant of the shared buzz-core MCP launch document; harness-only URL/header credential resolution, ACP capability gating, and wire/observer redaction remain in place. Local validation passed: 713 buzz-acp unit tests, 256 buzz-core unit tests, strict Clippy for both crates with warnings denied, formatting, and diff checks. The HTTP commit is now ddce720 and is ready for the requested review against the shared validation and redaction cases. |
|
@wolfyy970 — #4271 has now been updated and rebased onto the replacement #4164 head at 780561c. The HTTP change is the single child commit ddce720 and extends the shared buzz-core ConfiguredMcpServer document rather than defining a parallel schema. The affected suites are green: 713 buzz-acp tests, 256 buzz-core tests, strict Clippy with warnings denied, formatting, and diff checks. It is ready for your requested review against the shared validation and redaction cases. |
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com>
Signed-off-by: custard-pirate <custard-pirate@users.noreply.github.com>
ddce720 to
3204b92
Compare
wolfyy970
left a comment
There was a problem hiding this comment.
Thanks for restacking this on #4164. The ACP HTTP wire shape is right, and the capability check keeps credentials out of session/new for unsupported adapters.
I found three security blockers and two lifecycle or observability regressions in the inline notes. There is also one composition bug: the legacy name collision check only examines Stdio, so an HTTP server can duplicate the legacy server name.
Please add a fake-adapter test that negotiates HTTP and captures the exact session/new payload. Because #4164 has not shipped, I would also fold the HTTP variant into its version 1 definition before either PR merges so version 1 never changes meaning.
| name: &str, | ||
| description: &str, | ||
| ) -> Result<String, ConfigError> { | ||
| let contents = std::fs::read_to_string(path).map_err(|error| { |
There was a problem hiding this comment.
These credential sources are read with unbounded read_to_string. Relative paths, symlinks, FIFOs, devices, shared permissions and oversized files all pass, unlike the protected top-level config reader. An imported config could read any effective-user-readable file and send it as a URL or header. Please keep the core HTTP variant resolved (url plus header values) and resolve Project-owned selectors before the handoff. If selectors stay here, use the same bounded, no-follow, regular-file, owner-only reader on Unix and Windows.
| "remote MCP server '{name}' has invalid URL: {error}" | ||
| )) | ||
| })?; | ||
| let private_http = parsed.scheme() == "http" |
There was a problem hiding this comment.
This accepts bearer headers over any RFC1918 or ULA host while rejecting loopback. A private LAN is not a confidential transport. Please require HTTPS except for literal loopback, and reject credentials on cleartext loopback unless an explicit operator policy allows them.
| "remote MCP server '{name}' header credential resolved empty or contains a control delimiter" | ||
| ))); | ||
| } | ||
| if !header.value_prefix.is_empty() { |
There was a problem hiding this comment.
The delimiter check happens before value_prefix is appended. A prefix containing CR, LF or NUL bypasses the check, and Debug prints the prefix unchanged. Validate the final composed value and redact the prefix with the credential.
| for value in servers | ||
| .iter() | ||
| .flat_map(McpServer::sensitive_values) | ||
| .filter(|value| !value.is_empty()) |
There was a problem hiding this comment.
Every stdio environment value becomes an unrestricted substring pattern. Values such as 1, /, true or prod will turn unrelated errors and observer strings into [REDACTED]. That regresses existing stdio visibility. Keep structural redaction, but add a safe policy for short or common values and a regression test.
| channel_id: Option<Uuid>, | ||
| channel_type: Option<&str>, | ||
| ) -> Result<String, AcpError> { | ||
| validate_mcp_transport_capabilities( |
There was a problem hiding this comment.
The pool already knows this capability after initialize, but waits until the first session to reject the configuration. The agent can appear ready and then fail every turn. Validate each initialized slot before publishing readiness, with the same failure during lazy wakeup.
Signed-off-by: custard-pirate <custard-pirate@users.noreply.github.com>
70e27b7 to
749ef0c
Compare
|
@wolfyy970 Thank you for the careful review — the security, lifecycle, and composition notes were very helpful. We addressed the findings in signed commit
Our additional deployment findings were that transport negotiation and tool approval need to remain separate fail-closed gates: unattended agents must pre-authorize only the required tools per agent/server, without a global Validation is clean: 257 I agree that HTTP should be folded into #4164 before either PR merges so v1 never changes meaning. GitHub denied this account permission to fast-forward your fork branch (403), so I left the exact two signed commits and validation handoff on #4164: |
Summary
transport: "http"variantDependency and scope
This PR is stacked directly on #4164. That PR owns the versioned document, stdio transport, ordering, validation, redaction foundation, and
BUZZ_ACP_MCP_COMMANDcompatibility input. The HTTP commits should be folded into #4164 before merge so schema version 1 ships with one stable meaning.Concrete endpoints and credentials remain project-owned configuration. The shared launch document accepts only final resolved URLs and header values; secret-file and environment selectors are intentionally outside the shared schema.
Security and behavior
Debugimplementations and are removed from wire logging and observer framesValidation
cargo test -p buzz-core --lib— 257 passedcargo test -p buzz-acp --lib— 737 passedcargo test -p buzz-acp --test config_env— passedcargo clippy -p buzz-core -p buzz-acp --all-targets -- -D warningscargo fmt --all --checkgit diff --checkThe fake-adapter coverage includes the exact mixed legacy-stdio plus HTTP
session/newpayload. Release acceptance additionally requires exercising a harmless tool through each transport with a real adapter and publishing through constrainedsend.