feat(mcp): import a catalogue from the CLI, so a deploy needs no click - #863
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
============================================
- Coverage 86.70% 86.50% -0.20%
- Complexity 9926 9947 +21
============================================
Files 574 575 +1
Lines 32080 32154 +74
============================================
Hits 27814 27814
- Misses 4266 4340 +74
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
59e178a to
04ec820
Compare
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Review record — no Copilot review exists (monthly, account-wide quota), so the green gate is not a read of this change. The premise I checked before writing a line. The issue says the command must be "the module's button, not a second path". That is only true if the controller adds nothing, so I read Three decisions the issue left open, each with its reason in the body: identifier rather than uid and no new repository method; Functional rather than unit, and not by preference. The importer and both repositories are Three controls, each observed: One trap I hit and left documented in the test. Rector: this is the PR where I paid for skipping my own note. I ran cgl/phpstan/unit/functional at 8.4 and reported Rector as CI's to judge; CI failed on one
Not merging on this record — I hold no merge authorisation for this PR. Assisted by claude-code:claude-opus-5 — Session |
|
Self-review: 04ec820 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. |
A seeded MCP server had a record and no tools until somebody opened the module and pressed Import catalogue. On an instance rebuilt by a deploy that is a manual step after every fresh install, and the server supplies nothing until it happens. The command is the module's button, not a second path. It calls the same McpImportService::import() and adds no policy: every refusal reason, the SSRF gate, the ADR-170 operation budget and the catalogue reconciliation stay where they were. Verified before writing it — the controller does nothing but resolve the record and format the report. Servers are named by identifier, not uid: a uid is not knowable to whoever writes the deploy script. That needs no repository method. Identifiers are deliberately NOT unique in the table — soft-deleted rows keep theirs, which is why the service refuses on a twin rather than relying on an index — so the ambiguity is reachable here too and is reported once, naming both, instead of resolved by picking a row. --all walks findEnabled() rather than findUsable(): a server without a data class should get the service's reason, not be skipped in silence. A refusing server does not stop the others and does fail the run, and an installation with no enabled server exits zero, because a deploy runs --all unconditionally and a valid empty state must not go red. Registered explicitly although autoconfigure would have picked up the attribute, because schedulable is a decision: this is the only one of the four that is schedulable. The import is idempotent, and a catalogue changes on the server's schedule rather than ours. Functional rather than unit, and not by preference: the importer and both repositories are `final readonly`, so a faked import service is not constructible — and the acceptance criterion that matters most, that a second run writes nothing, cannot be shown by a fake that never writes a row. Only the HTTP client is scripted, which is what the neighbouring McpImportServiceTest does and says why. Three controls, each observed: breaking out of the loop on the first refusal fails the test that the healthy server was still imported; returning SUCCESS regardless of refusals fails the exit-code test; and silently taking the first of two twins fails the ambiguity test. ModuleSeamTest names the command, because Classes/Command/ is core and the MCP importer is the tool module: in a package split it moves to nr_llm_tools with the code it drives (ADR-090). Its docblock said "Seven classes are excluded BY NAME" while listing six — one left without the number following — so the count is gone rather than corrected. Closes #836 Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_01MNg1MysJVugv1xo2husknU Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
04ec820 to
4774c87
Compare
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Self-review: 4774c87 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. |



Closes #836. A seeded MCP server has a record and no tools until somebody opens the module and presses Import catalogue. On an instance rebuilt by a deploy that is a manual step after every fresh install, and the server supplies nothing until it happens.
It is the module's button, not a second path
Checked before writing anything:
McpServerController::importAction()resolves the record and formats the report — that is all it does. Every refusal reason, the SSRF gate, the ADR-170 operation budget and the catalogue reconciliation live inMcpImportService::import(). So the command holds no policy of its own, and the issue's "not a second path" is a property of the code rather than an intention.Three decisions the issue did not settle
Identifier, not uid — a uid is not knowable to whoever writes the deploy script. This needs no new repository method:
findEnabled()is filtered in the command. That matters, because identifiers are deliberately not unique in the table — soft-deleted rows keep theirs, which is exactly why the service refuses on a twin instead of relying on an index. The ambiguity is therefore reachable here too, and is reported once naming both rather than resolved by picking a row.--allwalksfindEnabled(), notfindUsable()— the latter filters out servers without a data class. Silently skipping one is worse than importing it and printing the service's reason, which names the missing setting.An empty installation exits zero. A deploy runs
--allunconditionally; "no enabled server yet" is a valid state, not a red pipeline. A refusing server does fail the run, and does not stop the others.Registered explicitly, because
schedulableis a decisionautoconfigure: trueand the../Classes/*resource import would have picked up#[AsCommand]on its own. The entry exists so the flag is chosen rather than defaulted: this is the only one of the four registered commands that is schedulable, because the import is idempotent and a catalogue changes on the server's schedule rather than ours.Functional, and not by preference
The importer and both repositories are
final readonly, so a faked import service is not constructible. More to the point: the acceptance criterion that matters most — a second run writes nothing — cannot be shown by a fake that never writes a row. Only the HTTP client is scripted, which is what the neighbouringMcpImportServiceTestdoes and says why.One trap worth naming:
McpTestServeranswers a queue, one entry per contact. A test importing two servers must script two, and getting that wrong reads as a refusal from the second server rather than as an exhausted fixture — which is how the--alltest failed first. The queue depth is now an explicit argument at every call site.Three controls, each observed
breakout of the loop on the first refusalRestored tree: 10 tests green.
An architecture rule said no, and it was right
Classes/Command/is "core"; the MCP importer is the tool module.ModuleSeamTestrejected the dependency (ADR-090), and the fix is the established one — three commands are already named in its exception list because they are a module's operational surface living in a shared directory. This is the fourth, and where it moves in a split (nr_llm_tools) is recorded both in the test and in the command's own docblock, as that docblock demands.Found while doing it: the docblock said "Seven classes are excluded BY NAME" and listed six — a class left the list without the number following. The count is gone rather than corrected, for the reason #793 is about.
Verification
-s functional -d sqlite(this class)-s unit(full)-s phpstan(carries phpat)-s cgl -ncomposer ci:test:changelogAssisted by claude-code:claude-opus-5 — Session