diff --git a/docs/prd/PRD-006-mcp-tool-integration.md b/docs/prd/PRD-006-mcp-tool-integration.md index ae7091bbe..5d51bff87 100644 --- a/docs/prd/PRD-006-mcp-tool-integration.md +++ b/docs/prd/PRD-006-mcp-tool-integration.md @@ -94,6 +94,13 @@ Runtime SHALL degrade gracefully when MCP server is unavailable: - Reconnection is attempted on next tool call - Diagnostics flag the outage +### MCP-009 Daemon-Bound Server Ownership + +Each configured MCP server SHALL have at most one live client connection per +Netclaw daemon. A local STDIO server process and its internal state are shared +by all sessions authorized to use that server; Netclaw session identity SHALL +not launch or select a separate MCP process. + ## Non-Goals (MVP) - Dynamic marketplace discovery of MCP servers @@ -110,6 +117,8 @@ Runtime SHALL degrade gracefully when MCP server is unavailable: 5. MCP tools appear in session tool definitions when server is enabled and granted. 6. Unavailable MCP server does not crash the session. +7. Calls from different authorized sessions to one local STDIO profile use the + same daemon-owned client and child process. ## Cross-References diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index 60ca5f447..6a3f13284 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -14,6 +14,19 @@ search_tools(query: "email") # keyword search After discovery, matched tools become callable for the session. +### MCP server state and concurrent callers + +One configured MCP server is one daemon-owned client connection. Local STDIO +servers therefore run as one process shared by every session authorized to use +that server; a Slack thread or subagent does not receive a private MCP process. +State held by the server is shared too. + +For Playwright, inspect the existing tabs before acting, create a new tab for +your work, and close only tabs you created. Tabs help callers coordinate, but +they are not security boundaries: cookies, local storage, permissions, and +other browser-context state may be shared. Do not assume another authorized +session's browser activity is private from yours. + Sessions receive granted tool categories. `builtin` is always granted. Other categories (`web`, `file`, `shell`, `scheduling`) depend on ACL config. If a tool is missing, it may not be granted for this session. diff --git a/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/.openspec.yaml b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/.openspec.yaml new file mode 100644 index 000000000..64105fc96 --- /dev/null +++ b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-14 diff --git a/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/design.md b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/design.md new file mode 100644 index 000000000..fa38d8fe7 --- /dev/null +++ b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/design.md @@ -0,0 +1,58 @@ +## Context + +`McpClientManager` already owns one client per configured MCP server in `_clients`. Playwright additionally enters a second path selected by command/name heuristics: the manager retains the startup client for discovery, creates a `ScopedClientHandle` per `ToolExecutionContext.SessionId`, and scans those handles for idle cleanup only during later scoped invocations. This makes process count proportional to recent Netclaw sessions and embeds Playwright-specific behavior in the generic MCP manager. + +MCP authorization is enforced before calls reach the invoker. Session actors and persistence do not own MCP processes and require no changes. + +## Goals / Non-Goals + +**Goals:** + +- Make configured MCP server identity the sole MCP client/process ownership key. +- Reuse the existing shared client invocation and reconnect path for Playwright. +- Delete the alternate scoped-client lifecycle and Playwright command rewriting. +- Preserve clear invocation failures, diagnostics, and deterministic daemon shutdown. + +**Non-Goals:** + +- Lazy startup or idle process reclamation. +- Per-session browser contexts or state isolation. +- New lifecycle configuration, pools, queues, or background maintenance. +- Changes to actor boundaries, persisted state, grants, or remote transports. + +## Decisions + +### One client per configured server + +`_clients[McpServerName]` remains the sole live-client collection. `InvokeAsync` always uses the existing shared invocation path. This matches the configured-resource model used by other MCP harnesses and bounds a local STDIO profile to one root child process per daemon. + +Alternative: retain session-scoped clients but cap them. Rejected because it preserves two lifecycle models, ownership state, cleanup scans, and Playwright-specific classification. + +### Share server-internal state across authorized sessions + +Netclaw session identity will not select or partition MCP clients. Authorization remains the access boundary; state held inside an MCP server is daemon-scoped. For Playwright, authorized callers may observe or affect the same browser context. + +Alternative: multiplex Playwright contexts through per-session HTTP connections. Rejected because the STDIO tool surface exposes no context-selection primitive and per-session connection management recreates the lifecycle machinery being removed. + +### Pass configured STDIO arguments unchanged + +The manager will not recognize Playwright or append `--isolated`. Operators and canonical browser configuration own server arguments. This removes hidden product-specific behavior and makes the launched process match persisted configuration. + +### Preserve startup and shutdown behavior + +This change does not add lazy creation or idle teardown. Enabled servers still connect and discover tools at daemon startup, reconnect through the existing failure path, and dispose on daemon shutdown. Those behaviors provide a smaller, independently reviewable baseline; on-demand residency can be considered separately if process evidence still justifies it. + +## Risks / Trade-offs + +- **Authorized sessions share browser state** → Document that MCP state is daemon-scoped and keep existing audience/server grants as the access boundary. +- **Concurrent calls may contend inside a stateful server** → Preserve the existing shared invocation behavior; add synchronization only if a reproducible server/client failure proves it necessary. +- **Removing implicit `--isolated` changes profile persistence** → Pass the canonical configured arguments exactly and test that contract; operators can explicitly configure `--isolated` when desired. +- **Startup residency remains** → Accept for this focused correction; the immediate unbounded multiplier is removed without adding a maintenance loop. + +## Migration Plan + +No configuration or persisted-state migration is required. Deploying the change collapses Playwright from a retained discovery client plus per-session clients to the single configured client. Rollback restores the former process model without data migration. + +## Open Questions + +None for this change. diff --git a/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/proposal.md b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/proposal.md new file mode 100644 index 000000000..d99ad9beb --- /dev/null +++ b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/proposal.md @@ -0,0 +1,37 @@ +## Why + +Local STDIO MCP servers are daemon-owned child processes, but Netclaw currently gives Playwright a second, session-scoped lifecycle that retains an unused discovery process and can launch one additional process per session. This multiplies heavyweight browser process trees and makes MCP process ownership depend on Slack thread identity instead of the configured server. + +Source PRD: PRD-006. + +## What Changes + +- Remove Playwright-specific session-scoped MCP clients and process fan-out. +- Treat every configured MCP server as one daemon-owned client connection; a local STDIO profile therefore owns at most one child process per daemon. +- Keep the process and its state shared by every session authorized to invoke that server. +- Stop adding Playwright's `--isolated` argument implicitly; configured command arguments pass through unchanged. +- Preserve existing startup discovery, reconnect, diagnostics, authorization, and daemon-shutdown behavior. +- Document that MCP server state is daemon-scoped rather than a Netclaw session-isolation boundary. + +In scope: MCP client ownership and invocation behavior for configured local STDIO servers, focused regression tests, and operator/agent guidance. + +Out of scope: lazy startup, idle shutdown, per-session browser contexts, client pools, queues, new lifecycle configuration, remote transport changes, and changes to the Playwright MCP server. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `netclaw-mcp`: Define one daemon-owned client per configured server and make local STDIO state shared across authorized Netclaw sessions. + +## Impact + +- Code: `McpClientManager` becomes smaller by deleting Playwright detection, scoped-client storage, scoped cleanup, and the alternate invocation path. +- Tests: focused MCP manager coverage proves calls from different session identities reuse one client/process path and configured arguments are not rewritten. +- Security: authorization remains enforced before MCP invocation, but an authorized MCP server's internal state is shared daemon-wide; sessions are not an isolation boundary for that state. +- Operations: one configured local STDIO server produces at most one root child process per daemon and is disposed during reconnect or daemon shutdown. +- Configuration/schema: unchanged. +- Dependencies and public APIs: unchanged. diff --git a/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/specs/netclaw-mcp/spec.md b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/specs/netclaw-mcp/spec.md new file mode 100644 index 000000000..f14fb7a75 --- /dev/null +++ b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/specs/netclaw-mcp/spec.md @@ -0,0 +1,41 @@ +## ADDED Requirements + +### Requirement: Configured MCP server has daemon-bound client ownership + +The system SHALL maintain at most one live MCP client connection for each enabled configured MCP server within a daemon process. For a local STDIO server, that connection SHALL own the server child process and SHALL be shared by every Netclaw session authorized to invoke the server. + +#### Scenario: Different sessions invoke one local STDIO server + +- **GIVEN** a local STDIO MCP server is enabled and available to two authorized sessions +- **WHEN** both sessions invoke tools from that server +- **THEN** both invocations use the same configured MCP client connection +- **AND** Netclaw does not launch a child process for either session identity + +#### Scenario: Session identity does not partition MCP state + +- **GIVEN** an authorized session changes state held by an MCP server +- **WHEN** another authorized session invokes that server +- **THEN** the second invocation uses the same daemon-scoped server state + +#### Scenario: Daemon shutdown owns local child cleanup + +- **GIVEN** an enabled local STDIO MCP server is connected +- **WHEN** the Netclaw daemon stops +- **THEN** Netclaw disposes the configured MCP client +- **AND** the client transport terminates its owned child process + +### Requirement: Configured STDIO command is launched without server-specific rewriting + +The system SHALL pass the configured command and arguments to a local STDIO MCP transport without adding arguments based on the server name, command text, or implementation identity. + +#### Scenario: Playwright arguments pass through unchanged + +- **GIVEN** a local STDIO profile invokes the Playwright MCP package without `--isolated` +- **WHEN** Netclaw creates its transport +- **THEN** the launched argument list does not contain an implicitly added `--isolated` argument + +#### Scenario: Explicit isolation argument is preserved + +- **GIVEN** a local STDIO profile explicitly configures `--isolated` +- **WHEN** Netclaw creates its transport +- **THEN** the launched argument list contains the configured argument exactly once diff --git a/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/tasks.md b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/tasks.md new file mode 100644 index 000000000..98567ecd4 --- /dev/null +++ b/openspec/changes/archive/2026-07-14-make-stdio-mcp-process-bound/tasks.md @@ -0,0 +1,16 @@ +## 1. Collapse MCP Client Ownership + +- [x] 1.1 Delete Playwright detection, implicit argument rewriting, scoped-client collections, cleanup, and disposal paths from `McpClientManager`. +- [x] 1.2 Route every configured MCP server invocation through the existing daemon-owned shared client and reconnect path. + +## 2. Automated Proof + +- [x] 2.1 Add focused tests proving different `ToolExecutionContext` session identities use one configured client/process path. +- [x] 2.2 Add focused tests proving STDIO arguments pass through unchanged, including explicit `--isolated` preservation. +- [x] 2.3 Run targeted MCP tests and the full relevant test project. + +## 3. Guidance and Quality Gates + +- [x] 3.1 Update `netclaw-operations` guidance to state that configured MCP servers and their state are daemon-scoped; bump the skill version. +- [x] 3.2 Confirm the eval suite is not applicable because the change does not alter production tool definitions, skill matching, prompts, or model behavior. +- [x] 3.3 Run OpenSpec validation, Slopwatch, file-header verification, and `git diff --check`. diff --git a/openspec/specs/netclaw-mcp/spec.md b/openspec/specs/netclaw-mcp/spec.md index d9b3cbdf5..af2bd7a97 100644 --- a/openspec/specs/netclaw-mcp/spec.md +++ b/openspec/specs/netclaw-mcp/spec.md @@ -49,6 +49,46 @@ The system SHALL validate MCP server connectivity and discovery. - **WHEN** operator runs MCP validation - **THEN** output indicates handshake status and discovered tool count +### Requirement: Configured MCP server has daemon-bound client ownership + +The system SHALL maintain at most one live MCP client connection for each enabled configured MCP server within a daemon process. For a local STDIO server, that connection SHALL own the server child process and SHALL be shared by every Netclaw session authorized to invoke the server. + +#### Scenario: Different sessions invoke one local STDIO server + +- **GIVEN** a local STDIO MCP server is enabled and available to two authorized sessions +- **WHEN** both sessions invoke tools from that server +- **THEN** both invocations use the same configured MCP client connection +- **AND** Netclaw does not launch a child process for either session identity + +#### Scenario: Session identity does not partition MCP state + +- **GIVEN** an authorized session changes state held by an MCP server +- **WHEN** another authorized session invokes that server +- **THEN** the second invocation uses the same daemon-scoped server state + +#### Scenario: Daemon shutdown owns local child cleanup + +- **GIVEN** an enabled local STDIO MCP server is connected +- **WHEN** the Netclaw daemon stops +- **THEN** Netclaw disposes the configured MCP client +- **AND** the client transport terminates its owned child process + +### Requirement: Configured STDIO command is launched without server-specific rewriting + +The system SHALL pass the configured command and arguments to a local STDIO MCP transport without adding arguments based on the server name, command text, or implementation identity. + +#### Scenario: Playwright arguments pass through unchanged + +- **GIVEN** a local STDIO profile invokes the Playwright MCP package without `--isolated` +- **WHEN** Netclaw creates its transport +- **THEN** the launched argument list does not contain an implicitly added `--isolated` argument + +#### Scenario: Explicit isolation argument is preserved + +- **GIVEN** a local STDIO profile explicitly configures `--isolated` +- **WHEN** Netclaw creates its transport +- **THEN** the launched argument list contains the configured argument exactly once + ### Requirement: Policy-gated MCP invocation The system SHALL apply ACL and grants before invoking MCP tools. diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpProcessBoundStdioTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpProcessBoundStdioTests.cs new file mode 100644 index 000000000..fbcf5baa3 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Mcp/McpProcessBoundStdioTests.cs @@ -0,0 +1,90 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using System.Diagnostics; +using System.Text.Json; +using Netclaw.Actors.Tools; +using Netclaw.Configuration; +using Netclaw.Tools; +using Xunit; + +namespace Netclaw.Daemon.Tests.Mcp; + +public sealed class McpProcessBoundStdioTests +{ + [Fact] + public async Task DifferentSessions_UseOneConfiguredProcess_WithoutArgumentRewriting() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var entry = CreateEntry("--netclaw-pass-through-probe"); + var registry = new ToolRegistry(); + await using var harness = McpSmokeHarness.Create( + new Dictionary { ["browser_playwright"] = entry }, registry); + + await harness.Manager.StartAsync(cts.Token); + + var first = await GetProcessInfoAsync(harness, "slack/channel/thread-a", cts.Token); + var second = await GetProcessInfoAsync(harness, "slack/channel/thread-b", cts.Token); + + Assert.Equal(first.ProcessId, second.ProcessId); + Assert.Contains("--netclaw-pass-through-probe", first.Arguments); + Assert.DoesNotContain("--isolated", first.Arguments); + + using var process = Process.GetProcessById(first.ProcessId); + await harness.Manager.StopAsync(cts.Token); + await process.WaitForExitAsync(cts.Token); + Assert.True(process.HasExited); + } + + [Fact] + public async Task ExplicitIsolatedArgument_IsPreservedExactlyOnce() + { + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(2)); + var registry = new ToolRegistry(); + await using var harness = McpSmokeHarness.Create( + new Dictionary + { + ["browser_playwright"] = CreateEntry("--isolated"), + }, + registry); + + await harness.Manager.StartAsync(cts.Token); + + var info = await GetProcessInfoAsync(harness, "slack/channel/thread", cts.Token); + + Assert.Single(info.Arguments, argument => argument == "--isolated"); + } + + private static McpServerEntry CreateEntry(params string[] extraArguments) + => new() + { + Transport = "stdio", + Command = "dotnet", + Arguments = [SmokeMcpServerLocator.LocateDll(), .. extraArguments], + Enabled = true, + }; + + private static async Task GetProcessInfoAsync( + McpSmokeHarness harness, + string sessionId, + CancellationToken ct) + { + var result = await harness.Manager.InvokeAsync( + "browser_playwright", + "process-info", + null, + new ToolExecutionContext(sessionId, null) { Audience = TrustAudience.Personal }, + ct); + + return JsonSerializer.Deserialize(result, JsonOptions)!; + } + + private static readonly JsonSerializerOptions JsonOptions = new() + { + PropertyNameCaseInsensitive = true, + }; + + private sealed record ProcessInfo(int ProcessId, string[] Arguments); +} diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 17dfdd08d..f63f214b8 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -17,8 +17,6 @@ namespace Netclaw.Daemon.Mcp; internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolInvoker, IMcpReconnectable { - private const string PlaywrightServerName = "browser_playwright"; - private readonly Dictionary _serverEntries; private readonly ToolRegistry _toolRegistry; private readonly ToolConfig _toolConfig; @@ -35,16 +33,6 @@ internal sealed class McpClientManager : IHostedService, IDisposable, IMcpToolIn private readonly ConcurrentDictionary _statuses = new(); - private readonly ConcurrentDictionary _sessionScopedServers = new(); - - private readonly ConcurrentDictionary>> _scopedClients = - new(StringComparer.OrdinalIgnoreCase); - - private readonly SemaphoreSlim _scopedCleanupGate = new(1, 1); - private readonly TimeSpan _scopedClientIdleTimeout = TimeSpan.FromMinutes(10); - private readonly TimeSpan _scopedCleanupInterval = TimeSpan.FromMinutes(1); - private long _nextScopedCleanupAtMs; - public McpClientManager( Dictionary serverEntries, ToolRegistry toolRegistry, @@ -74,7 +62,6 @@ public async Task StartAsync(CancellationToken cancellationToken) if (!entry.Enabled) { _statuses[serverName] = new McpServerStatus(serverName, McpConnectionState.Disabled, 0, null); - _sessionScopedServers.TryRemove(serverName, out _); _logger.LogInformation("MCP server '{Name}' is disabled, skipping", name); continue; } @@ -100,9 +87,6 @@ public async Task StopAsync(CancellationToken cancellationToken) _clients.Clear(); _sharedToolFunctions.Clear(); - _sessionScopedServers.Clear(); - - await DisposeAllScopedClientsAsync(); } public McpClient? GetClient(McpServerName serverName) @@ -135,7 +119,6 @@ public async Task TryReconnectAsync(McpServerName serverName, Cancellation } _sharedToolFunctions.TryRemove(serverName, out _); - await DisposeScopedClientsForServerAsync(serverName); return await ConnectAsync(serverName, entry, ct); } @@ -150,9 +133,6 @@ public async Task InvokeAsync( var server = new McpServerName(serverName); var tool = new ToolName(toolName); - if (UsesSessionScopedClient(server)) - return await InvokeScopedAsync(server, tool, arguments, context, ct); - return await InvokeSharedAsync(server, tool, arguments, ct); } @@ -194,38 +174,6 @@ private async Task InvokeSharedAsync( } } - private async Task InvokeScopedAsync( - McpServerName serverName, - ToolName toolName, - IDictionary? arguments, - ToolExecutionContext? context, - CancellationToken ct) - { - var scopeId = ResolveScopeId(context); - var handle = await GetOrCreateScopedClientHandleAsync(serverName, scopeId, ct); - - await CleanupIdleScopedClientsIfDueAsync(ct); - await handle.ExecutionGate.WaitAsync(ct); - - try - { - handle.Touch(_timeProvider.GetUtcNow()); - - if (!handle.Tools.TryGetValue(toolName.Value, out var function)) - { - throw new InvalidOperationException( - $"MCP tool '{toolName.Value}' is not available on server '{serverName.Value}'."); - } - - return await InvokeFunctionAsync(function, $"{serverName.Value}/{toolName.Value}", arguments, ct); - } - finally - { - handle.Touch(_timeProvider.GetUtcNow()); - handle.ExecutionGate.Release(); - } - } - // qualifiedToolName is the server-qualified "server/tool" name (not the bare // function.Name, which omits the server) so MCP error attribution matches the // bound-tool path (McpToolAdapter.Name) — otherwise the same error renders @@ -256,160 +204,6 @@ private bool TryGetSharedFunction(McpServerName serverName, string toolName, out return serverTools.TryGetValue(toolName, out function); } - private bool UsesSessionScopedClient(McpServerName serverName) - { - return _sessionScopedServers.TryGetValue(serverName, out var enabled) && enabled; - } - - private async Task GetOrCreateScopedClientHandleAsync( - McpServerName serverName, - string scopeId, - CancellationToken ct) - { - var key = BuildScopedClientKey(serverName.Value, scopeId); - - while (true) - { - var lazy = _scopedClients.GetOrAdd(key, _ => - new Lazy>( - () => CreateScopedClientHandleAsync(serverName), - LazyThreadSafetyMode.ExecutionAndPublication)); - - try - { - var handle = await lazy.Value.WaitAsync(ct); - handle.Touch(_timeProvider.GetUtcNow()); - return handle; - } - catch - { - _scopedClients.TryRemove(new KeyValuePair>>(key, lazy)); - await DisposeLazyScopedHandleAsync(lazy); - throw; - } - } - } - - private async Task CreateScopedClientHandleAsync(McpServerName serverName) - { - if (!_serverEntries.TryGetValue(serverName.Value, out var entry)) - { - throw new InvalidOperationException($"MCP server '{serverName.Value}' is not configured."); - } - - var client = await CreateClientAsync(serverName, entry, CancellationToken.None, updateStatusOnAuthFailure: false); - if (client is null) - throw new InvalidOperationException($"MCP server '{serverName.Value}' requires OAuth authorization."); - - var tools = await client.ListToolsAsync(cancellationToken: CancellationToken.None); - var toolMap = CreateFunctionMap(tools); - - _logger.LogInformation( - "Created scoped MCP client for server '{ServerName}' (tools={ToolCount})", - serverName.Value, - tools.Count); - - return new ScopedClientHandle(client, toolMap, _timeProvider.GetUtcNow()); - } - - private async Task DisposeScopedClientsForServerAsync(McpServerName serverName) - { - var prefix = serverName.Value + "::"; - - foreach (var (key, _) in _scopedClients.ToArray()) - { - if (!key.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) - continue; - - if (_scopedClients.TryRemove(key, out var lazy)) - await DisposeLazyScopedHandleAsync(lazy); - } - } - - private async Task DisposeAllScopedClientsAsync() - { - foreach (var (key, _) in _scopedClients.ToArray()) - { - if (_scopedClients.TryRemove(key, out var lazy)) - await DisposeLazyScopedHandleAsync(lazy); - } - } - - private async Task DisposeLazyScopedHandleAsync(Lazy> lazy) - { - if (!lazy.IsValueCreated) - return; - - try - { - var handle = await lazy.Value; - await handle.DisposeAsync(); - } - catch (Exception ex) - { - _logger.LogDebug(ex, "Error disposing scoped MCP client handle"); - } - } - - private async Task CleanupIdleScopedClientsIfDueAsync(CancellationToken ct) - { - if (_scopedClients.IsEmpty) - return; - - var nowMs = _timeProvider.GetUtcNow().ToUnixTimeMilliseconds(); - if (nowMs < Volatile.Read(ref _nextScopedCleanupAtMs)) - return; - - if (!await _scopedCleanupGate.WaitAsync(0, ct)) - return; - - try - { - nowMs = _timeProvider.GetUtcNow().ToUnixTimeMilliseconds(); - if (nowMs < Volatile.Read(ref _nextScopedCleanupAtMs)) - return; - - Volatile.Write( - ref _nextScopedCleanupAtMs, - nowMs + (long)_scopedCleanupInterval.TotalMilliseconds); - - var idleBeforeMs = nowMs - (long)_scopedClientIdleTimeout.TotalMilliseconds; - - foreach (var (key, lazy) in _scopedClients.ToArray()) - { - if (!lazy.IsValueCreated) - continue; - - Task handleTask; - try - { - handleTask = lazy.Value; - } - catch - { - continue; - } - - if (!handleTask.IsCompletedSuccessfully) - continue; - - var handle = handleTask.Result; - if (handle.LastUsedAtMs > idleBeforeMs) - continue; - - if (handle.ExecutionGate.CurrentCount == 0) - continue; - - if (_scopedClients.TryRemove(new KeyValuePair>>(key, lazy))) - await handle.DisposeAsync(); - } - } - finally - { - _scopedCleanupGate.Release(); - } - } - private async Task ConnectAsync(McpServerName name, McpServerEntry entry, CancellationToken ct) { // Holds the client until ownership passes to _clients. If the connect @@ -425,15 +219,12 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, var tools = await client.ListToolsAsync(cancellationToken: ct); var sharedFunctions = CreateFunctionMap(tools); - var requiresSessionScopedClient = RequiresSessionScopedClient(name, entry); - LogToolDrift(name, tools); _toolRegistry.WithMcpTools(name.Value, tools, entry.GrantCategory, this, _maxToolDescriptionChars, _maxToolSchemaWarnChars, _logger); _sharedToolFunctions[name] = sharedFunctions; - _sessionScopedServers[name] = requiresSessionScopedClient; _clients[name] = client; client = null; _statuses[name] = new McpServerStatus(name, McpConnectionState.Connected, tools.Count, null); @@ -457,7 +248,6 @@ private async Task ConnectAsync(McpServerName name, McpServerEntry entry, } _sharedToolFunctions.TryRemove(name, out _); - _sessionScopedServers.TryRemove(name, out _); var hasCachedTokens = _oauthService.GetTokenSet(name) is not null; var hasOAuthRuntimeHints = HasOAuthRuntimeHints(name, entry); @@ -557,12 +347,10 @@ private IClientTransport CreateTransport(McpServerName serverName, McpServerEntr { if (entry.Transport is "stdio") { - var args = BuildStdioArguments(serverName, entry); - return new StdioClientTransport(new StdioClientTransportOptions { Command = entry.Command!, - Arguments = args, + Arguments = entry.Arguments ?? [], EnvironmentVariables = entry.EnvironmentVariables.ToRawNullableValues(StringComparer.OrdinalIgnoreCase), Name = serverName.Value, ShutdownTimeout = TimeSpan.FromSeconds(10), @@ -742,21 +530,6 @@ private static bool IsAuthFailure(Exception ex) return null; } - private static string[] BuildStdioArguments(McpServerName serverName, McpServerEntry entry) - { - var args = entry.Arguments is { Length: > 0 } - ? entry.Arguments.ToList() - : []; - - if (IsPlaywrightServer(serverName, entry) - && !args.Contains("--isolated", StringComparer.OrdinalIgnoreCase)) - { - args.Add("--isolated"); - } - - return [.. args]; - } - private static Dictionary CreateFunctionMap(IList tools) { var map = new Dictionary(StringComparer.OrdinalIgnoreCase); @@ -767,47 +540,6 @@ private static Dictionary CreateFunctionMap(IList IsPlaywrightServer(serverName, entry); - - private static bool IsPlaywrightServer(McpServerName serverName, McpServerEntry entry) - { - if (serverName.Value.Equals(PlaywrightServerName, StringComparison.OrdinalIgnoreCase)) - return true; - - if (!string.IsNullOrWhiteSpace(entry.Command) - && entry.Command.Contains("playwright", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - - if (entry.Arguments is not { Length: > 0 }) - return false; - - foreach (var arg in entry.Arguments) - { - if (arg.Contains("@playwright/mcp", StringComparison.OrdinalIgnoreCase) - || arg.Contains("playwright/mcp", StringComparison.OrdinalIgnoreCase) - || arg.Contains("playwright-mcp", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - } - - return false; - } - - private string ResolveScopeId(ToolExecutionContext? context) - { - if (!string.IsNullOrWhiteSpace(context?.SessionId)) - return context.SessionId!; - - return $"sessionless/{_timeProvider.GetUtcNow().ToUnixTimeMilliseconds()}-{Guid.NewGuid():N}"; - } - - private static string BuildScopedClientKey(string serverName, string scopeId) - => $"{serverName}::{scopeId}"; - /// /// Compares discovered tools against /// across all audience profiles and logs warnings for drift. @@ -865,75 +597,8 @@ public void Dispose() catch (Exception ex) { _logger.LogDebug(ex, "Error disposing MCP client during shutdown"); } } - foreach (var lazy in _scopedClients.Values) - { - if (!lazy.IsValueCreated) - continue; - - try - { - var task = lazy.Value; - if (!task.IsCompletedSuccessfully) - continue; - - task.Result.Dispose(); - } - catch (Exception ex) - { - _logger.LogDebug(ex, "Error disposing scoped MCP client during shutdown"); - } - } - _clients.Clear(); _sharedToolFunctions.Clear(); - _scopedClients.Clear(); - _sessionScopedServers.Clear(); - } - - private sealed class ScopedClientHandle : IAsyncDisposable, IDisposable - { - private int _disposed; - - public ScopedClientHandle( - McpClient client, - Dictionary tools, - DateTimeOffset createdAt) - { - Client = client; - Tools = tools; - Touch(createdAt); - } - - public McpClient Client { get; } - public Dictionary Tools { get; } - public SemaphoreSlim ExecutionGate { get; } = new(1, 1); - - private long _lastUsedAtMs; - - public long LastUsedAtMs => Volatile.Read(ref _lastUsedAtMs); - - public void Touch(DateTimeOffset now) - { - Volatile.Write(ref _lastUsedAtMs, now.ToUnixTimeMilliseconds()); - } - - public async ValueTask DisposeAsync() - { - if (Interlocked.Exchange(ref _disposed, 1) == 1) - return; - - ExecutionGate.Dispose(); - await Client.DisposeAsync(); - } - - public void Dispose() - { - if (Interlocked.Exchange(ref _disposed, 1) == 1) - return; - - ExecutionGate.Dispose(); - (Client as IDisposable)?.Dispose(); - } } } diff --git a/tests/Netclaw.SmokeMcpServer/Program.cs b/tests/Netclaw.SmokeMcpServer/Program.cs index bfb5f60d3..840e73e11 100644 --- a/tests/Netclaw.SmokeMcpServer/Program.cs +++ b/tests/Netclaw.SmokeMcpServer/Program.cs @@ -14,11 +14,12 @@ // // Two modes: // stdio (default) -// Exposes three fully-deterministic tools whose output is a pure +// Exposes deterministic tools whose output is a pure // function of their input: // add(a, b) -> a + b // echo(text) -> text // record-tasks(tasks, ref) -> a summary of the structured arguments +// process-info() -> process ID and command-line arguments // // Determinism is the whole point: add(2, 2) is always 4, so a smoke // scenario can hard-assert on the tool RESULT even though the @@ -86,6 +87,15 @@ public static string RecordTasks( return $"reference={reference} count={tasks.Length} kinds=[{kinds}]"; } + [McpServerTool(Name = "process-info")] + [Description("Returns this server process ID and command-line arguments for lifecycle tests.")] + public static string ProcessInfo() + => JsonSerializer.Serialize(new + { + processId = Environment.ProcessId, + arguments = Environment.GetCommandLineArgs().Skip(1).ToArray(), + }); + /// /// HTTP-mode-only tool: returns the Authorization header attached to /// the most recent request the server received. Returns the literal @@ -139,6 +149,7 @@ private static async Task RunStdioAsync() McpServerTool.Create(Add, new McpServerToolCreateOptions { Name = "add" }), McpServerTool.Create(Echo, new McpServerToolCreateOptions { Name = "echo" }), McpServerTool.Create(RecordTasks, new McpServerToolCreateOptions { Name = "record-tasks" }), + McpServerTool.Create(ProcessInfo, new McpServerToolCreateOptions { Name = "process-info" }), }; var options = new McpServerOptions diff --git a/tests/smoke/scenarios/mcp-setup.sh b/tests/smoke/scenarios/mcp-setup.sh index f74cd1764..5a5b16bd9 100755 --- a/tests/smoke/scenarios/mcp-setup.sh +++ b/tests/smoke/scenarios/mcp-setup.sh @@ -3,7 +3,7 @@ # connects to it and indexes its tools. # # The deterministic test server (Netclaw.SmokeMcpServer) exposes -# add/echo/record-tasks over stdio. This scenario hard-verifies netclaw's +# add/echo/record-tasks/process-info over stdio. This scenario hard-verifies netclaw's # MCP integration: # `mcp add` records the server in config, and on daemon startup the daemon # spawns the stdio server, completes the MCP handshake, and registers its @@ -87,12 +87,12 @@ else die "daemon log: no 'MCP server ${MCP_SERVER_NAME} connected' line — stdio handshake failed" fi -# The test server exposes exactly three tools (add, echo, record-tasks) — +# The test server exposes exactly four tools (add, echo, record-tasks, process-info) — # confirm the daemon registered all of them. -if [[ "$connect_line" == *"(3 tools)"* ]]; then - pass "daemon log: MCP server registered 3 tools (add, echo, record-tasks)" +if [[ "$connect_line" == *"(4 tools)"* ]]; then + pass "daemon log: MCP server registered 4 tools (add, echo, record-tasks, process-info)" else - die "daemon log: expected '(3 tools)' in the connection line, got: $connect_line" + die "daemon log: expected '(4 tools)' in the connection line, got: $connect_line" fi summarize diff --git a/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png index d8b8659d8..c3db20a85 100644 Binary files a/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png and b/tests/smoke/screenshots/mcp-permissions-tool-grid.approved.png differ diff --git a/tests/smoke/tapes/screenshots/mcp-permissions.tape b/tests/smoke/tapes/screenshots/mcp-permissions.tape index d7f883098..90e437fa0 100644 --- a/tests/smoke/tapes/screenshots/mcp-permissions.tape +++ b/tests/smoke/tapes/screenshots/mcp-permissions.tape @@ -7,10 +7,10 @@ # # Frames captured: # mcp-permissions-server-list — ServerList state: smoke-math appears as -# "Connected, 3 tools" below the header. +# "Connected, 4 tools" below the header. # mcp-permissions-tool-grid — ToolGrid state: header rows (Audience, # Server enabled, Server default) and all -# three tool rows (add, echo, record-tasks) +# four tool rows (add, echo, record-tasks, process-info) # simultaneously visible. This is the direct # regression check for issue #1424 — the # scroll container must not overwrite the header. @@ -37,7 +37,7 @@ Type "netclaw model set main smoke-llm qwen2:0.5b" Enter Wait+Screen@10s /TAPE\$/ -# Register the deterministic smoke MCP server (add, echo, record-tasks). +# Register the deterministic smoke MCP server (add, echo, record-tasks, process-info). # --grant-all means every tool is auto-approved for all audiences. Type "netclaw mcp add --transport stdio --grant-all smoke-math -- __NETCLAW_SMOKE_MCP_SERVER__" Enter @@ -48,12 +48,12 @@ Type "netclaw daemon start" Enter Wait+Screen@20s /TAPE\$/ -# Poll until the daemon reports smoke-math as fully connected with 3 tools. +# Poll until the daemon reports smoke-math as fully connected with 4 tools. # Replaces the former fixed Sleep 5s: we wait for the actual CLI signal # rather than guessing. `netclaw mcp list` queries the daemon's cached -# server state; "connected (3 tools)" means the stdio handshake and tool +# server state; "connected (4 tools)" means the stdio handshake and tool # indexing are complete. The Wait+Screen@60s is a safety net for a hard hang. -Type "until netclaw mcp list 2>/dev/null | grep -q 'connected (3 tools)'; do sleep 2; done" +Type "until netclaw mcp list 2>/dev/null | grep -q 'connected (4 tools)'; do sleep 2; done" Enter Wait+Screen@60s /TAPE\$/ Show @@ -63,26 +63,26 @@ Type "netclaw mcp permissions" Enter # ─── Frame 1: ServerList ───────────────────────────────────────────── -# smoke-math should appear as "Connected, 3 tools". Do NOT anchor on -# "smoke-math" or "3 tools" alone — both already sit in the shell +# smoke-math should appear as "Connected, 4 tools". Do NOT anchor on +# "smoke-math" or "4 tools" alone — both already sit in the shell # scrollback before the TUI ever paints: # - "smoke-math" appears in the setup output above ("Added MCP server # 'smoke-math' (stdio)" / "...adjust approvals for 'smoke-math'."). -# - "3 tools" appears in this tape's own typed readiness-loop command, -# still visible on screen: `... grep -q 'connected (3 tools)' ...`. +# - "4 tools" appears in this tape's own typed readiness-loop command, +# still visible on screen: `... grep -q 'connected (4 tools)' ...`. # Immediately after Enter, before the TUI switches to the alternate # screen buffer, Wait+Screen can match that leftover transcript text and # return instantly, capturing the raw shell instead of the rendered TUI # (README.md rule 5: anchor on the *next view*, not text that predates # it). Anchor on TUI-only chrome instead: "MCP Permissions" is the page # title from McpToolPermissionsPage.BuildHeader (proves the alt screen -# painted), and "Connected, 3 tools" is the exact rendered server-row +# painted), and "Connected, 4 tools" is the exact rendered server-row # text from McpToolPermissionsPage.BuildServerList — "{Name} ({Status}, # {ToolCount} tools)" with capital-C Status and a comma, which the -# transcript's lowercase, comma-less "connected (3 tools)" never +# transcript's lowercase, comma-less "connected (4 tools)" never # produces. Neither anchor occurs anywhere in the shell transcript. Wait+Screen@15s /MCP Permissions/ -Wait+Screen@5s /Connected, 3 tools/ +Wait+Screen@5s /Connected, 4 tools/ # Sleep 3s: let the server-list frame fully settle before capturing. The # first match of the anchors above can be a transient render; the # daemon may push a state update (re-index, status refresh) immediately @@ -98,7 +98,7 @@ Screenshot "/tmp/shot-mcp-permissions-server-list.png" # land on an empty server list (daemon cleared it mid-transition), causing # the TUI to navigate to a blank or non-existent tool grid. Wait+Screen@20s /smoke-math/ -Wait+Screen@10s /3 tools/ +Wait+Screen@10s /4 tools/ # Sleep 5s: extended settle guard (was 2s). Daemon MCP state updates can # arrive at any point; 5s provides substantially more headroom under CI # load where the re-index cycle can be slow. @@ -107,7 +107,7 @@ Enter # ─── Frame 2: ToolGrid ─────────────────────────────────────────────── # All header rows (Server, Audience, Server enabled, Server default) plus -# all tool rows (add, echo, record-tasks) must be visible simultaneously. +# all tool rows (add, echo, record-tasks, process-info) must be visible simultaneously. # If the #1424 regression reappears, tool rows will overwrite the header. # Timeout is 30s (was 15s) to give the tool grid more headroom to load # under CI load. /Server default:/ (with colon) matches the rendered