Skip to content

fix(approvals): show MCP arguments in prompts#1689

Merged
Aaronontheweb merged 3 commits into
netclaw-dev:devfrom
Aaronontheweb:fix/mcp-approval-argument-preview
Jul 18, 2026
Merged

fix(approvals): show MCP arguments in prompts#1689
Aaronontheweb merged 3 commits into
netclaw-dev:devfrom
Aaronontheweb:fix/mcp-approval-argument-preview

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Collaborator

Closes #1685.

Summary

  • add an MCP-specific approval matcher that renders bounded, JSON-safe argument previews instead of repeating only the tool name
  • prioritize path, directory, file, URL, and URI arguments; summarize large strings, collections, and binary values; redact secret-like fields
  • strip Netclaw metadata schema-aware before building the operator-facing preview, while preserving declared MCP parameters with similar names
  • make MCP approval chrome describe a tool invocation rather than shell patterns or working-directory scope across CLI chat, Slack, Discord, and Mattermost
  • keep the stable approve_everywhere wire key while presenting its MCP semantics as Always allow this tool, including after actor recovery
  • add collapsed and expanded headless CLI snapshots generated through McpToolAdapter and ToolAccessPolicy
  • update the bundled operations skill with the new approval behavior

Example

MCP tool approval required: Dropbox/upload.
Invocation: Dropbox/upload(destination_directory="/Finance/Board Pack/2026/Q3", ...)
1. Once
2. This chat
3. Always allow this tool
4. Deny

Large content is summarized by character and line count, while secret values are redacted.

Validation

  • dotnet test Netclaw.slnx --no-restore
  • ./scripts/smoke/run-smoke.sh light — 22 tapes and 9 scenarios passed
  • dotnet slopwatch analyze — 0 issues
  • pwsh ./scripts/Add-FileHeaders.ps1 -Verify
  • git diff upstream/dev...HEAD --check

The model-backed eval suite could not start in this environment because NETCLAW_EVAL_PROVIDER_TYPE, NETCLAW_EVAL_PROVIDER_ENDPOINT, and NETCLAW_EVAL_MODEL_ID are not configured.

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves MCP tool approval prompts by including a bounded, redacted preview of MCP invocation arguments (instead of only the tool name), and updates all channel renderers to present MCP approvals as “tool invocations” with MCP-appropriate option labels.

Changes:

  • Add McpApprovalMatcher to render JSON-safe-ish, size-bounded argument previews with locator-first ordering and secret redaction, and strip Netclaw meta fields before formatting.
  • Update CLI TUI + Slack/Discord/Mattermost approval chrome to use MCP-specific titles/labels (“Invocation”, “Always allow this tool”) and suppress shell-specific patterns/verbs UI.
  • Add/extend approval-gate, matcher, channel, and snapshot tests; bump netclaw-operations skill guidance.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/Netclaw.Tools.Abstractions/ToolName.cs Adds ToolName.IsMcp to distinguish MCP tool names ({server}/{tool}) for UI decisions.
src/Netclaw.Security/SecretOutputRedactor.cs Introduces IsSecretKey helper used to redact secret-like argument keys.
src/Netclaw.Security/IToolApprovalMatcher.cs Adds McpApprovalMatcher that formats bounded, redacted MCP argument previews.
src/Netclaw.Security.Tests/ShellApprovalMatcherTests.cs Adds McpApprovalMatcherTests covering ordering, truncation, JSON element handling, redaction, and error cases.
src/Netclaw.Cli/Tui/ChatViewModel.cs MCP-specific approval summary/body formatting (“MCP tool approval required”, “Invocation”).
src/Netclaw.Cli/Tui/ChatPage.cs MCP-specific approval rendering in chat history (tool line + invocation line; suppress patterns).
src/Netclaw.Cli.Tests/Tui/ChatPageTests.cs Adds headless snapshot coverage for large MCP approvals through McpToolAdapter + ToolAccessPolicy.
src/Netclaw.Cli.Tests/Tui/snapshots/chat-mcp-approval-large-expanded.txt New expanded MCP approval snapshot.
src/Netclaw.Cli.Tests/Tui/snapshots/chat-mcp-approval-large-collapsed.txt New collapsed MCP approval snapshot.
src/Netclaw.Channels.Slack/SlackThreadBindingActor.cs Uses MCP-aware option labeling when rebuilding pending approval options.
src/Netclaw.Channels.Slack/SlackApprovalBlockBuilder.cs MCP-specific titles/labels (“Invocation”), and suppress shell verb bullets/scope chrome for MCP.
src/Netclaw.Channels.Mattermost/MattermostIngressMessages.cs Uses MCP-aware option labeling when reconstructing approval options.
src/Netclaw.Channels.Mattermost/MattermostApprovalPromptBuilder.cs MCP-specific approval/resolution wording and option labels; suppress patterns for MCP.
src/Netclaw.Channels.Discord/DiscordIngressMessages.cs Uses MCP-aware option labeling when reconstructing approval options.
src/Netclaw.Channels.Discord/DiscordApprovalPromptBuilder.cs MCP-specific approval/resolution wording and option labels; suppress patterns/verblists for MCP.
src/Netclaw.Actors/Tools/ToolAccessPolicy.cs Wires MCP approvals through meta-field stripping + McpApprovalMatcher; adjusts option-building for MCP semantics.
src/Netclaw.Actors/Sessions/LlmSessionActor.cs Uses MCP-aware option labels when parsing/constructing approval options from text replies.
src/Netclaw.Actors/Protocol/ApprovalOptionKeys.cs Adds context-aware LabelFor(optionKey, isMcpTool) and ApproveMcpToolLabel.
src/Netclaw.Actors.Tests/Tools/ToolApprovalGateTests.cs Verifies MCP approval display strips Netclaw meta fields, preserves colliding declared params, and uses MCP option labeling.
src/Netclaw.Actors.Tests/Channels/SlackApprovalBlockBuilderTests.cs Adds MCP prompt/resolution assertions for Slack formatting.
src/Netclaw.Actors.Tests/Channels/MattermostApprovalPromptBuilderTests.cs Adds MCP prompt/resolution assertions for Mattermost formatting.
src/Netclaw.Actors.Tests/Channels/DiscordApprovalPromptBuilderTests.cs Adds MCP prompt/resolution assertions for Discord formatting.
feeds/skills/.system/files/netclaw-operations/SKILL.md Documents MCP approval prompt behavior and bumps skill version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Netclaw.Security/IToolApprovalMatcher.cs Outdated
Comment thread src/Netclaw.Actors/Tools/ToolAccessPolicy.cs Outdated
@Aaronontheweb
Aaronontheweb disabled auto-merge July 18, 2026 01:43
@Aaronontheweb Aaronontheweb added mcp Model context protocol server / client issues. UX/DX UI / UX / DX friction issue or user-facing annoyances. labels Jul 18, 2026
@Aaronontheweb
Aaronontheweb merged commit 310ffea into netclaw-dev:dev Jul 18, 2026
15 checks passed
@Aaronontheweb
Aaronontheweb deleted the fix/mcp-approval-argument-preview branch July 18, 2026 13:24
Aaronontheweb added a commit that referenced this pull request Jul 18, 2026
….0-alpha.onnx.8 (#1695)

* release: prepare 0.25.0-beta.5 (#1666)

* fix(daemon): bound the daemon-stop session drain and give CLI stop kill headroom (#1673)

Two related shutdown-race defects:

1. The SIGTERM/daemon-stop CoordinatedShutdown drain task called
   SessionDrainHelper.DrainAsync with CancellationToken.None for the
   operation token, so a session parked on interactive tool approval
   (which can never ack) hung the call for the full 200s Akka
   before-service-unbind phase timeout, leaking the abandoned drain
   task. Fixed by bounding the drain with a TimeProvider-driven CTS
   sized to DaemonConfig.BoundedDrainTimeout (GracefulShutdownBudget
   minus a 10s safety margin), so it always finishes before the phase
   timeout fires. The drain's existing timeout logging now also lists
   which sessions didn't drain.

2. `netclaw daemon stop` waited only 10s before force-killing, and
   even after fixing that to match the daemon's own 200s phase
   timeout, the CLI and daemon would race exactly at the boundary
   (production evidence: a daemon force-killed ~100ms from a clean
   exit). Fixed by giving the CLI a 15s grace window to poll for exit
   after its budget elapses before escalating to SIGKILL, and adding
   TimeoutStopSec= to the generated systemd unit so systemd itself
   can't SIGKILL the cgroup out from under a still-waiting
   `netclaw daemon stop` (ExecStop=).

All the shutdown-timing constants now live in one place
(DaemonConfig.GracefulShutdownBudget and friends) so the layering
(bounded drain < Akka phase timeout < CLI budget+grace <
systemd TimeoutStopSec) can't drift out of lockstep again.

Fixes #1664, #1665

* fix(cli): reject numeric model modalities (#1677)

* fix(cli): handle model migration errors without crashing (#1678)

* fix(cli): handle model migration validation errors

* test(configuration): use non-resetting path joins

* fix(cli): report unresolved model references cleanly (#1680)

* chore(deps): bump actions/setup-dotnet from 5 to 6 (#1684)

Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 5 to 6.
- [Release notes](https://github.com/actions/setup-dotnet/releases)
- [Commits](actions/setup-dotnet@v5...v6)

---
updated-dependencies:
- dependency-name: actions/setup-dotnet
  dependency-version: '6'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* test(tools): remove obsolete shell timeout race (#1688)

* Fix authoritative attachment path guidance (#1686)

* fix(channels): clarify authoritative attachment paths

* chore: remove unnecessary openspec artifacts

* test(evals): cover authoritative attachment paths

* test(evals): stage attachment path fixture

* test(sessions): register session pipeline in shared fixture (#1690)

* test(sessions): make tool result assertion deterministic

* test(sessions): restore tool result regression coverage

* fix(approvals): show MCP arguments in prompts (#1689)

* fix(approvals): show MCP arguments in prompts

* fix(approvals): harden MCP invocation previews

* feat(install): automate shell PATH integration (#1687)

* feat(install): automate shell PATH integration for Unix installers

Replace manual PATH instructions with automatic shell profile modification.
The installer now detects the user's shell and writes a self-guarding env
script (~/.netclaw/env) that is sourced from the appropriate RC file.

Key changes:
- Add --skip-shell flag to opt out of automatic shell modification
- Detect shell via $SHELL (bash, zsh, fish supported; others get manual instructions)
- Write ~/.netclaw/env with colon-affixed case guard (rustup/fzf pattern)
- Append source line to correct RC: ~/.bashrc (Linux), ~/.profile (macOS bash),
  ~/.zshrc (zsh with ZDOTDIR support), ~/.config/fish/conf.d/netclaw.fish (fish)
- Duplicate prevention: grep -xF guard before appending to RC
- RC file validated with bash -n after modification

Smoke tests (section 9):
- Separate tests for bash-linux, zsh, and fish RC modification
- --skip-shell flag: verify no RC/env file created
- Unknown shell: verify graceful fallback message
- ZDOTDIR: verify zsh respects ZDOTDIR for RC location
- Duplicate prevention: second install adds no extra source lines

* feat(install): automate PATH modification for Windows installer

Replace manual PATH instructions with automatic User-scope PATH modification.
The installer now prepends the install dir to the User PATH and broadcasts
WM_SETTINGCHANGE to Explorer so new terminal windows pick up the change.

Key changes:
- Add -SkipShell switch to opt out of automatic PATH modification
- Read User-scope PATH (not merged $env:PATH) to avoid Machine entry corruption
- Prepend install dir to User PATH (highest priority)
- Update current session's $env:PATH
- Broadcast WM_SETTINGCHANGE via P/Invoke SendMessageTimeout
- Guard against duplicates: normalize trailing backslash before comparison
- Check 32K character limit before writing, warn if near limit

Smoke tests:
- PATH automation logic: duplicate detection with fake PATH strings
- Trailing backslash normalization
- Null User PATH handled as empty
- -SkipShell flag: verify install completes with skipped message

* fix(smoke): isolate real install test to temp HOME

The real install test now uses a temp HOME directory so shell integration
writes to the temp dir instead of the CI runner's real profile. This also
lets us verify the env script was created and the RC file was modified.

Adds 2 new assertions: env script exists, RC file sources env script.

* fix(smoke): cross-platform install smoke test fixes

- bash test: select .profile on macOS (login shell) vs .bashrc on Linux
- fish test: unset XDG_CONFIG_HOME so config path stays within temp HOME
- fish test: add duplicate prevention and re-install assertions
- Windows: remove false-positive regex check on success message

* fix(smoke): use @() array coercion for empty PATH entries count

$entries4 is a 0-element array which PowerShell can coerce to $null in
some contexts, causing '$entries4.Count' to throw ParentContainsErrorRecordException.
$(@entries4).Count forces array coercion so .Count always works.

* fix(install): validate PATH integration through real shells

* fix(install): make manifest fallback portable

* Harden installer PATH mutations

* fix(smoke): compare persisted physical paths

* fix(installer): harden shell path mutation

* test(installer): support Windows PowerShell path joins

* test(installer): normalize manifest fixture encoding

* test(installer): capture expected PowerShell failures

* release: prepare 0.25.0 stable (#1692)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mcp Model context protocol server / client issues. UX/DX UI / UX / DX friction issue or user-facing annoyances.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP tool approval prompts show no arguments — just the tool name

2 participants