From 2e48de5f634fef34b5b4465d142de87bc0558b65 Mon Sep 17 00:00:00 2001 From: "bakudies@microsoft.com" Date: Tue, 14 Jul 2026 13:49:48 -0700 Subject: [PATCH] Add architecture ledger and shared test fixtures (refactor PR 1) 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> --- AGENTS.md | 2 + docs/ARCHITECTURE.md | 127 +++++++++ openclaw-windows-node.slnx | 1 + .../GatewayRecordBuilder.cs | 46 +++ .../GatewayRegistryTests.cs | 9 +- .../OpenClaw.Connection.Tests.csproj | 1 + .../TestSupportFixtureTests.cs | 136 +++++++++ .../ArchitectureLedgerConsistencyTests.cs | 264 ++++++++++++++++++ tests/OpenClaw.TestSupport/CliHarness.cs | 52 ++++ .../Directory.Build.props | 22 ++ .../OpenClaw.TestSupport/EnvironmentScope.cs | 51 ++++ .../FakeMcpServer.cs | 44 ++- .../OpenClaw.TestSupport.csproj | 17 ++ .../SettingsDataBuilder.cs | 37 +++ tests/OpenClaw.TestSupport/TempDirectory.cs | 39 +++ .../AuthTokenTests.cs | 1 + .../OpenClaw.WinNode.Cli.Tests.csproj | 1 + .../RunAsyncTests.cs | 1 + 18 files changed, 838 insertions(+), 13 deletions(-) create mode 100644 docs/ARCHITECTURE.md create mode 100644 tests/OpenClaw.Connection.Tests/GatewayRecordBuilder.cs create mode 100644 tests/OpenClaw.Connection.Tests/TestSupportFixtureTests.cs create mode 100644 tests/OpenClaw.Shared.Tests/Architecture/ArchitectureLedgerConsistencyTests.cs create mode 100644 tests/OpenClaw.TestSupport/CliHarness.cs create mode 100644 tests/OpenClaw.TestSupport/Directory.Build.props create mode 100644 tests/OpenClaw.TestSupport/EnvironmentScope.cs rename tests/{OpenClaw.WinNode.Cli.Tests => OpenClaw.TestSupport}/FakeMcpServer.cs (73%) create mode 100644 tests/OpenClaw.TestSupport/OpenClaw.TestSupport.csproj create mode 100644 tests/OpenClaw.TestSupport/SettingsDataBuilder.cs create mode 100644 tests/OpenClaw.TestSupport/TempDirectory.cs diff --git a/AGENTS.md b/AGENTS.md index 40a4d0a97..e8659f413 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,6 +84,7 @@ Read `docs/TELEMETRY.md` before adding or changing OpenTelemetry configuration, Start with these docs before changing connection, pairing, node, MCP, or tray UX behavior: +- `docs/ARCHITECTURE.md` - **the living architecture ledger**. Required reading before touching any god object it lists. Records which responsibilities have been extracted (`authoritative`) and which must not be re-added to a god object (`closed`). Do not reintroduce a `closed` responsibility. - `docs/CONNECTION_ARCHITECTURE.md` - current gateway registry, connection manager, credential precedence, migration, MCP-only, and tray action behavior. - `docs/MCP_MODE.md` - local MCP server mode and the `EnableNodeMode` / `EnableMcpServer` matrix. - `docs/WINDOWS_NODE_TESTING.md` - Windows node capabilities, manual smokes, and gateway-dependent behavior. @@ -95,6 +96,7 @@ Start with these docs before changing connection, pairing, node, MCP, or tray UX `src\OpenClaw.Tray.WinUI\App.xaml.cs` and `src\OpenClaw.Tray.WinUI\Pages\ConnectionPage.xaml.cs` are active god-file reduction targets. When touching either file: +- **Read `docs/ARCHITECTURE.md` first.** It is the living ownership ledger. Before editing any file it lists, check its row(s); do not re-add anything marked `closed`. When you extract a responsibility, update the ledger in the same PR (flip/add the new owner to `authoritative`, mark the vacated responsibility `closed`) and add a guard test for high-regression closures. A PR that re-adds a `closed` responsibility must be rejected in review. - Prefer completing a real ownership transfer over moving code to partial classes. A new partial file is not progress unless it introduces a narrower owner, pure projection, policy, service, or tested seam. - Keep `App` as the composition root. Shrink it by delegating cohesive behavior to focused services, but do not relocate startup ordering into another god object. - Keep `ConnectionPage.xaml.cs` as the WinUI applicator until a pure row/plan/workflow seam exists. Do not move named-control setters into a presenter that just wraps the page. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md new file mode 100644 index 000000000..85d0c0aba --- /dev/null +++ b/docs/ARCHITECTURE.md @@ -0,0 +1,127 @@ +# OpenClaw Windows node — architecture ledger + +This document is the **living source of truth** for the architecture refactor +that decomposes the repository's god objects. It is required reading before you +touch any file listed in the ledger below. + +Its job is to stop the refactor from silently regressing: when a PR moves a +responsibility out of a god object, it records the move here and (for +high-regression closures) adds a guard test. A later PR that tries to move the +work back then shows up as either a visible ledger edit or a failing test. + +See `AGENTS.md` → "Architecture Guardrails" for the hard rules, and the full +multi-PR refactor plan for the reasoning behind each boundary. + +## How to use this document + +1. **Before editing** a file named in the ledger, read its row(s). Do not add + back anything a row marks `closed`. +2. **When you extract** a responsibility, in the same PR: + - Flip/add the ledger row for the new owner to `authoritative`. + - Mark the vacated responsibility in the old owner as `closed`. + - Update the "when you touch file X, extract toward Y" guidance below. + - Add a guard test for the closure when a silent revert would be dangerous. +3. **Prefer behavioral/golden guards.** Use `source-shape` guards only for a + concrete prohibited pattern (a banned helper signature, a forbidden direct + constructor call), never for broad architectural wishes, and always with a + `retirement_condition`. + +## Ownership rules + +- **View** (XAML + code-behind): layout, named-control wiring, lifecycle event + forwarding, minimal WinUI-only adapters. No gateway JSON parsing, no polling + loops, no settings mutation, no imperative row factories. +- **ViewModel / Presenter** (`OpenClaw.Tray.WinUI/ViewModels`, `.../Presentation`): + observable state, commands, pure projection. WinUI-free where practical — no + `Microsoft.UI.Xaml`, no `Application.Current`, no `Window`/`Frame`/`Brush`/`Color`, + no concrete `SettingsManager`. Unit-tested. +- **Service**: IO, gateway calls, registry/settings persistence, timers, process + execution, WebSocket/MCP hosting. No UI types. No background work started from + constructors. +- **App** (`App.xaml.cs`): composition root and top-level lifecycle only. + +## Single-source owners + +These are the canonical homes. Do not reintroduce private copies elsewhere. + +| Concern | Canonical owner | Status | +| --- | --- | --- | +| Test temp directories | `OpenClaw.TestSupport.TempDirectory` | authoritative | +| Test env var save/restore | `OpenClaw.TestSupport.EnvironmentScope` | authoritative | +| CLI stdout/stderr/env capture | `OpenClaw.TestSupport.CliHarness` | authoritative | +| Loopback MCP server for tests | `OpenClaw.TestSupport.FakeMcpServer` | authoritative | +| Gateway record test data | `OpenClaw.Connection.Tests.GatewayRecordBuilder` | authoritative | +| Settings test data | `OpenClaw.TestSupport.SettingsDataBuilder` | authoritative | +| JSON `JsonElement` coercion | `JsonReadHelpers` (planned) | planned | +| WSL/POSIX shell quoting | dedicated POSIX quote APIs (planned) | planned | +| Capability UI metadata | `NodeCapabilityUiCatalog` (planned) | planned | +| Capability registration/gating | `NodeCapabilityRegistrationPolicy` (planned) | planned | +| Local MCP exposure policy | `McpCapabilityPolicy` (planned) | planned | +| Gateway connect envelope | `ConnectEnvelopeBuilder` (planned) | planned | +| Gateway request tracking | `PendingRequestRegistry` (planned) | planned | + +## When you touch file X, extract toward Y + +| If you are editing… | Do not grow it. Extract toward… | +| --- | --- | +| `src/OpenClaw.Tray.WinUI/App.xaml.cs` | `IWindowManager`, `ITrayController`, `IActivationRouter`, `ISettingsChangeCoordinator`, `AppBootstrapper` | +| `src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs` | `ChatSendQueue`, `ChatBridgeEventPump`, `ChatHistoryLoader`, `ChatSnapshotProjector`, `AttachmentMetadataStore` | +| `src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs` | `TimelineScrollController`, `ChatBubbleRenderer`, `ToolCallCardRenderer`, `PermissionRequestCard`, `AttachmentBubbleRenderer` | +| `src/OpenClaw.Tray.WinUI/Chat/OpenClawComposer.cs` | `ComposerViewModel`, `SlashCommandPalette`, `AttachmentPreviewStrip`, `VoiceComposerController` | +| `src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs` | `ConnectionPagePlan` (pure), `ConnectionPageViewModel`, gateway row models | +| `src/OpenClaw.Tray.WinUI/Services/NodeService.cs` | `McpServerHost`, `CanvasWindowManager`, `MediaCapabilityHost`, `RecordingConsentService`, `NodeCapabilityRegistry` | +| `src/OpenClaw.Shared/OpenClawGatewayClient.cs` | `PendingRequestRegistry`, `ConnectEnvelopeBuilder`, `GatewayMessageRouter`, per-domain API facades | +| `src/OpenClaw.Shared/Models.cs` | per-domain model files + `*Mapper` classes | +| `src/OpenClaw.Shared/Capabilities/SystemCapability.cs` | `ExecApprovalService` | +| `src/OpenClaw.Connection/GatewayConnectionManager.cs` | `NodeConnectionCoordinator`, `BootstrapTokenLifecycle`, `DevicePairApprovalCoordinator` | +| `src/OpenClaw.SetupEngine/SetupSteps.cs` | one file per step; `WslShellClient`, `GatewayConfigScriptBuilder`, `KeepaliveProcessManager` | +| Any test hand-rolling a temp dir / env save-restore / CLI capture | `OpenClaw.TestSupport` fixtures | + +## Ledger + +The ledger is machine-readable and validated by +`OpenClaw.Shared.Tests/Architecture/ArchitectureLedgerConsistencyTests.cs`. +Rows live between the BEGIN/END markers, one per line, pipe-delimited, with a +leading and trailing pipe. Columns, in order: + +`id | status | old_owner | closed_responsibility | new_owner | allowed_residue | invariant | guard_test | guard_type | retirement_condition` + +- `status`: `planned` | `authoritative` | `closed` +- `guard_type`: `behavioral` | `golden` | `source-shape` | `review-only` +- For `authoritative`/`closed` rows, `guard_test` must name a test as `Type.Method` + (validated for format), OR `guard_type` must be `review-only` with a real + rationale in `guard_test` (placeholders like `-`/`none` are rejected). +- For `behavioral`/`golden` rows, the named `guard_test` must actually exist in + the `tests/` source tree — the consistency test scans for it, so renaming or + deleting a guard without updating the ledger fails CI. +- `source-shape` rows must set a concrete `retirement_condition`. +- No literal `|` characters inside a cell (they break the pipe-delimited parse). +- Use `-` for a genuinely empty cell (except where a value is required above). + + +| id | status | old_owner | closed_responsibility | new_owner | allowed_residue | invariant | guard_test | guard_type | retirement_condition | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | --- | +| test-temp-dir | authoritative | scattered test files | hand-rolled Path.GetTempPath temp dirs in migrated tests | OpenClaw.TestSupport.TempDirectory | pre-existing un-migrated tests until adopted | temp dirs are created unique and best-effort deleted | TestSupportFixtureTests.TempDirectory_CreatesAndDeletes | behavioral | when all temp-dir tests are migrated | +| test-env-scope | authoritative | scattered test files | hand-rolled env var save/restore in migrated tests | OpenClaw.TestSupport.EnvironmentScope | pre-existing un-migrated tests until adopted | env vars set in a test are restored on dispose | TestSupportFixtureTests.EnvironmentScope_RestoresOriginal | behavioral | when all env-mutating tests are migrated | +| test-cli-harness | authoritative | CLI test projects | duplicated stdout/stderr/env capture tuples | OpenClaw.TestSupport.CliHarness | - | stdout/stderr/env lookup are captured consistently | TestSupportFixtureTests.CliHarness_CapturesAndLooksUp | behavioral | when CLI tests adopt the harness | +| test-fake-mcp | authoritative | OpenClaw.WinNode.Cli.Tests | private internal FakeMcpServer copy | OpenClaw.TestSupport.FakeMcpServer | - | one loopback MCP server captures method/body/auth and returns canned/timeout responses | TestSupportFixtureTests.FakeMcpServer_CapturesRequest | behavioral | when all MCP-round-trip tests share it | +| test-gateway-builder | authoritative | OpenClaw.Connection.Tests | per-file MakeRecord(id,url) helpers | OpenClaw.Connection.Tests.GatewayRecordBuilder | pre-existing MakeRecord until migrated | gateway record test data has one builder | TestSupportFixtureTests.GatewayRecordBuilder_BuildsRecord | behavioral | when MakeRecord helpers are removed | +| test-settings-builder | authoritative | scattered test files | ad hoc SettingsData construction in migrated tests | OpenClaw.TestSupport.SettingsDataBuilder | pre-existing un-migrated tests until adopted | settings test data starts from production defaults | TestSupportFixtureTests.SettingsDataBuilder_StartsFromDefaults | behavioral | when settings tests adopt the builder | +| json-read-helpers | planned | OpenClaw.Shared (multiple files) | duplicate private GetString/TryGetString/FirstNonEmpty helpers | JsonReadHelpers | - | one JSON coercion helper with defined null/wrong-kind behavior | none | review-only | extracted in PR 2 | +| wsl-posix-quoting | planned | OpenClaw.SetupEngine/SetupSteps.cs | ad hoc ShellEscape with divergent wrap semantics | dedicated POSIX quote APIs | - | WSL scripts use POSIX quoting, never cmd/PowerShell quoting | none | review-only | extracted in PR 2-3 | +| app-window-manager | planned | src/OpenClaw.Tray.WinUI/App.xaml.cs | window creation/show/hide/shutdown | IWindowManager | composition/delegation only | startup/shutdown ordering deterministic; disposed once | none | review-only | extracted in Phase 3 | +| app-tray-controller | planned | src/OpenClaw.Tray.WinUI/App.xaml.cs | tray icon/menu/action routing | ITrayController | composition/delegation only | tray actions route unchanged | none | review-only | extracted in Phase 3 | +| app-activation-router | planned | src/OpenClaw.Tray.WinUI/App.xaml.cs | deep-link/toast/single-instance activation | IActivationRouter | composition/delegation only | activation routes land on the same UI/actions; current-user pipe security preserved | none | review-only | extracted in Phase 3 | +| chat-send-queue | planned | src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs | send queue/admission/abort state | ChatSendQueue | - | queued send/abort/generation semantics preserved | none | review-only | extracted in Phase 4 | +| gateway-pending-requests | planned | src/OpenClaw.Shared/OpenClawGatewayClient.cs | request-id -> method/completion tracking | PendingRequestRegistry | - | request ids never leak after disconnect; thread-safe | none | review-only | extracted in Phase 4 | +| connect-envelope | planned | src/OpenClaw.Shared/OpenClawGatewayClient.cs + WindowsNodeClient.cs | connect message + auth precedence + signature version | ConnectEnvelopeBuilder | - | credential precedence never downgrades a device token; v3->v2 fallback preserved | none | review-only | extracted in Phase 4 | + + +## Deferred test builders + +`DeviceIdentityBuilder` and `SetupContextBuilder` are intentionally **not** in +`OpenClaw.TestSupport` yet. `DeviceIdentity` is a stateful Ed25519 key/file +service (not a value type) and `SetupContext` needs setup logger/journal/command-runner +fakes. Both will be added alongside their subsystem PRs (gateway protocol and +SetupEngine, respectively) so `OpenClaw.TestSupport` does not take a heavy +dependency on `OpenClaw.SetupEngine` prematurely. diff --git a/openclaw-windows-node.slnx b/openclaw-windows-node.slnx index 0fb4c6ecd..05ea7fdff 100644 --- a/openclaw-windows-node.slnx +++ b/openclaw-windows-node.slnx @@ -29,6 +29,7 @@ + diff --git a/tests/OpenClaw.Connection.Tests/GatewayRecordBuilder.cs b/tests/OpenClaw.Connection.Tests/GatewayRecordBuilder.cs new file mode 100644 index 000000000..c58b4a1e7 --- /dev/null +++ b/tests/OpenClaw.Connection.Tests/GatewayRecordBuilder.cs @@ -0,0 +1,46 @@ +using OpenClaw.Connection; + +namespace OpenClaw.Connection.Tests; + +/// +/// Fluent builder for test data, replacing the +/// per-file MakeRecord(id, url) helpers. Starts from sensible defaults +/// (random id, a test wss URL) and overrides only what a test cares about. +/// See docs/ARCHITECTURE.md (ledger id test-gateway-builder). +/// +/// +/// Lives in this Connection-layer test project rather than the shared +/// OpenClaw.TestSupport so that Connection-free test projects (e.g. the +/// WinNode CLI tests) don't take a transitive dependency on +/// OpenClaw.Connection just to use the core fixtures. Graduate it to a +/// dedicated Connection-scoped support library if a second consumer appears. +/// +public sealed class GatewayRecordBuilder +{ + private string _id = "gw-" + Guid.NewGuid().ToString("N")[..8]; + private string _url = "wss://gateway.test"; + private string? _friendlyName; + private string? _sharedGatewayToken; + private string? _bootstrapToken; + private bool _isLocal; + private bool _requiresV2Signature; + + public GatewayRecordBuilder WithId(string id) { _id = id; return this; } + public GatewayRecordBuilder WithUrl(string url) { _url = url; return this; } + public GatewayRecordBuilder WithFriendlyName(string? name) { _friendlyName = name; return this; } + public GatewayRecordBuilder WithSharedGatewayToken(string? token) { _sharedGatewayToken = token; return this; } + public GatewayRecordBuilder WithBootstrapToken(string? token) { _bootstrapToken = token; return this; } + public GatewayRecordBuilder Local(bool isLocal = true) { _isLocal = isLocal; return this; } + public GatewayRecordBuilder RequiresV2Signature(bool requires = true) { _requiresV2Signature = requires; return this; } + + public GatewayRecord Build() => new() + { + Id = _id, + Url = _url, + FriendlyName = _friendlyName, + SharedGatewayToken = _sharedGatewayToken, + BootstrapToken = _bootstrapToken, + IsLocal = _isLocal, + RequiresV2Signature = _requiresV2Signature, + }; +} diff --git a/tests/OpenClaw.Connection.Tests/GatewayRegistryTests.cs b/tests/OpenClaw.Connection.Tests/GatewayRegistryTests.cs index 9d8776cf9..68518e74f 100644 --- a/tests/OpenClaw.Connection.Tests/GatewayRegistryTests.cs +++ b/tests/OpenClaw.Connection.Tests/GatewayRegistryTests.cs @@ -1,24 +1,25 @@ using OpenClaw.Shared; using OpenClaw.Connection; +using OpenClaw.TestSupport; namespace OpenClaw.Connection.Tests; public class GatewayRegistryTests : IDisposable { + private readonly TempDirectory _temp; private readonly string _tempDir; private readonly GatewayRegistry _registry; public GatewayRegistryTests() { - _tempDir = Path.Combine(Path.GetTempPath(), "openclaw-test-" + Guid.NewGuid().ToString("N")[..8]); - Directory.CreateDirectory(_tempDir); + _temp = new TempDirectory(); + _tempDir = _temp.Path; _registry = new GatewayRegistry(_tempDir); } public void Dispose() { - // slopwatch-ignore: SW003 Test cleanup or fixture teardown is best-effort and must not hide the test outcome. - try { Directory.Delete(_tempDir, true); } catch { } + _temp.Dispose(); } [Fact] diff --git a/tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj b/tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj index 286ba19a4..d65bb197d 100644 --- a/tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj +++ b/tests/OpenClaw.Connection.Tests/OpenClaw.Connection.Tests.csproj @@ -3,6 +3,7 @@ + diff --git a/tests/OpenClaw.Connection.Tests/TestSupportFixtureTests.cs b/tests/OpenClaw.Connection.Tests/TestSupportFixtureTests.cs new file mode 100644 index 000000000..17b1c3865 --- /dev/null +++ b/tests/OpenClaw.Connection.Tests/TestSupportFixtureTests.cs @@ -0,0 +1,136 @@ +using System.Net.Http; +using OpenClaw.Connection; +using OpenClaw.Shared; +using OpenClaw.TestSupport; + +namespace OpenClaw.Connection.Tests; + +/// +/// Self-tests for the shared OpenClaw.TestSupport fixtures. These are the +/// guard tests named by the architecture ledger rows (test-temp-dir, +/// test-env-scope, ...): if a future change breaks a fixture's contract, the +/// ledger's promise fails here. +/// +public sealed class TestSupportFixtureTests +{ + [Fact] + public void TempDirectory_CreatesAndDeletes() + { + string path; + using (var temp = new TempDirectory()) + { + path = temp.Path; + Assert.True(Directory.Exists(path)); + + var file = temp.Combine("sub", "file.txt"); + Directory.CreateDirectory(Path.GetDirectoryName(file)!); + File.WriteAllText(file, "x"); + Assert.True(File.Exists(file)); + } + + Assert.False(Directory.Exists(path)); + } + + [Fact] + public void EnvironmentScope_RestoresOriginal() + { + const string name = "OPENCLAW_TESTSUPPORT_PROBE"; + var original = Environment.GetEnvironmentVariable(name); + + using (var scope = new EnvironmentScope()) + { + scope.Set(name, "scoped-value"); + Assert.Equal("scoped-value", Environment.GetEnvironmentVariable(name)); + } + + Assert.Equal(original, Environment.GetEnvironmentVariable(name)); + } + + [Fact] + public void CliHarness_CapturesAndLooksUp() + { + using var harness = new CliHarness() + .WithEnv("OPENCLAW_ENDPOINT", "http://localhost:1234/"); + + harness.Out.Write("stdout-line"); + harness.Err.Write("stderr-line"); + + Assert.Equal("http://localhost:1234/", harness.EnvLookup("OPENCLAW_ENDPOINT")); + Assert.Null(harness.EnvLookup("OPENCLAW_MISSING")); + Assert.Equal("stdout-line", harness.StdOut); + Assert.Equal("stderr-line", harness.StdErr); + } + + [Fact] + public async Task FakeMcpServer_CapturesRequest() + { + using var server = new FakeMcpServer(); + using var client = new HttpClient(); + + using var request = new HttpRequestMessage(HttpMethod.Post, server.Url) + { + Content = new StringContent("{\"method\":\"tools/list\"}"), + }; + request.Headers.Add("Authorization", "Bearer test-token"); + using var response = await client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Equal("{\"method\":\"tools/list\"}", server.LastRequestBody); + Assert.Equal("POST", server.LastRequestMethod); + Assert.Equal("Bearer test-token", server.LastRequestAuthorization); + Assert.Contains("jsonrpc", body); + } + + [Fact] + public void GatewayRecordBuilder_BuildsRecord() + { + GatewayRecord record = new GatewayRecordBuilder() + .WithId("gw-1") + .WithUrl("wss://example") + .WithFriendlyName("Home") + .WithSharedGatewayToken("tok-123") + .Build(); + + Assert.Equal("gw-1", record.Id); + Assert.Equal("wss://example", record.Url); + Assert.Equal("Home", record.FriendlyName); + Assert.Equal("tok-123", record.SharedGatewayToken); + + // Defaults produce a usable, uniquely-identified record with no overrides. + var defaulted = new GatewayRecordBuilder().Build(); + Assert.False(string.IsNullOrWhiteSpace(defaulted.Id)); + Assert.False(string.IsNullOrWhiteSpace(defaulted.Url)); + } + + [Fact] + public void SettingsDataBuilder_StartsFromDefaults() + { + var defaults = new SettingsData(); + + var built = new SettingsDataBuilder() + .WithGatewayUrl("wss://gw") + .WithNodeMode(true) + .Build(); + + Assert.Equal("wss://gw", built.GatewayUrl); + Assert.True(built.EnableNodeMode); + // Untouched fields keep the production defaults. + Assert.Equal(defaults.AutoStart, built.AutoStart); + Assert.Equal(defaults.ShowNotifications, built.ShowNotifications); + } + + [Fact] + public void SettingsDataBuilder_BuildsIndependentInstances() + { + var builder = new SettingsDataBuilder().WithAutoStart(false); + + var first = builder.Build(); + // Mutating the builder after a build must not change the already-built result. + builder.WithAutoStart(true); + var second = builder.Build(); + + Assert.NotSame(first, second); + Assert.False(first.AutoStart); + Assert.True(second.AutoStart); + } +} diff --git a/tests/OpenClaw.Shared.Tests/Architecture/ArchitectureLedgerConsistencyTests.cs b/tests/OpenClaw.Shared.Tests/Architecture/ArchitectureLedgerConsistencyTests.cs new file mode 100644 index 000000000..9eb4ddceb --- /dev/null +++ b/tests/OpenClaw.Shared.Tests/Architecture/ArchitectureLedgerConsistencyTests.cs @@ -0,0 +1,264 @@ +using System.Text.RegularExpressions; + +namespace OpenClaw.Shared.Tests.Architecture; + +/// +/// Validates the machine-readable ownership ledger in docs/ARCHITECTURE.md. +/// The ledger is the anti-regression backbone of the architecture refactor: it +/// records which responsibilities have been extracted out of god objects and +/// which must not be re-added. This test keeps the ledger structurally sound so +/// it stays enforceable rather than aspirational. It intentionally validates +/// *structure*, not prose, so ordinary edits do not make it brittle. +/// +public sealed class ArchitectureLedgerConsistencyTests +{ + private static readonly string[] ExpectedColumns = + [ + "id", "status", "old_owner", "closed_responsibility", "new_owner", + "allowed_residue", "invariant", "guard_test", "guard_type", "retirement_condition", + ]; + + private static readonly HashSet ValidStatuses = + new(StringComparer.Ordinal) { "planned", "authoritative", "closed" }; + + private static readonly HashSet ValidGuardTypes = + new(StringComparer.Ordinal) { "behavioral", "golden", "source-shape", "review-only" }; + + private static readonly Regex GuardTestNameRegex = + new(@"^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)+$", RegexOptions.Compiled); + + private static bool IsPlaceholder(string value) + => string.IsNullOrWhiteSpace(value) + || value == "-" + || string.Equals(value, "none", StringComparison.OrdinalIgnoreCase); + + [Fact] + public void Ledger_is_present_and_well_formed() + { + var rows = ParseLedger(out var header); + + Assert.Equal(ExpectedColumns, header); + Assert.NotEmpty(rows); + + foreach (var row in rows) + { + Assert.Equal(ExpectedColumns.Length, row.Cells.Count); + foreach (var cell in row.Cells) + { + Assert.False(string.IsNullOrWhiteSpace(cell), + $"Ledger row '{row.Id}' has an empty cell; use '-' for genuinely empty cells."); + } + } + } + + [Fact] + public void Ledger_ids_are_unique() + { + var rows = ParseLedger(out _); + var duplicates = rows + .GroupBy(r => r.Id, StringComparer.Ordinal) + .Where(g => g.Count() > 1) + .Select(g => g.Key) + .ToList(); + + Assert.True(duplicates.Count == 0, + $"Duplicate ledger ids: {string.Join(", ", duplicates)}"); + } + + [Fact] + public void Ledger_status_and_guard_type_values_are_valid() + { + var rows = ParseLedger(out _); + foreach (var row in rows) + { + Assert.True(ValidStatuses.Contains(row.Status), + $"Ledger row '{row.Id}' has invalid status '{row.Status}'."); + Assert.True(ValidGuardTypes.Contains(row.GuardType), + $"Ledger row '{row.Id}' has invalid guard_type '{row.GuardType}'."); + } + } + + [Fact] + public void Authoritative_and_closed_rows_are_guarded_or_review_only() + { + var rows = ParseLedger(out _); + foreach (var row in rows.Where(r => r.Status is "authoritative" or "closed")) + { + if (row.GuardType == "review-only") + { + Assert.False(IsPlaceholder(row.GuardTest), + $"Ledger row '{row.Id}' is a review-only guard for a '{row.Status}' " + + "responsibility and must record a real rationale in guard_test, not a placeholder."); + continue; + } + + Assert.False(IsPlaceholder(row.GuardTest), + $"Ledger row '{row.Id}' is '{row.Status}' but names no guard_test. " + + "Authoritative/closed responsibilities must be protected by a guard test " + + "or documented as a review-only guard."); + + Assert.True(GuardTestNameRegex.IsMatch(row.GuardTest), + $"Ledger row '{row.Id}' guard_test '{row.GuardTest}' is not a valid " + + "'Type.Method' (or 'Namespace.Type.Method') reference."); + } + } + + [Fact] + public void Named_guard_tests_exist_in_test_sources() + { + var rows = ParseLedger(out _); + var testsRoot = LocateTestsRoot(); + var sources = Directory + .EnumerateFiles(testsRoot, "*.cs", SearchOption.AllDirectories) + .Where(p => + !p.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}") && + !p.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}")) + .Select(File.ReadAllText) + .ToList(); + + foreach (var row in rows.Where(r => r.GuardType is "behavioral" or "golden")) + { + // Rows without a concrete named guard are validated elsewhere. + if (IsPlaceholder(row.GuardTest) || !GuardTestNameRegex.IsMatch(row.GuardTest)) + { + continue; + } + + var lastDot = row.GuardTest.LastIndexOf('.'); + var typeName = row.GuardTest[..lastDot]; + var methodName = row.GuardTest[(lastDot + 1)..]; + var simpleType = typeName[(typeName.LastIndexOf('.') + 1)..]; + + var classPattern = new Regex($@"\b(class|record|struct)\s+{Regex.Escape(simpleType)}\b"); + var methodPattern = new Regex($@"\b{Regex.Escape(methodName)}\s*\("); + + var found = sources.Any(src => classPattern.IsMatch(src) && methodPattern.IsMatch(src)); + Assert.True(found, + $"Ledger row '{row.Id}' names guard_test '{row.GuardTest}', but no method " + + $"'{methodName}' was found in a type '{simpleType}' under tests/. If you renamed or " + + "removed the guard test, update docs/ARCHITECTURE.md in the same change."); + } + } + + [Fact] + public void Source_shape_guards_declare_a_retirement_condition() + { + var rows = ParseLedger(out _); + foreach (var row in rows.Where(r => r.GuardType == "source-shape")) + { + Assert.False(row.RetirementCondition == "-" || string.IsNullOrWhiteSpace(row.RetirementCondition), + $"Ledger row '{row.Id}' is a source-shape guard and must set a concrete " + + "retirement_condition so it does not ossify into a brittle permanent test."); + } + } + + private static List ParseLedger(out string[] header) + { + var path = LocateArchitectureDoc(); + var text = File.ReadAllText(path); + + var match = Regex.Match( + text, + @"(?.*?)", + RegexOptions.Singleline); + Assert.True(match.Success, + "docs/ARCHITECTURE.md is missing the / markers."); + + var lines = match.Groups["body"].Value + .Split('\n') + .Select(l => l.Trim()) + .Where(l => l.StartsWith('|')) + .ToList(); + + Assert.True(lines.Count >= 3, + "Ledger must contain a header row, a separator row, and at least one data row."); + + header = SplitRow(lines[0]); + + // lines[1] must be the markdown separator (---). Validate it rather than + // blindly skipping it, so a removed separator can't silently drop the + // first data row from validation. + var separator = SplitRow(lines[1]); + Assert.True(separator.Length == header.Length, + "Ledger separator row column count does not match the header row."); + Assert.All(separator, cell => Assert.Matches("^:?-{3,}:?$", cell)); + + var rows = new List(); + foreach (var line in lines.Skip(2)) + { + var cells = SplitRow(line); + rows.Add(new LedgerRow(cells)); + } + + return rows; + } + + private static string[] SplitRow(string line) + { + // Trim the leading/trailing pipe, then split on the interior pipes. + var trimmed = line.Trim().Trim('|'); + return trimmed.Split('|').Select(c => c.Trim()).ToArray(); + } + + private static string LocateArchitectureDoc() + { + // Honor an explicit repo-root override (used in linked git worktrees), + // otherwise walk up from the test bin folder to find docs/ARCHITECTURE.md. + var repoRoot = Environment.GetEnvironmentVariable("OPENCLAW_REPO_ROOT"); + if (!string.IsNullOrWhiteSpace(repoRoot)) + { + var direct = Path.Combine(repoRoot, "docs", "ARCHITECTURE.md"); + if (File.Exists(direct)) return direct; + } + + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 10 && dir is not null; i++) + { + var candidate = Path.Combine(dir, "docs", "ARCHITECTURE.md"); + if (File.Exists(candidate)) return candidate; + dir = Path.GetDirectoryName(dir); + } + + throw new FileNotFoundException( + "Could not locate docs/ARCHITECTURE.md from the test working directory. " + + "Set OPENCLAW_REPO_ROOT to the repository root when running in a linked worktree."); + } + + private static string LocateTestsRoot() + { + var repoRoot = Environment.GetEnvironmentVariable("OPENCLAW_REPO_ROOT"); + if (!string.IsNullOrWhiteSpace(repoRoot)) + { + var direct = Path.Combine(repoRoot, "tests"); + if (Directory.Exists(direct)) return direct; + } + + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 10 && dir is not null; i++) + { + var candidate = Path.Combine(dir, "tests"); + if (Directory.Exists(Path.Combine(candidate, "OpenClaw.Shared.Tests"))) return candidate; + dir = Path.GetDirectoryName(dir); + } + + throw new DirectoryNotFoundException( + "Could not locate the tests/ directory from the test working directory. " + + "Set OPENCLAW_REPO_ROOT to the repository root when running in a linked worktree."); + } + + private sealed class LedgerRow + { + public LedgerRow(string[] cells) + { + Cells = cells; + } + + public IReadOnlyList Cells { get; } + + public string Id => Cells.Count > 0 ? Cells[0] : ""; + public string Status => Cells.Count > 1 ? Cells[1] : ""; + public string GuardTest => Cells.Count > 7 ? Cells[7] : ""; + public string GuardType => Cells.Count > 8 ? Cells[8] : ""; + public string RetirementCondition => Cells.Count > 9 ? Cells[9] : ""; + } +} diff --git a/tests/OpenClaw.TestSupport/CliHarness.cs b/tests/OpenClaw.TestSupport/CliHarness.cs new file mode 100644 index 000000000..594d25115 --- /dev/null +++ b/tests/OpenClaw.TestSupport/CliHarness.cs @@ -0,0 +1,52 @@ +using System.Text; + +namespace OpenClaw.TestSupport; + +/// +/// Captures stdout/stderr and provides an environment lookup for CLI tests, +/// replacing the repeated (StringWriter Out, StringWriter Err) tuples +/// plus ad hoc env dictionaries in the CLI test projects. Optionally exposes a +/// shared for command/list-tools round trips. +/// See docs/ARCHITECTURE.md (ledger id test-cli-harness). +/// +public sealed class CliHarness : IDisposable +{ + private readonly Dictionary _env = new(StringComparer.OrdinalIgnoreCase); + private FakeMcpServer? _server; + + /// Captured standard output writer to pass to the CLI runner. + public StringWriter Out { get; } = new(new StringBuilder()); + + /// Captured standard error writer to pass to the CLI runner. + public StringWriter Err { get; } = new(new StringBuilder()); + + /// Lazily created loopback MCP server (created on first access). + public FakeMcpServer Server => _server ??= new FakeMcpServer(); + + /// Text written to stdout so far. + public string StdOut => Out.ToString(); + + /// Text written to stderr so far. + public string StdErr => Err.ToString(); + + /// Registers an environment value visible via . + public CliHarness WithEnv(string name, string? value) + { + _env[name] = value; + return this; + } + + /// + /// Environment lookup delegate for CLI runners that accept one. Returns null + /// for names that were not registered via . + /// + public Func EnvLookup + => name => _env.TryGetValue(name, out var value) ? value : null; + + public void Dispose() + { + _server?.Dispose(); + Out.Dispose(); + Err.Dispose(); + } +} diff --git a/tests/OpenClaw.TestSupport/Directory.Build.props b/tests/OpenClaw.TestSupport/Directory.Build.props new file mode 100644 index 000000000..ca12b2bef --- /dev/null +++ b/tests/OpenClaw.TestSupport/Directory.Build.props @@ -0,0 +1,22 @@ + + + + + + + net10.0 + enable + enable + false + true + all + + + diff --git a/tests/OpenClaw.TestSupport/EnvironmentScope.cs b/tests/OpenClaw.TestSupport/EnvironmentScope.cs new file mode 100644 index 000000000..de50bf2ff --- /dev/null +++ b/tests/OpenClaw.TestSupport/EnvironmentScope.cs @@ -0,0 +1,51 @@ +namespace OpenClaw.TestSupport; + +/// +/// Saves and restores environment variables for the duration of a test. +/// Replaces the repeated read-original / try-finally / SetEnvironmentVariable +/// restore dance (see e.g. SetupContextTests). Restores every variable it set, +/// in reverse order, on dispose. See docs/ARCHITECTURE.md (ledger id +/// test-env-scope). +/// +/// +/// Environment variables are process-global, so tests that use this fixture +/// must not run in parallel with other tests that read/write the same +/// variables. Keep such tests in a non-parallel xUnit collection (see the +/// existing EnvironmentVariableCollection pattern). +/// +public sealed class EnvironmentScope : IDisposable +{ + private readonly List<(string Name, string? Original)> _originals = new(); + + /// Creates an empty scope. Use to mutate variables. + public EnvironmentScope() + { + } + + /// Creates a scope and immediately sets one variable. + public EnvironmentScope(string name, string? value) + { + Set(name, value); + } + + /// + /// Sets an environment variable, remembering its prior value so it is + /// restored on dispose. Fluent: returns this scope. + /// + public EnvironmentScope Set(string name, string? value) + { + _originals.Add((name, Environment.GetEnvironmentVariable(name))); + Environment.SetEnvironmentVariable(name, value); + return this; + } + + public void Dispose() + { + for (var i = _originals.Count - 1; i >= 0; i--) + { + var (name, original) = _originals[i]; + Environment.SetEnvironmentVariable(name, original); + } + _originals.Clear(); + } +} diff --git a/tests/OpenClaw.WinNode.Cli.Tests/FakeMcpServer.cs b/tests/OpenClaw.TestSupport/FakeMcpServer.cs similarity index 73% rename from tests/OpenClaw.WinNode.Cli.Tests/FakeMcpServer.cs rename to tests/OpenClaw.TestSupport/FakeMcpServer.cs index dce023665..9382ee127 100644 --- a/tests/OpenClaw.WinNode.Cli.Tests/FakeMcpServer.cs +++ b/tests/OpenClaw.TestSupport/FakeMcpServer.cs @@ -2,17 +2,18 @@ using System.Net.Sockets; using System.Text; -namespace OpenClaw.WinNode.Cli.Tests; +namespace OpenClaw.TestSupport; /// /// Tiny loopback HTTP server that captures the request body and returns a -/// canned response. Lets the RunAsync tests exercise the real HttpClient code -/// path (timeouts, connection failures, JSON-RPC envelopes) without any -/// reliance on the running tray. +/// canned response. Lets CLI/MCP tests exercise the real HttpClient code path +/// (timeouts, connection failures, JSON-RPC envelopes) without any reliance on +/// the running tray. Shared single source: see docs/ARCHITECTURE.md +/// (ledger id test-fake-mcp). /// -internal sealed class FakeMcpServer : IDisposable +public sealed class FakeMcpServer : IDisposable { - private readonly HttpListener _listener = new(); + private readonly HttpListener _listener; private readonly CancellationTokenSource _cts = new(); private readonly Task _loop; @@ -32,12 +33,37 @@ internal sealed class FakeMcpServer : IDisposable public FakeMcpServer() { - Port = FindFreePort(); - _listener.Prefixes.Add($"http://127.0.0.1:{Port}/"); - _listener.Start(); + (Port, _listener) = StartListenerWithRetry(); _loop = Task.Run(AcceptLoopAsync); } + /// + /// Binding is a two-step "find a free port, then start an HttpListener on it" + /// operation with an inherent TOCTOU window: another process/test can grab the + /// port between steps. Retry on bind failure so this shared fixture does not + /// flake under parallel test runs. + /// + private static (int Port, HttpListener Listener) StartListenerWithRetry() + { + const int maxAttempts = 10; + for (var attempt = 1; ; attempt++) + { + var port = FindFreePort(); + var listener = new HttpListener(); + listener.Prefixes.Add($"http://127.0.0.1:{port}/"); + try + { + listener.Start(); + return (port, listener); + } + catch (HttpListenerException) when (attempt < maxAttempts) + { + // slopwatch-ignore: SW003 Test cleanup or fixture teardown is best-effort and must not hide the test outcome. + try { listener.Close(); } catch { } + } + } + } + private async Task AcceptLoopAsync() { while (!_cts.IsCancellationRequested && _listener.IsListening) diff --git a/tests/OpenClaw.TestSupport/OpenClaw.TestSupport.csproj b/tests/OpenClaw.TestSupport/OpenClaw.TestSupport.csproj new file mode 100644 index 000000000..e1df8442d --- /dev/null +++ b/tests/OpenClaw.TestSupport/OpenClaw.TestSupport.csproj @@ -0,0 +1,17 @@ + + + + + false + + + + + + + diff --git a/tests/OpenClaw.TestSupport/SettingsDataBuilder.cs b/tests/OpenClaw.TestSupport/SettingsDataBuilder.cs new file mode 100644 index 000000000..c9fedaf7a --- /dev/null +++ b/tests/OpenClaw.TestSupport/SettingsDataBuilder.cs @@ -0,0 +1,37 @@ +using OpenClaw.Shared; + +namespace OpenClaw.TestSupport; + +/// +/// Fluent builder for test data. Records the +/// requested overrides and replays them onto a fresh +/// (starting from the production defaults) on every call, so +/// each built instance is independent — mutating the builder after a build never +/// changes an already-built result, and no mutable list state is shared between +/// builds. See docs/ARCHITECTURE.md (ledger id test-settings-builder). +/// +public sealed class SettingsDataBuilder +{ + private readonly List> _mutations = new(); + + /// Applies an arbitrary mutation to the settings under construction. + public SettingsDataBuilder With(Action mutate) + { + _mutations.Add(mutate); + return this; + } + + public SettingsDataBuilder WithGatewayUrl(string? url) => With(d => d.GatewayUrl = url); + public SettingsDataBuilder WithNodeMode(bool enabled) => With(d => d.EnableNodeMode = enabled); + public SettingsDataBuilder WithAutoStart(bool enabled) => With(d => d.AutoStart = enabled); + + public SettingsData Build() + { + var data = new SettingsData(); + foreach (var mutate in _mutations) + { + mutate(data); + } + return data; + } +} diff --git a/tests/OpenClaw.TestSupport/TempDirectory.cs b/tests/OpenClaw.TestSupport/TempDirectory.cs new file mode 100644 index 000000000..f69eaf449 --- /dev/null +++ b/tests/OpenClaw.TestSupport/TempDirectory.cs @@ -0,0 +1,39 @@ +namespace OpenClaw.TestSupport; + +/// +/// Creates a unique temporary directory for a test and best-effort deletes it +/// on dispose. Replaces the repeated +/// Path.Combine(Path.GetTempPath(), "openclaw-test-" + Guid...) plus +/// Directory.Delete(..., true) boilerplate scattered across the suite. +/// See docs/ARCHITECTURE.md (ledger id test-temp-dir). +/// +public sealed class TempDirectory : IDisposable +{ + /// Absolute path to the created directory. + public string Path { get; } + + public TempDirectory(string prefix = "openclaw-test-") + { + Path = System.IO.Path.Combine( + System.IO.Path.GetTempPath(), + prefix + Guid.NewGuid().ToString("N")[..8]); + Directory.CreateDirectory(Path); + } + + /// Combines relative parts against the temp directory root. + public string Combine(params string[] parts) + { + var all = new string[parts.Length + 1]; + all[0] = Path; + Array.Copy(parts, 0, all, 1, parts.Length); + return System.IO.Path.Combine(all); + } + + public override string ToString() => Path; + + public void Dispose() + { + // slopwatch-ignore: SW003 Test cleanup or fixture teardown is best-effort and must not hide the test outcome. + try { Directory.Delete(Path, recursive: true); } catch { } + } +} diff --git a/tests/OpenClaw.WinNode.Cli.Tests/AuthTokenTests.cs b/tests/OpenClaw.WinNode.Cli.Tests/AuthTokenTests.cs index 4f3ed7fcd..a5eff2668 100644 --- a/tests/OpenClaw.WinNode.Cli.Tests/AuthTokenTests.cs +++ b/tests/OpenClaw.WinNode.Cli.Tests/AuthTokenTests.cs @@ -4,6 +4,7 @@ using System.Security.AccessControl; using System.Security.Principal; using OpenClaw.Shared; +using OpenClaw.TestSupport; using OpenClaw.WinNode.Cli; namespace OpenClaw.WinNode.Cli.Tests; diff --git a/tests/OpenClaw.WinNode.Cli.Tests/OpenClaw.WinNode.Cli.Tests.csproj b/tests/OpenClaw.WinNode.Cli.Tests/OpenClaw.WinNode.Cli.Tests.csproj index 0ad15c792..cc748c7ea 100644 --- a/tests/OpenClaw.WinNode.Cli.Tests/OpenClaw.WinNode.Cli.Tests.csproj +++ b/tests/OpenClaw.WinNode.Cli.Tests/OpenClaw.WinNode.Cli.Tests.csproj @@ -2,6 +2,7 @@ + diff --git a/tests/OpenClaw.WinNode.Cli.Tests/RunAsyncTests.cs b/tests/OpenClaw.WinNode.Cli.Tests/RunAsyncTests.cs index 4ba749bd8..b65b98ccc 100644 --- a/tests/OpenClaw.WinNode.Cli.Tests/RunAsyncTests.cs +++ b/tests/OpenClaw.WinNode.Cli.Tests/RunAsyncTests.cs @@ -2,6 +2,7 @@ using System.Net.Http; using System.Net.Sockets; using System.Text.Json; +using OpenClaw.TestSupport; using OpenClaw.WinNode.Cli; namespace OpenClaw.WinNode.Cli.Tests;