fix(reborn): allow installed-local MCP over loopback HTTP - #6033
serrrfirat wants to merge 7 commits into
Conversation
github-actions
Bot
added
scope: docs
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughReborn adds installed-local MCP dispatch over plaintext HTTP at literal IPv4 loopback endpoints. Canonical endpoint parsing, exact egress policies, production-composition integration coverage, and MCP transport documentation were updated; hosted MCP discovery remains HTTPS-only. ChangesInstalled-local MCP loopback transport
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RebornIntegrationHarnessBuilder
participant HostRuntimeCapabilityHarness
participant MCP Egress Planner
participant MockMCPServer
RebornIntegrationHarnessBuilder->>HostRuntimeCapabilityHarness: configure installed-local MCP
HostRuntimeCapabilityHarness->>MCP Egress Planner: register manifest and derive exact loopback policy
MCP Egress Planner->>MockMCPServer: dispatch tools/call
MockMCPServer-->>HostRuntimeCapabilityHarness: return tool result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for plaintext HTTP Model Context Protocol (MCP) servers when the URL host is a literal IPv4 loopback address, while continuing to reject localhost, private LAN addresses, and plaintext remote endpoints. It updates the capability mapping, network policies, and documentation to reflect this change. The review feedback suggests two improvements: removing a redundant filter check in installed_local_mcp_loopback_target and centralizing the duplicated literal_loopback_host helper function to avoid code duplication across crates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub(crate) fn installed_local_mcp_loopback_target( | ||
| package: &ExtensionPackage, | ||
| ) -> Option<NetworkTargetPattern> { | ||
| (package.manifest.source == ManifestSource::InstalledLocal) | ||
| .then(|| hosted_http_mcp_endpoint(package)) | ||
| .flatten() | ||
| .filter(|endpoint| endpoint.is_loopback_http) | ||
| .map(|endpoint| endpoint.network_target()) | ||
| } |
There was a problem hiding this comment.
The .filter(|endpoint| endpoint.is_loopback_http) check is redundant here because hosted_http_mcp_endpoint already filters out non-loopback HTTP endpoints for InstalledLocal packages. We can safely remove this filter to simplify the code.
pub(crate) fn installed_local_mcp_loopback_target(
package: &ExtensionPackage,
) -> Option<NetworkTargetPattern> {
(package.manifest.source == ManifestSource::InstalledLocal)
.then(|| hosted_http_mcp_endpoint(package))
.flatten()
.map(|endpoint| endpoint.network_target())
}There was a problem hiding this comment.
Reviewed; no code change: INCORRECT — the repeated loopback predicate is deliberate defense-in-depth at the private-range exception boundary, so future endpoint-policy changes cannot silently widen the exception.
| fn literal_loopback_host(url: &url::Url) -> bool { | ||
| match url.host() { | ||
| Some(url::Host::Ipv4(address)) => address.is_loopback(), | ||
| Some(url::Host::Ipv6(_) | url::Host::Domain(_)) | None => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
The literal_loopback_host helper function is duplicated here and in crates/ironclaw_reborn_composition/src/extension_host/mcp.rs. To maintain drift-resistance and avoid duplicating domain knowledge locally, consider making this function public and centralizing it so both crates can reuse the same implementation.
References
- Prefer routing through centralized helper functions that define domain boundaries or strip transient fields (e.g.,
credential_owner_scope()) to maintain drift-resistance and avoid duplicating domain knowledge locally, even if it introduces minor performance overhead (like cloning) on non-hot paths.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — McpHttpEndpoint now owns the canonical literal-IPv4-loopback predicate and the runtime planner delegates to it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_extensions/src/hosted_mcp_discovery.rs`:
- Around line 112-117: Centralize the literal_loopback_host implementation so
only one canonical helper performs the IPv4 loopback check. Keep or move the
helper from crates/ironclaw_extensions/src/hosted_mcp_discovery.rs:112-117 into
a shared location, then replace the duplicate in
crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:202-207 with an
import and update its callers; preserve the existing behavior for IPv6, domain
hosts, and absent hosts.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f01d32d2-0a5e-4637-a6c6-d2913d59c201
📒 Files selected for processing (10)
FEATURE_PARITY.mdcrates/ironclaw_extensions/src/hosted_mcp_discovery.rscrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_host/mcp.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime/approval.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/extension_surface.rsdocs/capabilities/mcp.mddocs/extensions/building-a-tool.mddocs/reborn/contracts/mcp.md
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.67% — 311084 / 363131 lines Per-crate breakdown (60 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-6033 environment in ironclaw-ci-preview
|
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Restore local MCP connectivity in Reborn by permitting plaintext HTTP only for tightly pinned literal IPv4 loopback endpoints installed locally.
Shape: normal, no modifiers — a single non-stacked PR; 432 changed lines across 10 files, below mega/generated/mechanical thresholds.
Coverage: complete from an exact local-git comparison (c23277f...ef87044); buckets: 6 production, 4 docs, 0 tests, 0 generated/vendor, 0 CI, 0 config; no packetization; no oversized files; no failed reviewers or limitations.
Stats: 8 findings from 10 raw, 8 after overlap/same-line dedup, across 2 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
Conventions
- Medium Restrict
HostBundledendpoints to HTTPS (crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:178-181, confidence 100) — anchor:FEATURE_PARITY.md:340
The source match accepts loopback HTTP for every host-bundled package, exceeding the advertised InstalledLocal-only trust boundary. Also flagged by Approach and Bugs. - Medium Production MCP policy change lacks integration coverage (
crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:178-192, confidence 100) — anchor:AGENTS.md:100-102
The production-wired source → lifecycle → grant → private-range policy path has only crate-local tests, contrary to the repository's Integration-First rule.
Tests
- Medium Installed-local MCP dispatch lacks production-path integration coverage (
crates/ironclaw_reborn_composition/src/runtime/local_dev/extension_surface.rs:183-190, confidence 100) — anchor:extension_surface.rs:183
Existing integration coverage manually supplies a planner and policy, so it cannot catch failures in the newly changed production composition path. - Medium Cover denied
RegistryInstalledsource (crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:178-181, confidence 100) — anchor:mcp.rs:181
The explicit RegistryInstalled denial has no regression test using an otherwise valid loopback endpoint. - Low Composition-side IPv6 loopback rejection is untested (
crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:202-205, confidence 100) — anchor:mcp.rs:205
The composition-owned parser separately rejects IPv6, but its caller-level rejection cases omithttp://[::1]. - Low Default-port pinning is tested only below the planner caller (
crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:148-156, confidence 100) — anchor:mcp.rs:155
The helper test cannot detect the planner emitting an unpinnedport: Nonefor an omitted-port endpoint.
Local Patterns
- Low Rename the endpoint type now that it also represents installed-local MCP (
crates/ironclaw_reborn_composition/src/extension_host/mcp.rs:106, confidence 75) — anchor:mcp.rs:164-180
HostedMcpEndpointnow represents both host-bundled and installed-local endpoints, making the source-policy boundary harder to navigate.
Maintainability
- Medium Centralize MCP endpoint admission in the manifest owner (
crates/ironclaw_extensions/src/hosted_mcp_discovery.rs:98-117, confidence 95) — anchor:hosted_mcp_discovery.rs:98
Two crates now independently implement the same security-sensitive URL-shape and literal-loopback predicate, creating drift risk between discovery and dispatch.
| if let Some(policy) = gsuite_network_policy_for(&capability.provider) { | ||
| return policy; | ||
| } | ||
| if let Some(target) = &capability.local_mcp_loopback_target { |
There was a problem hiding this comment.
Medium — Installed-local MCP dispatch lacks production-path integration coverage.
No test in tests/integration/ installs and activates an InstalledLocal loopback MCP package and dispatches its capability through production composition. Existing tests/integration/mcp.rs coverage uses a HostBundled test package, StaticMcpHostHttpEgressPlanner, and a manually supplied loopback policy that explicitly bypasses the production lifecycle, egress planner, and network-policy path changed here. It therefore cannot catch failures to propagate local_mcp_loopback_target, mint the private-range exception, or reach the server through the newly enabled user-visible flow.
Fix: tests::integration::mcp::installed_local_loopback_mcp_dispatch_reaches_server_through_production_composition covering install, activation, scripted tool dispatch, and a recorded tools/call at the loopback server
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — installed_local_mcp_dispatches_through_production_composition now installs the local package, dispatches through production composition, and asserts the recorded MCP wire call and model-visible result. Verification: the targeted integration test passes.
| match package.manifest.source { | ||
| ManifestSource::HostBundled => Some(endpoint), | ||
| ManifestSource::InstalledLocal if endpoint.is_loopback_http => Some(endpoint), | ||
| ManifestSource::InstalledLocal | ManifestSource::RegistryInstalled => None, |
There was a problem hiding this comment.
Medium — Cover denied RegistryInstalled source.
The new source gate explicitly denies RegistryInstalled packages, and the PR claims this invariant, but the rejection tests construct every denied package as InstalledLocal. No adjacent or integration test exercises a registry-installed package using an otherwise valid literal-loopback HTTP endpoint, so a regression that grants this source the local exception would pass.
Fix: tests::extension_host::mcp::planner_rejects_registry_installed_loopback_http covering a RegistryInstalled package at http://127.0.0.1:4321/mcp and asserting an empty egress policy
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — planner_denies_registry_installed_loopback_mcp exercises an otherwise valid literal-loopback endpoint and confirms no egress target is granted.
| fn literal_loopback_host(url: &url::Url) -> bool { | ||
| match url.host() { | ||
| Some(url::Host::Ipv4(address)) => address.is_loopback(), | ||
| Some(url::Host::Ipv6(_) | url::Host::Domain(_)) | None => false, |
There was a problem hiding this comment.
Low — Composition-side IPv6 loopback rejection is untested.
The composition crate has its own literal_loopback_host implementation, separate from the similarly named function tested in ironclaw_extensions. Its planner rejection cases cover localhost, LAN IPv4, and remote HTTPS but omit http://[::1], so the explicit IPv6-denial constraint is not protected at the runtime egress planner that controls dispatch.
Fix: tests::extension_host::mcp::planner_rejects_installed_local_ipv6_loopback_http covering http://[::1]:4321/mcp and asserting no allowed target or credential injection
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — the planner caller rejection table now includes http://[::1]:4321/mcp and confirms the endpoint remains denied.
| && target.host_pattern.eq_ignore_ascii_case(&self.host_pattern) | ||
| && target.port == self.port | ||
| } | ||
|
|
||
| fn matches_url(&self, url: &str) -> bool { | ||
| Self::parse(url).is_some_and(|request_endpoint| request_endpoint == *self) | ||
| } | ||
|
|
||
| fn network_target(&self) -> NetworkTargetPattern { |
There was a problem hiding this comment.
Low — Default-port pinning is tested only below the planner caller.
The omitted-port test calls installed_local_mcp_loopback_target directly and asserts Some(80), while the side-effect-gating caller is RegistryMcpEgressPlanner::plan. The explicit-port planner test cannot detect a regression where the planner stops using the pinned target and emits port: None only for omitted-port URLs. A caller-level planner test is required by the repository's testing rule for transforms that gate network side effects.
Fix: tests::extension_host::mcp::planner_pins_omitted_loopback_http_port_to_80 covering a manifest and request at http://127.0.0.1/mcp and asserting the planner emits exactly port 80 with the private-range exception
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — planner_pins_installed_local_default_http_port drives RegistryMcpEgressPlanner::plan and asserts an omitted loopback HTTP port becomes exactly 80.
| HostedMcpEndpoint::parse(url) | ||
| let endpoint = HostedMcpEndpoint::parse(url)?; | ||
| match package.manifest.source { | ||
| ManifestSource::HostBundled => Some(endpoint), |
There was a problem hiding this comment.
Medium — Restrict HostBundled endpoints to HTTPS.
HostedMcpEndpoint::parse accepts literal-loopback HTTP, and this source match returns that endpoint for every HostBundled package. hosted_mcp_discovery.rs:102-104 independently enables the same host-bundled path. That conflicts with the stated installed-local-only scope recorded in FEATURE_PARITY.md:340 and the user-facing support statement in docs/capabilities/mcp.md:10-13, expanding the source/trust contract beyond the advertised behavior.
Fix: Reject HTTP endpoints for ManifestSource::HostBundled in both runtime planning and hosted discovery, leaving the loopback exception exclusively for InstalledLocal.
Also flagged by: approach/Medium, bugs/Medium
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — host-bundled loopback HTTP is rejected and covered by a planner caller test; the plaintext exception remains InstalledLocal-only.
| return None; | ||
| } | ||
| HostedMcpEndpoint::parse(url) | ||
| let endpoint = HostedMcpEndpoint::parse(url)?; |
There was a problem hiding this comment.
Medium — Production MCP policy change lacks integration coverage.
The assigned diff changes the production-wired path from extension source and runtime URL through capability grants, private-range policy, and MCP HTTP egress, but adds only crate-local tests. No tests/integration/ test drives an InstalledLocal manifest through the composed runtime to a loopback MCP server, and the PR body gives no reason that the integration tier cannot reach this path. Existing tests/integration/mcp.rs covers a directly wired mock MCP backend, not the new installation-source/lifecycle/policy projection.
Fix: Extend the Reborn MCP integration suite to install or restore an InstalledLocal loopback MCP package, invoke it through the composed runtime, and assert both the recorded HTTP call and exact network-policy seam.
There was a problem hiding this comment.
Reviewed; no code change: DUPLICATE and ALREADY ADDRESSED by the production-composed installed-local MCP integration test, which verifies dispatch, the wire request, and the returned tool result.
| NetworkTargetPattern { | ||
| scheme: Some(self.scheme), | ||
| host_pattern: self.host_pattern.clone(), | ||
| // NetworkTargetPattern::port = None matches any port. Pin the HTTP |
There was a problem hiding this comment.
Low — Rename the endpoint type now that it also represents installed-local MCP.
HostedMcpEndpoint and hosted_http_mcp_endpoint now represent both HostBundled and InstalledLocal packages, while the accompanying documentation deliberately broadens the concept to HTTP MCP. Keeping the old hosted-only names makes source-policy checks harder to navigate and invites future callers to assume this parser is restricted to host-bundled providers.
Fix: Rename the internal endpoint vocabulary consistently, for example HttpMcpEndpoint, http_mcp_endpoint, mcp_http_url_allowed, and mcp_http_network_policy, while retaining explicitly hosted names only for schema-discovery behavior that remains host-bundled.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — the canonical shared type is now McpHttpEndpoint, while McpEgressEndpoint is the accurately named runtime-policy wrapper.
| } | ||
|
|
||
| #[test] | ||
| fn hosted_mcp_recognizes_plaintext_only_for_literal_loopback() { |
There was a problem hiding this comment.
Medium — Centralize MCP endpoint admission in the manifest owner.
The accepted MCP URL shape is now independently implemented here and in extension_host/mcp.rs:114-207, including an identical literal-IPv4-loopback predicate. Discovery and runtime policy must agree on this security-sensitive fact, but future endpoint changes currently require editing two parsers in different crates; divergence would let a package pass discovery while being rejected or interpreted differently at dispatch.
Fix: Define one typed, side-effect-free HTTP MCP endpoint parser in ironclaw_extensions, which owns declarative runtime metadata, and have both hosted discovery and Reborn composition consume its parsed result. Keep source-specific admission at callers while deleting duplicate URL component checks.
There was a problem hiding this comment.
Reviewed; no code change: ALREADY ADDRESSED — endpoint parsing and exact URL identity now live in the manifest-owning ironclaw_extensions::McpHttpEndpoint; source-specific admission remains at host callers.
ef87044 to
9a7a0db
Compare
github-actions
Bot
added
size: XL
serrrfirat
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_reborn_composition/src/extension_host/mcp.rs`:
- Around line 127-131: Update allows_target to treat an audience port of None as
a wildcard and normalize omitted HTTP ports to the default port before comparing
with self.parsed.port. Preserve scheme and host matching, and add
RegistryMcpEgressPlanner::plan coverage for credential injection with both
explicit and omitted loopback ports.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68e8504d-2bcf-4a69-80fe-2589c06c5ecc
⛔ Files ignored due to path filters (4)
CHANGELOG.mdis excluded by!CHANGELOG.mddocs/zh/extensions/mcp.mdxis excluded by!docs/zh/**tests/fixtures/installed_local_mcp/schemas/mock-mcp/mock.input.v1.jsonis excluded by!tests/fixtures/**tests/fixtures/installed_local_mcp/schemas/mock-mcp/mock.output.v1.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (29)
FEATURE_PARITY.mdcrates/ironclaw_extensions/AGENTS.mdcrates/ironclaw_extensions/src/hosted_mcp_discovery.rscrates/ironclaw_extensions/src/lib.rscrates/ironclaw_extensions/src/mcp_http_endpoint.rscrates/ironclaw_reborn_composition/CLAUDE.mdcrates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rscrates/ironclaw_reborn_composition/src/extension_host/mcp.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime/approval.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/extension_surface.rsdocs/capabilities/mcp.mddocs/extensions/building-a-tool.mddocs/extensions/mcp.mdxdocs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.mddocs/reborn/contracts/extensions.mddocs/reborn/contracts/host-runtime.mddocs/reborn/contracts/mcp.mddocs/reborn/contracts/runtime-profiles.mddocs/reborn/contracts/runtime-selection.mddocs/reborn/how-to-port-tool-to-reborn.mdtests/integration/CLAUDE.mdtests/integration/mcp.rstests/integration/support/builder.rstests/integration/support/capability_backend.rstests/integration/support/harness/mod.rstests/integration/support/harness/options.rstests/integration/support/harness/profiles/mock_mcp.rstests/integration/support/harness_mcp.rs
…t-fix # Conflicts: # CHANGELOG.md # crates/ironclaw_extension_host/src/mcp.rs # crates/ironclaw_extensions/src/hosted_mcp_discovery.rs # crates/ironclaw_extensions/src/lib.rs # crates/ironclaw_reborn_composition/src/extension_host/extension_lifecycle.rs # crates/ironclaw_reborn_composition/src/factory.rs # crates/ironclaw_reborn_composition/src/runtime/approval.rs # crates/ironclaw_reborn_composition/src/runtime/local_dev/extension_surface.rs # docs/capabilities/mcp.md # docs/extensions/building-a-tool.md # docs/extensions/mcp.mdx # docs/internal/2026-06-26-legacy-vs-reborn-feature-comparison.md # docs/zh/extensions/mcp.mdx # tests/integration/support/harness/mod.rs # tests/integration/support/harness/options.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_extension_host/src/mcp.rs`:
- Around line 773-776: Update the installed-local credential audience
construction around NetworkTargetPattern to use the endpoint scheme, permitting
HTTP only for InstalledLocal literal IPv4 loopback endpoints while preserving
HTTPS requirements elsewhere. Adjust the manifest validation in the relevant v2
audience validation flow to allow that same narrowly scoped HTTP case. Add
planner-level tests through the credential-injection caller covering
installed-local HTTP loopback endpoints with explicit port 80 and omitted port
80.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 543e1f7f-427e-4df8-81bc-fadf4e6cc621
📒 Files selected for processing (1)
crates/ironclaw_extension_host/src/mcp.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/mcp.rs (1)
358-363: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the redaction assertions to the same subsequent model request.
assert_model_request_containsscans every captured request (tests/integration/support/assertions.rs:240-254), so the socket path and[REDACTED]assertions can succeed against different turns—or unrelated prompt content. This does not prove that the post-error request contains the sanitized cause as a bounded unit.🤖 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 `@tests/integration/mcp.rs` around lines 358 - 363, Update the assertions following the MCP backend error in the integration test to inspect the same subsequent captured model request for both the socket path and “[REDACTED]”. Use an assertion helper or request-specific capture that validates the sanitized cause as one bounded unit, rather than calling assert_model_request_contains separately across all captured requests.
🤖 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.
Outside diff comments:
In `@tests/integration/mcp.rs`:
- Around line 358-363: Update the assertions following the MCP backend error in
the integration test to inspect the same subsequent captured model request for
both the socket path and “[REDACTED]”. Use an assertion helper or
request-specific capture that validates the sanitized cause as one bounded unit,
rather than calling assert_model_request_contains separately across all captured
requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b76a3a0c-3bee-4980-b599-0a494dfeb7ef
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (1)
tests/integration/mcp.rs
Review follow-up
|
This branch was successfully deployed
1 participant
Summary
main, preserving the newironclaw_extension_hostownership boundary.Change Type
Linked Issue
Fixes #5998
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test --features integrationif database-backed or integration behavior changedtools/call, no Authorization header, and persisted final replyreview-prorpr-shepherd --fixwas run before requesting reviewTargeted warning-as-error lint was run for every affected package:
cargo clippy -p ironclaw_extensions -p ironclaw_extension_host -p ironclaw_reborn_composition -p ironclaw_reborn_integration_tests --all-targets --all-features -- -D warningsTest Strategy
User behavior: An installed-local MCP process can be called over
http://127.x.x.x:<exact-port>/<exact-path>without TLS. All broader plaintext or non-local variants still fail closed.Risk areas:
Tests added or updated:
scripts/reborn-e2e-rust.sh architecture.What the tests prove: Only installed-local literal IPv4 loopback HTTP receives the narrow exception; the endpoint remains exact by scheme/host/port/path; no credential is injected for the public local fixture; hosted HTTPS behavior and MCP error/SSE paths still work; ownership boundaries remain intact.
Commands run:
cargo fmt --all -- --checkcargo test -p ironclaw_extensionscargo test -p ironclaw_extension_host mcpcargo test -p ironclaw_reborn_integration_tests --test reborn_integration_mcp installed_local_mcp_dispatches_through_production_composition -- --exact --nocapturecargo test -p ironclaw_reborn_integration_tests --test reborn_integration_mcpcargo test -p ironclaw_architecturecargo clippy -p ironclaw_extensions -p ironclaw_extension_host -p ironclaw_reborn_composition -p ironclaw_reborn_integration_tests --all-targets --all-features -- -D warningsbash scripts/reborn-e2e-rust.sh architectureSecurity Impact
This intentionally widens network access for one narrowly identified case: an
InstalledLocalMCP manifest using plaintext HTTP to a literal IPv4 loopback address. The grant and runtime planner both pin the exact scheme, address, port, and path.localhost, IPv6, LAN/private non-loopback, remote HTTP,RegistryInstalled, host-bundled loopback HTTP, and stdio remain denied. The runtime response remains capped at 2 MiB.Reborn Trust-Boundary Checklist
serde(default)fields fail closed or have migration tests. N/A: no persisted or serde-default field added.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Disallowed endpoints continue to produce policy denial.InstalledLocal,HostBundled, andRegistryInstalledremain distinct.Database Impact
None. No migrations or persistence schema changes.
Blast Radius
Touches MCP manifest parsing, extension lifecycle capability metadata, local-dev grant policy, MCP egress planning, and the Reborn integration harness. Hosted MCP remains on the existing HTTPS path.
Rollback Plan
Revert the local-loopback MCP commits. Installed-local plaintext MCP calls will return to failing closed; there is no data migration or persisted compatibility step to undo.
Review Follow-Through
Merge conflicts were resolved by keeping current
mainownership inironclaw_extension_host. The current review focus is the narrowness of the loopback exception and parity between grant-time and dispatch-time endpoint policy.Review track: C (security/runtime)