Skip to content

Refactor PR 1: architecture ledger + shared test fixtures#990

Merged
shanselman merged 1 commit into
mainfrom
bkudiess-refactor-architecture-plan
Jul 14, 2026
Merged

Refactor PR 1: architecture ledger + shared test fixtures#990
shanselman merged 1 commit into
mainfrom
bkudiess-refactor-architecture-plan

Conversation

@bkudiess

@bkudiess bkudiess commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

First PR of the god-object refactor. It establishes the anti-regression foundation the rest of the plan depends on: a living architecture ledger plus a shared test-support library. This is deliberately the safest PR to go first because it makes zero production changes.

  • docs/ARCHITECTURE.md — living ownership ledger (machine-readable table): ownership rules, god-object map, "when you touch X → extract toward Y", and single-source owners. AGENTS.md now points at it as required reading and forbids re-adding closed responsibilities.
  • ArchitectureLedgerConsistencyTests — validates the ledger schema, guard_test Type.Method format, and that named guard tests actually exist in source (renaming/deleting a guard without updating the ledger fails CI).
  • tests/OpenClaw.TestSupport (Shared-only) — TempDirectory, EnvironmentScope, CliHarness, FakeMcpServer, SettingsDataBuilder.
  • GatewayRecordBuilder lives in OpenClaw.Connection.Tests so the shared lib stays Connection-free (no transitive Connection dep leaks onto the CLI tests).
  • Migrations proving the fixtures: GatewayRegistryTestsTempDirectory; the WinNode FakeMcpServer internal copy de-duplicated to the shared one.

Scope / behavior guarantee

No src/ (production) files changed or added — every change is under tests/, docs/, or the solution file. The shipped app's runtime behavior cannot change because none of its code changed. The one intentional non-additive change is test-only: FakeMcpServer now retries the port bind (strictly more robust).

Validation

.\build.ps1                                    # all 5 src projects built
dotnet test tests/OpenClaw.Shared.Tests        -> Passed 2760, Skipped 31, Failed 0
dotnet test tests/OpenClaw.Tray.Tests          -> Passed 1705, Failed 0
dotnet test tests/OpenClaw.Connection.Tests    -> Passed  429, Failed 0
dotnet test tests/OpenClaw.WinNode.Cli.Tests   -> Passed  126, Failed 0

Anti-regression guard, proven by negative test (temporarily broke the ledger, confirmed CI fails, then restored):

  • Duplicate ledger id → Ledger_ids_are_unique FAILS: Duplicate ledger ids: test-temp-dir.
  • Guard test renamed to a nonexistent method → Named_guard_tests_exist_in_test_sources FAILS: no method 'NoSuchGuardMethod' was found in a type 'TestSupportFixtureTests'.

Real behavior proof

Ran this PR's build in no isolation (owned the Release single-instance), enabled Local MCP Server, then exercised the live server. Runtime is unaffected by the change (as expected — no src/ diff), and the MCP contract this PR actually touches (the FakeMcpServer dedup) works end-to-end.

Live app / connection (app.status):

{
  "connectionStatus": "Connected",
  "overallState": "Ready",
  "operatorState": "Connected",
  "nodeState": "Connected",
  "nodeConnected": true,
  "nodePaired": true,
  "gatewayVersion": "2026.6.11",
  "sessionCount": 1,
  "nodeCount": 1
}

MCP discoverywinnode --list-tools: 52 tools returned (system.*, app.*, canvas.*, device.*, screen.*, stt.*, tts.*).

MCP invocationwinnode --command:

  • device.info"appVersion": "0.6.13-bkudiess-refactor-architecture-plan.1" (confirms the live server is this branch's build).
  • system.which {"bins":["cmd","git"]} → resolved both paths.

Raw MCP JSON-RPC over http://127.0.0.1:8765/:

  • tools/listjsonrpc 2.0, 52 tools.
  • tools/call device.statusisError=false, real system snapshot returned.

Runtime log (%LOCALAPPDATA%\OpenClawTray\openclaw-tray.log, tokens redacted):

[INFO] Capabilities registered: system, canvas, screen, tts, stt, device (6 caps)
[INFO] [MCP] HTTP server listening on http://<host>:8765/
[INFO] [NodeService] AttachClient DONE: Capabilities=6, Commands=25
[INFO] Node status changed: Connected
[DEBUG] [MCP] tools/call system.which
[DEBUG] [MCP] tools/call device.info
[DEBUG] [MCP] tools/call device.status
[DEBUG] [MCP] tools/call app.navigate
[DEBUG] [MCP] tools/call app.status

Computer-use UI proof — captured the Permissions page (non-personal) via app.navigate permissions. Copied UI diagnostics from the live window:

Header: Permissions — THE-HIVE
Node mode: on
Node active — "Providing 6 capabilities: canvas, device, screen, stt, system, tts"
Capabilities: System tools = on, Browser control = off, Camera = off
Connection pill: Connected

Rubber-duck review

Dual-model adversarial review (GPT-5.5 + Claude Opus 4.8) on the diff. All consensus findings fixed before this PR:

  • SettingsDataBuilder aliasing (returned a shared instance) → now replays overrides onto a fresh instance per Build(); independence test added.
  • Ledger over-claimed protection → added Type.Method format + source-existence enforcement.
  • FakeMcpServer port TOCTOU race → retry on bind failure.
  • TestSupportConnection layering leak → made TestSupport Shared-only; builder moved to Connection.Tests.
  • Plus review-only / separator-row validation hardening in the ledger test.

Not verified / blocked

  • Gateway-mediated nodes invoke path not separately exercised; node connection to the live gateway (Connected, nodePaired: true) is shown in app.status, and MCP discovery/invocation covers the command contract this PR touches.
  • UI screenshot captured during validation as a computer-use image; embedded here as copied UI diagnostics (text) rather than a hosted image.

Cleanup

The no-isolation proof run used the real profile: Local MCP Server was toggled on for the proof and restored to its prior value (off) afterward, and the temporary tray instances were stopped. The other worktree's tray instance was stopped to free the single-instance lock and was not relaunched.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

Foundation for the god-object refactor's anti-regression mechanism.

- docs/ARCHITECTURE.md: living ownership ledger (schema + rows) and AGENTS.md pointer
- ArchitectureLedgerConsistencyTests: validates ledger schema, guard-test Type.Method
  format, and that named guard tests actually exist in the tests source
- tests/OpenClaw.TestSupport (Shared-only): TempDirectory, EnvironmentScope, CliHarness,
  FakeMcpServer, SettingsDataBuilder
- GatewayRecordBuilder lives in OpenClaw.Connection.Tests so the shared lib stays
  Connection-free (no transitive Connection dep on CLI tests)
- Migrated GatewayRegistryTests to TempDirectory; de-duplicated the WinNode FakeMcpServer

No production (src/) changes: tests and docs only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@shanselman shanselman merged commit af96e1f into main Jul 14, 2026
19 checks passed
@shanselman shanselman deleted the bkudiess-refactor-architecture-plan branch July 14, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants