refactor(ddd): isolate MCP adapter contracts - #272
Conversation
|
Warning Review limit reachedNext included review available in 34 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
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 |
seonghobae
left a comment
There was a problem hiding this comment.
Current-head standards finding: originweave-mcp is a transport-agnostic external adapter boundary, but both modern validators currently require MCP-Protocol-Version unconditionally. MCP 2026-07-28 transport semantics make request-body _meta.io.modelcontextprotocol/protocolVersion and client capabilities the transport-independent source of truth; Streamable HTTP mirrors selected metadata into headers and defines mismatch rejection, while stdio carries the JSON-RPC body without an HTTP header. The current generic constructor therefore cannot represent a valid modern stdio tools/call (and ValidatedMcpToolsListRequest::new has the same transport conflation).
Repair acceptance: add a realistic RED that admits the same valid 2026-07-28 body metadata over stdio without an HTTP header while continuing to require/match the header for Streamable HTTP. Make transport binding explicit at the MCP adapter boundary (or provide equivalently unambiguous binding-specific constructors); do not move this distinction into core/policy. Preserve fail-closed behavior for missing body protocol metadata/capabilities, HTTP header/body mismatch, unsupported protocol generation, malformed method/tool routing, and unknown tools. Cover both tools/call and tools/list, and update doctoring/traceability to the 2026-07-28 transport overview/versioning pages. Do not claim RED/GREEN from currently queued hosted runs.
|
Fresh exact-head queue RCA for |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head hosted RED is now actionable. CI 33722893474 checked out 80272f1. Rust contracts job 100545578684 passed repository contracts, rustfmt, workspace check, and the full test suite, then failed strict Clippy because three integration-test success paths use prohibited Result::expect: mcp_modern_request_metadata.rs:80 and mcp_stdio_transport.rs:8,43. Production coverage job 100545578332 completed measurement and failed exact enforcement; its artifact shows the uncovered production surface is the private routing::McpToolBoundaryError Display implementation. This is not a runner/workflow failure and does not require weakening Clippy or coverage. Minimal repair should stay adapter-local and test-only: remove the prohibited expect calls without changing behavior, and execute the private deterministic routing-error Display/source contract so every existing production arm is covered. Keep the MCP adapter inward dependency and policy/browser authority boundary unchanged.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head verification note for cae3e02: the hosted failure on 80272f1 was repaired without production/API/workflow changes. The three prohibited expect() success assertions are gone, and the previously uncovered private routing-error diagnostic implementation is now exercised across every variant with source-free Error semantics. Fresh CI/security/MV3/CodeQL runs are materialized but non-terminal, so this remains Draft and no GREEN transfers from predecessor heads. Keep #273 parent-first blocked until exact-current terminal evidence exists; only then adopt current protected main non-destructively.
Signed-off-by: Seongho Bae <me@seonghobae.me>
|
Current-main reconciliation is pushed at exact head |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head verification note: CI 33876274503 on predecessor cae3e02c... is now terminal. Production coverage 101033892627 passed exact enforcement; Rust contracts 101033892459 failed only at rustfmt after repository contracts passed. Canonical artifact 9944034273 contains one formatting diff in mcp_stdio_transport.rs, and current commit fe124e447cad3f679e22337fb6fbdfd135ab3652 applies exactly that diff. This does not transfer predecessor GREEN to the new head: current CI/security/SAST/CodeQL/MV3 runs must complete on fe124e... before parent-first reconstruction of #273.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head verification update: CI 33893256407 is now terminal success. Rust contracts 101089771465 passed repository contracts, rustfmt, full Rust tests, strict Clippy and rustdoc; Production coverage 101089771864 passed exact enforcement. Manifest V3 33893256372 also succeeded. Security 33893256413, Semgrep 33893256402 and CodeQL 33893256357 remain queued, so keep Draft and do not restack #273 yet.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current reconciliation review for b1cae8ad1cbd8eb6992037c830aea30b9aa436b3: the branch now adopts protected main@87c4daa... non-destructively, preserves #286's CI/MV3 lifecycle contracts, keeps .github/** identical to protected main, retains originweave-mcp workspace membership, and isolates MCP-specific dependency-direction assertions in focused tests/test_mcp_adapter_repository_contract.py. Fresh compare is behind 0 with merge base exactly current protected main. The predecessor ready_for_review event reproduced #279's skip defect; this Ready synchronize has materialized fresh CI/MV3/Security/Semgrep/CodeQL runs. Do not transfer predecessor GREEN or reconstruct #273 until this exact generation reaches terminal required evidence.
|
Exact-head review evidence for |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current verification update for b1cae8ad1cbd8eb6992037c830aea30b9aa436b3: repository-native CI 33929881742, Manifest V3 33929881782, Security Scan 33929881739, and SAST Semgrep 33929881729 are terminal success on this unchanged head. CodeQL PR 33929881687 is terminal failure, but the failure is not a product-source finding: its language-detection job succeeds and each compatibility shard reaches the central dispatch handoff. For example python job 101238565589 validates the live exact PR/head, obtains GitHub OIDC and a repository-scoped App token, dispatches codeql-scan, then intentionally fails with VERDICT_STATE=pending while awaiting an authenticated codeql-dispatch/python terminal status and rerun. A fresh exact-head commit-status read contains no codeql-dispatch/* verdicts. I routed this consumer evidence to canonical central queue/dispatch owner ContextualWisdomLab/.github#712 as comment 5551060102. Do not change MCP source to satisfy this control-plane stall, synthesize statuses, or promote the four GREEN sibling workflows as a substitute for CodeQL. #273 remains parent-first blocked until a genuine exact-head CodeQL verdict/rerun completes and normal review/governance gates are satisfied.
|
Scheduled review-feedback autofix for this PR head.
|
Scope
This PR keeps MCP as an external protocol adapter.
originweave-mcpdepends inward on stable core contracts and the protocol-independent policy API;originweave-coreandoriginweave-policydo not depend outward on MCP transport types. Route validation proves protocol integrity only and grants no browser, network, policy, secret, approval, evidence, or ambient execution authority.Protected
mainis exact87c4daa1830bac5a5228b6036752ad5633232085through #286. Exact current head isb1cae8ad1cbd8eb6992037c830aea30b9aa436b3, Ready and mergeable. Fresh compare is 52 ahead / 0 behind with merge base exactly current protected main. The effective PR delta contains no.github/**mutation.Original stdio RED and repair
Test-only
bbe6b219a33f78e3b8b1c0166a00e5c34a2ede22specified modern MCP 2026-07-28 stdio admission fortools/callandtools/listwithout fabricated HTTP routing headers while retaining required bodyprotocolVersionandclientCapabilitiesevidence. CI33646560232produced the intended hosted RED because the stdio constructors did not exist.Production
09ffcccfd91d478120642a4db9bda501655e4533added adapter-localnew_for_stdioconstructors while reusing the bounded routing/catalog/cursor validators. Stdio and Streamable HTTP remain distinct adapter transports; no MCP transport type or authorization decision moved into core/policy.80272f18422c9946077ad9bd674f603db8f020damade the transport distinction durable indocs/traceability/mcp-authority-route.md.Hosted quality-gate RED and causal repair
CI
33722893474on exact80272f18422c9946077ad9bd674f603db8f020daexposed two real defects: strict Clippy rejected three integration-test.expect(...)calls, and exact production coverage exposed the private six-armrouting::McpToolBoundaryError::Displaysurface. The branch repaired those without weakening Clippy, coverage, or DDD boundaries:eda00dc5c87ce6164e687d13bea7ce0aa55b909fremoves the prohibitedexpectfrom modern request-metadata success assertions;c0e587e36045f4128b4c1dd414298c1171abe600removes the two stdioexpectcalls; andcae3e02cd2edc08db06111fb309a5b437c5a6598executes all six existing private routing-errorDisplayarms and verifies source-free error semantics without widening that internal type.The later exact CI
33876274503oncae3e02c...narrowed the remaining defect to canonical rustfmt after Production coverage had already succeeded. Canonical formatting artifact9944034273identified one exact diff incrates/originweave-mcp/tests/mcp_stdio_transport.rs. Commitfe124e447cad3f679e22337fb6fbdfd135ab3652applied only that artifact-derived formatting repair.Protected-main reconciliation
At unchanged
fe124e..., native CI33893256407and MV333893256372were GREEN, while same-generation Security/Semgrep/CodeQL were cancelled. Protected main then advanced through #286 and changed CI/MV3 lifecycle controls plus their repository contracts.This branch repaired that divergence non-destructively rather than rebasing or overwriting the intervening generation:
7763bd5398380d958b0d1aa09be510b6365ffedakeeps the prior refactor(ddd): isolate MCP adapter contracts #272 head as first parent and protectedmain@87c4daa...as second parent while adopting the protected CI/MV3 workflow blobs and MV3 contract exactly;55b2f06b038faab50496d33e1affcbb5517af0e5reconciles the overlapping generic repository contract so it retains ci: stop draft PR jobs from occupying runners #286's lifecycle/Draft assertions and adds onlyoriginweave-mcpworkspace membership;975492b156803210bd08f5ae27bd78a5da48c693moves the MCP-specific dependency-direction assertions into focusedtests/test_mcp_adapter_repository_contract.pyinstead of making the generic governance contract own adapter details; andb1cae8ad1cbd8eb6992037c830aea30b9aa436b3updates the MCP traceability record to the current protected generation and records that the effective PR delta is workflow-free.Fresh compare from current protected main is behind 0 and contains 17 semantic paths: the existing MCP crate/source/tests/docs delta, one workspace-member assertion, and the focused MCP repository contract. No protected workflow semantics are reintroduced or authored by this lane.
Verification state
The
ready_for_reviewevent on predecessor exact975492b1...reproduced #279's #286 lifecycle defect: CI33929826137and MV333929826220completedskippedalthough the PR had become Ready. Those results remain diagnostic only.Fresh exact-current execution on
b1cae8ad1cbd8eb6992037c830aea30b9aa436b3is now terminal except for the central CodeQL verdict:33929881742: success;33929881782: success;33929881739: success;33929881729: success;33929881687: failure.The CodeQL failure is not an observed MCP source finding. Language detection completed successfully. The
python,actions, andjavascript-typescriptcompatibility jobs each successfully completedRequest current-head CodeQL scan dispatch, then failed atRelease runner or enforce current-head CodeQL verdict. The exact jobs are101238565589,101238565594, and101238565606. This remains fail-closed and belongs to the canonical central queue/dispatch owner.github#712; do not mutate leaf.github/**, synthesize status, transfer predecessor GREEN, or treat a queued central run as a verdict.PR #273 remains the dependent documentation child. Reconstruct it non-destructively only after this exact parent generation has a terminal authenticated CodeQL verdict; preserve only its validated documentation/fitness delta and do not transfer predecessor checks.
Standards and dependency boundary
The MCP
2026-07-28revision makes requests self-describing through per-request_meta: protocol version and client capabilities belong to the request body, while Streamable HTTP correlates transport metadata separately. Optional or self-reported client identity is not OriginWeave authorization input. The complete MCP adapter remains Planned; this slice does not claim complete stdio runtime framing, Streamable HTTP serialization, OAuth deployment, browser I/O, secret materialization, persistence, or evidence export.No self-approval, bypass, force-push, destructive rebase, workflow/ruleset/secret mutation, gate weakening, tag, release, or publication is authorized.