Configurable ability namespaces, isError on failed tool calls, misconfiguration warnings#40
Merged
Merged
Conversation
replaces hardcoded mainwp/ filter with an abilityNamespaces allowlist
(default ['mainwp']); non-primary namespaces get a {ns}__ tool prefix.
reverse tool->ability lookup now goes through a cache index.
Build the ability and tool name indices into locals and assign all cache state together, so a collision throw during a refresh leaves the existing index intact instead of serving a partially built one. Allow hyphenated namespaces in ability names so configs like acme-corp round-trip through execute, while forbidding leading and trailing hyphens in both namespace regexes and the settings schema. Drop abilities whose names fail the strict format check at fetch time with a logged warning, so a malformed upstream name never surfaces as an invalid MCP tool name; the execute-time check remains as a second layer. Correct the docs on what depends on the mainwp namespace staying configured: the site resource returns an error payload and site ID completions come back empty when it is filtered out. New tests cover collision-safe refresh, hyphenated namespace round-trips, multi-namespace tool surfacing, malformed-name filtering, tools and categories cache invalidation on namespace changes, env var dedup and error attribution, and the prefix-related category listing behavior. Sync package-lock.json to beta.3.
The changelog covers the configurable namespace work: the new abilityNamespaces setting, the removed toolNameToAbilityName export, and the cache and malformed-name hardening. The version stays at beta.3, the bump the feature commit already made over the published beta.2; the lockfile is synced to match.
Failed tool calls returned a normal MCP result with the error JSON in
content but no isError flag, so MCP clients and agent frameworks that
branch on isError treated failures as successes.
executeTool now returns the full CallToolResult shape ({ content,
isError? }) instead of bare content. isError: true is set on every
failed-call path: unknown tool, input validation failure, ability
execution failure, cancellation, safe-mode block, and confirmation
rejections (INVALID_PARAMETER, CONFLICTING_PARAMETERS, PREVIEW_REQUIRED,
PREVIEW_EXPIRED). CONFIRMATION_REQUIRED previews and idempotent NO_CHANGE
responses stay non-error since the call did what was asked. The JSON
error body in content is unchanged, so existing consumers still parse it.
Verified live over stdio against the testbed dashboard: unknown tool and
invalid input both return isError: true; successful calls carry no flag.
A server configured with a namespace allowlist that excludes 'mainwp'
(e.g. MAINWP_ABILITY_NAMESPACES="acme") booted clean, advertised zero
tools, and broke the built-in resources with nothing in the logs
explaining why.
Two warnings now cover this:
- Startup warns when 'mainwp' is not in abilityNamespaces, since the
mainwp://site/{id} resource calls mainwp/get-site-v1 and site ID
prompt completions call mainwp/list-sites-v1, both of which fail when
the namespace is filtered out (matches the 'Keep mainwp in the list'
section in docs/configuration.md).
- fetchAbilities warns when the namespace filter leaves zero abilities,
naming the configured namespaces and the pre-filter count.
Verified live: booting with MAINWP_ABILITY_NAMESPACES="acme" against
the testbed dashboard logs both warnings on stderr.
CodeRabbit pointed out the zero-abilities warning claimed a namespace mismatch even when the dashboard itself returned no abilities. An empty upstream now logs 'Dashboard returned no abilities' instead.
Tool counts had drifted: README said both 'Over 60 tools' and '64 tools', docs/configuration.md said 64, and the live count varies by dashboard version (the testbed currently exposes 62). Both docs now use the same 'around 60 tools' phrasing with a note that the exact count varies by Dashboard version, so the docs stop going stale on every dashboard release.
The entry covered only the namespace work. The combined PR ships the isError flag on failed tool calls and the startup warnings in the same release, so the changelog now mentions both.
CodeRabbit flagged that both namespace integration tests dropped the execution result. They now assert the parsed body and the absence of isError. The suggested input schemas were skipped: the server does not validate arguments against ability schemas at execute time, and schema-less abilities are a supported case.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
WalkthroughThis PR implements configurable ability namespace allowlisting for the MCP server. It adds a new ChangesAbility namespace allowlisting and tool execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
The
mainwpability namespace was hardcoded into the filter, the tool name conversion, and the reverse lookup, so abilities registered by third-party extensions under their own namespaces were invisible to the server. While smoke-testing the fix for that, two behavioral issues and a docs drift surfaced, and everything lands here together since it was validated as one build.Configurable namespaces.
config.abilityNamespaces(env varMAINWP_ABILITY_NAMESPACES, settings.json fieldabilityNamespaceswith schema support) defaults to["mainwp"]. The first entry is the primary namespace and keeps unprefixed tool names; abilities from other configured namespaces are exposed as{namespace}__{tool}so MCP tool names stay collision-free. Reverse lookup goes through an index built duringfetchAbilitiesthat throws on tool-name collisions rather than silently shadowing one ability under another's name. The ability and category caches carry a namespace signature so a config change invalidates them instead of serving stale, wrongly-filtered data. Documented in the README anddocs/configuration.md, including guidance to keepmainwpin the list because the built-inmainwp://site/{id}resource and site ID prompt completions callmainwp/get-site-v1andmainwp/list-sites-v1directly.isError on failed tool calls. Failed tool calls came back as ordinary MCP results: the error JSON sat in
content[0].text, theisErrorflag was missing, and any client or agent framework that branches onisErrortreated the failure as a success. The cause was thatexecuteToolcaught every error and serialized it into plain content, so theisError: truebranch in the CallTool handler never fired.executeToolnow returns the full CallToolResult shape ({ content, isError? }) and the handler passes it through. Every failed path setsisError: true: unknown tool, input validation failure, ability execution failure, cancellation, safe-mode blocks, and confirmation rejections (PREVIEW_REQUIRED,PREVIEW_EXPIRED,INVALID_PARAMETER,CONFLICTING_PARAMETERS).CONFIRMATION_REQUIREDpreviews and idempotentNO_CHANGEresponses remain ordinary results because those calls did what was asked. The JSON error bodies are byte-for-byte unchanged, so existing consumers keep parsing them. One judgment call worth a reviewer's eye:SAFE_MODE_BLOCKEDalso setsisError: true, since a refused call reads as a failed call under the MCP spec.Misconfiguration warnings. A server started with
MAINWP_ABILITY_NAMESPACES="acme"boots clean, advertises zero tools, and errors on the built-in resources, with nothing in the logs explaining why. Startup now warns whenmainwpis missing fromabilityNamespaces, and after the ability fetch a second warning fires when the namespace filter leaves zero abilities, naming the configured namespaces and the pre-filter count. An empty upstream logs its own message (Dashboard returned no abilities) so a dead API is distinguishable from a filter mismatch.Docs tool counts. The README said both "Over 60 tools" and "64 tools",
docs/configuration.mdsaid 64, and a live server currently exposes 62. The count moves with the dashboard version, so both files now say "around 60 tools" with a note that the exact count varies by Dashboard version.Testing. Unit and integration tests cover the namespace round-trips, every
isErrorpath and its absence on success, previews, andNO_CHANGE, and both warnings via the mockLogger pattern. Verified live over stdio against a real dashboard:tools/listcount_sites_v1(success)isErrorisError: true, code -32002get_site_v1withsite_id: -1isError: true, code -32602get_site_v1for nonexistent site 999999isError: true, upstreammainwp_site_not_founduser_confirmedwithout previewisError: true,PREVIEW_REQUIREDdelete_site_v1underMAINWP_SAFE_MODE=trueisError: true,SAFE_MODE_BLOCKEDMAINWP_ABILITY_NAMESPACES=acmefetchedCount: 64)To test the full build locally: check out this branch,
npm ci && npm run build, and point your MCP client atnode /path/to/mainwp-mcp/dist/index.js, or ask for thenpm packtarball.Summary by CodeRabbit
New Features
abilityNamespacessetting to control which tools are exposed.{namespace}__toolnamenaming convention.Bug Fixes
isErrorflag.Documentation