fix(mcp): accept either tool-name form in config, CLI, and doctor#1154
Merged
Merged
Conversation
PR netclaw-dev#1153 split the canonical/LLM-facing tool name distinction along the runtime audience boundary, but three operator-facing surfaces still matched the exact stored string and would silently ignore a mis-typed form: - `ToolApprovalConfig.TryGetExplicitMode` (runtime override lookup): if the operator pasted `notion__create-pages` into ToolOverrides (the LLM-facing alias seen in audit logs or transcripts), the runtime queried with the canonical `notion/create-pages`, missed the entry, and the tool fell through to DefaultMode — a silent security misconfiguration. - `netclaw approvals revoke --tool <name>` (and `list --tool`, and `trust-verb --tool`): the on-disk store is keyed by canonical names. A `--tool notion__create-pages` flag matched no grant and the CLI reported "no approvals found" despite the grant being there. - `ToolAudienceProfilesDoctorCheck`: the missing-approval-default scan only recognized per-tool overrides with the `{server}/` prefix. An operator with `notion__create-pages` overrides was warned to add an approval default they had already configured — except the entries themselves were dead (see TryGetExplicitMode above). Fix: introduce `LlmFacingToolName.TryReverseSanitizedToCanonical` — a pure-string heuristic that maps `{server}__{tool}` back to `{server}/{tool}`. By convention first-party tool names never contain `__`, so the heuristic is unambiguous in practice; a future first-party tool that legitimately needs `__` would require the helper to grow a registry-aware overload (documented in remarks). - ToolApprovalConfig.TryGetExplicitMode inlines the equivalent reverse-resolution (no Tools.Abstractions dep from Configuration) so the runtime override lookup tries both forms. - ApprovalsCommand normalizes the `--tool` flag through TryReverseSanitizedToCanonical for all four call sites: list filter, revoke single-pattern filter, revoke-all store call, and trust-verb persistence. The trust-verb path persists under the canonical name so the runtime gate finds the grant. - ToolAudienceProfilesDoctorCheck accepts both `{server}/` and `{server}__` prefixes when detecting per-tool overrides. Tests added: - `LlmFacingToolName.TryReverseSanitizedToCanonical_*` — sanitized → canonical mapping, identity for first-party names and already- canonical inputs, null for malformed `__foo` / `foo__`. - `TryGetExplicitMode_finds_override_written_with_LlmFacing_key` — the runtime lookup honors the alias key. - `TryGetExplicitMode_canonical_override_still_wins_when_both_forms_present` — exact match always takes precedence. - `Revoke_all_resolves_LlmFacing_alias_to_canonical_stored_key` and `List_filter_by_LlmFacing_alias_matches_canonical_stored_key`. - `TrustVerb_persists_under_canonical_name_when_passed_LlmFacing_alias` — operators get a grant that the runtime gate can actually use. - `McpServerWithPerToolOverrideUnderLlmFacingKey_DoesNotTriggerWarning` — doctor matches the same shape the runtime accepts. 1968 Actors + 743 Cli + 333 Configuration tests pass; slopwatch clean; headers verified.
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.
Summary
Follow-up to #1153. That PR fixed the LLM-vs-operator audience split for the runtime hot path, but three operator-facing surfaces still did exact-string matching on tool names and would silently ignore the LLM-facing alias if an operator pasted it:
ToolApprovalConfig.TryGetExplicitMode— runtime override lookup. An operator who wrotenotion__create-pages(the form they saw in audit logs or transcripts before refactor(mcp): make canonical/LLM-facing tool name boundary type-enforced #1153 tightened the surfaces) got no override applied; the tool silently fell through toDefaultMode. Security misconfiguration risk.netclaw approvalsCLI (list --tool,revoke --tool,revoke --all --tool,trust-verb --tool) — the on-disk store is canonical-keyed; the LLM-facing alias passed as--toolmatched nothing.trust-verb --tool notion__create-pages ...would also persist under the alias key, creating a dead grant the runtime gate would never find.ToolAudienceProfilesDoctorCheck— the "MCP server reachable on Personal without approval default" scan only recognized{server}/prefixes inToolOverrides. Operators with sanitized-form override keys were warned to add coverage they had already (futilely) configured.Fix
Pure-string helper on
LlmFacingToolName:Returns the canonical form (
{server}/{tool}) when the input looks like the sanitized alias ({server}__{tool}); returnsnullfor first-party names, already-canonical inputs, and malformed shapes (__foo,foo__). The convention "first-party tool names never contain__" makes this unambiguous in practice; a future first-party tool that legitimately needs__would need the helper to grow a registry-aware overload (documented in remarks).Then:
ToolApprovalConfig.TryGetExplicitModeinlines the same reverse-resolution (noTools.Abstractionsdep fromConfiguration— wrong layer direction) so the runtime hot-path lookup tries both forms with the canonical-exact-match keeping precedence when both shapes exist.ApprovalsCommandnormalizes the--toolflag at all four call sites: list filter, revoke single-pattern filter, revoke-all store call, and trust-verb persistence (canonical only — so the grant the operator just made is something the runtime can actually consume).ToolAudienceProfilesDoctorCheckaccepts both{server}/and{server}__prefixes when scanningToolOverrides.Test plan
dotnet build— cleandotnet test— 1968 Actors / 743 Cli / 333 Configuration tests passLlmFacingToolName.TryReverseSanitizedToCanonical_*— round-trip for MCP aliases, identity for first-party / canonical / empty, null for malformed shapesToolApprovalConfig.TryGetExplicitMode— alias key resolved, canonical-exact-match precedenceApprovalsCommand— list filter, revoke-all, trust-verb persistence under canonicalToolAudienceProfilesDoctorCheck— alias-prefix coveragedotnet slopwatch analyze— no new violations./scripts/Add-FileHeaders.ps1 -Verify— all files have headersOut of scope
JSON schema (
netclaw-config.v1.schema.json) still validatesToolOverrideskeys only by value-shape — neither form is rejected. With this PR both forms work at runtime, so schema-level enforcement is no longer urgent. Doctor remains the surface where misuse would be flagged; a future--fixresolver could rewrite alias keys to canonical for operator hygiene.