Skip to content

fix(mcp): say why a tool was skipped on import - #849

Merged
CybotTM merged 1 commit into
mainfrom
fix/mcp-import-reason
Aug 20, 2026
Merged

fix(mcp): say why a tool was skipped on import#849
CybotTM merged 1 commit into
mainfrom
fix/mcp-import-reason

Conversation

@CybotTM

@CybotTM CybotTM commented Aug 20, 2026

Copy link
Copy Markdown
Member

Reported from the demo instance: the MCP module's last import error read

"ask_question" has no usable parameter schema and was skipped.

That sentence is true and gives the operator nothing. It does not separate a malformed schema from an oversized one from a well-formed one this import deliberately refuses, and there is no way to tell which of the three happened without reading the normaliser.

What it says now: the reason, and for the common case the keyword whose removal would widen what the tool accepts:

"ask_question" was skipped: it uses "anyOf", which this import does not carry: dropping the keyword would widen what the tool accepts, letting a model produce arguments the server then rejects.

Verified against the real server. https://mcp.deepwiki.com/mcp declares ask_question's repoName as anyOf[string, array<string>]; its two sibling tools import fine, which is exactly the "two imported DeepWiki tools" the demo seeds. The new unit test carries that schema verbatim rather than a constructed one.

The rejection rules are unchanged. rejectionReason() mirrors normalise() step for step, in the same order, on the same data — including that the keyword walk runs over the FILTERED schema. A keyword sitting in a key that gets dropped anyway (an examples block, say) is therefore never reported as the cause; there is a test for that, because a plausible-but-wrong reason would be worse than the vague one it replaces.

Whether the union should be refused at all is a separate question, filed as #848 rather than decided here.

Tests: three new unit tests (real schema refused with the keyword named, a passing sibling schema reports no reason, a keyword in a dropped key is not blamed). Gates run locally: cgl, PHPStan level 10, the normaliser unit suite, and the functional McpImport suite on sqlite — all green; CI covers the eight-cell matrix.

Assisted by claude-code:claude-fable-5 — Session

"has no usable parameter schema" is true and unactionable: it does not
separate a malformed schema from an oversized one from a well-formed one
this import deliberately refuses. The report now names the reason, and
for the common case the keyword whose removal would widen what the tool
accepts.

Concretely: DeepWiki's ask_question declares repoName as string-or-list
via anyOf, so the whole tool is refused while its two siblings import.
The operator saw only that something was unusable.

The rejection rules are unchanged. rejectionReason() mirrors normalise()
step for step and walks the FILTERED schema, so a keyword sitting in a
key that is dropped anyway is never reported as the cause.

Assisted-by: claude-code:claude-fable-5
Agent-Session: https://claude.ai/code/session_0144iD1P22LotW8rxmxrNGro
Agent-Host: 0493f0
Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
Copilot AI lite review requested due to automatic review settings August 20, 2026 17:34
@CybotTM
CybotTM requested a review from a team as a code owner August 20, 2026 17:34
@github-actions github-actions Bot added documentation Improvements or additions to documentation tests Test-related changes labels Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.74%. Comparing base (38408a9) to head (6c84fdc).

Files with missing lines Patch % Lines
Classes/Service/Tool/Mcp/McpSchemaNormalizer.php 82.05% 7 Missing ⚠️
Classes/Service/Tool/Mcp/McpImportService.php 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #849      +/-   ##
============================================
- Coverage     86.75%   86.74%   -0.02%     
- Complexity     9880     9899      +19     
============================================
  Files           573      573              
  Lines         31949    31991      +42     
============================================
+ Hits          27717    27749      +32     
- Misses         4232     4242      +10     
Flag Coverage Δ
unit 65.10% <74.41%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Classes/Service/Tool/Mcp/McpImportService.php 85.26% <0.00%> (-2.79%) ⬇️
Classes/Service/Tool/Mcp/McpSchemaNormalizer.php 92.55% <82.05%> (-7.45%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@CybotTM

CybotTM commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

Self-review: 6c84fdc

The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation tests Test-related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants