Skip to content

Commit 3ec0592

Browse files
authored
feat(mcp): import a catalogue from the CLI, so a deploy needs no click (#863)
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. ```bash vendor/bin/typo3 nrllm:mcp:import deepwiki vendor/bin/typo3 nrllm:mcp:import --all ``` ## 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 in `McpImportService::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. **`--all` walks `findEnabled()`, not `findUsable()`** — 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 `--all` unconditionally; "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 `schedulable` is a decision `autoconfigure: true` and 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 neighbouring `McpImportServiceTest` does and says why. One trap worth naming: `McpTestServer` answers 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 `--all` test failed first. The queue depth is now an explicit argument at every call site. ## Three controls, each observed | defect injected | result | |---|---| | `break` out of the loop on the first refusal | fails — the healthy server was no longer imported | | return SUCCESS regardless of refusals | fails — exit-code assertion | | silently take the first of two twins | fails — ambiguity assertion | Restored tree: 10 tests green. ## An architecture rule said no, and it was right `Classes/Command/` is "core"; the MCP importer is the tool module. `ModuleSeamTest` rejected 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 | gate | result | |---|---| | `-s functional -d sqlite` (this class) | 10 tests, 52 assertions, exit 0 | | `-s unit` (full) | 7288 tests, 24766 assertions, exit 0 | | `-s phpstan` (carries phpat) | No errors | | `-s cgl -n` | SUCCESS | | `composer ci:test:changelog` | exit 0 | _Assisted by claude-code:claude-opus-5 — [Session](https://claude.ai/code/session_01MNg1MysJVugv1xo2husknU)_
2 parents fe97638 + 4774c87 commit 3ec0592

6 files changed

Lines changed: 556 additions & 4 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
66

77
## [Unreleased]
88

9+
### Added
10+
11+
- **`nrllm:mcp:import` imports an MCP catalogue from the CLI**, so a seeded server gets its tools on deploy instead of waiting for somebody to press **Import catalogue** in the module. Takes a server identifier, or `--all` for every enabled server; prints imported, skipped and orphaned counts per server plus the reason for each skipped tool, and exits non-zero when a server refused. It is the module's button rather than a second path — the same refusals, the same guard rails and the same reconciliation, so a second run against an unchanged catalogue writes nothing. With `--all` a refusing server does not stop the others, and an installation with no enabled server is not a failure (#836).
12+
913
### Fixed
1014

1115
- **A merge conflict marker shipped in the ADR index.** `Documentation/Adr/Index.rst` ended on `||||||| c6503022` — a hand-resolved merge kept both sides and left the diff3 base marker behind as the file's last line, so the toctree carried a stray entry through v0.32.0. Nothing failed: the other guards read structure (status fields, citations, a rendered page) and a stray line at the end of a toctree is none of those to any of them. `NoConflictMarkersTest` now reads the bytes, and both marker shapes were watched failing before it was trusted.
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
<?php
2+
3+
/*
4+
* Copyright (c) 2025-2026 Netresearch DTT GmbH
5+
* SPDX-License-Identifier: GPL-2.0-or-later
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace Netresearch\NrLlm\Command;
11+
12+
use Netresearch\NrLlm\Domain\ValueObject\McpImportReport;
13+
use Netresearch\NrLlm\Domain\ValueObject\McpServerRecord;
14+
use Netresearch\NrLlm\Service\Tool\Mcp\McpImportService;
15+
use Netresearch\NrLlm\Service\Tool\Mcp\McpServerRepository;
16+
use Symfony\Component\Console\Attribute\AsCommand;
17+
use Symfony\Component\Console\Command\Command;
18+
use Symfony\Component\Console\Input\InputArgument;
19+
use Symfony\Component\Console\Input\InputInterface;
20+
use Symfony\Component\Console\Input\InputOption;
21+
use Symfony\Component\Console\Output\OutputInterface;
22+
use Symfony\Component\Console\Style\SymfonyStyle;
23+
24+
/**
25+
* Imports an MCP server's advertised catalogue from the CLI.
26+
*
27+
* A server record can be seeded, but until this existed its catalogue could
28+
* only arrive by a person opening the MCP Servers module and pressing **Import
29+
* catalogue**. An instance rebuilt by a deploy therefore had one manual step
30+
* after every fresh install, and no tools for that server until somebody
31+
* clicked.
32+
*
33+
* This is the module's button, not a second path: it calls the same
34+
* {@see McpImportService::import()} and adds nothing. Every refusal reason, the
35+
* SSRF gate, the operation budget (ADR-170) and the catalogue reconciliation
36+
* live in that service, which is why this class holds no policy of its own.
37+
*
38+
* It lives in `Classes/Command/` with every other command but belongs to the
39+
* tool module, and `ModuleSeamTest` names it for that reason: in a package
40+
* split it moves to nr_llm_tools with the MCP code it drives (ADR-090).
41+
*
42+
* Servers are addressed by identifier rather than uid, because a uid is not
43+
* knowable to whoever writes the deploy script and an identifier is what the
44+
* seed sets. Identifiers are not unique in the table — soft-deleted rows keep
45+
* theirs, which is why the service refuses an import when two ENABLED servers
46+
* share one rather than relying on a database constraint. The same ambiguity
47+
* can reach this command, so it is reported here rather than resolved by
48+
* picking a row.
49+
*/
50+
#[AsCommand(
51+
name: 'nrllm:mcp:import',
52+
description: "Import an MCP server's advertised tool catalogue.",
53+
)]
54+
/**
55+
* @internal Not part of the @api surface; may change without notice (ADR-127).
56+
*/
57+
final class ImportMcpCatalogueCommand extends Command
58+
{
59+
public function __construct(
60+
private readonly McpImportService $importer,
61+
private readonly McpServerRepository $servers,
62+
) {
63+
parent::__construct();
64+
}
65+
66+
protected function configure(): void
67+
{
68+
$this->addArgument(
69+
'identifier',
70+
InputArgument::OPTIONAL,
71+
"The MCP server's identifier, as set on the record. Omit it and pass --all instead.",
72+
);
73+
74+
$this->addOption(
75+
'all',
76+
'a',
77+
InputOption::VALUE_NONE,
78+
'Import every enabled server. One failing server does not stop the others.',
79+
);
80+
}
81+
82+
protected function execute(InputInterface $input, OutputInterface $output): int
83+
{
84+
$io = new SymfonyStyle($input, $output);
85+
$identifier = $input->getArgument('identifier');
86+
$identifier = is_string($identifier) ? $identifier : null;
87+
88+
$all = $input->getOption('all') === true;
89+
90+
if ($all && $identifier !== null) {
91+
$io->error('Pass either an identifier or --all, not both.');
92+
93+
return Command::INVALID;
94+
}
95+
96+
if (!$all && ($identifier === null || $identifier === '')) {
97+
$io->error('Name a server identifier, or pass --all to import every enabled server.');
98+
99+
return Command::INVALID;
100+
}
101+
102+
$servers = $all ? $this->servers->findEnabled() : $this->resolve($identifier ?? '', $io);
103+
if ($servers === null) {
104+
return Command::FAILURE;
105+
}
106+
107+
if ($servers === []) {
108+
// Not a failure: an installation with no enabled server is a valid
109+
// state, and a deploy that runs --all unconditionally must not go
110+
// red because this one has none yet.
111+
$io->warning('No enabled MCP server to import.');
112+
113+
return Command::SUCCESS;
114+
}
115+
116+
$refused = 0;
117+
foreach ($servers as $server) {
118+
if (!$this->report($this->importer->import($server), $server, $io)) {
119+
++$refused;
120+
}
121+
}
122+
123+
if ($refused > 0) {
124+
$io->error(sprintf('%d of %d server(s) refused the import.', $refused, count($servers)));
125+
126+
return Command::FAILURE;
127+
}
128+
129+
return Command::SUCCESS;
130+
}
131+
132+
/**
133+
* The one enabled server carrying this identifier, or null with the reason printed.
134+
*
135+
* @return list<McpServerRecord>|null
136+
*/
137+
private function resolve(string $identifier, SymfonyStyle $io): ?array
138+
{
139+
$matches = array_values(array_filter(
140+
$this->servers->findEnabled(),
141+
static fn(McpServerRecord $server): bool => $server->identifier === $identifier,
142+
));
143+
144+
if ($matches === []) {
145+
$io->error(sprintf('No enabled MCP server has the identifier "%s".', $identifier));
146+
147+
return null;
148+
}
149+
150+
if (count($matches) > 1) {
151+
// The service refuses this case too, but it would refuse it once per
152+
// row and read as two unrelated failures. Said here, once, in the
153+
// terms the operator has to act on.
154+
$io->error(sprintf(
155+
'%d enabled servers share the identifier "%s". Identifiers name the imported tools, so they must be unique among enabled servers.',
156+
count($matches),
157+
$identifier,
158+
));
159+
160+
return null;
161+
}
162+
163+
return $matches;
164+
}
165+
166+
/**
167+
* Prints one server's outcome. Returns false when the import was refused.
168+
*/
169+
private function report(McpImportReport $report, McpServerRecord $server, SymfonyStyle $io): bool
170+
{
171+
if ($report->refused) {
172+
$io->writeln(sprintf(
173+
'<error>%s: refused</error> — %s',
174+
$server->identifier,
175+
$report->skipReasons[0] ?? 'no reason given',
176+
));
177+
178+
return false;
179+
}
180+
181+
// Printed even when all three are zero: "imported 0" after a successful
182+
// contact means the catalogue matched what was already stored, which is
183+
// the expected result of the second run and worth seeing.
184+
$io->writeln(sprintf(
185+
'<info>%s</info>: %d imported, %d skipped, %d orphaned',
186+
$server->identifier,
187+
$report->imported,
188+
$report->skipped,
189+
$report->orphaned,
190+
));
191+
192+
foreach ($report->skipReasons as $reason) {
193+
$io->writeln(' skipped: ' . $reason);
194+
}
195+
196+
return true;
197+
}
198+
}

Configuration/Services.yaml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,17 @@ services:
508508
description: 'Store a provider API key read from STDIN and link it to the provider record.'
509509
schedulable: false
510510

511+
# The only schedulable one of the four: the import is idempotent — a second
512+
# run against an unchanged catalogue reconciles to the same rows — and a
513+
# catalogue changes on the server's schedule, not ours. Registered explicitly
514+
# rather than left to autoconfigure so the flag is a decision, not a default.
515+
Netresearch\NrLlm\Command\ImportMcpCatalogueCommand:
516+
tags:
517+
- name: 'console.command'
518+
command: 'nrllm:mcp:import'
519+
description: "Import an MCP server's advertised tool catalogue."
520+
schedulable: true
521+
511522

512523
# ========================================
513524
# nr-vault integration

Documentation/Administration/McpServers.rst

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,30 @@ How it works
2424
rendered. Tool input schemas are normalised into the supported subset
2525
on import; a tool whose schema cannot be expressed is skipped rather
2626
than silently weakened.
27+
28+
The same import runs from the CLI, which is what a deploy needs: a
29+
seeded server has a record but no catalogue until somebody presses the
30+
button, and on a rebuilt instance that is a manual step after every
31+
fresh install.
32+
33+
.. code-block:: bash
34+
:caption: Import one server, or every enabled one
35+
36+
vendor/bin/typo3 nrllm:mcp:import deepwiki
37+
vendor/bin/typo3 nrllm:mcp:import --all
38+
39+
The command is the module's button, not a second path: the same
40+
refusals, the same guard rails, the same reconciliation. It prints one
41+
line per server — imported, skipped and orphaned counts, plus the
42+
reason for each skipped tool — and exits non-zero when a server refused.
43+
With ``--all`` a refusing server does not stop the others, and an
44+
installation with no enabled server is not a failure. Running it twice
45+
changes nothing: the second run reconciles against the same catalogue.
46+
47+
Servers are named by their identifier rather than their uid, because a
48+
uid is not knowable to whoever writes the deploy script. Two *enabled*
49+
servers sharing an identifier is refused with both named — identifiers
50+
name the imported tools, so they cannot collide.
2751
3. **Enable individual tools** — imported tools start disabled and are
2852
switched on one by one, exactly like the builtin tools in the
2953
:ref:`Tools module <administration-tools>`.

Tests/Architecture/ModuleSeamTest.php

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
namespace Netresearch\NrLlm\Tests\Architecture;
1111

1212
use Netresearch\NrLlm\Command\CancelAgentRunCommand;
13+
use Netresearch\NrLlm\Command\ImportMcpCatalogueCommand;
1314
use Netresearch\NrLlm\Command\PurgePrivacyDataCommand;
1415
use Netresearch\NrLlm\Command\ReapStaleAgentRunsCommand;
1516
use Netresearch\NrLlm\Form\Tca\ToolGroupItems;
@@ -181,13 +182,18 @@ public function testNothingOutsideTheBackendDependsOnIt(): Rule
181182
* into `nr_llm_tools` would make the two packages mutually dependent.
182183
*
183184
* "Core" here is the remainder: everything that is not one of the mapped
184-
* module namespaces above. Seven classes are excluded BY NAME rather than by
185-
* directory, because they are the tool module's own operational surface
186-
* living in shared directories — in a split each moves WITH its module,
187-
* so their coupling is ownership, not leakage:
185+
* module namespaces above. The classes listed below are excluded BY NAME
186+
* rather than by directory, because they are the tool module's own
187+
* operational surface living in shared directories — in a split each moves
188+
* WITH its module, so their coupling is ownership, not leakage. The list is
189+
* not counted here: it said "seven" while carrying six, because a class
190+
* left it without the number following.
188191
*
189192
* - `CancelAgentRunCommand`, `ReapStaleAgentRunsCommand` — CLI entry
190193
* points of the agent runtime (→ nr_llm_tools)
194+
* - `ImportMcpCatalogueCommand` — the CLI entry point of the MCP
195+
* catalogue import, the same operation the MCP Servers module runs
196+
* (→ nr_llm_tools)
191197
* - `PurgePrivacyDataCommand` — sweeps, among others, the agent-run
192198
* tables through their repository (→ split into per-module sweeps when
193199
* the packages separate)
@@ -227,6 +233,7 @@ public function testCoreDoesNotDependOnTheToolModule(): Rule
227233
Selector::inNamespace(self::NS_WIDGETS),
228234
Selector::inNamespace(self::NS_TESTS),
229235
Selector::classname(CancelAgentRunCommand::class),
236+
Selector::classname(ImportMcpCatalogueCommand::class),
230237
Selector::classname(PurgePrivacyDataCommand::class),
231238
Selector::classname(ReapStaleAgentRunsCommand::class),
232239
Selector::classname(ToolGroupItems::class),

0 commit comments

Comments
 (0)