Skip to content

refactor(gateway): extract protocol core owners - #1095

Merged
karkarl merged 2 commits into
mainfrom
bkudiess-gateway-protocol-core
Aug 14, 2026
Merged

refactor(gateway): extract protocol core owners#1095
karkarl merged 2 commits into
mainfrom
bkudiess-gateway-protocol-core

Conversation

@bkudiess

@bkudiess bkudiess commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Extract operator and node connect-envelope/auth/signature construction into the internal, WinUI-free ConnectEnvelopeBuilder.
  • Keep role, scopes, capabilities, selected credentials, v3-to-v2 fallback, persistence, logging, and transport policy in the existing clients.
  • Replace four pending-request maps and two locks in OpenClawGatewayClient with one generation-safe PendingRequestRegistry.
  • Add exact wire/signing goldens, deterministic response/error/timeout/drain races, secret-safe formatting guards, and architecture closure tests.

Ownership transfer

Old owner New owner Preserved invariant
Duplicate anonymous connect envelopes in operator/node clients ConnectEnvelopeBuilder Exact protocol 3/4 JSON, property/null shape, v2/v3 signing bytes, timestamp/nonce semantics, auth/signature credential equality, and operator/node metadata differences
Inline request method map plus chat/wizard/approval completion maps PendingRequestRegistry One terminal claimant, exact method/category correlation, no request leak after drain, stale handles cannot remove a new generation, caller timeout/error taxonomy remains unchanged

Credential precedence remains outside the builder: paired device token, then shared token, then bootstrap according to the existing client/Connection policy. WindowsNodeClient does not use the pending registry.

Validation

Validated exact published head 2a5fafb89a8c8bafcdda0a6f82d9c55c3257cd5e. Every focused and real-gateway test below executed; none skipped or no-op'd.

Proof group Passed Failed Skipped
Signed-envelope compatibility 34 0 0
Credential precedence 28 0 0
Pending-request lifecycle 31 0 0
Real setup/connect gateway E2E 18 0 0
Real revocation recovery E2E 1 0 0
Real network recovery E2E 2 0 0
Total focused + real proof 114 0 0

The 34 signed-envelope tests cover the complete operator/node profile matrix under both v2 and v3 signing, exact auth-field and signature-token equality, deterministic signature/payload formats, canonical metadata, protocol 3/4, property/null shape, nonce and timestamp semantics, initial signature-version selection, fallback on signature rejection, operator signing failure propagation, node soft signing failure, and default-formatting redaction.

The 28 CredentialResolverTests cover operator and node precedence independently: device token, then shared token, then bootstrap token, including missing, corrupt, unreadable, and detailed fallback classifications. Precedence remains outside ConnectEnvelopeBuilder.

The 31 pending-lifecycle tests cover tracked, chat, wizard, and approval registration and typed resolution; exactly-once complete/fault; duplicate and late response handling; closed/reopen behavior; generation-safe stale handles; exact cancellation taxonomy; idempotent drain; response-vs-drain, error-vs-response, timeout-remove-vs-response, and 100-iteration register-vs-drain races. Five of these use a real loopback WebSocket to verify in-flight chat and approval shutdown classification, real dispose during an approval wait, response-first priority, and timeout classification.

Real behavior proof

All real gateway/client proof ran at the exact published head with isolated WSL/tray state:

  • Setup/connect gateway E2E: 18/18 passed, zero skips. This covered real gateway installation, signed challenge/connect/hello flows, operator and node pairing, QR and shared-token paths, gateway restart recovery, MXC execution, invalid/reused setup handling, and cleanup.
  • Device revocation/recovery: 1/1 passed, zero skips. Device removal recovered through shared-token reconnection.
  • Network stop/start recovery: 2/2 passed, zero skips. Tray and node recovered after one and repeated gateway restarts.

Redacted result artifacts:

  • TestResults/PR1095Confidence/setup-connect.trx
  • TestResults/PR1095Confidence/revocation-recovery.trx
  • TestResults/PR1095Confidence/network-recovery.trx
  • Supporting isolated logs: TestResults/E2E/<run-id>/setup-engine.jsonl, e2e-fixture.log, and uninstall-engine.jsonl

Credential values, setup codes, nonces, signatures, and keys are intentionally omitted from public proof.

Remaining maintainer judgment: there is no single real-gateway test that severs the socket while every pending category is simultaneously active and then asserts every category-specific exception. The constituent behavior is covered by deterministic all-category registry tests, real loopback chat/approval shutdown and dispose tests, and real gateway restart/recovery E2E. This audit found no correctness defect requiring a code change.

Review

  • Independent exact-full-diff reviews found no significant issue.
  • A lifecycle-focused rubber-duck audit found no actionable defect.
  • The bundled automated reviewer failed closed before review because its isolated Windows diff exceeded its safe bundle limit. No truncated or partial result was accepted.

Architecture

Adds authoritative and closed ledger rows for connect-envelope and gateway-pending-requests. Source guards prevent reintroducing inline connect envelopes, auth helpers, pending maps, or the old Track/Take/Clear helper family.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. labels Aug 5, 2026
@clawsweeper

clawsweeper Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 14, 2026, 2:57 PM ET / 18:57 UTC.

ClawSweeper review

What this changes

The PR extracts signed gateway connect-envelope construction and operator pending-request tracking into dedicated shared protocol owners, with compatibility and lifecycle tests.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep open. This collaborator-owned, actively landing refactor is not on current main, and the rebased head is based on main with substantial current-head proof; it requires maintainer merge judgment rather than backlog cleanup.

Priority: P2
Reviewed head: beed12a0e8e500b7e93f82918dd7d165db918ecb
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) Substantial current-head runtime proof and targeted validation support a mergeable refactor pending maintainer confirmation of the rebased contract boundary.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The current-head collaborator update includes real gateway, MCP, node invocation, and isolated-app evidence; the prepared image is unrelated earlier chat proof and was not used for this verdict.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The current-head collaborator update includes real gateway, MCP, node invocation, and isolated-app evidence; the prepared image is unrelated earlier chat proof and was not used for this verdict.
Evidence reviewed 6 items Current main does not already contain the extraction: The architecture ledger still marks gateway request tracking and connect-envelope ownership as planned on current main.
Rebased branch includes current main: The current main SHA is an ancestor of the PR head, so the stated rebase target is present in the branch.
Current-head validation and proof: The collaborator update identifies head beed12a and reports the required build, Shared and Tray suites, focused protocol tests, a real gateway E2E, real MCP output, and a real WSL Gateway to Windows-node invocation.
Findings None None.
Security None None.

How this fits together

Gateway clients create signed connect requests and manage in-flight operator calls before responses or disconnects reach connection, chat, wizard, and approval workflows. The new owners centralize these protocol mechanics while credential selection and transport policy remain in the clients.

flowchart LR
  A[Gateway challenge] --> B[Connect envelope builder]
  B --> C[Operator or node client]
  C --> D[Pending request registry]
  D --> E[Responses or disconnects]
  E --> F[Connection chat wizard approvals]
Loading

Decision needed

Question Recommendation
Should this collaborator-owned gateway protocol refactor be merged once the exact rebased head's required checks and typed-disconnect preservation are confirmed? Merge after current-head confirmation: Confirm the rebased head retains typed remote-close behavior and let the required checks gate landing.

Why: The change is intentionally large on a connection-critical boundary, and collaborator-authored items require explicit maintainer judgment rather than automated cleanup.

Before merge

  • Resolve merge risk (P1) - This refactor changes deployed gateway signing and in-flight disconnect behavior, so a missed preservation detail could affect connection recovery or pending wizard, chat, and approval callers.
  • Resolve merge risk (P1) - The PR-head source blobs could not be independently materialized in this read-only checkout because the promisor remote was unavailable; current-head CI and maintainer source review remain the appropriate final check.
  • Complete next step (P2) - A collaborator-owned, actively landing PR needs maintainer sign-off and current-head checks, not an automated repair lane.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Protocol-owner extraction 10 files affected; production +904/-351, tests +1,235/-64, docs +7/-5 The change moves a large gateway-critical behavior boundary and adds extensive focused coverage.

Merge-risk options

Maintainer options:

  1. Confirm preserved gateway contracts (recommended)
    Before merge, verify the rebased registry passes the remote close exception through all pending categories and preserves v2/v3 signed-envelope output.
  2. Pause for reduced scope
    If maintainers do not want to accept protocol-core refactor risk now, pause the PR and retain the existing client ownership.

Technical review

Best possible solution:

Land only after confirming the rebased registry retains current main's typed remote-close propagation and exact signed-envelope compatibility.

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

Not applicable: this is a behavior-preserving refactor PR, and the relevant evidence is contract and real-path validation rather than a user bug reproduction.

Is this the best way to solve the issue?

Unclear: dedicated protocol owners are a maintainable design, but this review environment could not independently materialize the rebased source for the final line-level confirmation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 17a8b0363a82.

Labels

Label justifications:

  • P2: This is a substantial but bounded gateway refactor rather than a confirmed live regression.
  • merge-risk: 🚨 compatibility: The new envelope owner emits signed connect payloads consumed by deployed gateways.
  • merge-risk: 🚨 availability: The registry decides how in-flight gateway callers complete during disconnect and recovery.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The current-head collaborator update includes real gateway, MCP, node invocation, and isolated-app evidence; the prepared image is unrelated earlier chat proof and was not used for this verdict.
  • proof: sufficient: Contributor real behavior proof is sufficient. The current-head collaborator update includes real gateway, MCP, node invocation, and isolated-app evidence; the prepared image is unrelated earlier chat proof and was not used for this verdict.

Evidence

What I checked:

  • Current main does not already contain the extraction: The architecture ledger still marks gateway request tracking and connect-envelope ownership as planned on current main. (docs/ARCHITECTURE.md:162, 17a8b0363a82)
  • Rebased branch includes current main: The current main SHA is an ancestor of the PR head, so the stated rebase target is present in the branch. (beed12a0e8e5)
  • Current-head validation and proof: The collaborator update identifies head beed12a and reports the required build, Shared and Tray suites, focused protocol tests, a real gateway E2E, real MCP output, and a real WSL Gateway to Windows-node invocation. (beed12a0e8e5)
  • Protocol history: Gateway protocol-client foundation appears to date to this commit by Barbara Kudiess; current-main history also records gateway and pairing hardening by Ranjesh. (src/OpenClaw.Shared/OpenClawGatewayClient.cs:184, 2cae69ba6ca4)
  • Existing disconnect contract: Current main drains pending requests on disconnect with GatewayConnectionLostException carrying remote close code and description, which is the compatibility behavior the extraction must preserve. (src/OpenClaw.Shared/OpenClawGatewayClient.cs:194, 17a8b0363a82)
  • Review limitation: The checkout has the PR commits and metadata, but its promisor clone could not materialize PR-head source blobs because the configured GitHub remote could not resolve; no unverified line-level finding is asserted. (beed12a0e8e5)

Likely related people:

  • Barbara Kudiess: Authored the recorded gateway protocol-client foundation and the current main commit. (role: feature originator and recent area contributor; confidence: high; commits: 2cae69ba6ca4, 17a8b0363a82; files: src/OpenClaw.Shared/OpenClawGatewayClient.cs, docs/ARCHITECTURE.md)
  • Ranjesh: Authored the recorded gateway connection and pairing hardening commit on the affected client history. (role: gateway and pairing hardening contributor; confidence: medium; commits: ea36b12f9e4c; files: src/OpenClaw.Shared/OpenClawGatewayClient.cs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Confirm the exact rebased source retains typed remote-close propagation for registry-drained waiters.

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 (15 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-08T14:24:26.089Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard restart disconnect details
  • reviewed 2026-08-08T15:40:08.057Z sha 2a5fafb :: found issues before merge. :: [P1] Preserve typed wizard restart disconnect details
  • reviewed 2026-08-09T00:06:32.732Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard restart disconnect details
  • reviewed 2026-08-09T03:16:44.227Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard restart disconnect details
  • reviewed 2026-08-09T09:28:52.822Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed disconnects for wizard waiters
  • reviewed 2026-08-09T11:58:48.889Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard disconnect details
  • reviewed 2026-08-09T14:51:27.010Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard disconnect details
  • reviewed 2026-08-14T17:42:09.612Z sha 2a5fafb :: needs changes before merge. :: [P1] Preserve typed wizard disconnect details

@bkudiess
bkudiess force-pushed the bkudiess-gateway-protocol-core branch 3 times, most recently from df74c4e to 2a5fafb Compare August 6, 2026 04:59
@bkudiess

bkudiess commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased unchanged B0 protocol layer onto latest main cdcb74f4 at head 2a5fafb8. Combined B0+B1 validation is green: Shared 3469, Connection 562, Tray 2116, CLI 126, focused B0 236, setup 16, revocation 1, network 2, MCP 51 tools, and #1078 native-tool proof. @clawsweeper re-review

@bkudiess

bkudiess commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Exact-head confidence transcript

Validated PR head 2a5fafb8. No source changes were made during this audit.

Executed proof Passed Failed Skipped
Signed v2/v3 envelope compatibility 34 0 0
Operator/node credential precedence 28 0 0
Pending registration/resolve/timeout/drain/races 31 0 0
Real setup/connect 18 0 0
Real revocation recovery 1 0 0
Real network recovery 2 0 0
Total 114 0 0

Inspectable coverage:

Redacted TRX outputs: TestResults/PR1095Confidence/setup-connect.trx, revocation-recovery.trx, and network-recovery.trx.

Maintainer judgment remains whether to require one composite real-gateway test with every pending category active during the same socket sever. The constituent behavior is already covered by all-category registry races, loopback shutdown/dispose tests, and gateway restart/recovery E2E. No correctness defect was found.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. proof: sufficient Contributor real behavior proof is sufficient. and removed 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. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 7, 2026
@karkarl karkarl added the status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. label Aug 14, 2026
bkudiess and others added 2 commits August 14, 2026 10:38
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 0a17604d-e1a5-47ae-bdb5-54ff34887ae7
@karkarl

karkarl commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Validation

Final local rebased HEAD: beed12a0e8e500b7e93f82918dd7d165db918ecb

Remote update: the rebased branch was force-pushed with lease from 2a5fafb89a8c8bafcdda0a6f82d9c55c3257cd5e to beed12a0e8e500b7e93f82918dd7d165db918ecb; CI was retriggered.

  • Rebasing target: origin/main at 17a8b0363a82b510f2a2d989d4cc397e985d8459.
  • Conflicts resolved in OpenClawGatewayClient.cs, WindowsNodeClient.cs, and OpenClawGatewayClientTests.cs. Resolution preserved main's fail-closed handshake authorization, node connect request correlation, WebSocket close metadata, and the extracted protocol-core owners.
  • git range-diff plus final git diff --name-status origin/main..HEAD confirms the branch remains the intended 10-file protocol-core extraction. No later refactor(connection): extract domain coordinators #1101 layer or unrelated files are included.
  • ./build.ps1: passed, all five projects built.
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore: 3692 passed, 32 skipped, 0 failed.
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore: 2468 passed, 0 failed.
  • Focused ConnectEnvelopeBuilder / PendingRequestRegistry / OpenClawGatewayClient / WindowsNodeClient protocol suite: 346 passed, 0 failed.
  • dotnet test ./tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj --no-restore: 574 passed, 0 failed.
  • Real gateway E2E, FullSetup_SafeNodeInvocation_RoutesThroughRealGateway: 1 passed, 0 failed in 3.84 minutes.
  • Rubber-duck review: no blocking findings. Two coverage suggestions were independently rejected because ClosedRegistrationIsRejected_AndReopenAcceptsRegistration already covers all request registration categories.
  • Structured autoreview did not invoke a model: its fail-closed secret-like-content scan matched credential-shaped test fixtures. No autoreview-clean claim is made.

Real behavior proof

The screenshots were captured from beed12a0e8e500b7e93f82918dd7d165db918ecb using ./run-app-local.ps1 -NoBuild -Isolated -AllowNonMain, cropped to the OpenClaw window, and contain no tokens or credential values.

Connection settings, isolated MCP-only runtime

OpenClaw Connection settings showing MCP-only capabilities running

Diagnostics and gateway state

OpenClaw Diagnostics showing disconnected gateway state and diagnostic actions

Copied local MCP proof from the same app instance:

app.navigate => {"navigated":true,"page":"connection"}
app.connection.status => {
  "connectionState":"Idle",
  "effectiveMode":"LocalMcpOnly",
  "gateway":null,
  "operator":{"connected":false},
  "node":{"intended":false,"connected":false},
  "mcp":{"enabled":true,"running":true,"error":null}
}

Sanitized real WSL Gateway -> Windows node -> node.invoke proof:

Connection poll: status=Connected, nodeConnected=True, nodePaired=True
WSL result: exit=0, timedOut=False
{
  "ok": true,
  "command": "system.which",
  "payload": { "bins": { "cmd": "C:\\WINDOWS\\system32\\cmd.exe" } }
}
Teardown complete.

No gateway blocker remains: the real gateway path was exercised successfully. The rebased branch is pushed and GitHub reports the PR mergeable; CI is running on this exact head.

@karkarl
karkarl force-pushed the bkudiess-gateway-protocol-core branch from 2a5fafb to beed12a Compare August 14, 2026 18:52
@clawsweeper clawsweeper Bot added 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 14, 2026
@karkarl
karkarl merged commit 323f46d into main Aug 14, 2026
25 checks passed
@karkarl
karkarl deleted the bkudiess-gateway-protocol-core branch August 14, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 🚢 actively landing A maintainer or agent is actively driving this item through implementation, validation, or merge. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants