Skip to content

refactor(cua): fold the MCP adapter onto a core-owned strict stdio client - #141432

Merged
steipete merged 2 commits into
mainfrom
steipete/cua-mcp-strict-stdio-client
Sep 7, 2026
Merged

refactor(cua): fold the MCP adapter onto a core-owned strict stdio client#141432
steipete merged 2 commits into
mainfrom
steipete/cua-mcp-strict-stdio-client

Conversation

@steipete

@steipete steipete commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

#141027 moved the CUA computer plugin's MCP subprocess onto core's stdio transport and the MCP SDK's request correlation, but the plugin still re-implemented the strict client around them: a private transport subclass, a fake client for the shared startup deadline, the handshake, request admission, abort/timeout/write-failure fatal mapping, JSON-RPC error mapping, stderr tail, and shutdown ordering, plus a private decoder that carried two SDK impedance hacks (wire id = SDK id + 1, and JSON-RPC error frames re-wrapped inside a result envelope). The consolidation landed at +216 net production lines instead of the expected deletion.

Why This Change Was Made

Core now owns one strict stdio MCP client (src/agents/mcp-stdio-client.ts + src/agents/mcp-stdio-frame-decoder.ts): process ownership through the existing transport and lifecycle helpers, bounded newline framing with fail-closed frame validation, exact-version handshake under the shared startup deadline, pending-request admission, client-wide fatal cancellation and timeouts without SDK cancellation notifications, non-timeout JSON-RPC errors kept local, and confirmed-or-uncertain shutdown. The caller supplies only its error taxonomy. The plugin adapter keeps CUA policy: environment scrub and launch arguments, COMPUTER_DRIVER_* error prefixes, tool-result normalization into CUA's ToolResult, action/enum mapping, session state, and availability.

Two SDK impedance hacks are gone rather than moved. The real driver answers JSON-RPC id 0 normally (probed on the bundled cua-driver 0.22.0 and proven again on pinned 0.22.2 in Testbox), so ids are no longer shifted. JSON-RPC error frames now map through the SDK's McpError, so the local error text carries the code (CUA MCP request failed: MCP error <code>: <message>); the driver reports tool failures as isError results, so this path is not exercised in normal operation. Server-originated notifications and requests are now accepted and routed by the SDK instead of being treated as fatal invalid responses.

The plugin-sdk mcpStdioRuntime seam (unshipped, landed in #141027 the same day) now exposes only createMcpStdioClient; the transport and lifecycle helpers are core-internal again.

User Impact

CUA action and wire behavior is preserved: same launch arguments and exact environment, same 256 MiB frame bound, 64-request admission, 10 s startup and 120 s request deadlines, same fatal cancellation, and the same daemon-ownership boundary (proxy disposal never stops a separately started daemon). Two error texts change as described above. No configuration, dependency, protocol-version, or persistent-store change.

Production delta: −54 net lines (+380 / −434) (plugin −312: mcp-driver-client.ts 682 → 462 lines, mcp-response-decoder.ts deleted, core +268: mcp-stdio-client.ts 57 → 240 lines, new mcp-stdio-frame-decoder.ts 85 lines; SDK loader −10).

Evidence

  • CI repair: the initial head added two callback lint suppressions and failed the production suppression inventory. The same callbacks now initialize as typed fields on the private transport subclass, with no suppression or allowlist change. The inventory and both MCP client suites passed (23 tests); focused core lint passed.
  • Final alias cleanup: node scripts/run-vitest.mjs src/agents/mcp-stdio-client.test.ts extensions/cua-computer/src/mcp-driver-client.test.ts 19 passed; focused core lint passed.
  • Plugin suite: node scripts/run-vitest.mjs extensions/cua-computer 182 passed (15 files). Three approved fixture edits in mcp-driver-client.test.ts reflect the removed hacks: initialize id 0, the incompatible-version reply id 0, and a numeric JSON-RPC error code plus SDK-formatted error text. Every other assertion is unchanged.
  • Core: src/agents/mcp-stdio-frame-decoder.test.ts + src/agents/mcp-stdio-client.test.ts + existing transport/stderr/lifecycle suites node scripts/run-vitest.mjs src/agents/mcp-*.test.ts: 250 passed, 1 Windows-only skip.
  • pnpm plugin-sdk:surface:check: public 152 entrypoints / 4445 exports / 2627 callables (no budget edits). pnpm build passed (declaration graph 3,475,869 / 5,315,536 bytes, 152 public subpaths verified). node scripts/check-changed.mjs passed every phase (ratchets, format, typecheck core/tests/extensions, dead-export scans, core lint) except the extension-lint phase, which stops at the documented nested-checkout declaration guard in this worktree; targeted run-oxlint.mjs on the changed extension files passed, and hosted CI owns the full lint lane.
  • Exact-head CI: green for f7be3456fd31ad62bab8379d4e7a019d200958ce: https://github.com/openclaw/openclaw/actions/runs/34151833032.
  • MCP SDK 1.30.0 source contract inspected directly: dist/esm/shared/protocol.js wraps existing transport callbacks before startup, clears pending response handlers on close, and sends cancellation before rejecting a timeout; dist/esm/types.js embeds the same ResultSchema in JSONRPCResultResponseSchema. The strict decoder validates before SDK correlation, and fatal timeout/cancellation behavior is covered by the client and CUA suites.
  • Codex autoreview (local mode, P1 threshold): scoped-clean, no accepted or actionable findings (coordinator-provided review; unchanged transport/lifecycle/error owners supplied as evidence).
  • Testbox (blacksmith-testbox, lease tbx_01m1yh3pra91a9rt8dcaec28kz, now stopped): plugin suite 182 passed; pinned real cua-driver 0.22.2 live proof passed, both wrapper exits 0. The wrapper verified source 76df29ba5e83d9250962eadae7ac831bd46bf245 and tree 2e1e262740a7b73450085065a253f91f7d240301 for both runs. The owned daemon under Xvfb survived proxy disposal; repeated get_session_state and reconnection succeeded; process cleanup was confirmed and scratch removed. Shared lease backing run: https://github.com/openclaw/openclaw/actions/runs/34150655274. Crabbox's external portal sync timed out after both successful commands; their timing JSON and local command logs record success independently of the backing workflow status.

@steipete
refactor(cua): fold the MCP adapter onto a core-owned strict stdio cl…
76df29b 
…ient

Core now owns the strict stdio MCP client the CUA computer plugin used privately: process
ownership, bounded fail-closed framing, exact-version handshake, request admission, fatal
cancellation/timeouts without SDK cancellation notifications, request-local JSON-RPC errors,
and confirmed-or-uncertain shutdown. The plugin keeps CUA policy only. Two SDK impedance
hacks are removed rather than moved: JSON-RPC ids are no longer shifted (the real driver
answers id 0) and JSON-RPC error frames map through the SDK's McpError, so that local error
text now carries the code. Server-originated notifications and requests are routed by the SDK
instead of being fatal. The unshipped plugin-sdk mcpStdioRuntime seam now exposes only
createMcpStdioClient. Net −52 production lines; no SDK budget or baseline changes.
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation agents Agent runtime and tooling plugin: cua-computer size: XL maintainer Maintainer-authored PR labels Sep 7, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 7, 2026, 2:46 PM ET / 18:46 UTC.

ClawSweeper review

What this changes

Moves the CUA computer plugin’s MCP connection management and frame validation into a shared core client while retaining plugin-specific launch settings and tool-result handling.

Merge readiness

⛔ Blocked before merge - 1 item remains

This remains useful work that current main does not implement. No actionable introduced defect was found; the latest discussion resolves the prior compatibility concern, and the recorded real-driver proof supports the refactor.

Priority: P2
Reviewed head: f7be3456fd31ad62bab8379d4e7a019d200958ce

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused consolidation with useful real-driver proof, preserved boundary coverage, and no actionable findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (logs): Recorded Testbox evidence exercises the CUA adapter and shared client against pinned cua-driver 0.22.2 under Xvfb, showing initialization, repeated calls, daemon survival after proxy disposal, reconnection, and confirmed cleanup; the later callback-only revision has supplemental focused validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): Recorded Testbox evidence exercises the CUA adapter and shared client against pinned cua-driver 0.22.2 under Xvfb, showing initialization, repeated calls, daemon survival after proxy disposal, reconnection, and confirmed cleanup; the later callback-only revision has supplemental focused validation.
Evidence reviewed 8 items Introduced implementation: The merge-base comparison moves startup, request admission, fatal-error handling, and cleanup into the core client and narrows the lazy SDK loader to its configured factory.
Current main still needs the consolidation: Fetched main retains the low-level client whose caller supplies initialization and lifecycle policy. The merged related PR #141027 is the foundation for this work, not a replacement implementing this consolidation.
Compatibility disposition: The supplied comment #141432 (comment) explicitly accepts the factory-only design for the unshipped seam. Repository search identifies CUA as its sole production consumer, migrated here; no supported external consumer was identified.
Findings None None.
Security None None.

How this fits together

The CUA computer plugin sends computer-control requests through an MCP proxy to a driver daemon. The shared core client manages request correlation, validation, deadlines, and proxy cleanup before returning results to the plugin.

flowchart LR
  A[Computer tool requests] --> B[CUA plugin adapter]
  B --> C[Shared strict MCP client]
  C --> D[Owned proxy process]
  D --> E[Driver daemon]
  E --> D
  D --> F[Validate and correlate replies]
  F --> B
  C --> G[Retire connection and clean up proxy]
Loading

Before merge

  • Resolve merge risk (P1) - Independent inspection of the pinned MCP SDK source, linked Testbox logs, and the earlier-head callback diff was limited by unavailable local objects and failed remote retrieval; the supplied contract inspection and runtime evidence remain the basis for those checks.
Agent review details

Security

None.

PR surface

Source -54, Tests +239, Docs +5. Total +190 across 9 files.

View PR surface stats
Area Files Added Removed Net
Source 5 380 434 -54
Tests 3 243 4 +239
Docs 1 12 7 +5
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 9 635 445 +190

Review metrics

Metric Value Why it matters
SDK loader contract 4 returned members become 1 configured factory The sole repository production consumer migrates together, and the discussion explicitly accepts this unshipped API change.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep one core-owned strict MCP lifecycle and the narrow lazy factory, with CUA-specific policy confined to the plugin.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep one core-owned strict MCP lifecycle and the narrow lazy factory, with CUA-specific policy confined to the plugin.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a refactor rather than a reported defect; existing boundary tests and recorded real-driver observations cover the preserved behavior.

Is this the best way to solve the issue?

Yes: consolidating framing and lifecycle handling under the existing core owner removes duplication while retaining CUA policy, and the API direction is explicitly accepted in the discussion.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ce8fb5328d34.

Labels

Label changes:

  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: This is a bounded MCP lifecycle consolidation with meaningful maintenance value and no established urgent user-facing regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): Recorded Testbox evidence exercises the CUA adapter and shared client against pinned cua-driver 0.22.2 under Xvfb, showing initialization, repeated calls, daemon survival after proxy disposal, reconnection, and confirmed cleanup; the later callback-only revision has supplemental focused validation.
  • proof: sufficient: Contributor real behavior proof is sufficient. Recorded Testbox evidence exercises the CUA adapter and shared client against pinned cua-driver 0.22.2 under Xvfb, showing initialization, repeated calls, daemon survival after proxy disposal, reconnection, and confirmed cleanup; the later callback-only revision has supplemental focused validation.

Evidence

What I checked:

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-07T18:19:59.919Z sha 76df29b :: blocked before merge. :: none
  • reviewed 2026-09-07T18:37:41.021Z sha f7be345 :: blocked before merge. :: none

@steipete
fix(cua): bind MCP callbacks without lint suppressions
f7be345 
Initialize the strict client's callbacks as typed fields on its private
transport subclass. This preserves the same failure and exit handling while
removing the two new suppressions rejected by the production lint inventory.
Keep the suppression allowlist and tests unchanged.

Validation: suppression inventory and both MCP client suites passed (23 tests);
focused core lint passed. Total production delta is now -54 lines.
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Sep 7, 2026
@steipete

steipete commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Addressing the prior proof items and the remaining compatibility risk in the ClawSweeper review of f7be3456fd31:

  • The factory-only SDK loader is the approved maintainer design. The seam is unshipped, and CUA is its sole production consumer; that consumer is migrated in this PR. There is no shipped compatibility contract requiring the three removed loader members.
  • The pinned cua-driver 0.22.2 proof is now complete on the reviewed head 76df29ba5e83d9250962eadae7ac831bd46bf245. Blacksmith Testbox verified that source and its tree. The 182-test plugin suite and the real-driver probe both exited 0. The probe completed initialization, repeated session-state calls, proxy disposal with the daemon surviving, and reconnection; it reported process-cleanup {confirmed:true} and scratch-cleanup {removed:true}. The lease is stopped. Shared backing run: https://github.com/openclaw/openclaw/actions/runs/34150655274. Per-command portal sync timed out after success, so the wrapper timing JSON and captured command output are the outcome records.
  • MCP SDK 1.30.0 was inspected directly. In dist/esm/shared/protocol.js, connect wraps existing callbacks before starting the transport, _onclose clears pending response handlers, and request cancellation sends notifications/cancelled before rejection. In dist/esm/types.js, JSONRPCResultResponseSchema uses the same ResultSchema passed to Protocol.request. Malformed results are rejected by the strict decoder before correlation. The relevant client and CUA contract tests passed.
  • The subsequent head f7be3456fd31ad62bab8379d4e7a019d200958ce only moves the same callback bodies into typed fields on the private transport subclass to remove two CI-rejected lint suppressions. No allowlist or baseline changed. Its suppression inventory plus both MCP client suites passed (23 tests), focused lint passed, and exact-head CI is green: https://github.com/openclaw/openclaw/actions/runs/34151833032.

The PR body records this evidence. The completed review of f7be3456fd31ad62bab8379d4e7a019d200958ce now reports sufficient real behavior proof and no actionable code findings. The remaining pre-release compatibility risk is accepted under the explicit maintainer-approved factory-only design; no supported external consumer or shipped contract was identified. This disposition will be recorded in the native review artifacts.

@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Sep 7, 2026
@steipete
steipete merged commit 829b7d6 into main Sep 7, 2026
190 checks passed
@steipete
steipete deleted the steipete/cua-mcp-strict-stdio-client branch September 7, 2026 18:50
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Sep 7, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 8, 2026
@steipete
refactor(cua): fold the MCP adapter onto a core-owned strict stdio cl…
4ccac98 
…ient (openclaw#141432)

* refactor(cua): fold the MCP adapter onto a core-owned strict stdio client

Core now owns the strict stdio MCP client the CUA computer plugin used privately: process
ownership, bounded fail-closed framing, exact-version handshake, request admission, fatal
cancellation/timeouts without SDK cancellation notifications, request-local JSON-RPC errors,
and confirmed-or-uncertain shutdown. The plugin keeps CUA policy only. Two SDK impedance
hacks are removed rather than moved: JSON-RPC ids are no longer shifted (the real driver
answers id 0) and JSON-RPC error frames map through the SDK's McpError, so that local error
text now carries the code. Server-originated notifications and requests are routed by the SDK
instead of being fatal. The unshipped plugin-sdk mcpStdioRuntime seam now exposes only
createMcpStdioClient. Net −52 production lines; no SDK budget or baseline changes.

* fix(cua): bind MCP callbacks without lint suppressions

Initialize the strict client's callbacks as typed fields on its private
transport subclass. This preserves the same failure and exit handling while
removing the two new suppressions rejected by the production lint inventory.
Keep the suppression allowlist and tests unchanged.

Validation: suppression inventory and both MCP client suites passed (23 tests);
focused core lint passed. Total production delta is now -54 lines.
@clawsweeper clawsweeper Bot mentioned this pull request Sep 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling docs Improvements or additions to documentation maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. plugin: cua-computer proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XL status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@steipete