From 2ad8719a6e5c1ca9cd63756119a168a2fd88c1c3 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 17 Aug 2026 20:12:11 -0500 Subject: [PATCH 1/6] fix(mcp): expose new remote tools under the server default posture (#1959) A remote MCP server can add a tool after the operator sets custom per-tool rules. The frozen McpServerToolGrants allow-list hid the new tool. The operator expected the new tool to inherit the server default posture. Make the per-tool grant posture-aware. In All posture the grant list is additive: an unnamed tool passes the audience check and inherits the server default. Allowlist posture (Team/Public) keeps the closed allow-list and stays fail-closed for an unseen tool. Model a disabled tool as approval mode Deny, and hide a Deny MCP tool from the exposed tool list for every audience. The model never receives a tool the policy will block. The TUI checkbox and the `netclaw mcp tools` grant/revoke map disable to Deny in open posture; enable over a Deny server default writes an explicit Approval override so the toggle always takes effect. The daemon drift warning fires only for Allowlist posture; All-posture grant lists are additive and produce no drift. Resolve the audience profile one time for each tool-filter pass instead of one time for each tool. The exposure checks take the resolved profile, so a large tool catalog does not re-resolve the profile per tool. Update the netclaw-operations skill with the posture-aware permission model. --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/tools.md | 29 ++- .../fix-mcp-new-tool-exposure/.openspec.yaml | 2 + .../fix-mcp-new-tool-exposure/design.md | 113 +++++++++ .../fix-mcp-new-tool-exposure/proposal.md | 56 +++++ .../specs/netclaw-acl/spec.md | 89 +++++++ .../specs/tool-approval-gates/spec.md | 42 ++++ .../fix-mcp-new-tool-exposure/tasks.md | 109 +++++++++ .../Tools/McpToolAudienceGrantsTests.cs | 231 ++++++++++++++++++ src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 43 +++- .../Tools/ToolAudienceProfileResolver.cs | 38 ++- src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs | 150 ++++++++++++ .../Mcp/McpToolPermissionsViewModelTests.cs | 166 +++++++++++++ src/Netclaw.Cli/Mcp/McpCommand.cs | 106 +++++++- .../Mcp/McpToolPermissionsViewModel.cs | 61 +++++ .../Mcp/McpToolDriftTests.cs | 62 +++++ src/Netclaw.Daemon/Mcp/McpClientManager.cs | 81 ++++-- 17 files changed, 1329 insertions(+), 51 deletions(-) create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/design.md create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/proposal.md create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md create mode 100644 openspec/changes/fix-mcp-new-tool-exposure/tasks.md create mode 100644 src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 8667f09e6..713f37ab2 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.53.0" + version: "2.54.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index 51219963d..389f5d0d4 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -81,7 +81,9 @@ you cannot turn off the approval prompts at `mcp add` time. Inside the TUI (`netclaw mcp permissions`): -- `Enter` toggles the highlighted tool's grant +- `Enter` toggles the highlighted tool. In open (`All`) posture the toggle sets + `Deny` (disabled) or clears it (inherit the server default). In `Allowlist` + posture it adds or removes the tool from the grant list. - `A` toggles all tools on/off for the current audience - `E` enables/disables the whole server for the current audience - `M` cycles the **server default** approval mode (`Auto → Approval → Deny → Auto`) @@ -96,9 +98,22 @@ Approval-mode resolution precedence (for MCP tools): 3. Fail-closed fallback (Personal audience, shell/file-edit matcher family) 4. Audience `DefaultMode` -Newly discovered tools on an existing server automatically inherit the -server default; you do not need to re-run `permissions` after the server -learns a new tool. +A tool whose effective mode is `Deny` is **removed from the tool list the model +sees** — the model never receives a tool it cannot call. + +Per-tool grants are posture-aware: + +- **Open (`All`) posture** (Personal by default): the grant list is *additive*. + A newly discovered tool is exposed and inherits the server default. Disable a + specific tool by setting it to `Deny`. From the CLI: `netclaw mcp tools + --revoke --audience personal` writes the `Deny` override, and + `--grant ` clears it. +- **`Allowlist` posture** (Team/Public by default): the grant list is *closed*. + A tool stays hidden until it is granted, so a newly discovered tool is not + exposed until the operator adds it. + +You do not need to re-run `permissions` after a server learns a new tool; in +open posture it appears automatically under the server default. ### Migrating existing MCP servers @@ -106,6 +121,12 @@ Servers added to `netclaw.json` before this behavior shipped stay untouched — their tool grants, `ApprovalPolicy.McpServerDefaults`, and `ToolOverrides` entries are not rewritten during an upgrade. +A grant snapshot stored for an `All`-posture audience (for example a Personal +snapshot written by an older build) is now *inert*: the additive grant list no +longer restricts tools, so a newly discovered tool still appears. To disable a +specific tool, set a `Deny` override in `netclaw mcp permissions` (or +`netclaw mcp tools --revoke --audience `). + `netclaw doctor` will emit a warning for each enabled MCP server that Personal can reach (`McpServersMode = All`) but has no `ApprovalPolicy.McpServerDefaults[server]` entry and no `notion/*`-style diff --git a/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml b/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml new file mode 100644 index 000000000..95672402a --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-18 diff --git a/openspec/changes/fix-mcp-new-tool-exposure/design.md b/openspec/changes/fix-mcp-new-tool-exposure/design.md new file mode 100644 index 000000000..bb78197e6 --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/design.md @@ -0,0 +1,113 @@ +## Context + +An MCP tool has two independent access axes in Netclaw: + +- Approval mode (`ToolApprovalConfig`: `ToolOverrides` -> `McpServerDefaults` -> + `DefaultMode`). For an unseen tool this correctly falls through to the server + default. +- Enabled state (`ToolAudienceProfile.McpServerToolGrants`, a positive + allow-list). For an unseen tool this fails closed (hidden). + +`ToolAudienceProfileResolver.IsMcpToolAllowed` enforces the enabled axis. It +gates exposure (`ToolAccessPolicy.IsToolExposed`) and invocation +(`AuthorizeInvocationCore`, deny reason +`mcp_tool_not_allowed_for_audience_profile`). The two sibling checks +`IsMcpServerAllowed` and `IsToolAllowed` short-circuit to `true` in `All` +posture. `IsMcpToolAllowed` does not. It always treats a grant entry as a closed +allow-list. + +The MCP Permissions TUI seeds `McpServerToolGrants[server]` from a point-in-time +snapshot of the discovered tools. A later remote-added tool is absent from the +snapshot, so the resolver hides it. The daemon detects this drift but only logs +a warning. + +The allow-list cannot separate two states: "operator disabled this tool" and +"tool did not exist yet." Both states read as "absent from the list." The fix +must move the explicit-disable signal off the allow-list. + +## Goals / Non-Goals + +**Goals:** + +- A new MCP tool inherits the server default posture in open (`All`) posture. +- A disabled MCP tool leaves the tool list that the model sees. +- Team and Public audiences stay fail-closed for unseen tools. +- Reuse the existing `ToolApprovalMode.Deny`. Add no new config property. + +**Non-Goals:** + +- Change built-in (non-MCP) tool exposure logic. +- Rewrite persisted config files during migration. +- Reconstruct historical per-tool disable choices from a snapshot. + +## Decisions + +### Decision 1: `Deny` is the single "disabled" signal, and `Deny` hides the tool + +A tool with effective approval mode `Deny` leaves the exposed tool list. Today +`IsToolExposed` does not read the approval mode, so a `Deny` tool is shown and +then blocked at invocation. The MCP branch of `IsToolExposed` now resolves the +approval mode and returns `false` on `Deny`. + +Rationale: an LLM must not receive a tool that it cannot call. A hidden tool +prevents a wasted turn. This reuses the approval-mode resolver +(`GetApprovalMode` / `ResolveApprovalMode`) instead of a new code path. + +Alternative considered: a new `McpServerToolDenials` deny-list field. Rejected. +It duplicates state that `Deny` already carries and needs a schema change plus +plumbing. + +### Decision 2: `McpServerToolGrants` becomes posture-aware + +`IsMcpToolAllowed` returns `true` for a tool absent from the grant list when the +audience posture is `All`. It keeps the closed allow-list for `Allowlist` +posture. This matches the existing pattern in `IsMcpServerAllowed` and +`IsToolAllowed`. + +Rationale: `All` posture means "expose everything unless explicitly denied." +`Allowlist` posture means "expose only the listed items." The per-tool layer now +follows the same rule as the server and built-in-tool layers. + +Alternative considered: auto-reconcile new tools into the grant list at +discovery time. Rejected. The daemon would write operator config silently, and a +blind union would wrongly grant new tools to Team and Public. + +### Decision 3: TUI and CLI express disable as `Deny` in open posture + +In `All` posture the MCP Permissions checkbox toggles between `Deny` (disabled, +hidden) and a cleared override (server default). In `Allowlist` posture the +checkbox keeps writing the `McpServerToolGrants` allow-list. The `netclaw mcp` +grant/revoke/snapshot paths follow the same posture rule. + +Rationale: the persisted representation must match what the runtime consumes. A +disabled tool in open posture persists as a `Deny` override, which the resolver +reads back as hidden. + +## Risks / Trade-offs + +- [A `Deny` tool that operators expected to see-but-refuse now disappears] -> + This is the intended behavior and matches the issue. Document it in the + `netclaw-operations` skill and the change notes. +- [A pre-existing `All`-posture grant snapshot silently stops filtering] -> The + snapshot was almost always a full catalog, so no tool changes state in + practice. The TUI shows real tool state, and `tools.md` documents the inert + behavior. +- [An operator genuinely curated a Personal subset via the allow-list] -> The + feature is new/MVP. Such an operator re-expresses the subset with `Deny`. An + active warning, if wanted, belongs in `netclaw doctor` (operator-run, + idempotent), not a daemon log line. +- [Scope creep to built-in tools] -> Explicitly out of scope. The MCP branch of + `IsToolExposed` is the only exposure path that changes. + +## Migration Plan + +1. Deploy the resolver and exposure changes. No config schema field changes. +2. On tool discovery, when a profile in `All` posture carries a + `McpServerToolGrants[server]` entry, treat it as inert for the closed-list + decision. No config rewrite; the TUI and `tools.md` cover the behavior. +3. Rollback: revert the resolver and exposure changes. The persisted config is + unchanged, so a rollback restores the prior closed-list behavior. + +## Open Questions + +None. The model and scope are settled. diff --git a/openspec/changes/fix-mcp-new-tool-exposure/proposal.md b/openspec/changes/fix-mcp-new-tool-exposure/proposal.md new file mode 100644 index 000000000..878715193 --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/proposal.md @@ -0,0 +1,56 @@ +## Why + +A remote MCP server can add a new tool after the operator sets custom per-tool +rules. Today the new tool disappears from the agent (issue #1959). The operator +expects the new tool to inherit the server default posture: available under +`Approval`, or available and auto-approved under `Auto`. A new tool must never +become silently unavailable. + +## What Changes + +- In open (`All`) posture, `McpServerToolGrants` becomes an additive layer, not + a closed allow-list. A tool that is absent from the grant list passes the + audience check and inherits the server default approval posture. +- `Allowlist` posture (Team, Public) keeps the closed allow-list. An unseen tool + stays hidden. Least-trust audiences remain fail-closed. +- A tool with effective approval mode `Deny` is removed from the tool list that + the model sees. Today a `Deny` tool is shown and then blocked at invocation. + **BREAKING** for the exposed-tool surface: a `Deny` MCP tool is now hidden. +- The MCP Permissions TUI and the `netclaw mcp` CLI express "disable one tool" in + open posture as `Deny`, not as omission from the allow-list. +- The daemon drift warning fires only for `Allowlist` posture. Open posture has + no drift, because unseen tools are exposed by default. + +Scope: this change applies "Deny hides the tool" to MCP tools only. Built-in +tools keep their current exposure logic. + +## Capabilities + +### New Capabilities + + + +### Modified Capabilities + +- `netclaw-acl`: the per-tool `McpServerToolGrants` layer changes from an + always-closed allow-list to a posture-aware layer. `All` posture treats the + list as additive; `Allowlist` posture keeps it closed. +- `tool-approval-gates`: a tool with effective approval mode `Deny` is removed + from the exposed tool list, in addition to the existing invocation block. + +## Impact + +- `src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs` — + `IsMcpToolAllowed` becomes posture-aware. +- `src/Netclaw.Actors/Tools/ToolAccessPolicy.cs` — `IsToolExposed` hides a + `Deny` MCP tool. +- `src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs`, + `McpToolPermissionsPage.cs`, `McpCommand.cs` — checkbox and CLI map "disable" + to `Deny` in open posture. +- `src/Netclaw.Daemon/Mcp/McpClientManager.cs` — drift warning scoped to + `Allowlist` posture. +- Config migration: a pre-existing `All`-posture `McpServerToolGrants` snapshot + becomes inert. No config rewrite. The daemon logs a one-time notice. +- Security: Team and Public stay fail-closed. Personal gains correct new-tool + exposure under the server default posture. +- Skill `netclaw-operations` gains updated MCP permissions guidance. diff --git a/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md b/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md new file mode 100644 index 000000000..153e6e64c --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md @@ -0,0 +1,89 @@ +## MODIFIED Requirements + +### Requirement: Tool and data grants + +The system SHALL enforce explicit grants for tool and data access. Grants SHALL +be organized into specific tool grant categories: `shell`, `web_search`, +`web_fetch`, `github`, `mcp:{server_name}`, `config_write`, and +`schedule_write`. Each grant SHALL specify the allowed senders and channels to +which it applies. + +For MCP tools, the system SHALL support an additional per-tool grant layer via +`McpServerToolGrants` on each audience profile. This per-tool check SHALL +execute after the server-level `AllowedMcpServers` check. The per-tool check +SHALL be posture-aware and follow the audience `McpServersMode`. + +- When `McpServersMode` is `Allowlist`, the grant list SHALL be closed. Only a + tool that the list names SHALL pass the audience check. A tool that the list + does not name SHALL be denied. This keeps least-trust audiences fail-closed. +- When `McpServersMode` is `All`, the grant list SHALL be additive. A tool that + the list names SHALL pass. A tool that the list does not name SHALL also pass + and SHALL inherit the server default approval posture. + +Each `ToolAudienceProfile` SHALL support an optional `ApprovalPolicy` of type +`ToolApprovalConfig`. The `ApprovalPolicy` SHALL define a `DefaultMode` (Auto, +Approval, Deny) and per-tool overrides via `ToolOverrides`. The approval check +SHALL execute after the tool access grant check passes. Tools in Approval mode +SHALL surface approval context for the executor, and the executor SHALL consult +`IToolApprovalService` before execution. Tools in Deny mode SHALL be blocked +without an approval prompt. + +#### Scenario: Missing grant blocks tool call + +- **WHEN** a tool call is attempted without a matching grant +- **THEN** execution is denied with a policy reason code + +#### Scenario: Category-specific grant allows tool + +- **GIVEN** ACL grants `web_search` for sender `U12345` on channel `C99999` +- **WHEN** sender `U12345` requests a web search in channel `C99999` +- **THEN** ACL evaluation returns allow for the `web_search` tool category + +#### Scenario: MCP server-scoped grant + +- **GIVEN** ACL grants `mcp:memorizer` for sender `U12345` +- **WHEN** sender `U12345` requests an MCP tool from the `memorizer` server +- **THEN** ACL evaluation returns allow +- **AND** MCP tools from other servers without explicit grants are denied + +#### Scenario: MCP tool blocked by per-tool grant in allowlist posture + +- **GIVEN** the session's audience `McpServersMode` is `Allowlist` +- **AND** the audience allows `memorizer` server via `AllowedMcpServers` +- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` +- **WHEN** the agent invokes `memorizer/store` +- **THEN** the invocation is denied with reason `mcp_tool_not_allowed_for_audience_profile` + +#### Scenario: MCP tool allowed by per-tool grant in allowlist posture + +- **GIVEN** the session's audience `McpServersMode` is `Allowlist` +- **AND** the audience allows `memorizer` server via `AllowedMcpServers` +- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` +- **WHEN** the agent invokes `memorizer/search_memories` +- **THEN** the invocation is allowed + +#### Scenario: New MCP tool passes the per-tool check in open posture + +- **GIVEN** the session's audience `McpServersMode` is `All` +- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` +- **AND** the server adds a new tool `store` that the list does not name +- **WHEN** the agent invokes `memorizer/store` +- **THEN** the per-tool check passes +- **AND** the invocation inherits the server default approval posture + +#### Scenario: New MCP tool stays fail-closed in allowlist posture + +- **GIVEN** the session's audience `McpServersMode` is `Allowlist` +- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` +- **AND** the server adds a new tool `store` that the list does not name +- **WHEN** the agent invokes `memorizer/store` +- **THEN** the invocation is denied with reason `mcp_tool_not_allowed_for_audience_profile` + +#### Scenario: Tool granted but requires approval + +- **GIVEN** the session has a grant for `shell_execute` +- **AND** the Personal `ApprovalPolicy` sets `shell_execute` to Approval mode +- **AND** the command pattern `git push` is not already approved in `IToolApprovalService` +- **WHEN** the agent invokes `shell_execute` with `git push origin main` +- **THEN** the grant check passes +- **AND** the approval check returns `RequiresApproval` diff --git a/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md b/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md new file mode 100644 index 000000000..949d3e72f --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md @@ -0,0 +1,42 @@ +## ADDED Requirements + +### Requirement: MCP tool exposure honors Deny and server default + +The system SHALL remove an MCP tool from the exposed tool list when the tool +effective approval mode is `Deny` for the session audience. The model SHALL NOT +receive a tool that the policy will block. This exposure rule SHALL apply in +addition to the existing invocation block for `Deny` tools. + +An MCP tool that has no per-tool override SHALL inherit the server default +approval posture through the existing precedence (`ToolOverrides` -> +`McpServerDefaults` -> `DefaultMode`). A newly discovered MCP tool that the +operator never named SHALL therefore be exposed under the server default. It +SHALL be auto-approved when the server default is `Auto`. It SHALL be +approval-gated when the server default is `Approval`. + +This exposure rule SHALL apply to MCP tools only. Built-in tool exposure logic +SHALL NOT change. + +#### Scenario: MCP tool in Deny mode is removed from the exposed list + +- **GIVEN** an MCP tool whose effective approval mode is `Deny` for the session's audience +- **WHEN** the runtime builds the tool list for the model +- **THEN** the tool is absent from the exposed tool list +- **AND** the tool is absent from the discoverable tool list + +#### Scenario: New MCP tool is exposed under an Approval server default + +- **GIVEN** the session's audience `McpServersMode` is `All` +- **AND** the server default approval mode for `dropbox` is `Approval` +- **AND** the server adds a new tool `get_upload_url` that no override names +- **WHEN** the runtime builds the tool list for the model +- **THEN** `dropbox/get_upload_url` is present in the exposed tool list +- **AND** the effective approval mode for `dropbox/get_upload_url` is `Approval` + +#### Scenario: New MCP tool is exposed under an Auto server default + +- **GIVEN** the session's audience `McpServersMode` is `All` +- **AND** the server default approval mode for `dropbox` is `Auto` +- **AND** the server adds a new tool `get_upload_url` that no override names +- **WHEN** the agent invokes `dropbox/get_upload_url` +- **THEN** the tool executes immediately without an approval prompt diff --git a/openspec/changes/fix-mcp-new-tool-exposure/tasks.md b/openspec/changes/fix-mcp-new-tool-exposure/tasks.md new file mode 100644 index 000000000..4d717c961 --- /dev/null +++ b/openspec/changes/fix-mcp-new-tool-exposure/tasks.md @@ -0,0 +1,109 @@ +# Tasks: fix-mcp-new-tool-exposure + +## 1. Runtime ACL: posture-aware per-tool grant + +- [x] 1.1 Update `ToolAudienceProfileResolver.IsMcpToolAllowed` + (`src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs`): when the profile + `McpServersMode` is `All`, return `true` for a tool that the grant list does + not name. Keep the closed allow-list for `Allowlist` posture. +- [x] 1.2 Update the XML doc comment on `IsMcpToolAllowed` to state the + posture-aware behavior. +- [x] 1.3 Add resolver unit tests: `All` posture passes an unnamed tool; + `Allowlist` posture denies an unnamed tool; both postures pass a named tool. + +## 2. Runtime exposure: Deny hides the MCP tool + +- [x] 2.1 In `ToolAccessPolicy.IsToolExposed` MCP branch + (`src/Netclaw.Actors/Tools/ToolAccessPolicy.cs`), after the server and + per-tool ACL checks pass, resolve the MCP tool effective approval mode via the + audience `ApprovalPolicy` precedence (`ToolOverrides` -> `McpServerDefaults` -> + `DefaultMode`) and return `false` when the mode is `Deny`. +- [x] 2.2 Reuse the existing approval-mode resolver + (`ResolveApprovalMode` / `McpApprovalMatcher`). Add a small helper only if the + exposure path cannot supply invocation arguments; the MCP mode resolution does + not depend on arguments. +- [x] 2.3 Keep the change scoped to MCP tools. Do not alter built-in tool + exposure. +- [x] 2.4 Add exposure unit tests: a `Deny` MCP tool is absent from + `FilterExposedTools` and `FilterDiscoverableTools`; an `Auto`/`Approval` MCP + tool stays present. + +## 3. Regression: new tool inherits server default + +- [x] 3.1 Add a regression test for issue #1959: `All` posture, a grant snapshot + that does not name a newly discovered tool. Assert the new tool is exposed. +- [x] 3.2 Assert the new tool effective mode equals the server default. Cover + both `McpServerDefaults` = `Approval` and `McpServerDefaults` = `Auto`. +- [x] 3.3 Add a fail-closed guard test: `Allowlist` posture (Team/Public) keeps a + newly discovered tool hidden. + +## 4. TUI: MCP Permissions page + +- [x] 4.1 In `McpToolPermissionsViewModel` + (`src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs`), for `All` posture, map + the tool checkbox to the approval override: unchecked writes `Deny`, checked + clears the override (server default). Stop seeding a full snapshot allow-list + in `All` posture. For `Allowlist` posture, keep writing `McpServerToolGrants`. +- [x] 4.2 Update `IsToolGranted`/`GetEffectiveMode` so a row reflects `Deny` as + disabled in `All` posture and reflects allow-list membership in `Allowlist` + posture. +- [x] 4.3 Update row rendering in `McpToolPermissionsPage` + (`src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs`) so a disabled row (Deny or + un-granted) is greyed and the `(def)`/`(override)` badge stays correct. +- [x] 4.4 Add headless ViewModel tests: uncheck in `All` posture persists `Deny`; + check clears the override; `Allowlist` posture still writes the allow-list. + +## 5. CLI: `netclaw mcp` + +- [x] 5.1 In `McpCommand` (`src/Netclaw.Cli/Mcp/McpCommand.cs`), make + grant/revoke/`--snapshot` posture-aware. In `All` posture, "revoke" writes + `Deny`; in `Allowlist` posture, "revoke" removes from the allow-list. +- [x] 5.2 Add CLI tests for the posture-aware grant/revoke behavior. + +## 6. Daemon: drift warning + +- [x] 6.1 In `McpClientManager.LogToolDrift` + (`src/Netclaw.Daemon/Mcp/McpClientManager.cs`), fire the ungranted/stale + drift warning only for `Allowlist` posture audiences. `All`-posture grant + lists are additive, so they produce no drift. (An inert-snapshot log notice + was implemented then removed in review: it was low-value operator noise, and + the daemon restart on a config change already resets any per-server state. + An active warning, if wanted, belongs in `netclaw doctor`.) +- [x] 6.2 Add a daemon test (`McpToolDriftTests`) that asserts an `All`-posture + grant snapshot produces no drift and an `Allowlist` grant list still reports + ungranted/stale tools. + +## 7. Cross-boundary contract test + +- [x] 7.1 Add a producer/consumer test: the TUI ViewModel writes `Deny` on + uncheck; a fresh `ToolAccessPolicy`/resolver built from the written config + hides the tool. Prove the persisted representation matches the runtime consumer. + +## 8. Migration / round-trip + +- [x] 8.1 Add a load/round-trip test: a config with an `All`-posture full-snapshot + `McpServerToolGrants` plus a new discovered tool exposes the new tool. + +## 9. Skill, docs, quality gates + +- [x] 9.1 Update the `netclaw-operations` skill + (`feeds/skills/.system/files/netclaw-operations/SKILL.md`) with the MCP + permissions model: `Deny` hides a tool; new tools inherit the server default in + open posture; Team/Public stay fail-closed. Bump `metadata.version`. +- [x] 9.2 Run `dotnet slopwatch analyze` and fix any new violation. +- [x] 9.3 Run `./scripts/Add-FileHeaders.ps1 -Verify` for copyright headers. +- [ ] 9.4 Run `./scripts/smoke/run-smoke.sh` for the MCP Permissions page if the + prompt flow changed. **Deferred to CI:** the local environment has no `vhs` + and the harness needs `sudo` to install Ollama. The `mcp-permissions` tape only + smokes page-open/no-daemon/exit; it never reaches the tool grid, so it does not + exercise the changed toggle logic. Headless ViewModel tests cover that behavior. +- [ ] 9.5 Run the eval suite if tool exposure/grant-category behavior needs a new + case. **Deferred to CI:** the eval runner needs an external provider endpoint + (`NETCLAW_EVAL_PROVIDER_ENDPOINT`) unavailable here. The change is a runtime ACL + fix and additive skill documentation; it adds no tool, schema, or grant category, + so no new eval case is warranted. Runtime/integration tests cover the behavior. + +## 10. OpenSpec finish + +- [ ] 10.1 Run `/opsx-verify` then `/opsx-sync` and `/opsx-archive` after the + implementation lands and gates pass. diff --git a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs index 0feff4548..810a30790 100644 --- a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs @@ -377,8 +377,239 @@ public void McpServerToolGrants_DeserializesFromJson() Assert.False(policy.IsToolExposed(CreateMcpTool("memorizer", "store"), TeamContext())); } + // ── Posture-aware per-tool grants (issue #1959) ── + + [Fact] + public void AllPosture_ToolAbsentFromGrantSnapshot_IsExposed() + { + // Regression for #1959. Personal is All posture. A grant snapshot names + // the tools that existed when the operator configured the server. A tool + // the server adds later is absent from the snapshot, but the additive + // grant layer must still expose it. + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy", "delete", "move"] + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), PersonalContext())); + // A tool the snapshot names stays exposed too. + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); + } + + [Fact] + public void AllowlistPosture_ToolAbsentFromGrantSnapshot_StaysHidden() + { + // Team is Allowlist posture. The grant list stays closed so least-trust + // audiences remain fail-closed for a newly discovered tool. + var config = CreateConfigWithTeamServer("dropbox"); + config.AudienceProfiles.Team.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy", "delete", "move"] + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), TeamContext())); + } + + [Fact] + public void AllPosture_NewTool_InheritsApprovalServerDefault() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy"] + }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + McpServerDefaults = new Dictionary(StringComparer.Ordinal) + { + ["dropbox"] = ToolApprovalMode.Approval + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + var newTool = CreateMcpTool("dropbox", "get_upload_url"); + + // Approval is not Deny, so the tool is exposed ... + Assert.True(policy.IsToolExposed(newTool, PersonalContext())); + // ... and the new tool inherits the server default at invocation. + var decision = policy.AuthorizeInvocation(newTool, CreateExecutionContext(TrustAudience.Personal)); + Assert.True(decision.NeedsApproval); + } + + [Fact] + public void AllPosture_NewTool_InheritsAutoServerDefault() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy"] + }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + McpServerDefaults = new Dictionary(StringComparer.Ordinal) + { + ["dropbox"] = ToolApprovalMode.Auto + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + var newTool = CreateMcpTool("dropbox", "get_upload_url"); + + Assert.True(policy.IsToolExposed(newTool, PersonalContext())); + var decision = policy.AuthorizeInvocation(newTool, CreateExecutionContext(TrustAudience.Personal)); + Assert.True(decision.Allowed); + Assert.False(decision.NeedsApproval); + } + + // ── Deny hides an MCP tool from exposure ── + + [Fact] + public void AllPosture_DenyTool_HiddenFromExposure() + { + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["dropbox/delete"] = ToolApprovalMode.Deny + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + // The Deny tool is removed from the list the model sees. + Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "delete"), PersonalContext())); + // A sibling tool with no Deny stays exposed. + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); + } + + [Fact] + public void AllPosture_DenyTool_StillDeniedAtInvocation() + { + // The exposure hide is in addition to the existing invocation block. + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["dropbox/delete"] = ToolApprovalMode.Deny + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + var decision = policy.AuthorizeInvocation( + CreateMcpTool("dropbox", "delete"), + CreateExecutionContext(TrustAudience.Personal)); + + Assert.False(decision.Allowed); + Assert.Equal("tool_denied_by_approval_policy", decision.DenyReason); + } + + [Fact] + public void FilterExposedTools_RemovesDenyTool_KeepsNewTool() + { + var registry = new ToolRegistry(); + var copyTool = CreateMcpTool("dropbox", "copy"); + var deleteTool = CreateMcpTool("dropbox", "delete"); + var newTool = CreateMcpTool("dropbox", "get_upload_url"); + registry.Register(copyTool); + registry.Register(deleteTool); + registry.Register(newTool); + + var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; + config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy", "delete"] + }; + config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["dropbox/delete"] = ToolApprovalMode.Deny + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + var aiTools = new[] { copyTool.ToAITool(), deleteTool.ToAITool(), newTool.ToAITool() }; + var filtered = policy.FilterExposedTools(aiTools, registry, PersonalTrustContext()); + + var names = filtered.Select(t => ((AIFunction)t).Name).ToList(); + Assert.Contains("dropbox__copy", names); // granted, not denied + Assert.Contains("dropbox__get_upload_url", names); // new tool, additive in All posture + Assert.DoesNotContain("dropbox__delete", names); // Deny → hidden + } + + [Fact] + public void AllowlistPosture_GrantedTool_WithDenyOverride_IsHidden() + { + // Deny hides a tool for EVERY audience, not only All posture: a tool that + // is granted (passes the allow-list) but carries a Deny override is + // removed from the exposed list rather than shown-then-refused. + var config = CreateConfigWithTeamServer("dropbox"); + config.AudienceProfiles.Team.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy", "delete"] + }; + config.AudienceProfiles.Team.ApprovalPolicy = new ToolApprovalConfig + { + ToolOverrides = new Dictionary(StringComparer.Ordinal) + { + ["dropbox/delete"] = ToolApprovalMode.Deny + } + }; + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + + Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "delete"), TeamContext())); // granted but Deny → hidden + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), TeamContext())); // granted, no Deny + } + + // ── Migration: an All-posture snapshot is additive after load ── + + [Fact] + public void AllPosture_GrantSnapshot_DeserializesAndExposesNewTool() + { + // A config written before the server added a tool carries a full snapshot + // under Personal (All posture). After load the snapshot is additive: the + // new tool is exposed rather than treated as disabled. + var json = """ + { + "ShellMode": "HostAllowed", + "AudienceProfiles": { + "Personal": { + "McpServersMode": "All", + "McpServerToolGrants": { + "dropbox": ["copy", "delete", "move"] + } + } + } + } + """; + + var config = System.Text.Json.JsonSerializer.Deserialize(json, + new System.Text.Json.JsonSerializerOptions + { + Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() } + }); + + Assert.NotNull(config); + var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), PersonalContext())); + Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); + } + // ── Helpers ── + private static EffectiveTrustContext PersonalTrustContext() => new( + DeploymentPosture.Personal, + TrustAudience.Personal, + TrustAudience.Personal, + TrustAudience.Personal, + TrustBoundary.TrustedInstance, + PrincipalClassification.TrustedInternal, + TransportAuthenticity.Verified, + PayloadTaint.Trusted, + null, null, false, false, null); + private static McpToolAdapter CreateMcpTool(string serverName, string toolName, string? description = null) { var func = AIFunctionFactory.Create(() => "result", toolName, description ?? toolName); diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index ed224291d..0b8098448 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -108,7 +108,11 @@ public IReadOnlyList FilterExposedTools( IEnumerable tools, ToolRegistry registry, EffectiveTrustContext? trustContext) - => tools + { + // Resolve the audience profile ONCE for the whole pass, not per tool. + var audience = ResolveAudience(trustContext); + var profile = _profileResolver.ResolveProfile(audience); + return tools .Where(tool => { var name = GetToolName(tool); @@ -116,14 +120,20 @@ public IReadOnlyList FilterExposedTools( return true; var registration = registry.GetRegistrationByToolName(name); - return registration is null || IsToolExposed(registration, trustContext); + return registration is null || IsToolExposed(registration.Tool, profile, audience); }) .ToList(); + } public IReadOnlyList FilterDiscoverableTools( IEnumerable tools, ToolInvocationContext context) - => tools.Where(tool => IsToolExposed(tool, context)).ToList(); + { + // Resolve the audience profile ONCE for the whole pass, not per tool. + var audience = ResolveAudience(context); + var profile = _profileResolver.ResolveProfile(audience); + return tools.Where(tool => IsToolExposed(tool, profile, audience)).ToList(); + } public bool IsToolExposed(ToolRegistration registration, EffectiveTrustContext? trustContext) => IsToolExposed(registration.Tool, ResolveAudience(trustContext)); @@ -135,16 +145,37 @@ public bool IsMcpServerExposed(McpServerName serverName, TrustAudience audience) => _profileResolver.IsMcpServerAllowed(serverName, audience); internal bool IsToolExposed(INetclawTool tool, TrustAudience audience) + => IsToolExposed(tool, _profileResolver.ResolveProfile(audience), audience); + + // Core exposure decision. Takes the already-resolved audience profile so a + // filter pass over many tools resolves the profile once and reuses it for + // every per-tool ACL and approval check. + internal bool IsToolExposed(INetclawTool tool, ToolAudienceProfile profile, TrustAudience audience) { // Feature-disabled tools are hidden for ALL audiences if (IsFeatureDisabledTool(tool.Name)) return false; if (tool is McpToolAdapter mcp) - return _profileResolver.IsMcpServerAllowed(new McpServerName(mcp.ServerName), audience) - && _profileResolver.IsMcpToolAllowed(new McpServerName(mcp.ServerName), new ToolName(mcp.BareToolName), audience); + { + var mcpServer = new McpServerName(mcp.ServerName); + if (!_profileResolver.IsMcpServerAllowed(mcpServer, profile) + || !_profileResolver.IsMcpToolAllowed(mcpServer, new ToolName(mcp.BareToolName), profile)) + return false; + + // A tool whose effective approval mode is Deny is "disabled": remove + // it from the exposed list so the model never receives a tool the + // policy will always block. This applies to EVERY audience, not only + // All posture — a granted-but-Deny tool in an Allowlist audience is + // hidden too, since showing a tool that always fails helps no one. + // Reuses the shared approval precedence (ToolOverrides → + // McpServerDefaults → DefaultMode) so exposure and invocation cannot + // drift. A newly discovered tool has no Deny entry, so it stays + // exposed under the server default posture. + return profile.ApprovalPolicy?.GetEffectiveMode(mcp.Name) != ToolApprovalMode.Deny; + } - if (!_profileResolver.IsToolAllowed(new ToolName(tool.Name), audience)) + if (!_profileResolver.IsToolAllowed(new ToolName(tool.Name), profile)) return false; if (IsShellCoupledTool(tool)) diff --git a/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs b/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs index 7d8982423..f18c885bd 100644 --- a/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs +++ b/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs @@ -77,12 +77,18 @@ public bool IsToolAllowed(ToolName toolName, ToolInvocationContext context) => IsToolAllowed(toolName, context.Audience); public bool IsToolAllowed(ToolName toolName, TrustAudience audience) + => IsToolAllowed(toolName, ResolveProfile(audience)); + + /// + /// Profile-accepting overload. Callers that filter many tools for one + /// audience resolve the profile once and pass it in, so a filter pass does + /// not re-resolve the profile per tool. + /// + public bool IsToolAllowed(ToolName toolName, ToolAudienceProfile profile) { if (!IsProfileManagedTool(toolName)) return true; - var profile = ResolveProfile(audience); - if (profile.ToolsMode == ToolProfileMode.All) return true; @@ -103,10 +109,16 @@ public bool IsMcpServerAllowed(McpServerName serverName, TrustAudience audience) /// /// Checks whether a specific tool from an MCP server is allowed for the given audience. - /// Returns true if: - /// - The profile has no (null), or - /// - The server has no entry in the grants dictionary, or - /// - The tool name appears in the server's grant list. + /// The per-tool grant list is posture-aware: + /// - No (null) → all tools pass. + /// - The server has no entry in the grants dictionary → all tools pass. + /// - The tool name appears in the server's grant list → passes. + /// - The tool name is absent and the audience + /// is → passes. The grant list is additive, so a tool the + /// server added after the operator wrote the list inherits the server default posture. + /// - The tool name is absent and the audience + /// is → denied. The closed allow-list keeps + /// least-trust audiences fail-closed. /// public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, TrustAudience audience) { @@ -164,7 +176,7 @@ public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolIn private static TrustAudience ResolveAudience(ToolInvocationContext context) => context.Audience; - private static bool IsMcpServerAllowed(McpServerName serverName, ToolAudienceProfile profile) + public bool IsMcpServerAllowed(McpServerName serverName, ToolAudienceProfile profile) { if (profile.McpServersMode == ToolProfileMode.All) return true; @@ -172,7 +184,7 @@ private static bool IsMcpServerAllowed(McpServerName serverName, ToolAudiencePro return profile.AllowedMcpServers.Contains(serverName.Value, StringComparer.OrdinalIgnoreCase); } - private static bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) + public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) { if (profile.McpServerToolGrants is not { } grants) return true; @@ -180,7 +192,15 @@ private static bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName if (!grants.TryGetValue(serverName.Value, out var allowedTools)) return true; - return allowedTools.Contains(toolName.Value, StringComparer.Ordinal); + if (allowedTools.Contains(toolName.Value, StringComparer.Ordinal)) + return true; + + // The tool is not named in the grant list. In All posture the grant list + // is additive, not a closed allow-list: an unnamed tool (for example one + // the server added after the operator wrote the list) still passes and + // inherits the server default approval posture. Allowlist posture stays + // closed so least-trust audiences remain fail-closed. + return profile.McpServersMode == ToolProfileMode.All; } private static bool IsProfileManagedTool(ToolName toolName) diff --git a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs index 5869db920..be511f9ca 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs @@ -663,6 +663,156 @@ public async Task ReadMcpError_MalformedBodyFallsBackToHttpStatusAndReason() Assert.Equal("HTTP 502 Bad Gateway", message); } + // ── Posture-aware `mcp tools` grant/revoke/snapshot (issue #1959) ── + + [Fact] + public async Task Tools_Revoke_AllPosture_WritesDenyOverrideNotGrantAllowlist() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--revoke", "delete", "--audience", "personal"], + _paths, daemonApi, _output); + + Assert.Equal(0, exitCode); + using var doc = ReadConfigFile(_paths.NetclawConfigPath); + var personal = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles").GetProperty("Personal"); + Assert.Equal( + "Deny", + personal.GetProperty("ApprovalPolicy").GetProperty("ToolOverrides").GetProperty("dropbox/delete").GetString()); + // In All posture the CLI must NOT write a closed allow-list. + Assert.False(personal.TryGetProperty("McpServerToolGrants", out _)); + } + + [Fact] + public async Task Tools_Grant_AllPosture_ClearsDenyOverride() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "ToolOverrides": { "dropbox/delete": "Deny" } } + } } } + } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--grant", "delete", "--audience", "personal"], + _paths, daemonApi, _output); + + Assert.Equal(0, exitCode); + using var doc = ReadConfigFile(_paths.NetclawConfigPath); + var overrides = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") + .GetProperty("Personal").GetProperty("ApprovalPolicy").GetProperty("ToolOverrides"); + Assert.False(overrides.TryGetProperty("dropbox/delete", out _)); + } + + [Fact] + public async Task Tools_Revoke_AllowlistPosture_WritesGrantAllowlist() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Team": { + "McpServersMode": "Allowlist", + "AllowedMcpServers": ["dropbox"] + } } } + } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--revoke", "delete", "--audience", "team"], + _paths, daemonApi, _output); + + Assert.Equal(0, exitCode); + using var doc = ReadConfigFile(_paths.NetclawConfigPath); + var grants = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") + .GetProperty("Team").GetProperty("McpServerToolGrants").GetProperty("dropbox") + .EnumerateArray().Select(static e => e.GetString()).ToList(); + Assert.Contains("copy", grants); + Assert.DoesNotContain("delete", grants); + } + + [Fact] + public async Task Tools_Snapshot_AllPosture_IsRejected() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--snapshot", "--audience", "personal"], + _paths, daemonApi, _output); + + Assert.Equal(1, exitCode); + Assert.Contains("All posture", _output.ToString(), StringComparison.Ordinal); + } + + [Fact] + public async Task Tools_Grant_AllPosture_OverServerDefaultDeny_WritesApprovalOverride() + { + // Server default Deny hides every tool. `--grant` must actually enable the + // tool; clearing the override would inherit Deny, so it writes Approval. + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "McpServerDefaults": { "dropbox": "Deny" } } + } } } + } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--grant", "copy", "--audience", "personal"], + _paths, daemonApi, _output); + + Assert.Equal(0, exitCode); + using var doc = ReadConfigFile(_paths.NetclawConfigPath); + var overrides = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") + .GetProperty("Personal").GetProperty("ApprovalPolicy").GetProperty("ToolOverrides"); + Assert.Equal("Approval", overrides.GetProperty("dropbox/copy").GetString()); + } + + [Fact] + public async Task Tools_Snapshot_NoAudience_AllPostureOnly_GivesAdditiveGuidance() + { + // The server is reachable only by Personal (All posture). A no-audience + // snapshot must not falsely report "not allowed by any audience profile". + File.WriteAllText(_paths.NetclawConfigPath, """ + { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--snapshot"], + _paths, daemonApi, _output); + + Assert.Equal(1, exitCode); + var output = _output.ToString(); + Assert.Contains("All posture", output, StringComparison.Ordinal); + Assert.DoesNotContain("not allowed by any audience profile", output, StringComparison.Ordinal); + } + + private static DaemonApi ToolsDaemonApi(string serverName, params string[] tools) + { + var body = JsonSerializer.Serialize(tools); + return CreateDaemonApi(request => request.RequestUri!.AbsolutePath == $"/api/mcp/tools/{serverName}" + ? new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(body, Encoding.UTF8, "application/json"), + } + : new HttpResponseMessage(HttpStatusCode.NotFound)); + } + private static JsonDocument ReadConfigFile(string path) { return JsonDocument.Parse(File.ReadAllText(path)); diff --git a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs index 318962e0a..98e74aebb 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs @@ -5,10 +5,13 @@ // ----------------------------------------------------------------------- using System.Net.Http; using System.Text.Json; +using Microsoft.Extensions.AI; using Microsoft.Extensions.Configuration; +using Netclaw.Actors.Tools; using Netclaw.Cli.Daemon; using Netclaw.Cli.Mcp; using Netclaw.Configuration; +using Netclaw.Security; using Netclaw.Tests.Utilities; using Netclaw.Tools; using Xunit; @@ -409,6 +412,169 @@ public void Save_DoesNotMutateTheLiveInMemoryProfile() Assert.Equal("Allowlist", GetAudienceProfile(doc, "Personal").GetProperty("McpServersMode").GetString()); } + [Fact] + public void ToggleTool_AllPosture_UncheckPersistsDenyOverrideNotGrantAllowlist() + { + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy", "delete" }); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + // Personal is All posture: every tool starts enabled. + Assert.True(vm.IsToolGranted(new ToolName("delete"))); + + // Unchecking disables the tool by writing a Deny override. + vm.ToggleTool(new ToolName("delete")); + Assert.False(vm.IsToolGranted(new ToolName("delete"))); + Assert.Equal(ToolApprovalMode.Deny, vm.GetEffectiveMode(new ToolName("delete")).Mode); + + Assert.True(vm.Save()); + + var personal = GetAudienceProfile(JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)), "Personal"); + Assert.Equal( + "Deny", + personal.GetProperty("ApprovalPolicy").GetProperty("ToolOverrides").GetProperty("dropbox/delete").GetString()); + // The checkbox must NOT persist a closed McpServerToolGrants allow-list in All posture. + Assert.False(personal.TryGetProperty("McpServerToolGrants", out _)); + } + + [Fact] + public void ToggleTool_AllPosture_ReCheckClearsDenyOverride() + { + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + vm.ToggleTool(new ToolName("copy")); // disable → Deny + Assert.False(vm.IsToolGranted(new ToolName("copy"))); + vm.ToggleTool(new ToolName("copy")); // re-enable → inherit server default + Assert.True(vm.IsToolGranted(new ToolName("copy"))); + + Assert.True(vm.Save()); + + var personal = GetAudienceProfile(JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)), "Personal"); + // Re-enabling clears the override, so no per-tool entry is persisted. + Assert.False( + personal.TryGetProperty("ApprovalPolicy", out var ap) + && ap.TryGetProperty("ToolOverrides", out var overrides) + && overrides.TryGetProperty("dropbox/copy", out _)); + } + + [Fact] + public void ToggleTool_AllPosture_EnablesOverServerDefaultDeny() + { + // When the server default is Deny (all tools hidden by default), the + // checkbox must still be able to turn a single tool on. Clearing the + // override would inherit Deny, so enable writes an explicit Approval. + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "McpServerDefaults": { "dropbox": "Deny" } } + } } } + } + """); + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + // Server default Deny → the tool starts disabled (unchecked). + Assert.False(vm.IsToolGranted(new ToolName("copy"))); + + // Enabling must actually take effect (not a dead toggle). + vm.ToggleTool(new ToolName("copy")); + Assert.True(vm.IsToolGranted(new ToolName("copy"))); + Assert.Equal(ToolApprovalMode.Approval, vm.GetEffectiveMode(new ToolName("copy")).Mode); + } + + [Fact] + public void ToggleTool_AllowlistPosture_StillWritesGrantAllowlist() + { + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("notion"), new[] { "create-pages", "search" }); + vm.SetSelectedAudienceForTests(TrustAudience.Team); + + // Team is Allowlist posture. Enable the server, then uncheck one tool. + vm.ToggleServerAccess(); + vm.ToggleTool(new ToolName("search")); + Assert.True(vm.IsToolGranted(new ToolName("create-pages"))); + Assert.False(vm.IsToolGranted(new ToolName("search"))); + + Assert.True(vm.Save()); + + var team = GetAudienceProfile(JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)), "Team"); + var grants = team.GetProperty("McpServerToolGrants").GetProperty("notion") + .EnumerateArray().Select(static e => e.GetString()).ToList(); + Assert.Contains("create-pages", grants); + Assert.DoesNotContain("search", grants); + } + + [Fact] + public void ToggleTool_AllPosture_Deny_HidesToolFromRuntimePolicy() + { + // Cross-boundary contract: the TUI (producer) writes a Deny override; a + // runtime ToolAccessPolicy (consumer) built from that written config hides + // the tool. Proves the persisted representation matches the consumer. + // Seed a realistic Personal profile (All posture, as init writes it) so + // the reloaded config keeps the open posture. + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { + "AudienceProfiles": { + "Personal": { "McpServersMode": "All" } + } + } + } + """); + + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy", "delete" }); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + vm.ToggleTool(new ToolName("delete")); // disable via Deny + Assert.True(vm.Save()); + + var toolConfig = LoadToolConfigFromDisk(); + var policy = new ToolAccessPolicy( + toolConfig, + new EffectivePolicyDefaults( + DeploymentPosture.Personal, + TrustAudience.Personal, + ShellExecutionMode.HostAllowed, + UsedStrictFallback: false), + new ShellCommandPolicy(), + new ToolPathPolicy([])); + + var personalContext = new ToolInvocationContext( + new ToolRunScope + { + Session = new ToolSessionScope.Bound("slack/thread-1", null), + Audience = TrustAudience.Personal, + InlineOutputBudget = InlineOutputBudget.Default, + InteractiveApproval = new InteractiveApprovalCapability.Unavailable(), + }, + ToolExecutionTimeout.Default); + + Assert.False(policy.IsToolExposed(McpTool("dropbox", "delete"), personalContext)); // Deny → hidden + Assert.True(policy.IsToolExposed(McpTool("dropbox", "copy"), personalContext)); // enabled + } + + private static McpToolAdapter McpTool(string serverName, string toolName) + => new(AIFunctionFactory.Create(() => "result", toolName, toolName), serverName, toolName); + + private ToolConfig LoadToolConfigFromDisk() + { + using var doc = JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)); + var tools = doc.RootElement.GetProperty("Tools"); + return JsonSerializer.Deserialize( + tools.GetRawText(), + new JsonSerializerOptions + { + Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() } + })!; + } + private static void CycleServerDefault(McpToolPermissionsViewModel vm, bool reverse) { if (reverse) diff --git a/src/Netclaw.Cli/Mcp/McpCommand.cs b/src/Netclaw.Cli/Mcp/McpCommand.cs index cec11ded0..2705ae108 100644 --- a/src/Netclaw.Cli/Mcp/McpCommand.cs +++ b/src/Netclaw.Cli/Mcp/McpCommand.cs @@ -1209,6 +1209,18 @@ private static int RunToolsList( private static string FormatGrantStatus(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) { + // In All posture the grant list is additive: a tool is exposed unless its + // effective approval mode is Deny (the disable signal). This mirrors the + // runtime resolver and exposure gate. + if (profile.McpServersMode == ToolProfileMode.All) + { + var mode = profile.ApprovalPolicy?.GetEffectiveMode($"{serverName.Value}/{toolName.Value}") + ?? ToolApprovalMode.Auto; + return mode == ToolApprovalMode.Deny + ? "- " // disabled via Deny + : "✱ "; // exposed (additive) + } + if (profile.McpServerToolGrants is null) return "\u2731 "; // ✱ = all (no grants configured) @@ -1237,6 +1249,7 @@ private static int RunToolsSnapshot( }; var updated = 0; + var skippedAllPosture = false; foreach (var audienceName in audienceNames) { var profile = audienceName switch @@ -1246,8 +1259,24 @@ private static int RunToolsSnapshot( _ => profiles.Personal }; - var serverAllowed = profile.McpServersMode == ToolProfileMode.All - || profile.AllowedMcpServers.Contains(serverName.Value, StringComparer.OrdinalIgnoreCase); + // A snapshot pins the current tool set as a closed allow-list. That + // only makes sense in Allowlist posture. In All posture the grant + // list is additive, so a snapshot would not restrict anything; point + // the operator at --revoke, which disables a tool via a Deny override. + if (profile.McpServersMode == ToolProfileMode.All) + { + if (targetAudience is not null) + { + writer.WriteLine($"The {audienceName} audience exposes MCP servers in All posture; a snapshot is additive and would not restrict tools."); + writer.WriteLine($"Use `netclaw mcp tools {serverName.Value} --revoke --audience {audienceName.ToLowerInvariant()}` to disable specific tools."); + return 1; + } + + skippedAllPosture = true; + continue; + } + + var serverAllowed = profile.AllowedMcpServers.Contains(serverName.Value, StringComparer.OrdinalIgnoreCase); if (!serverAllowed) { @@ -1273,7 +1302,16 @@ private static int RunToolsSnapshot( if (updated == 0) { - writer.WriteLine($"Server '{serverName.Value}' is not allowed by any audience profile. Nothing to snapshot."); + if (skippedAllPosture) + { + writer.WriteLine($"Server '{serverName.Value}' is only reachable by audience(s) in All posture, where a snapshot is additive and would not restrict tools."); + writer.WriteLine($"Use `netclaw mcp tools {serverName.Value} --revoke --audience ` to disable specific tools."); + } + else + { + writer.WriteLine($"Server '{serverName.Value}' is not allowed by any audience profile. Nothing to snapshot."); + } + return 1; } @@ -1308,7 +1346,53 @@ private static int RunToolsGrantRevoke( return 1; } - // Build the updated tool set + // Open (All) posture: the grant list is additive, so writing an + // allow-list would be inert. "Disable" is a Deny approval override and + // "enable" clears it, matching the TUI and the runtime exposure gate. + if (profile.McpServersMode == ToolProfileMode.All) + { + var (allConfig, _) = LoadConfigFiles(paths); + var allToolsSection = GetOrCreateSection(allConfig, "Tools"); + var allProfilesSection = GetOrCreateSection(allToolsSection, "AudienceProfiles"); + var allAudienceSection = GetOrCreateSection(allProfilesSection, audienceName); + var approvalPolicy = GetOrCreateSection(allAudienceSection, "ApprovalPolicy"); + var toolOverrides = GetOrCreateSection(approvalPolicy, "ToolOverrides"); + + // Enable = clear the override so the tool inherits the server + // default. But if the server default is itself Deny, clearing would + // leave the tool hidden; write an explicit Approval override instead + // (secure-by-default: exposed, but gated) so enable always takes effect. + var serverDefaultIsDeny = InheritedServerMode(profile, serverName.Value) == ToolApprovalMode.Deny; + + if (grantTools is not null) + foreach (var tool in grantTools) + { + var key = $"{serverName.Value}/{tool}"; + if (serverDefaultIsDeny) + toolOverrides[key] = ToolApprovalMode.Approval.ToString(); + else + toolOverrides.Remove(key); + } + + if (revokeTools is not null) + foreach (var tool in revokeTools) + toolOverrides[$"{serverName.Value}/{tool}"] = ToolApprovalMode.Deny.ToString(); + + WriteConfigFile(paths.NetclawConfigPath, allConfig); + + var allChanges = new List(); + if (grantTools is { Count: > 0 }) + allChanges.Add($"enabled {grantTools.Count}"); + if (revokeTools is { Count: > 0 }) + allChanges.Add($"disabled {revokeTools.Count}"); + + writer.WriteLine( + $"Updated {audienceName} profile for '{serverName.Value}': {string.Join(", ", allChanges)} tool(s). " + + "Disabled tools carry a Deny approval override; every other tool inherits the server default."); + return 0; + } + + // Build the updated tool set (Allowlist posture: closed allow-list) HashSet currentTools; if (profile.McpServerToolGrants is { } existing && existing.TryGetValue(serverName.Value, out var currentList)) @@ -1365,6 +1449,20 @@ private static ToolAudienceProfile ResolveProfile(TrustAudience audience, ToolAu }; } + // The approval mode a tool with no explicit override inherits for this + // server: the server default, else the audience default, else Auto. Mirrors + // ToolApprovalConfig precedence for the server-level (non-tool) case. + private static ToolApprovalMode InheritedServerMode(ToolAudienceProfile profile, string serverName) + { + var approvalPolicy = profile.ApprovalPolicy; + if (approvalPolicy is null) + return ToolApprovalMode.Auto; + + return approvalPolicy.McpServerDefaults.TryGetValue(serverName, out var serverDefault) + ? serverDefault + : approvalPolicy.DefaultMode; + } + private static ToolConfig LoadToolConfig(NetclawPaths paths) { if (!File.Exists(paths.NetclawConfigPath)) diff --git a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs index 31cf838ae..dda8898fc 100644 --- a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs +++ b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs @@ -366,6 +366,15 @@ public ToolApprovalMode GetServerDefault() return ToolApprovalMode.Auto; } + /// + /// True when the selected audience exposes MCP servers in + /// posture. In All posture the per-tool checkbox maps to the approval axis (Deny = + /// disabled) rather than the closed + /// allow-list, so a newly discovered tool stays enabled under the server default. + /// + private bool IsAllPostureForSelectedAudience() + => ResolveProfile(SelectedAudience).McpServersMode == ToolProfileMode.All; + public bool IsToolGranted(ToolName toolName) { if (SelectedServer is null) @@ -373,6 +382,20 @@ public bool IsToolGranted(ToolName toolName) var audienceName = AudienceName(SelectedAudience); + // In All posture the checkbox means "not disabled": a tool is enabled + // unless its effective approval mode is Deny. Disable is expressed as a + // Deny override, so a newly discovered tool (no override) reads as enabled + // and inherits the server default. This mirrors the runtime resolver, + // which treats the grant list as additive in All posture. + if (IsAllPostureForSelectedAudience()) + { + if (_pendingServerAccess.TryGetValue((audienceName, SelectedServer), out var pendingAllAccess) + && !pendingAllAccess) + return false; + + return GetEffectiveMode(toolName).Mode != ToolApprovalMode.Deny; + } + // Check pending grants first if (_pendingGrants.TryGetValue(SelectedServer, out var serverGrants) && serverGrants.TryGetValue(audienceName, out var tools)) @@ -414,6 +437,20 @@ public void ToggleAll() var audienceName = AudienceName(SelectedAudience); + // In All posture, "disable all" writes a Deny override for each tool and + // "enable all" clears the overrides (inherit the server default). + if (IsAllPostureForSelectedAudience()) + { + foreach (var tool in DiscoveredTools) + { + _pendingToolOverrides[(audienceName, SelectedServer, tool)] = + anyGranted ? ToolApprovalMode.Deny : null; + } + + NotifyStateChanged(); + return; + } + if (!_pendingGrants.TryGetValue(SelectedServer, out var serverGrants)) { serverGrants = []; @@ -434,6 +471,30 @@ public void ToggleTool(ToolName toolName) var audienceName = AudienceName(SelectedAudience); + // In All posture the checkbox toggles the Deny approval override, not the + // allow-list. Enabled → set Deny (disable). Disabled → enable: clear the + // override so the tool inherits the server default — but if the server + // default is itself Deny, clearing would leave the tool hidden, so write + // an explicit Approval override (secure-by-default: exposed, but gated) + // so the checkbox always turns the tool on. + if (IsAllPostureForSelectedAudience()) + { + var overrideKey = (audienceName, SelectedServer, toolName.Value); + if (GetEffectiveMode(toolName).Mode == ToolApprovalMode.Deny) + { + _pendingToolOverrides[overrideKey] = GetServerDefault() == ToolApprovalMode.Deny + ? ToolApprovalMode.Approval + : null; + } + else + { + _pendingToolOverrides[overrideKey] = ToolApprovalMode.Deny; + } + + NotifyStateChanged(); + return; + } + if (!_pendingGrants.TryGetValue(SelectedServer, out var serverGrants)) { serverGrants = []; diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs new file mode 100644 index 000000000..5d3dc74fa --- /dev/null +++ b/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs @@ -0,0 +1,62 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Configuration; +using Netclaw.Daemon.Mcp; +using Xunit; + +namespace Netclaw.Daemon.Tests.Mcp; + +public sealed class McpToolDriftTests +{ + private static readonly McpServerName Server = new("dropbox"); + + [Fact] + public void AllPosture_GrantSnapshot_ProducesNoDrift() + { + var profiles = new ToolAudienceProfiles(); + // Personal is All posture by default. + profiles.Personal.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy"] + }; + + var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy", "get_upload_url"]); + + // The server added get_upload_url after the snapshot. In All posture the + // grant list is additive, so there is no drift to warn about. + Assert.Empty(report.Ungranted); + Assert.Empty(report.Stale); + } + + [Fact] + public void AllowlistPosture_DiscoveredToolMissingFromGrants_ReportsUngranted() + { + var profiles = new ToolAudienceProfiles(); + // Team is Allowlist posture by default. + profiles.Team.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy"] + }; + + var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy", "delete"]); + + Assert.Contains("delete", report.Ungranted); + } + + [Fact] + public void AllowlistPosture_GrantNamesMissingTool_ReportsStale() + { + var profiles = new ToolAudienceProfiles(); + profiles.Team.McpServerToolGrants = new Dictionary> + { + ["dropbox"] = ["copy", "gone"] + }; + + var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy"]); + + Assert.Contains("gone", report.Stale); + } +} diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index ea5f9e2c6..c873f0b19 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -1954,46 +1954,73 @@ private static void WriteField(Stream stream, string value) private void LogToolDrift(McpServerName serverName, IReadOnlyList discoveredTools) { - var profiles = _toolConfig.AudienceProfiles; - var allGrantedTools = new HashSet(StringComparer.Ordinal); - var hasAnyGrants = false; - - foreach (var profile in profiles.GetAllProfiles()) - { - if (profile.McpServerToolGrants is not { } grants - || !grants.TryGetValue(serverName.Value, out var tools)) - continue; - - hasAnyGrants = true; - foreach (var tool in tools) - allGrantedTools.Add(tool); - } - - if (!hasAnyGrants) - return; - - var discoveredNames = new HashSet( - discoveredTools.Select(t => t.Name), StringComparer.Ordinal); - var ungranted = discoveredNames.Except(allGrantedTools).ToList(); - var stale = allGrantedTools.Except(discoveredNames).ToList(); + var report = ComputeToolDrift( + _toolConfig.AudienceProfiles, + serverName, + discoveredTools.Select(t => t.Name).ToList()); - if (ungranted.Count > 0) + if (report.Ungranted.Count > 0) { _logger.LogWarning( - "MCP server '{Name}' exposes {Count} tool(s) not granted to any audience: {Tools}. " + + "MCP server '{Name}' exposes {Count} tool(s) not granted to any allowlisted audience: {Tools}. " + "Review and add to McpServerToolGrants if intended.", - serverName.Value, ungranted.Count, string.Join(", ", ungranted)); + serverName.Value, report.Ungranted.Count, string.Join(", ", report.Ungranted)); } - if (stale.Count > 0) + if (report.Stale.Count > 0) { _logger.LogWarning( "McpServerToolGrants for '{Name}' contains {Count} tool(s) not found on server: {Tools}. " + "These may have been removed or renamed.", - serverName.Value, stale.Count, string.Join(", ", stale)); + serverName.Value, report.Stale.Count, string.Join(", ", report.Stale)); } } + /// + /// Drift between a server's discovered tools and its configured grants. + /// and are computed only from + /// Allowlist-posture grant lists, which alone close the tool set. All-posture + /// grant lists are additive (they no longer restrict tools), so they never + /// produce drift. + /// + internal sealed record ToolDriftReport( + IReadOnlyList Ungranted, + IReadOnlyList Stale); + + internal static ToolDriftReport ComputeToolDrift( + ToolAudienceProfiles profiles, + McpServerName serverName, + IReadOnlyList discoveredToolNames) + { + var allowlistGranted = new HashSet(StringComparer.Ordinal); + var hasAllowlistGrants = false; + + foreach (var profile in profiles.GetAllProfiles()) + { + if (profile.McpServerToolGrants is not { } grants + || !grants.TryGetValue(serverName.Value, out var tools)) + continue; + + // A grant list under All posture is additive, so it no longer hides + // ungranted tools. Only Allowlist-posture grant lists genuinely close + // the set, so only those drive the ungranted/stale drift warnings. + if (profile.McpServersMode == ToolProfileMode.All) + continue; + + hasAllowlistGrants = true; + foreach (var tool in tools) + allowlistGranted.Add(tool); + } + + if (!hasAllowlistGrants) + return new ToolDriftReport([], []); + + var discoveredNames = new HashSet(discoveredToolNames, StringComparer.Ordinal); + return new ToolDriftReport( + discoveredNames.Except(allowlistGranted).ToList(), + allowlistGranted.Except(discoveredNames).ToList()); + } + public void Dispose() { var emergencyCleanup = false; From 8651244fc3c0b9b0e2fb2fd423a2fd2e4ed41008 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Mon, 17 Aug 2026 22:12:42 -0500 Subject: [PATCH 2/6] Simplify MCP tool access policy --- .../netclaw-operations/references/tools.md | 48 ++--- .../fix-mcp-new-tool-exposure/.openspec.yaml | 2 - .../fix-mcp-new-tool-exposure/design.md | 113 ---------- .../fix-mcp-new-tool-exposure/proposal.md | 56 ----- .../specs/netclaw-acl/spec.md | 89 -------- .../specs/tool-approval-gates/spec.md | 42 ---- .../fix-mcp-new-tool-exposure/tasks.md | 109 ---------- .../Tools/McpToolAudienceGrantsTests.cs | 200 ++---------------- src/Netclaw.Actors/Tools/ToolAccessPolicy.cs | 50 ++--- .../Tools/ToolAudienceProfileResolver.cs | 40 +--- src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs | 43 +--- .../Mcp/McpToolPermissionsViewModelTests.cs | 87 +------- src/Netclaw.Cli/Mcp/McpCommand.cs | 43 +--- .../Mcp/McpToolPermissionsViewModel.cs | 28 +-- .../Mcp/McpToolDriftTests.cs | 62 ------ src/Netclaw.Daemon/Mcp/McpClientManager.cs | 82 +++---- 16 files changed, 116 insertions(+), 978 deletions(-) delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/design.md delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/proposal.md delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md delete mode 100644 openspec/changes/fix-mcp-new-tool-exposure/tasks.md delete mode 100644 src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index 389f5d0d4..c99cd2222 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -56,15 +56,14 @@ additionally cannot discover or load skills, subagents, memory tools, scheduling tools, or the `web_search` / `web_fetch` tools regardless of feature flags. -### Adding MCP servers (fail-closed by default) +### Add MCP servers -`netclaw mcp add` writes new MCP servers with **zero granted tools** and -per-audience approval defaults so freshly added servers are never silently -exposed: +`netclaw mcp add` writes an approval default for each audience. +The command writes closed tool grants for the Team and Public audiences. | Audience | Grants | Approval default | |----------|--------|------------------| -| Personal | `[]` (empty list — all tools denied until the operator opts in) | `Approval` | +| Personal | Not used (`All` posture) | `Auto` | | Team | `[]` | `Approval` | | Public | `[]` | `Deny` | @@ -74,10 +73,8 @@ per-server approval mode. Bare `netclaw mcp tools` is a read-only CLI view of the same state; both commands surface a discoverability hint toward the TUI. -Escape hatch: `netclaw mcp add --grant-all` keeps the legacy "null grants -= all tools pass" behavior for CI. Even with `--grant-all`, the per-audience -approval defaults (Personal/Team=Approval, Public=Deny) are still written — -you cannot turn off the approval prompts at `mcp add` time. +The `--grant-all` option skips the closed grants for Team and Public. +The option does not change the approval defaults. Inside the TUI (`netclaw mcp permissions`): @@ -95,37 +92,28 @@ Approval-mode resolution precedence (for MCP tools): 1. Exact `ToolOverrides["{server}/{tool}"]` override 2. `McpServerDefaults[{server}]` default -3. Fail-closed fallback (Personal audience, shell/file-edit matcher family) -4. Audience `DefaultMode` +3. Audience `DefaultMode` -A tool whose effective mode is `Deny` is **removed from the tool list the model -sees** — the model never receives a tool it cannot call. +A tool with an effective `Deny` mode does not appear in the model tool list. -Per-tool grants are posture-aware: +The MCP server posture controls tool grants: -- **Open (`All`) posture** (Personal by default): the grant list is *additive*. - A newly discovered tool is exposed and inherits the server default. Disable a - specific tool by setting it to `Deny`. From the CLI: `netclaw mcp tools - --revoke --audience personal` writes the `Deny` override, and - `--grant ` clears it. -- **`Allowlist` posture** (Team/Public by default): the grant list is *closed*. - A tool stays hidden until it is granted, so a newly discovered tool is not - exposed until the operator adds it. +- `All` does not use `McpServerToolGrants`. +- A new tool inherits the server approval default in `All` posture. +- `Allowlist` exposes only the tools in `McpServerToolGrants`. +- An exact tool override always takes precedence. -You do not need to re-run `permissions` after a server learns a new tool; in -open posture it appears automatically under the server default. +Use `--revoke` to write a `Deny` override in `All` posture. +Use `--grant` to remove a `Deny` override or enable a tool above a `Deny` default. -### Migrating existing MCP servers +### Existing MCP servers Servers added to `netclaw.json` before this behavior shipped stay untouched — their tool grants, `ApprovalPolicy.McpServerDefaults`, and `ToolOverrides` entries are not rewritten during an upgrade. -A grant snapshot stored for an `All`-posture audience (for example a Personal -snapshot written by an older build) is now *inert*: the additive grant list no -longer restricts tools, so a newly discovered tool still appears. To disable a -specific tool, set a `Deny` override in `netclaw mcp permissions` (or -`netclaw mcp tools --revoke --audience `). +An existing grant snapshot has no effect when its audience uses `All` posture. +The update does not remove or replace any exact tool override. `netclaw doctor` will emit a warning for each enabled MCP server that Personal can reach (`McpServersMode = All`) but has no diff --git a/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml b/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml deleted file mode 100644 index 95672402a..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/.openspec.yaml +++ /dev/null @@ -1,2 +0,0 @@ -schema: spec-driven -created: 2026-08-18 diff --git a/openspec/changes/fix-mcp-new-tool-exposure/design.md b/openspec/changes/fix-mcp-new-tool-exposure/design.md deleted file mode 100644 index bb78197e6..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/design.md +++ /dev/null @@ -1,113 +0,0 @@ -## Context - -An MCP tool has two independent access axes in Netclaw: - -- Approval mode (`ToolApprovalConfig`: `ToolOverrides` -> `McpServerDefaults` -> - `DefaultMode`). For an unseen tool this correctly falls through to the server - default. -- Enabled state (`ToolAudienceProfile.McpServerToolGrants`, a positive - allow-list). For an unseen tool this fails closed (hidden). - -`ToolAudienceProfileResolver.IsMcpToolAllowed` enforces the enabled axis. It -gates exposure (`ToolAccessPolicy.IsToolExposed`) and invocation -(`AuthorizeInvocationCore`, deny reason -`mcp_tool_not_allowed_for_audience_profile`). The two sibling checks -`IsMcpServerAllowed` and `IsToolAllowed` short-circuit to `true` in `All` -posture. `IsMcpToolAllowed` does not. It always treats a grant entry as a closed -allow-list. - -The MCP Permissions TUI seeds `McpServerToolGrants[server]` from a point-in-time -snapshot of the discovered tools. A later remote-added tool is absent from the -snapshot, so the resolver hides it. The daemon detects this drift but only logs -a warning. - -The allow-list cannot separate two states: "operator disabled this tool" and -"tool did not exist yet." Both states read as "absent from the list." The fix -must move the explicit-disable signal off the allow-list. - -## Goals / Non-Goals - -**Goals:** - -- A new MCP tool inherits the server default posture in open (`All`) posture. -- A disabled MCP tool leaves the tool list that the model sees. -- Team and Public audiences stay fail-closed for unseen tools. -- Reuse the existing `ToolApprovalMode.Deny`. Add no new config property. - -**Non-Goals:** - -- Change built-in (non-MCP) tool exposure logic. -- Rewrite persisted config files during migration. -- Reconstruct historical per-tool disable choices from a snapshot. - -## Decisions - -### Decision 1: `Deny` is the single "disabled" signal, and `Deny` hides the tool - -A tool with effective approval mode `Deny` leaves the exposed tool list. Today -`IsToolExposed` does not read the approval mode, so a `Deny` tool is shown and -then blocked at invocation. The MCP branch of `IsToolExposed` now resolves the -approval mode and returns `false` on `Deny`. - -Rationale: an LLM must not receive a tool that it cannot call. A hidden tool -prevents a wasted turn. This reuses the approval-mode resolver -(`GetApprovalMode` / `ResolveApprovalMode`) instead of a new code path. - -Alternative considered: a new `McpServerToolDenials` deny-list field. Rejected. -It duplicates state that `Deny` already carries and needs a schema change plus -plumbing. - -### Decision 2: `McpServerToolGrants` becomes posture-aware - -`IsMcpToolAllowed` returns `true` for a tool absent from the grant list when the -audience posture is `All`. It keeps the closed allow-list for `Allowlist` -posture. This matches the existing pattern in `IsMcpServerAllowed` and -`IsToolAllowed`. - -Rationale: `All` posture means "expose everything unless explicitly denied." -`Allowlist` posture means "expose only the listed items." The per-tool layer now -follows the same rule as the server and built-in-tool layers. - -Alternative considered: auto-reconcile new tools into the grant list at -discovery time. Rejected. The daemon would write operator config silently, and a -blind union would wrongly grant new tools to Team and Public. - -### Decision 3: TUI and CLI express disable as `Deny` in open posture - -In `All` posture the MCP Permissions checkbox toggles between `Deny` (disabled, -hidden) and a cleared override (server default). In `Allowlist` posture the -checkbox keeps writing the `McpServerToolGrants` allow-list. The `netclaw mcp` -grant/revoke/snapshot paths follow the same posture rule. - -Rationale: the persisted representation must match what the runtime consumes. A -disabled tool in open posture persists as a `Deny` override, which the resolver -reads back as hidden. - -## Risks / Trade-offs - -- [A `Deny` tool that operators expected to see-but-refuse now disappears] -> - This is the intended behavior and matches the issue. Document it in the - `netclaw-operations` skill and the change notes. -- [A pre-existing `All`-posture grant snapshot silently stops filtering] -> The - snapshot was almost always a full catalog, so no tool changes state in - practice. The TUI shows real tool state, and `tools.md` documents the inert - behavior. -- [An operator genuinely curated a Personal subset via the allow-list] -> The - feature is new/MVP. Such an operator re-expresses the subset with `Deny`. An - active warning, if wanted, belongs in `netclaw doctor` (operator-run, - idempotent), not a daemon log line. -- [Scope creep to built-in tools] -> Explicitly out of scope. The MCP branch of - `IsToolExposed` is the only exposure path that changes. - -## Migration Plan - -1. Deploy the resolver and exposure changes. No config schema field changes. -2. On tool discovery, when a profile in `All` posture carries a - `McpServerToolGrants[server]` entry, treat it as inert for the closed-list - decision. No config rewrite; the TUI and `tools.md` cover the behavior. -3. Rollback: revert the resolver and exposure changes. The persisted config is - unchanged, so a rollback restores the prior closed-list behavior. - -## Open Questions - -None. The model and scope are settled. diff --git a/openspec/changes/fix-mcp-new-tool-exposure/proposal.md b/openspec/changes/fix-mcp-new-tool-exposure/proposal.md deleted file mode 100644 index 878715193..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/proposal.md +++ /dev/null @@ -1,56 +0,0 @@ -## Why - -A remote MCP server can add a new tool after the operator sets custom per-tool -rules. Today the new tool disappears from the agent (issue #1959). The operator -expects the new tool to inherit the server default posture: available under -`Approval`, or available and auto-approved under `Auto`. A new tool must never -become silently unavailable. - -## What Changes - -- In open (`All`) posture, `McpServerToolGrants` becomes an additive layer, not - a closed allow-list. A tool that is absent from the grant list passes the - audience check and inherits the server default approval posture. -- `Allowlist` posture (Team, Public) keeps the closed allow-list. An unseen tool - stays hidden. Least-trust audiences remain fail-closed. -- A tool with effective approval mode `Deny` is removed from the tool list that - the model sees. Today a `Deny` tool is shown and then blocked at invocation. - **BREAKING** for the exposed-tool surface: a `Deny` MCP tool is now hidden. -- The MCP Permissions TUI and the `netclaw mcp` CLI express "disable one tool" in - open posture as `Deny`, not as omission from the allow-list. -- The daemon drift warning fires only for `Allowlist` posture. Open posture has - no drift, because unseen tools are exposed by default. - -Scope: this change applies "Deny hides the tool" to MCP tools only. Built-in -tools keep their current exposure logic. - -## Capabilities - -### New Capabilities - - - -### Modified Capabilities - -- `netclaw-acl`: the per-tool `McpServerToolGrants` layer changes from an - always-closed allow-list to a posture-aware layer. `All` posture treats the - list as additive; `Allowlist` posture keeps it closed. -- `tool-approval-gates`: a tool with effective approval mode `Deny` is removed - from the exposed tool list, in addition to the existing invocation block. - -## Impact - -- `src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs` — - `IsMcpToolAllowed` becomes posture-aware. -- `src/Netclaw.Actors/Tools/ToolAccessPolicy.cs` — `IsToolExposed` hides a - `Deny` MCP tool. -- `src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs`, - `McpToolPermissionsPage.cs`, `McpCommand.cs` — checkbox and CLI map "disable" - to `Deny` in open posture. -- `src/Netclaw.Daemon/Mcp/McpClientManager.cs` — drift warning scoped to - `Allowlist` posture. -- Config migration: a pre-existing `All`-posture `McpServerToolGrants` snapshot - becomes inert. No config rewrite. The daemon logs a one-time notice. -- Security: Team and Public stay fail-closed. Personal gains correct new-tool - exposure under the server default posture. -- Skill `netclaw-operations` gains updated MCP permissions guidance. diff --git a/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md b/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md deleted file mode 100644 index 153e6e64c..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/specs/netclaw-acl/spec.md +++ /dev/null @@ -1,89 +0,0 @@ -## MODIFIED Requirements - -### Requirement: Tool and data grants - -The system SHALL enforce explicit grants for tool and data access. Grants SHALL -be organized into specific tool grant categories: `shell`, `web_search`, -`web_fetch`, `github`, `mcp:{server_name}`, `config_write`, and -`schedule_write`. Each grant SHALL specify the allowed senders and channels to -which it applies. - -For MCP tools, the system SHALL support an additional per-tool grant layer via -`McpServerToolGrants` on each audience profile. This per-tool check SHALL -execute after the server-level `AllowedMcpServers` check. The per-tool check -SHALL be posture-aware and follow the audience `McpServersMode`. - -- When `McpServersMode` is `Allowlist`, the grant list SHALL be closed. Only a - tool that the list names SHALL pass the audience check. A tool that the list - does not name SHALL be denied. This keeps least-trust audiences fail-closed. -- When `McpServersMode` is `All`, the grant list SHALL be additive. A tool that - the list names SHALL pass. A tool that the list does not name SHALL also pass - and SHALL inherit the server default approval posture. - -Each `ToolAudienceProfile` SHALL support an optional `ApprovalPolicy` of type -`ToolApprovalConfig`. The `ApprovalPolicy` SHALL define a `DefaultMode` (Auto, -Approval, Deny) and per-tool overrides via `ToolOverrides`. The approval check -SHALL execute after the tool access grant check passes. Tools in Approval mode -SHALL surface approval context for the executor, and the executor SHALL consult -`IToolApprovalService` before execution. Tools in Deny mode SHALL be blocked -without an approval prompt. - -#### Scenario: Missing grant blocks tool call - -- **WHEN** a tool call is attempted without a matching grant -- **THEN** execution is denied with a policy reason code - -#### Scenario: Category-specific grant allows tool - -- **GIVEN** ACL grants `web_search` for sender `U12345` on channel `C99999` -- **WHEN** sender `U12345` requests a web search in channel `C99999` -- **THEN** ACL evaluation returns allow for the `web_search` tool category - -#### Scenario: MCP server-scoped grant - -- **GIVEN** ACL grants `mcp:memorizer` for sender `U12345` -- **WHEN** sender `U12345` requests an MCP tool from the `memorizer` server -- **THEN** ACL evaluation returns allow -- **AND** MCP tools from other servers without explicit grants are denied - -#### Scenario: MCP tool blocked by per-tool grant in allowlist posture - -- **GIVEN** the session's audience `McpServersMode` is `Allowlist` -- **AND** the audience allows `memorizer` server via `AllowedMcpServers` -- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` -- **WHEN** the agent invokes `memorizer/store` -- **THEN** the invocation is denied with reason `mcp_tool_not_allowed_for_audience_profile` - -#### Scenario: MCP tool allowed by per-tool grant in allowlist posture - -- **GIVEN** the session's audience `McpServersMode` is `Allowlist` -- **AND** the audience allows `memorizer` server via `AllowedMcpServers` -- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` -- **WHEN** the agent invokes `memorizer/search_memories` -- **THEN** the invocation is allowed - -#### Scenario: New MCP tool passes the per-tool check in open posture - -- **GIVEN** the session's audience `McpServersMode` is `All` -- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` -- **AND** the server adds a new tool `store` that the list does not name -- **WHEN** the agent invokes `memorizer/store` -- **THEN** the per-tool check passes -- **AND** the invocation inherits the server default approval posture - -#### Scenario: New MCP tool stays fail-closed in allowlist posture - -- **GIVEN** the session's audience `McpServersMode` is `Allowlist` -- **AND** `McpServerToolGrants` for this audience lists `["search_memories", "get"]` -- **AND** the server adds a new tool `store` that the list does not name -- **WHEN** the agent invokes `memorizer/store` -- **THEN** the invocation is denied with reason `mcp_tool_not_allowed_for_audience_profile` - -#### Scenario: Tool granted but requires approval - -- **GIVEN** the session has a grant for `shell_execute` -- **AND** the Personal `ApprovalPolicy` sets `shell_execute` to Approval mode -- **AND** the command pattern `git push` is not already approved in `IToolApprovalService` -- **WHEN** the agent invokes `shell_execute` with `git push origin main` -- **THEN** the grant check passes -- **AND** the approval check returns `RequiresApproval` diff --git a/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md b/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md deleted file mode 100644 index 949d3e72f..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/specs/tool-approval-gates/spec.md +++ /dev/null @@ -1,42 +0,0 @@ -## ADDED Requirements - -### Requirement: MCP tool exposure honors Deny and server default - -The system SHALL remove an MCP tool from the exposed tool list when the tool -effective approval mode is `Deny` for the session audience. The model SHALL NOT -receive a tool that the policy will block. This exposure rule SHALL apply in -addition to the existing invocation block for `Deny` tools. - -An MCP tool that has no per-tool override SHALL inherit the server default -approval posture through the existing precedence (`ToolOverrides` -> -`McpServerDefaults` -> `DefaultMode`). A newly discovered MCP tool that the -operator never named SHALL therefore be exposed under the server default. It -SHALL be auto-approved when the server default is `Auto`. It SHALL be -approval-gated when the server default is `Approval`. - -This exposure rule SHALL apply to MCP tools only. Built-in tool exposure logic -SHALL NOT change. - -#### Scenario: MCP tool in Deny mode is removed from the exposed list - -- **GIVEN** an MCP tool whose effective approval mode is `Deny` for the session's audience -- **WHEN** the runtime builds the tool list for the model -- **THEN** the tool is absent from the exposed tool list -- **AND** the tool is absent from the discoverable tool list - -#### Scenario: New MCP tool is exposed under an Approval server default - -- **GIVEN** the session's audience `McpServersMode` is `All` -- **AND** the server default approval mode for `dropbox` is `Approval` -- **AND** the server adds a new tool `get_upload_url` that no override names -- **WHEN** the runtime builds the tool list for the model -- **THEN** `dropbox/get_upload_url` is present in the exposed tool list -- **AND** the effective approval mode for `dropbox/get_upload_url` is `Approval` - -#### Scenario: New MCP tool is exposed under an Auto server default - -- **GIVEN** the session's audience `McpServersMode` is `All` -- **AND** the server default approval mode for `dropbox` is `Auto` -- **AND** the server adds a new tool `get_upload_url` that no override names -- **WHEN** the agent invokes `dropbox/get_upload_url` -- **THEN** the tool executes immediately without an approval prompt diff --git a/openspec/changes/fix-mcp-new-tool-exposure/tasks.md b/openspec/changes/fix-mcp-new-tool-exposure/tasks.md deleted file mode 100644 index 4d717c961..000000000 --- a/openspec/changes/fix-mcp-new-tool-exposure/tasks.md +++ /dev/null @@ -1,109 +0,0 @@ -# Tasks: fix-mcp-new-tool-exposure - -## 1. Runtime ACL: posture-aware per-tool grant - -- [x] 1.1 Update `ToolAudienceProfileResolver.IsMcpToolAllowed` - (`src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs`): when the profile - `McpServersMode` is `All`, return `true` for a tool that the grant list does - not name. Keep the closed allow-list for `Allowlist` posture. -- [x] 1.2 Update the XML doc comment on `IsMcpToolAllowed` to state the - posture-aware behavior. -- [x] 1.3 Add resolver unit tests: `All` posture passes an unnamed tool; - `Allowlist` posture denies an unnamed tool; both postures pass a named tool. - -## 2. Runtime exposure: Deny hides the MCP tool - -- [x] 2.1 In `ToolAccessPolicy.IsToolExposed` MCP branch - (`src/Netclaw.Actors/Tools/ToolAccessPolicy.cs`), after the server and - per-tool ACL checks pass, resolve the MCP tool effective approval mode via the - audience `ApprovalPolicy` precedence (`ToolOverrides` -> `McpServerDefaults` -> - `DefaultMode`) and return `false` when the mode is `Deny`. -- [x] 2.2 Reuse the existing approval-mode resolver - (`ResolveApprovalMode` / `McpApprovalMatcher`). Add a small helper only if the - exposure path cannot supply invocation arguments; the MCP mode resolution does - not depend on arguments. -- [x] 2.3 Keep the change scoped to MCP tools. Do not alter built-in tool - exposure. -- [x] 2.4 Add exposure unit tests: a `Deny` MCP tool is absent from - `FilterExposedTools` and `FilterDiscoverableTools`; an `Auto`/`Approval` MCP - tool stays present. - -## 3. Regression: new tool inherits server default - -- [x] 3.1 Add a regression test for issue #1959: `All` posture, a grant snapshot - that does not name a newly discovered tool. Assert the new tool is exposed. -- [x] 3.2 Assert the new tool effective mode equals the server default. Cover - both `McpServerDefaults` = `Approval` and `McpServerDefaults` = `Auto`. -- [x] 3.3 Add a fail-closed guard test: `Allowlist` posture (Team/Public) keeps a - newly discovered tool hidden. - -## 4. TUI: MCP Permissions page - -- [x] 4.1 In `McpToolPermissionsViewModel` - (`src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs`), for `All` posture, map - the tool checkbox to the approval override: unchecked writes `Deny`, checked - clears the override (server default). Stop seeding a full snapshot allow-list - in `All` posture. For `Allowlist` posture, keep writing `McpServerToolGrants`. -- [x] 4.2 Update `IsToolGranted`/`GetEffectiveMode` so a row reflects `Deny` as - disabled in `All` posture and reflects allow-list membership in `Allowlist` - posture. -- [x] 4.3 Update row rendering in `McpToolPermissionsPage` - (`src/Netclaw.Cli/Mcp/McpToolPermissionsPage.cs`) so a disabled row (Deny or - un-granted) is greyed and the `(def)`/`(override)` badge stays correct. -- [x] 4.4 Add headless ViewModel tests: uncheck in `All` posture persists `Deny`; - check clears the override; `Allowlist` posture still writes the allow-list. - -## 5. CLI: `netclaw mcp` - -- [x] 5.1 In `McpCommand` (`src/Netclaw.Cli/Mcp/McpCommand.cs`), make - grant/revoke/`--snapshot` posture-aware. In `All` posture, "revoke" writes - `Deny`; in `Allowlist` posture, "revoke" removes from the allow-list. -- [x] 5.2 Add CLI tests for the posture-aware grant/revoke behavior. - -## 6. Daemon: drift warning - -- [x] 6.1 In `McpClientManager.LogToolDrift` - (`src/Netclaw.Daemon/Mcp/McpClientManager.cs`), fire the ungranted/stale - drift warning only for `Allowlist` posture audiences. `All`-posture grant - lists are additive, so they produce no drift. (An inert-snapshot log notice - was implemented then removed in review: it was low-value operator noise, and - the daemon restart on a config change already resets any per-server state. - An active warning, if wanted, belongs in `netclaw doctor`.) -- [x] 6.2 Add a daemon test (`McpToolDriftTests`) that asserts an `All`-posture - grant snapshot produces no drift and an `Allowlist` grant list still reports - ungranted/stale tools. - -## 7. Cross-boundary contract test - -- [x] 7.1 Add a producer/consumer test: the TUI ViewModel writes `Deny` on - uncheck; a fresh `ToolAccessPolicy`/resolver built from the written config - hides the tool. Prove the persisted representation matches the runtime consumer. - -## 8. Migration / round-trip - -- [x] 8.1 Add a load/round-trip test: a config with an `All`-posture full-snapshot - `McpServerToolGrants` plus a new discovered tool exposes the new tool. - -## 9. Skill, docs, quality gates - -- [x] 9.1 Update the `netclaw-operations` skill - (`feeds/skills/.system/files/netclaw-operations/SKILL.md`) with the MCP - permissions model: `Deny` hides a tool; new tools inherit the server default in - open posture; Team/Public stay fail-closed. Bump `metadata.version`. -- [x] 9.2 Run `dotnet slopwatch analyze` and fix any new violation. -- [x] 9.3 Run `./scripts/Add-FileHeaders.ps1 -Verify` for copyright headers. -- [ ] 9.4 Run `./scripts/smoke/run-smoke.sh` for the MCP Permissions page if the - prompt flow changed. **Deferred to CI:** the local environment has no `vhs` - and the harness needs `sudo` to install Ollama. The `mcp-permissions` tape only - smokes page-open/no-daemon/exit; it never reaches the tool grid, so it does not - exercise the changed toggle logic. Headless ViewModel tests cover that behavior. -- [ ] 9.5 Run the eval suite if tool exposure/grant-category behavior needs a new - case. **Deferred to CI:** the eval runner needs an external provider endpoint - (`NETCLAW_EVAL_PROVIDER_ENDPOINT`) unavailable here. The change is a runtime ACL - fix and additive skill documentation; it adds no tool, schema, or grant category, - so no new eval case is warranted. Runtime/integration tests cover the behavior. - -## 10. OpenSpec finish - -- [ ] 10.1 Run `/opsx-verify` then `/opsx-sync` and `/opsx-archive` after the - implementation lands and gates pass. diff --git a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs index 810a30790..6acfe6b7e 100644 --- a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs @@ -377,44 +377,12 @@ public void McpServerToolGrants_DeserializesFromJson() Assert.False(policy.IsToolExposed(CreateMcpTool("memorizer", "store"), TeamContext())); } - // ── Posture-aware per-tool grants (issue #1959) ── - - [Fact] - public void AllPosture_ToolAbsentFromGrantSnapshot_IsExposed() - { - // Regression for #1959. Personal is All posture. A grant snapshot names - // the tools that existed when the operator configured the server. A tool - // the server adds later is absent from the snapshot, but the additive - // grant layer must still expose it. - var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; - config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy", "delete", "move"] - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), PersonalContext())); - // A tool the snapshot names stays exposed too. - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); - } - - [Fact] - public void AllowlistPosture_ToolAbsentFromGrantSnapshot_StaysHidden() - { - // Team is Allowlist posture. The grant list stays closed so least-trust - // audiences remain fail-closed for a newly discovered tool. - var config = CreateConfigWithTeamServer("dropbox"); - config.AudienceProfiles.Team.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy", "delete", "move"] - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - - Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), TeamContext())); - } - - [Fact] - public void AllPosture_NewTool_InheritsApprovalServerDefault() + [Theory] + [InlineData(ToolApprovalMode.Auto, false)] + [InlineData(ToolApprovalMode.Approval, true)] + public void AllPosture_NewTool_InheritsServerDefault( + ToolApprovalMode serverDefault, + bool needsApproval) { var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> @@ -425,101 +393,31 @@ public void AllPosture_NewTool_InheritsApprovalServerDefault() { McpServerDefaults = new Dictionary(StringComparer.Ordinal) { - ["dropbox"] = ToolApprovalMode.Approval + ["dropbox"] = serverDefault } }; var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); var newTool = CreateMcpTool("dropbox", "get_upload_url"); - // Approval is not Deny, so the tool is exposed ... Assert.True(policy.IsToolExposed(newTool, PersonalContext())); - // ... and the new tool inherits the server default at invocation. - var decision = policy.AuthorizeInvocation(newTool, CreateExecutionContext(TrustAudience.Personal)); - Assert.True(decision.NeedsApproval); + Assert.Equal( + needsApproval, + policy.AuthorizeInvocation(newTool, CreateExecutionContext(TrustAudience.Personal)).NeedsApproval); } [Fact] - public void AllPosture_NewTool_InheritsAutoServerDefault() - { - var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; - config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy"] - }; - config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig - { - McpServerDefaults = new Dictionary(StringComparer.Ordinal) - { - ["dropbox"] = ToolApprovalMode.Auto - } - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - var newTool = CreateMcpTool("dropbox", "get_upload_url"); - - Assert.True(policy.IsToolExposed(newTool, PersonalContext())); - var decision = policy.AuthorizeInvocation(newTool, CreateExecutionContext(TrustAudience.Personal)); - Assert.True(decision.Allowed); - Assert.False(decision.NeedsApproval); - } - - // ── Deny hides an MCP tool from exposure ── - - [Fact] - public void AllPosture_DenyTool_HiddenFromExposure() - { - var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; - config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig - { - ToolOverrides = new Dictionary(StringComparer.Ordinal) - { - ["dropbox/delete"] = ToolApprovalMode.Deny - } - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - - // The Deny tool is removed from the list the model sees. - Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "delete"), PersonalContext())); - // A sibling tool with no Deny stays exposed. - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); - } - - [Fact] - public void AllPosture_DenyTool_StillDeniedAtInvocation() - { - // The exposure hide is in addition to the existing invocation block. - var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; - config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig - { - ToolOverrides = new Dictionary(StringComparer.Ordinal) - { - ["dropbox/delete"] = ToolApprovalMode.Deny - } - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - - var decision = policy.AuthorizeInvocation( - CreateMcpTool("dropbox", "delete"), - CreateExecutionContext(TrustAudience.Personal)); - - Assert.False(decision.Allowed); - Assert.Equal("tool_denied_by_approval_policy", decision.DenyReason); - } - - [Fact] - public void FilterExposedTools_RemovesDenyTool_KeepsNewTool() + public void FilterExposedTools_HidesDenyOverrideAndKeepsNewTool() { var registry = new ToolRegistry(); - var copyTool = CreateMcpTool("dropbox", "copy"); - var deleteTool = CreateMcpTool("dropbox", "delete"); + var deniedTool = CreateMcpTool("dropbox", "delete"); var newTool = CreateMcpTool("dropbox", "get_upload_url"); - registry.Register(copyTool); - registry.Register(deleteTool); + registry.Register(deniedTool); registry.Register(newTool); var config = new ToolConfig { ShellMode = ShellExecutionMode.HostAllowed }; config.AudienceProfiles.Personal.McpServerToolGrants = new Dictionary> { - ["dropbox"] = ["copy", "delete"] + ["dropbox"] = ["delete"] }; config.AudienceProfiles.Personal.ApprovalPolicy = new ToolApprovalConfig { @@ -530,71 +428,13 @@ public void FilterExposedTools_RemovesDenyTool_KeepsNewTool() }; var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - var aiTools = new[] { copyTool.ToAITool(), deleteTool.ToAITool(), newTool.ToAITool() }; - var filtered = policy.FilterExposedTools(aiTools, registry, PersonalTrustContext()); - - var names = filtered.Select(t => ((AIFunction)t).Name).ToList(); - Assert.Contains("dropbox__copy", names); // granted, not denied - Assert.Contains("dropbox__get_upload_url", names); // new tool, additive in All posture - Assert.DoesNotContain("dropbox__delete", names); // Deny → hidden - } - - [Fact] - public void AllowlistPosture_GrantedTool_WithDenyOverride_IsHidden() - { - // Deny hides a tool for EVERY audience, not only All posture: a tool that - // is granted (passes the allow-list) but carries a Deny override is - // removed from the exposed list rather than shown-then-refused. - var config = CreateConfigWithTeamServer("dropbox"); - config.AudienceProfiles.Team.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy", "delete"] - }; - config.AudienceProfiles.Team.ApprovalPolicy = new ToolApprovalConfig - { - ToolOverrides = new Dictionary(StringComparer.Ordinal) - { - ["dropbox/delete"] = ToolApprovalMode.Deny - } - }; - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - - Assert.False(policy.IsToolExposed(CreateMcpTool("dropbox", "delete"), TeamContext())); // granted but Deny → hidden - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), TeamContext())); // granted, no Deny - } - - // ── Migration: an All-posture snapshot is additive after load ── - - [Fact] - public void AllPosture_GrantSnapshot_DeserializesAndExposesNewTool() - { - // A config written before the server added a tool carries a full snapshot - // under Personal (All posture). After load the snapshot is additive: the - // new tool is exposed rather than treated as disabled. - var json = """ - { - "ShellMode": "HostAllowed", - "AudienceProfiles": { - "Personal": { - "McpServersMode": "All", - "McpServerToolGrants": { - "dropbox": ["copy", "delete", "move"] - } - } - } - } - """; - - var config = System.Text.Json.JsonSerializer.Deserialize(json, - new System.Text.Json.JsonSerializerOptions - { - Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() } - }); + var filtered = policy.FilterExposedTools( + [deniedTool.ToAITool(), newTool.ToAITool()], + registry, + PersonalTrustContext()); - Assert.NotNull(config); - var policy = new ToolAccessPolicy(config, Defaults, new ShellCommandPolicy(), new ToolPathPolicy([])); - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "get_upload_url"), PersonalContext())); - Assert.True(policy.IsToolExposed(CreateMcpTool("dropbox", "copy"), PersonalContext())); + var exposed = Assert.Single(filtered); + Assert.Equal("dropbox__get_upload_url", ((AIFunction)exposed).Name); } // ── Helpers ── diff --git a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs index 0b8098448..ffdfa0d6f 100644 --- a/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs +++ b/src/Netclaw.Actors/Tools/ToolAccessPolicy.cs @@ -108,11 +108,7 @@ public IReadOnlyList FilterExposedTools( IEnumerable tools, ToolRegistry registry, EffectiveTrustContext? trustContext) - { - // Resolve the audience profile ONCE for the whole pass, not per tool. - var audience = ResolveAudience(trustContext); - var profile = _profileResolver.ResolveProfile(audience); - return tools + => tools .Where(tool => { var name = GetToolName(tool); @@ -120,20 +116,14 @@ public IReadOnlyList FilterExposedTools( return true; var registration = registry.GetRegistrationByToolName(name); - return registration is null || IsToolExposed(registration.Tool, profile, audience); + return registration is null || IsToolExposed(registration, trustContext); }) .ToList(); - } public IReadOnlyList FilterDiscoverableTools( IEnumerable tools, ToolInvocationContext context) - { - // Resolve the audience profile ONCE for the whole pass, not per tool. - var audience = ResolveAudience(context); - var profile = _profileResolver.ResolveProfile(audience); - return tools.Where(tool => IsToolExposed(tool, profile, audience)).ToList(); - } + => tools.Where(tool => IsToolExposed(tool, context)).ToList(); public bool IsToolExposed(ToolRegistration registration, EffectiveTrustContext? trustContext) => IsToolExposed(registration.Tool, ResolveAudience(trustContext)); @@ -145,37 +135,21 @@ public bool IsMcpServerExposed(McpServerName serverName, TrustAudience audience) => _profileResolver.IsMcpServerAllowed(serverName, audience); internal bool IsToolExposed(INetclawTool tool, TrustAudience audience) - => IsToolExposed(tool, _profileResolver.ResolveProfile(audience), audience); - - // Core exposure decision. Takes the already-resolved audience profile so a - // filter pass over many tools resolves the profile once and reuses it for - // every per-tool ACL and approval check. - internal bool IsToolExposed(INetclawTool tool, ToolAudienceProfile profile, TrustAudience audience) { // Feature-disabled tools are hidden for ALL audiences if (IsFeatureDisabledTool(tool.Name)) return false; if (tool is McpToolAdapter mcp) - { - var mcpServer = new McpServerName(mcp.ServerName); - if (!_profileResolver.IsMcpServerAllowed(mcpServer, profile) - || !_profileResolver.IsMcpToolAllowed(mcpServer, new ToolName(mcp.BareToolName), profile)) - return false; - - // A tool whose effective approval mode is Deny is "disabled": remove - // it from the exposed list so the model never receives a tool the - // policy will always block. This applies to EVERY audience, not only - // All posture — a granted-but-Deny tool in an Allowlist audience is - // hidden too, since showing a tool that always fails helps no one. - // Reuses the shared approval precedence (ToolOverrides → - // McpServerDefaults → DefaultMode) so exposure and invocation cannot - // drift. A newly discovered tool has no Deny entry, so it stays - // exposed under the server default posture. - return profile.ApprovalPolicy?.GetEffectiveMode(mcp.Name) != ToolApprovalMode.Deny; - } - - if (!_profileResolver.IsToolAllowed(new ToolName(tool.Name), profile)) + return _profileResolver.IsMcpServerAllowed(new McpServerName(mcp.ServerName), audience) + && _profileResolver.IsMcpToolAllowed( + new McpServerName(mcp.ServerName), + new ToolName(mcp.BareToolName), + audience) + && _profileResolver.ResolveProfile(audience).ApprovalPolicy?.GetEffectiveMode(mcp.Name) + != ToolApprovalMode.Deny; + + if (!_profileResolver.IsToolAllowed(new ToolName(tool.Name), audience)) return false; if (IsShellCoupledTool(tool)) diff --git a/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs b/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs index f18c885bd..05966c41f 100644 --- a/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs +++ b/src/Netclaw.Actors/Tools/ToolAudienceProfileResolver.cs @@ -77,18 +77,12 @@ public bool IsToolAllowed(ToolName toolName, ToolInvocationContext context) => IsToolAllowed(toolName, context.Audience); public bool IsToolAllowed(ToolName toolName, TrustAudience audience) - => IsToolAllowed(toolName, ResolveProfile(audience)); - - /// - /// Profile-accepting overload. Callers that filter many tools for one - /// audience resolve the profile once and pass it in, so a filter pass does - /// not re-resolve the profile per tool. - /// - public bool IsToolAllowed(ToolName toolName, ToolAudienceProfile profile) { if (!IsProfileManagedTool(toolName)) return true; + var profile = ResolveProfile(audience); + if (profile.ToolsMode == ToolProfileMode.All) return true; @@ -108,17 +102,8 @@ public bool IsMcpServerAllowed(McpServerName serverName, TrustAudience audience) } /// - /// Checks whether a specific tool from an MCP server is allowed for the given audience. - /// The per-tool grant list is posture-aware: - /// - No (null) → all tools pass. - /// - The server has no entry in the grants dictionary → all tools pass. - /// - The tool name appears in the server's grant list → passes. - /// - The tool name is absent and the audience - /// is → passes. The grant list is additive, so a tool the - /// server added after the operator wrote the list inherits the server default posture. - /// - The tool name is absent and the audience - /// is → denied. The closed allow-list keeps - /// least-trust audiences fail-closed. + /// Checks whether an MCP tool passes the audience profile. + /// Per-tool grants apply only when the profile uses . /// public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, TrustAudience audience) { @@ -176,7 +161,7 @@ public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolIn private static TrustAudience ResolveAudience(ToolInvocationContext context) => context.Audience; - public bool IsMcpServerAllowed(McpServerName serverName, ToolAudienceProfile profile) + private static bool IsMcpServerAllowed(McpServerName serverName, ToolAudienceProfile profile) { if (profile.McpServersMode == ToolProfileMode.All) return true; @@ -184,23 +169,18 @@ public bool IsMcpServerAllowed(McpServerName serverName, ToolAudienceProfile pro return profile.AllowedMcpServers.Contains(serverName.Value, StringComparer.OrdinalIgnoreCase); } - public bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) + private static bool IsMcpToolAllowed(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) { - if (profile.McpServerToolGrants is not { } grants) + if (profile.McpServersMode == ToolProfileMode.All) return true; - if (!grants.TryGetValue(serverName.Value, out var allowedTools)) + if (profile.McpServerToolGrants is not { } grants) return true; - if (allowedTools.Contains(toolName.Value, StringComparer.Ordinal)) + if (!grants.TryGetValue(serverName.Value, out var allowedTools)) return true; - // The tool is not named in the grant list. In All posture the grant list - // is additive, not a closed allow-list: an unnamed tool (for example one - // the server added after the operator wrote the list) still passes and - // inherits the server default approval posture. Allowlist posture stays - // closed so least-trust audiences remain fail-closed. - return profile.McpServersMode == ToolProfileMode.All; + return allowedTools.Contains(toolName.Value, StringComparer.Ordinal); } private static bool IsProfileManagedTool(ToolName toolName) diff --git a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs index be511f9ca..aa5b2815d 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs @@ -663,8 +663,6 @@ public async Task ReadMcpError_MalformedBodyFallsBackToHttpStatusAndReason() Assert.Equal("HTTP 502 Bad Gateway", message); } - // ── Posture-aware `mcp tools` grant/revoke/snapshot (issue #1959) ── - [Fact] public async Task Tools_Revoke_AllPosture_WritesDenyOverrideNotGrantAllowlist() { @@ -683,7 +681,6 @@ public async Task Tools_Revoke_AllPosture_WritesDenyOverrideNotGrantAllowlist() Assert.Equal( "Deny", personal.GetProperty("ApprovalPolicy").GetProperty("ToolOverrides").GetProperty("dropbox/delete").GetString()); - // In All posture the CLI must NOT write a closed allow-list. Assert.False(personal.TryGetProperty("McpServerToolGrants", out _)); } @@ -713,30 +710,28 @@ public async Task Tools_Grant_AllPosture_ClearsDenyOverride() } [Fact] - public async Task Tools_Revoke_AllowlistPosture_WritesGrantAllowlist() + public async Task Tools_Grant_AllPosture_PreservesApprovalOverride() { File.WriteAllText(_paths.NetclawConfigPath, """ { "configVersion": 1, - "Tools": { "AudienceProfiles": { "Team": { - "McpServersMode": "Allowlist", - "AllowedMcpServers": ["dropbox"] + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "ToolOverrides": { "dropbox/copy": "Approval" } } } } } } """); var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); var exitCode = await McpCommand.RunAsync( - ["mcp", "tools", "dropbox", "--revoke", "delete", "--audience", "team"], + ["mcp", "tools", "dropbox", "--grant", "copy", "--audience", "personal"], _paths, daemonApi, _output); Assert.Equal(0, exitCode); using var doc = ReadConfigFile(_paths.NetclawConfigPath); - var grants = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") - .GetProperty("Team").GetProperty("McpServerToolGrants").GetProperty("dropbox") - .EnumerateArray().Select(static e => e.GetString()).ToList(); - Assert.Contains("copy", grants); - Assert.DoesNotContain("delete", grants); + var overrides = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") + .GetProperty("Personal").GetProperty("ApprovalPolicy").GetProperty("ToolOverrides"); + Assert.Equal("Approval", overrides.GetProperty("dropbox/copy").GetString()); } [Fact] @@ -758,8 +753,6 @@ public async Task Tools_Snapshot_AllPosture_IsRejected() [Fact] public async Task Tools_Grant_AllPosture_OverServerDefaultDeny_WritesApprovalOverride() { - // Server default Deny hides every tool. `--grant` must actually enable the - // tool; clearing the override would inherit Deny, so it writes Approval. File.WriteAllText(_paths.NetclawConfigPath, """ { "configVersion": 1, @@ -782,26 +775,6 @@ public async Task Tools_Grant_AllPosture_OverServerDefaultDeny_WritesApprovalOve Assert.Equal("Approval", overrides.GetProperty("dropbox/copy").GetString()); } - [Fact] - public async Task Tools_Snapshot_NoAudience_AllPostureOnly_GivesAdditiveGuidance() - { - // The server is reachable only by Personal (All posture). A no-audience - // snapshot must not falsely report "not allowed by any audience profile". - File.WriteAllText(_paths.NetclawConfigPath, """ - { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } - """); - var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); - - var exitCode = await McpCommand.RunAsync( - ["mcp", "tools", "dropbox", "--snapshot"], - _paths, daemonApi, _output); - - Assert.Equal(1, exitCode); - var output = _output.ToString(); - Assert.Contains("All posture", output, StringComparison.Ordinal); - Assert.DoesNotContain("not allowed by any audience profile", output, StringComparison.Ordinal); - } - private static DaemonApi ToolsDaemonApi(string serverName, params string[] tools) { var body = JsonSerializer.Serialize(tools); diff --git a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs index 98e74aebb..7399f1766 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs @@ -5,13 +5,10 @@ // ----------------------------------------------------------------------- using System.Net.Http; using System.Text.Json; -using Microsoft.Extensions.AI; using Microsoft.Extensions.Configuration; -using Netclaw.Actors.Tools; using Netclaw.Cli.Daemon; using Netclaw.Cli.Mcp; using Netclaw.Configuration; -using Netclaw.Security; using Netclaw.Tests.Utilities; using Netclaw.Tools; using Xunit; @@ -419,10 +416,8 @@ public void ToggleTool_AllPosture_UncheckPersistsDenyOverrideNotGrantAllowlist() vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy", "delete" }); vm.SetSelectedAudienceForTests(TrustAudience.Personal); - // Personal is All posture: every tool starts enabled. Assert.True(vm.IsToolGranted(new ToolName("delete"))); - // Unchecking disables the tool by writing a Deny override. vm.ToggleTool(new ToolName("delete")); Assert.False(vm.IsToolGranted(new ToolName("delete"))); Assert.Equal(ToolApprovalMode.Deny, vm.GetEffectiveMode(new ToolName("delete")).Mode); @@ -433,7 +428,6 @@ public void ToggleTool_AllPosture_UncheckPersistsDenyOverrideNotGrantAllowlist() Assert.Equal( "Deny", personal.GetProperty("ApprovalPolicy").GetProperty("ToolOverrides").GetProperty("dropbox/delete").GetString()); - // The checkbox must NOT persist a closed McpServerToolGrants allow-list in All posture. Assert.False(personal.TryGetProperty("McpServerToolGrants", out _)); } @@ -444,15 +438,14 @@ public void ToggleTool_AllPosture_ReCheckClearsDenyOverride() vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); vm.SetSelectedAudienceForTests(TrustAudience.Personal); - vm.ToggleTool(new ToolName("copy")); // disable → Deny + vm.ToggleTool(new ToolName("copy")); Assert.False(vm.IsToolGranted(new ToolName("copy"))); - vm.ToggleTool(new ToolName("copy")); // re-enable → inherit server default + vm.ToggleTool(new ToolName("copy")); Assert.True(vm.IsToolGranted(new ToolName("copy"))); Assert.True(vm.Save()); var personal = GetAudienceProfile(JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)), "Personal"); - // Re-enabling clears the override, so no per-tool entry is persisted. Assert.False( personal.TryGetProperty("ApprovalPolicy", out var ap) && ap.TryGetProperty("ToolOverrides", out var overrides) @@ -460,17 +453,14 @@ public void ToggleTool_AllPosture_ReCheckClearsDenyOverride() } [Fact] - public void ToggleTool_AllPosture_EnablesOverServerDefaultDeny() + public void ToggleTool_AllPosture_EnablesOverDefaultDeny() { - // When the server default is Deny (all tools hidden by default), the - // checkbox must still be able to turn a single tool on. Clearing the - // override would inherit Deny, so enable writes an explicit Approval. File.WriteAllText(_paths.NetclawConfigPath, """ { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All", - "ApprovalPolicy": { "McpServerDefaults": { "dropbox": "Deny" } } + "ApprovalPolicy": { "DefaultMode": "Deny" } } } } } """); @@ -478,10 +468,8 @@ public void ToggleTool_AllPosture_EnablesOverServerDefaultDeny() vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); vm.SetSelectedAudienceForTests(TrustAudience.Personal); - // Server default Deny → the tool starts disabled (unchecked). Assert.False(vm.IsToolGranted(new ToolName("copy"))); - // Enabling must actually take effect (not a dead toggle). vm.ToggleTool(new ToolName("copy")); Assert.True(vm.IsToolGranted(new ToolName("copy"))); Assert.Equal(ToolApprovalMode.Approval, vm.GetEffectiveMode(new ToolName("copy")).Mode); @@ -494,7 +482,6 @@ public void ToggleTool_AllowlistPosture_StillWritesGrantAllowlist() vm.InitializeForTests(new McpServerName("notion"), new[] { "create-pages", "search" }); vm.SetSelectedAudienceForTests(TrustAudience.Team); - // Team is Allowlist posture. Enable the server, then uncheck one tool. vm.ToggleServerAccess(); vm.ToggleTool(new ToolName("search")); Assert.True(vm.IsToolGranted(new ToolName("create-pages"))); @@ -509,72 +496,6 @@ public void ToggleTool_AllowlistPosture_StillWritesGrantAllowlist() Assert.DoesNotContain("search", grants); } - [Fact] - public void ToggleTool_AllPosture_Deny_HidesToolFromRuntimePolicy() - { - // Cross-boundary contract: the TUI (producer) writes a Deny override; a - // runtime ToolAccessPolicy (consumer) built from that written config hides - // the tool. Proves the persisted representation matches the consumer. - // Seed a realistic Personal profile (All posture, as init writes it) so - // the reloaded config keeps the open posture. - File.WriteAllText(_paths.NetclawConfigPath, """ - { - "configVersion": 1, - "Tools": { - "AudienceProfiles": { - "Personal": { "McpServersMode": "All" } - } - } - } - """); - - var vm = CreateVm(); - vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy", "delete" }); - vm.SetSelectedAudienceForTests(TrustAudience.Personal); - - vm.ToggleTool(new ToolName("delete")); // disable via Deny - Assert.True(vm.Save()); - - var toolConfig = LoadToolConfigFromDisk(); - var policy = new ToolAccessPolicy( - toolConfig, - new EffectivePolicyDefaults( - DeploymentPosture.Personal, - TrustAudience.Personal, - ShellExecutionMode.HostAllowed, - UsedStrictFallback: false), - new ShellCommandPolicy(), - new ToolPathPolicy([])); - - var personalContext = new ToolInvocationContext( - new ToolRunScope - { - Session = new ToolSessionScope.Bound("slack/thread-1", null), - Audience = TrustAudience.Personal, - InlineOutputBudget = InlineOutputBudget.Default, - InteractiveApproval = new InteractiveApprovalCapability.Unavailable(), - }, - ToolExecutionTimeout.Default); - - Assert.False(policy.IsToolExposed(McpTool("dropbox", "delete"), personalContext)); // Deny → hidden - Assert.True(policy.IsToolExposed(McpTool("dropbox", "copy"), personalContext)); // enabled - } - - private static McpToolAdapter McpTool(string serverName, string toolName) - => new(AIFunctionFactory.Create(() => "result", toolName, toolName), serverName, toolName); - - private ToolConfig LoadToolConfigFromDisk() - { - using var doc = JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)); - var tools = doc.RootElement.GetProperty("Tools"); - return JsonSerializer.Deserialize( - tools.GetRawText(), - new JsonSerializerOptions - { - Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() } - })!; - } - private static void CycleServerDefault(McpToolPermissionsViewModel vm, bool reverse) { if (reverse) diff --git a/src/Netclaw.Cli/Mcp/McpCommand.cs b/src/Netclaw.Cli/Mcp/McpCommand.cs index 2705ae108..7677e6c5d 100644 --- a/src/Netclaw.Cli/Mcp/McpCommand.cs +++ b/src/Netclaw.Cli/Mcp/McpCommand.cs @@ -1209,16 +1209,13 @@ private static int RunToolsList( private static string FormatGrantStatus(McpServerName serverName, ToolName toolName, ToolAudienceProfile profile) { - // In All posture the grant list is additive: a tool is exposed unless its - // effective approval mode is Deny (the disable signal). This mirrors the - // runtime resolver and exposure gate. if (profile.McpServersMode == ToolProfileMode.All) { var mode = profile.ApprovalPolicy?.GetEffectiveMode($"{serverName.Value}/{toolName.Value}") ?? ToolApprovalMode.Auto; return mode == ToolApprovalMode.Deny - ? "- " // disabled via Deny - : "✱ "; // exposed (additive) + ? "- " + : "✱ "; } if (profile.McpServerToolGrants is null) @@ -1249,7 +1246,6 @@ private static int RunToolsSnapshot( }; var updated = 0; - var skippedAllPosture = false; foreach (var audienceName in audienceNames) { var profile = audienceName switch @@ -1259,20 +1255,15 @@ private static int RunToolsSnapshot( _ => profiles.Personal }; - // A snapshot pins the current tool set as a closed allow-list. That - // only makes sense in Allowlist posture. In All posture the grant - // list is additive, so a snapshot would not restrict anything; point - // the operator at --revoke, which disables a tool via a Deny override. if (profile.McpServersMode == ToolProfileMode.All) { if (targetAudience is not null) { - writer.WriteLine($"The {audienceName} audience exposes MCP servers in All posture; a snapshot is additive and would not restrict tools."); + writer.WriteLine($"The {audienceName} audience uses All posture, which does not use tool grants."); writer.WriteLine($"Use `netclaw mcp tools {serverName.Value} --revoke --audience {audienceName.ToLowerInvariant()}` to disable specific tools."); return 1; } - skippedAllPosture = true; continue; } @@ -1302,16 +1293,7 @@ private static int RunToolsSnapshot( if (updated == 0) { - if (skippedAllPosture) - { - writer.WriteLine($"Server '{serverName.Value}' is only reachable by audience(s) in All posture, where a snapshot is additive and would not restrict tools."); - writer.WriteLine($"Use `netclaw mcp tools {serverName.Value} --revoke --audience ` to disable specific tools."); - } - else - { - writer.WriteLine($"Server '{serverName.Value}' is not allowed by any audience profile. Nothing to snapshot."); - } - + writer.WriteLine($"No Allowlist audience permits server '{serverName.Value}'. Nothing to snapshot."); return 1; } @@ -1346,9 +1328,6 @@ private static int RunToolsGrantRevoke( return 1; } - // Open (All) posture: the grant list is additive, so writing an - // allow-list would be inert. "Disable" is a Deny approval override and - // "enable" clears it, matching the TUI and the runtime exposure gate. if (profile.McpServersMode == ToolProfileMode.All) { var (allConfig, _) = LoadConfigFiles(paths); @@ -1358,16 +1337,16 @@ private static int RunToolsGrantRevoke( var approvalPolicy = GetOrCreateSection(allAudienceSection, "ApprovalPolicy"); var toolOverrides = GetOrCreateSection(approvalPolicy, "ToolOverrides"); - // Enable = clear the override so the tool inherits the server - // default. But if the server default is itself Deny, clearing would - // leave the tool hidden; write an explicit Approval override instead - // (secure-by-default: exposed, but gated) so enable always takes effect. var serverDefaultIsDeny = InheritedServerMode(profile, serverName.Value) == ToolApprovalMode.Deny; if (grantTools is not null) foreach (var tool in grantTools) { var key = $"{serverName.Value}/{tool}"; + if (profile.ApprovalPolicy?.ToolOverrides.TryGetValue(key, out var currentMode) == true + && currentMode != ToolApprovalMode.Deny) + continue; + if (serverDefaultIsDeny) toolOverrides[key] = ToolApprovalMode.Approval.ToString(); else @@ -1392,7 +1371,6 @@ private static int RunToolsGrantRevoke( return 0; } - // Build the updated tool set (Allowlist posture: closed allow-list) HashSet currentTools; if (profile.McpServerToolGrants is { } existing && existing.TryGetValue(serverName.Value, out var currentList)) @@ -1449,9 +1427,6 @@ private static ToolAudienceProfile ResolveProfile(TrustAudience audience, ToolAu }; } - // The approval mode a tool with no explicit override inherits for this - // server: the server default, else the audience default, else Auto. Mirrors - // ToolApprovalConfig precedence for the server-level (non-tool) case. private static ToolApprovalMode InheritedServerMode(ToolAudienceProfile profile, string serverName) { var approvalPolicy = profile.ApprovalPolicy; @@ -1503,7 +1478,7 @@ private static int WriteHelp(TextWriter writer) writer.WriteLine("Flags for 'add':"); writer.WriteLine(" --grant-all CI escape hatch. Skip the empty-grants writes and leave tool"); writer.WriteLine(" grants null (legacy \"all pass\" behavior). Approval defaults"); - writer.WriteLine(" (Personal=Approval, Team=Approval, Public=Deny) are still written."); + writer.WriteLine(" (Personal=Auto, Team=Approval, Public=Deny) are still written."); writer.WriteLine(" --auth Start the OAuth flow immediately after adding (HTTP/SSE only)."); writer.WriteLine(" --client-id Pre-registered OAuth client ID for servers that do not support"); writer.WriteLine(" dynamic client registration."); diff --git a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs index dda8898fc..f3a7f328c 100644 --- a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs +++ b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs @@ -363,15 +363,10 @@ public ToolApprovalMode GetServerDefault() && approvalPolicy.McpServerDefaults.TryGetValue(SelectedServer, out var configMode)) return configMode; - return ToolApprovalMode.Auto; + return approvalPolicy?.DefaultMode ?? ToolApprovalMode.Auto; } - /// - /// True when the selected audience exposes MCP servers in - /// posture. In All posture the per-tool checkbox maps to the approval axis (Deny = - /// disabled) rather than the closed - /// allow-list, so a newly discovered tool stays enabled under the server default. - /// + /// Checks whether the selected profile exposes all MCP servers. private bool IsAllPostureForSelectedAudience() => ResolveProfile(SelectedAudience).McpServersMode == ToolProfileMode.All; @@ -382,11 +377,6 @@ public bool IsToolGranted(ToolName toolName) var audienceName = AudienceName(SelectedAudience); - // In All posture the checkbox means "not disabled": a tool is enabled - // unless its effective approval mode is Deny. Disable is expressed as a - // Deny override, so a newly discovered tool (no override) reads as enabled - // and inherits the server default. This mirrors the runtime resolver, - // which treats the grant list as additive in All posture. if (IsAllPostureForSelectedAudience()) { if (_pendingServerAccess.TryGetValue((audienceName, SelectedServer), out var pendingAllAccess) @@ -437,14 +427,16 @@ public void ToggleAll() var audienceName = AudienceName(SelectedAudience); - // In All posture, "disable all" writes a Deny override for each tool and - // "enable all" clears the overrides (inherit the server default). if (IsAllPostureForSelectedAudience()) { + var enabledMode = GetServerDefault() == ToolApprovalMode.Deny + ? ToolApprovalMode.Approval + : (ToolApprovalMode?)null; + foreach (var tool in DiscoveredTools) { _pendingToolOverrides[(audienceName, SelectedServer, tool)] = - anyGranted ? ToolApprovalMode.Deny : null; + anyGranted ? ToolApprovalMode.Deny : enabledMode; } NotifyStateChanged(); @@ -471,12 +463,6 @@ public void ToggleTool(ToolName toolName) var audienceName = AudienceName(SelectedAudience); - // In All posture the checkbox toggles the Deny approval override, not the - // allow-list. Enabled → set Deny (disable). Disabled → enable: clear the - // override so the tool inherits the server default — but if the server - // default is itself Deny, clearing would leave the tool hidden, so write - // an explicit Approval override (secure-by-default: exposed, but gated) - // so the checkbox always turns the tool on. if (IsAllPostureForSelectedAudience()) { var overrideKey = (audienceName, SelectedServer, toolName.Value); diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs deleted file mode 100644 index 5d3dc74fa..000000000 --- a/src/Netclaw.Daemon.Tests/Mcp/McpToolDriftTests.cs +++ /dev/null @@ -1,62 +0,0 @@ -// ----------------------------------------------------------------------- -// -// Copyright (C) 2026 - 2026 Petabridge, LLC -// -// ----------------------------------------------------------------------- -using Netclaw.Configuration; -using Netclaw.Daemon.Mcp; -using Xunit; - -namespace Netclaw.Daemon.Tests.Mcp; - -public sealed class McpToolDriftTests -{ - private static readonly McpServerName Server = new("dropbox"); - - [Fact] - public void AllPosture_GrantSnapshot_ProducesNoDrift() - { - var profiles = new ToolAudienceProfiles(); - // Personal is All posture by default. - profiles.Personal.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy"] - }; - - var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy", "get_upload_url"]); - - // The server added get_upload_url after the snapshot. In All posture the - // grant list is additive, so there is no drift to warn about. - Assert.Empty(report.Ungranted); - Assert.Empty(report.Stale); - } - - [Fact] - public void AllowlistPosture_DiscoveredToolMissingFromGrants_ReportsUngranted() - { - var profiles = new ToolAudienceProfiles(); - // Team is Allowlist posture by default. - profiles.Team.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy"] - }; - - var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy", "delete"]); - - Assert.Contains("delete", report.Ungranted); - } - - [Fact] - public void AllowlistPosture_GrantNamesMissingTool_ReportsStale() - { - var profiles = new ToolAudienceProfiles(); - profiles.Team.McpServerToolGrants = new Dictionary> - { - ["dropbox"] = ["copy", "gone"] - }; - - var report = McpClientManager.ComputeToolDrift(profiles, Server, ["copy"]); - - Assert.Contains("gone", report.Stale); - } -} diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index a5a04c9c1..74290070d 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -1954,73 +1954,47 @@ private static void WriteField(Stream stream, string value) private void LogToolDrift(McpServerName serverName, IReadOnlyList discoveredTools) { - var report = ComputeToolDrift( - _toolConfig.AudienceProfiles, - serverName, - discoveredTools.Select(t => t.Name).ToList()); + var profiles = _toolConfig.AudienceProfiles; + var allGrantedTools = new HashSet(StringComparer.Ordinal); + var hasAnyGrants = false; + + foreach (var profile in profiles.GetAllProfiles()) + { + if (profile.McpServersMode == ToolProfileMode.All + || profile.McpServerToolGrants is not { } grants + || !grants.TryGetValue(serverName.Value, out var tools)) + continue; + + hasAnyGrants = true; + foreach (var tool in tools) + allGrantedTools.Add(tool); + } + + if (!hasAnyGrants) + return; + + var discoveredNames = new HashSet( + discoveredTools.Select(t => t.Name), StringComparer.Ordinal); + var ungranted = discoveredNames.Except(allGrantedTools).ToList(); + var stale = allGrantedTools.Except(discoveredNames).ToList(); - if (report.Ungranted.Count > 0) + if (ungranted.Count > 0) { _logger.LogWarning( - "MCP server '{Name}' exposes {Count} tool(s) not granted to any allowlisted audience: {Tools}. " + + "MCP server '{Name}' exposes {Count} tool(s) not granted to any audience: {Tools}. " + "Review and add to McpServerToolGrants if intended.", - serverName.Value, report.Ungranted.Count, string.Join(", ", report.Ungranted)); + serverName.Value, ungranted.Count, string.Join(", ", ungranted)); } - if (report.Stale.Count > 0) + if (stale.Count > 0) { _logger.LogWarning( "McpServerToolGrants for '{Name}' contains {Count} tool(s) not found on server: {Tools}. " + "These may have been removed or renamed.", - serverName.Value, report.Stale.Count, string.Join(", ", report.Stale)); + serverName.Value, stale.Count, string.Join(", ", stale)); } } - /// - /// Drift between a server's discovered tools and its configured grants. - /// and are computed only from - /// Allowlist-posture grant lists, which alone close the tool set. All-posture - /// grant lists are additive (they no longer restrict tools), so they never - /// produce drift. - /// - internal sealed record ToolDriftReport( - IReadOnlyList Ungranted, - IReadOnlyList Stale); - - internal static ToolDriftReport ComputeToolDrift( - ToolAudienceProfiles profiles, - McpServerName serverName, - IReadOnlyList discoveredToolNames) - { - var allowlistGranted = new HashSet(StringComparer.Ordinal); - var hasAllowlistGrants = false; - - foreach (var profile in profiles.GetAllProfiles()) - { - if (profile.McpServerToolGrants is not { } grants - || !grants.TryGetValue(serverName.Value, out var tools)) - continue; - - // A grant list under All posture is additive, so it no longer hides - // ungranted tools. Only Allowlist-posture grant lists genuinely close - // the set, so only those drive the ungranted/stale drift warnings. - if (profile.McpServersMode == ToolProfileMode.All) - continue; - - hasAllowlistGrants = true; - foreach (var tool in tools) - allowlistGranted.Add(tool); - } - - if (!hasAllowlistGrants) - return new ToolDriftReport([], []); - - var discoveredNames = new HashSet(discoveredToolNames, StringComparer.Ordinal); - return new ToolDriftReport( - discoveredNames.Except(allowlistGranted).ToList(), - allowlistGranted.Except(discoveredNames).ToList()); - } - public void Dispose() { var emergencyCleanup = false; From 530ab7e73880dc0eeda3f33d2fee10cee3eb9ae4 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 18 Aug 2026 07:52:53 -0500 Subject: [PATCH 3/6] Fix MCP exact overrides in All mode --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/tools.md | 14 +++---- .../Tools/McpToolAudienceGrantsTests.cs | 2 +- src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs | 38 ++++++++++++++++--- .../Mcp/McpToolPermissionsViewModelTests.cs | 36 ++++++++++++++++-- src/Netclaw.Cli/Mcp/McpCommand.cs | 25 +++++++++++- .../Mcp/McpToolPermissionsViewModel.cs | 38 +++++++++++++++---- 7 files changed, 126 insertions(+), 29 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 81fed82f5..4e7b2e754 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.54.0" + version: "2.55.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index c99cd2222..a424b46e2 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -63,7 +63,7 @@ The command writes closed tool grants for the Team and Public audiences. | Audience | Grants | Approval default | |----------|--------|------------------| -| Personal | Not used (`All` posture) | `Auto` | +| Personal | Not used (`All` MCP server mode) | `Auto` | | Team | `[]` | `Approval` | | Public | `[]` | `Deny` | @@ -78,9 +78,9 @@ The option does not change the approval defaults. Inside the TUI (`netclaw mcp permissions`): -- `Enter` toggles the highlighted tool. In open (`All`) posture the toggle sets +- `Enter` toggles the highlighted tool. In the `All` MCP server mode, the toggle sets `Deny` (disabled) or clears it (inherit the server default). In `Allowlist` - posture it adds or removes the tool from the grant list. + mode, it adds or removes the tool from the grant list. - `A` toggles all tools on/off for the current audience - `E` enables/disables the whole server for the current audience - `M` cycles the **server default** approval mode (`Auto → Approval → Deny → Auto`) @@ -96,14 +96,14 @@ Approval-mode resolution precedence (for MCP tools): A tool with an effective `Deny` mode does not appear in the model tool list. -The MCP server posture controls tool grants: +The MCP server mode controls tool grants: - `All` does not use `McpServerToolGrants`. -- A new tool inherits the server approval default in `All` posture. +- A new tool inherits the server approval default in the `All` mode. - `Allowlist` exposes only the tools in `McpServerToolGrants`. - An exact tool override always takes precedence. -Use `--revoke` to write a `Deny` override in `All` posture. +Use `--revoke` to write a `Deny` override in the `All` mode. Use `--grant` to remove a `Deny` override or enable a tool above a `Deny` default. ### Existing MCP servers @@ -112,7 +112,7 @@ Servers added to `netclaw.json` before this behavior shipped stay untouched — their tool grants, `ApprovalPolicy.McpServerDefaults`, and `ToolOverrides` entries are not rewritten during an upgrade. -An existing grant snapshot has no effect when its audience uses `All` posture. +An existing grant snapshot has no effect when its audience uses the `All` mode. The update does not remove or replace any exact tool override. `netclaw doctor` will emit a warning for each enabled MCP server that diff --git a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs index 6acfe6b7e..8afe68a32 100644 --- a/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs +++ b/src/Netclaw.Actors.Tests/Tools/McpToolAudienceGrantsTests.cs @@ -380,7 +380,7 @@ public void McpServerToolGrants_DeserializesFromJson() [Theory] [InlineData(ToolApprovalMode.Auto, false)] [InlineData(ToolApprovalMode.Approval, true)] - public void AllPosture_NewTool_InheritsServerDefault( + public void AllMcpServersMode_NewTool_InheritsServerDefault( ToolApprovalMode serverDefault, bool needsApproval) { diff --git a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs index aa5b2815d..d65f31f98 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpCommandTests.cs @@ -664,7 +664,7 @@ public async Task ReadMcpError_MalformedBodyFallsBackToHttpStatusAndReason() } [Fact] - public async Task Tools_Revoke_AllPosture_WritesDenyOverrideNotGrantAllowlist() + public async Task Tools_Revoke_AllMcpServersMode_WritesDenyOverrideNotGrantAllowlist() { File.WriteAllText(_paths.NetclawConfigPath, """ { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } @@ -685,7 +685,7 @@ public async Task Tools_Revoke_AllPosture_WritesDenyOverrideNotGrantAllowlist() } [Fact] - public async Task Tools_Grant_AllPosture_ClearsDenyOverride() + public async Task Tools_Grant_AllMcpServersMode_ClearsDenyOverride() { File.WriteAllText(_paths.NetclawConfigPath, """ { @@ -710,7 +710,33 @@ public async Task Tools_Grant_AllPosture_ClearsDenyOverride() } [Fact] - public async Task Tools_Grant_AllPosture_PreservesApprovalOverride() + public async Task Tools_Grant_AllMcpServersMode_ClearsAliasDenyOverride() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "ToolOverrides": { "dropbox__delete": "Deny" } } + } } } + } + """); + var daemonApi = ToolsDaemonApi("dropbox", "copy", "delete"); + + var exitCode = await McpCommand.RunAsync( + ["mcp", "tools", "dropbox", "--grant", "delete", "--audience", "personal"], + _paths, daemonApi, _output); + + Assert.Equal(0, exitCode); + using var doc = ReadConfigFile(_paths.NetclawConfigPath); + var overrides = doc.RootElement.GetProperty("Tools").GetProperty("AudienceProfiles") + .GetProperty("Personal").GetProperty("ApprovalPolicy").GetProperty("ToolOverrides"); + Assert.False(overrides.TryGetProperty("dropbox/delete", out _)); + Assert.False(overrides.TryGetProperty("dropbox__delete", out _)); + } + + [Fact] + public async Task Tools_Grant_AllMcpServersMode_PreservesApprovalOverride() { File.WriteAllText(_paths.NetclawConfigPath, """ { @@ -735,7 +761,7 @@ public async Task Tools_Grant_AllPosture_PreservesApprovalOverride() } [Fact] - public async Task Tools_Snapshot_AllPosture_IsRejected() + public async Task Tools_Snapshot_AllMcpServersMode_IsRejected() { File.WriteAllText(_paths.NetclawConfigPath, """ { "configVersion": 1, "Tools": { "AudienceProfiles": { "Personal": { "McpServersMode": "All" } } } } @@ -747,11 +773,11 @@ public async Task Tools_Snapshot_AllPosture_IsRejected() _paths, daemonApi, _output); Assert.Equal(1, exitCode); - Assert.Contains("All posture", _output.ToString(), StringComparison.Ordinal); + Assert.Contains("All MCP server mode", _output.ToString(), StringComparison.Ordinal); } [Fact] - public async Task Tools_Grant_AllPosture_OverServerDefaultDeny_WritesApprovalOverride() + public async Task Tools_Grant_AllMcpServersMode_OverServerDefaultDeny_WritesApprovalOverride() { File.WriteAllText(_paths.NetclawConfigPath, """ { diff --git a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs index 7399f1766..40eafb540 100644 --- a/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs +++ b/src/Netclaw.Cli.Tests/Mcp/McpToolPermissionsViewModelTests.cs @@ -410,7 +410,7 @@ public void Save_DoesNotMutateTheLiveInMemoryProfile() } [Fact] - public void ToggleTool_AllPosture_UncheckPersistsDenyOverrideNotGrantAllowlist() + public void ToggleTool_AllMcpServersMode_UncheckPersistsDenyOverrideNotGrantAllowlist() { var vm = CreateVm(); vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy", "delete" }); @@ -432,7 +432,7 @@ public void ToggleTool_AllPosture_UncheckPersistsDenyOverrideNotGrantAllowlist() } [Fact] - public void ToggleTool_AllPosture_ReCheckClearsDenyOverride() + public void ToggleTool_AllMcpServersMode_ReCheckClearsDenyOverride() { var vm = CreateVm(); vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); @@ -453,7 +453,35 @@ public void ToggleTool_AllPosture_ReCheckClearsDenyOverride() } [Fact] - public void ToggleTool_AllPosture_EnablesOverDefaultDeny() + public void ToggleTool_AllMcpServersMode_RespectsAliasDenyOverride() + { + File.WriteAllText(_paths.NetclawConfigPath, """ + { + "configVersion": 1, + "Tools": { "AudienceProfiles": { "Personal": { + "McpServersMode": "All", + "ApprovalPolicy": { "ToolOverrides": { "dropbox__copy": "Deny" } } + } } } + } + """); + var vm = CreateVm(); + vm.InitializeForTests(new McpServerName("dropbox"), new[] { "copy" }); + vm.SetSelectedAudienceForTests(TrustAudience.Personal); + + Assert.False(vm.IsToolGranted(new ToolName("copy"))); + + vm.ToggleTool(new ToolName("copy")); + Assert.True(vm.IsToolGranted(new ToolName("copy"))); + Assert.True(vm.Save()); + + var personal = GetAudienceProfile(JsonDocument.Parse(File.ReadAllText(_paths.NetclawConfigPath)), "Personal"); + var overrides = personal.GetProperty("ApprovalPolicy").GetProperty("ToolOverrides"); + Assert.False(overrides.TryGetProperty("dropbox/copy", out _)); + Assert.False(overrides.TryGetProperty("dropbox__copy", out _)); + } + + [Fact] + public void ToggleTool_AllMcpServersMode_EnablesOverDefaultDeny() { File.WriteAllText(_paths.NetclawConfigPath, """ { @@ -476,7 +504,7 @@ public void ToggleTool_AllPosture_EnablesOverDefaultDeny() } [Fact] - public void ToggleTool_AllowlistPosture_StillWritesGrantAllowlist() + public void ToggleTool_AllowlistMcpServersMode_StillWritesGrantAllowlist() { var vm = CreateVm(); vm.InitializeForTests(new McpServerName("notion"), new[] { "create-pages", "search" }); diff --git a/src/Netclaw.Cli/Mcp/McpCommand.cs b/src/Netclaw.Cli/Mcp/McpCommand.cs index 7677e6c5d..83c620728 100644 --- a/src/Netclaw.Cli/Mcp/McpCommand.cs +++ b/src/Netclaw.Cli/Mcp/McpCommand.cs @@ -1259,7 +1259,7 @@ private static int RunToolsSnapshot( { if (targetAudience is not null) { - writer.WriteLine($"The {audienceName} audience uses All posture, which does not use tool grants."); + writer.WriteLine($"The {audienceName} audience uses the All MCP server mode, which does not use tool grants."); writer.WriteLine($"Use `netclaw mcp tools {serverName.Value} --revoke --audience {audienceName.ToLowerInvariant()}` to disable specific tools."); return 1; } @@ -1343,7 +1343,8 @@ private static int RunToolsGrantRevoke( foreach (var tool in grantTools) { var key = $"{serverName.Value}/{tool}"; - if (profile.ApprovalPolicy?.ToolOverrides.TryGetValue(key, out var currentMode) == true + var aliasKey = $"{serverName.Value}__{tool}"; + if (TryGetExactMcpOverride(profile.ApprovalPolicy, serverName.Value, tool, out var currentMode) && currentMode != ToolApprovalMode.Deny) continue; @@ -1351,11 +1352,16 @@ private static int RunToolsGrantRevoke( toolOverrides[key] = ToolApprovalMode.Approval.ToString(); else toolOverrides.Remove(key); + + toolOverrides.Remove(aliasKey); } if (revokeTools is not null) foreach (var tool in revokeTools) + { toolOverrides[$"{serverName.Value}/{tool}"] = ToolApprovalMode.Deny.ToString(); + toolOverrides.Remove($"{serverName.Value}__{tool}"); + } WriteConfigFile(paths.NetclawConfigPath, allConfig); @@ -1438,6 +1444,21 @@ private static ToolApprovalMode InheritedServerMode(ToolAudienceProfile profile, : approvalPolicy.DefaultMode; } + private static bool TryGetExactMcpOverride( + ToolApprovalConfig? policy, + string serverName, + string toolName, + out ToolApprovalMode mode) + { + if (policy is not null + && (policy.ToolOverrides.TryGetValue($"{serverName}/{toolName}", out mode) + || policy.ToolOverrides.TryGetValue($"{serverName}__{toolName}", out mode))) + return true; + + mode = default; + return false; + } + private static ToolConfig LoadToolConfig(NetclawPaths paths) { if (!File.Exists(paths.NetclawConfigPath)) diff --git a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs index f3a7f328c..b8f4081ad 100644 --- a/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs +++ b/src/Netclaw.Cli/Mcp/McpToolPermissionsViewModel.cs @@ -291,8 +291,11 @@ private void CycleToolOverrideCore(ToolName toolName, int direction) } else { - var exactKey = $"{SelectedServer}/{toolName.Value}"; - current = ResolveProfile(SelectedAudience).ApprovalPolicy?.ToolOverrides.TryGetValue(exactKey, out var configMode) == true + current = TryGetExactToolOverride( + ResolveProfile(SelectedAudience).ApprovalPolicy, + SelectedServer, + toolName.Value, + out var configMode) ? configMode : null; } @@ -329,8 +332,7 @@ private void CycleToolOverrideCore(ToolName toolName, int direction) } else if (approvalPolicy is not null) { - var exactKey = $"{SelectedServer}/{toolName.Value}"; - if (approvalPolicy.ToolOverrides.TryGetValue(exactKey, out var configExact)) + if (TryGetExactToolOverride(approvalPolicy, SelectedServer, toolName.Value, out var configExact)) return (configExact, false); } @@ -367,7 +369,7 @@ public ToolApprovalMode GetServerDefault() } /// Checks whether the selected profile exposes all MCP servers. - private bool IsAllPostureForSelectedAudience() + private bool UsesAllMcpServersMode() => ResolveProfile(SelectedAudience).McpServersMode == ToolProfileMode.All; public bool IsToolGranted(ToolName toolName) @@ -377,7 +379,7 @@ public bool IsToolGranted(ToolName toolName) var audienceName = AudienceName(SelectedAudience); - if (IsAllPostureForSelectedAudience()) + if (UsesAllMcpServersMode()) { if (_pendingServerAccess.TryGetValue((audienceName, SelectedServer), out var pendingAllAccess) && !pendingAllAccess) @@ -427,7 +429,7 @@ public void ToggleAll() var audienceName = AudienceName(SelectedAudience); - if (IsAllPostureForSelectedAudience()) + if (UsesAllMcpServersMode()) { var enabledMode = GetServerDefault() == ToolApprovalMode.Deny ? ToolApprovalMode.Approval @@ -463,7 +465,7 @@ public void ToggleTool(ToolName toolName) var audienceName = AudienceName(SelectedAudience); - if (IsAllPostureForSelectedAudience()) + if (UsesAllMcpServersMode()) { var overrideKey = (audienceName, SelectedServer, toolName.Value); if (GetEffectiveMode(toolName).Mode == ToolApprovalMode.Deny) @@ -639,20 +641,40 @@ private void SaveToolOverrides(Dictionary profilesSection) var (approvalSection, inMemoryPolicy) = GetOrCreateApprovalPolicy(profilesSection, audienceName); var toolOverrides = ConfigFileHelper.GetOrCreateSection(approvalSection, "ToolOverrides"); var exactKey = $"{serverName}/{toolName}"; + var aliasKey = $"{serverName}__{toolName}"; if (mode is null) { toolOverrides.Remove(exactKey); + toolOverrides.Remove(aliasKey); inMemoryPolicy.ToolOverrides.Remove(exactKey); + inMemoryPolicy.ToolOverrides.Remove(aliasKey); } else { toolOverrides[exactKey] = mode.Value.ToString(); + toolOverrides.Remove(aliasKey); inMemoryPolicy.ToolOverrides[exactKey] = mode.Value; + inMemoryPolicy.ToolOverrides.Remove(aliasKey); } } } + private static bool TryGetExactToolOverride( + ToolApprovalConfig? policy, + string serverName, + string toolName, + out ToolApprovalMode mode) + { + if (policy is not null + && (policy.ToolOverrides.TryGetValue($"{serverName}/{toolName}", out mode) + || policy.ToolOverrides.TryGetValue($"{serverName}__{toolName}", out mode))) + return true; + + mode = default; + return false; + } + public void DiscardChanges() { _pendingGrants.Clear(); From 271e1f8e937666e6ea8f17ba953984a1086b4b71 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 18 Aug 2026 07:55:11 -0500 Subject: [PATCH 4/6] Clarify MCP Allowlist messages --- .../skills/.system/files/netclaw-operations/references/tools.md | 2 +- src/Netclaw.Cli/Mcp/McpCommand.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index a424b46e2..c79aacf41 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -101,7 +101,7 @@ The MCP server mode controls tool grants: - `All` does not use `McpServerToolGrants`. - A new tool inherits the server approval default in the `All` mode. - `Allowlist` exposes only the tools in `McpServerToolGrants`. -- An exact tool override always takes precedence. +- Within the approval policy, an exact tool override wins over the server default. Use `--revoke` to write a `Deny` override in the `All` mode. Use `--grant` to remove a `Deny` override or enable a tool above a `Deny` default. diff --git a/src/Netclaw.Cli/Mcp/McpCommand.cs b/src/Netclaw.Cli/Mcp/McpCommand.cs index 83c620728..ed7262461 100644 --- a/src/Netclaw.Cli/Mcp/McpCommand.cs +++ b/src/Netclaw.Cli/Mcp/McpCommand.cs @@ -1299,7 +1299,7 @@ private static int RunToolsSnapshot( WriteConfigFile(paths.NetclawConfigPath, config); writer.WriteLine($"Snapshot complete: {discoveredTools.Count} tools from '{serverName.Value}' written to McpServerToolGrants for {updated} audience profile(s)."); - writer.WriteLine("New tools added by the server will not be exposed until you update the grants."); + writer.WriteLine("New tools stay hidden from these Allowlist audiences until you update the grants."); return 0; } From 1727b1a16e9045f13cc3ba159afdc05bc3327c64 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 18 Aug 2026 08:05:50 -0500 Subject: [PATCH 5/6] Update the operations skill version test --- src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs b/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs index 3b099c411..a1d5d2ecc 100644 --- a/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs +++ b/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs @@ -59,7 +59,7 @@ public void Operations_skill_and_project_reference_share_tool_and_directory_orde var skill = File.ReadAllText(Path.Combine(skillDirectory, "SKILL.md")); var projects = File.ReadAllText(Path.Combine(skillDirectory, "references", "projects.md")); - Assert.Contains("version: \"2.56.0\"", skill, StringComparison.Ordinal); + Assert.Contains("version: \"2.57.0\"", skill, StringComparison.Ordinal); Assert.Contains("use `file_read` for a known local file read", skill, StringComparison.Ordinal); Assert.Contains("use `web_search` for external discovery", skill, StringComparison.Ordinal); Assert.Contains("use `shell_execute` for local search", skill, StringComparison.Ordinal); From 78f5177ff8db92af9a3cb769269874ef25e50793 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Tue, 18 Aug 2026 08:11:33 -0500 Subject: [PATCH 6/6] Correct the MCP Allowlist guidance --- feeds/skills/.system/files/netclaw-operations/SKILL.md | 2 +- .../.system/files/netclaw-operations/references/tools.md | 5 +++-- src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index 4ded622c2..bf6168709 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.57.0" + version: "2.58.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/tools.md b/feeds/skills/.system/files/netclaw-operations/references/tools.md index c79aacf41..517a31a80 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/tools.md +++ b/feeds/skills/.system/files/netclaw-operations/references/tools.md @@ -100,13 +100,14 @@ The MCP server mode controls tool grants: - `All` does not use `McpServerToolGrants`. - A new tool inherits the server approval default in the `All` mode. -- `Allowlist` exposes only the tools in `McpServerToolGrants`. +- `Allowlist` applies `McpServerToolGrants` when the server has an entry. +- An absent server entry adds no per-tool filter. - Within the approval policy, an exact tool override wins over the server default. Use `--revoke` to write a `Deny` override in the `All` mode. Use `--grant` to remove a `Deny` override or enable a tool above a `Deny` default. -### Existing MCP servers +### MCP servers in old configurations Servers added to `netclaw.json` before this behavior shipped stay untouched — their tool grants, `ApprovalPolicy.McpServerDefaults`, and `ToolOverrides` diff --git a/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs b/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs index a1d5d2ecc..1f868b715 100644 --- a/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs +++ b/src/Netclaw.Daemon.Tests/BuiltInSkillSeedingTests.cs @@ -59,7 +59,7 @@ public void Operations_skill_and_project_reference_share_tool_and_directory_orde var skill = File.ReadAllText(Path.Combine(skillDirectory, "SKILL.md")); var projects = File.ReadAllText(Path.Combine(skillDirectory, "references", "projects.md")); - Assert.Contains("version: \"2.57.0\"", skill, StringComparison.Ordinal); + Assert.Contains("version: \"2.58.0\"", skill, StringComparison.Ordinal); Assert.Contains("use `file_read` for a known local file read", skill, StringComparison.Ordinal); Assert.Contains("use `web_search` for external discovery", skill, StringComparison.Ordinal); Assert.Contains("use `shell_execute` for local search", skill, StringComparison.Ordinal);