fix(approvals): show MCP arguments in prompts#1689
Merged
Aaronontheweb merged 3 commits intoJul 18, 2026
Merged
Conversation
There was a problem hiding this comment.
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
McpApprovalMatcherto 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-operationsskill 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.
Aaronontheweb
disabled auto-merge
July 18, 2026 01:43
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1685.
Summary
approve_everywherewire key while presenting its MCP semantics asAlways allow this tool, including after actor recoveryMcpToolAdapterandToolAccessPolicyExample
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 passeddotnet slopwatch analyze— 0 issuespwsh ./scripts/Add-FileHeaders.ps1 -Verifygit diff upstream/dev...HEAD --checkThe model-backed eval suite could not start in this environment because
NETCLAW_EVAL_PROVIDER_TYPE,NETCLAW_EVAL_PROVIDER_ENDPOINT, andNETCLAW_EVAL_MODEL_IDare not configured.