Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions docs/prd/PRD-006-mcp-tool-integration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down
13 changes: 13 additions & 0 deletions feeds/skills/.system/files/netclaw-operations/references/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-07-14
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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`.
40 changes: 40 additions & 0 deletions openspec/specs/netclaw-mcp/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
90 changes: 90 additions & 0 deletions src/Netclaw.Daemon.Tests/Mcp/McpProcessBoundStdioTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// -----------------------------------------------------------------------
// <copyright file="McpProcessBoundStdioTests.cs" company="Petabridge, LLC">
// Copyright (C) 2026 - 2026 Petabridge, LLC <https://petabridge.com>
// </copyright>
// -----------------------------------------------------------------------
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<string, McpServerEntry> { ["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<string, McpServerEntry>
{
["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<ProcessInfo> 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<ProcessInfo>(result, JsonOptions)!;
}

private static readonly JsonSerializerOptions JsonOptions = new()
{
PropertyNameCaseInsensitive = true,
};

private sealed record ProcessInfo(int ProcessId, string[] Arguments);
}
Loading
Loading