fix: clear() wiping dynamically registered tools by tracking discovered state on references#251
fix: clear() wiping dynamically registered tools by tracking discovered state on references#251soyuka wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
soyuka
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
soyuka
force-pushed
the
fix/clear
branch
3 times, most recently
from
8e7853b to
ba326ac
Compare
chr-hertel
added
the
needs more work
soyuka
force-pushed
the
fix/clear
branch
3 times, most recently
from
c3f8c24 to
274e61e
Compare
…ed state on references clear() previously removed all non-manual elements, which destroyed dynamically registered tools when setDiscoveryState() was called on the next request cycle. Add isDiscovered flag to ElementReference so clear() only removes elements that were imported via setDiscoveryState(), preserving both manual and dynamic registrations.
There was a problem hiding this comment.
Pull request overview
This PR refines the capability registry’s lifecycle semantics by distinguishing discovered registrations (imported via setDiscoveryState()) from manual and dynamic registrations, so that clear() no longer wipes dynamically registered elements across request cycles.
Changes:
- Add an
isDiscoveredflag toElementReference(and propagate through reference subclasses). - Update
Registry::clear(),Registry::getDiscoveryState(), andRegistry::setDiscoveryState()to operate onisDiscoveredinstead of “non-manual”. - Expand unit coverage around
clear()/discovery-state round-trips and update HTTP inspector snapshots to reflect manual-over-discovered priority.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Capability/Registry.php |
Switches clearing/exporting/importing logic to rely on isDiscovered, and prevents discovered imports from overwriting existing manual/dynamic entries. |
src/Capability/Registry/ElementReference.php |
Introduces isDiscovered state on element references. |
src/Capability/Registry/ToolReference.php |
Propagates isDiscovered into the base ElementReference. |
src/Capability/Registry/ResourceReference.php |
Propagates isDiscovered into the base ElementReference. |
src/Capability/Registry/PromptReference.php |
Propagates isDiscovered into the base ElementReference. |
src/Capability/Registry/ResourceTemplateReference.php |
Propagates isDiscovered into the base ElementReference. |
tests/Unit/Capability/RegistryTest.php |
Updates/extends tests to validate discovered-only clearing, dynamic preservation, and discovery-state behavior. |
tests/Inspector/Http/snapshots/HttpCombinedRegistrationTest-resources_read-config_priority.json |
Updates snapshot output to reflect manual override behavior. |
tests/Inspector/Http/snapshots/HttpCombinedRegistrationTest-resources_list.json |
Updates snapshot output to reflect manual override behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Import new discovered elements — skip any that conflict with manual or dynamic registrations | ||
| foreach ($state->getTools() as $name => $tool) { | ||
| $this->tools[$name] = $tool; | ||
| if (!isset($this->tools[$name])) { | ||
| $this->tools[$name] = new ToolReference($tool->tool, $tool->handler, isDiscovered: true); | ||
| } | ||
| } |
There was a problem hiding this comment.
When a discovered element from the incoming DiscoveryState conflicts with an existing manual/dynamic registration, it is silently skipped. This makes it hard to understand why certain discovered capabilities never appear. Consider logging a debug message for each skipped tool/resource/prompt/template (similar to the existing conflict logs in registerTool/registerResource/...).
| $this->resourceTemplates[$uriTemplate] = new ResourceTemplateReference($template->resourceTemplate, $template->handler, completionProviders: $template->completionProviders, isDiscovered: true); | ||
| } | ||
| } | ||
|
|
||
| // Dispatch events for the imported elements |
There was a problem hiding this comment.
Because conflicting discovered elements are now skipped, the event-dispatch section below should avoid dispatching *ListChangedEvent events solely based on the incoming DiscoveryState being non-empty. As written, it can dispatch events even when nothing was actually imported (all entries conflicted and were skipped), which may trigger unnecessary downstream work. Consider tracking whether any tools/resources/prompts/templates were actually added and dispatch only when something changed.
clear() previously removed all non-manual elements, which destroyed dynamically registered tools when setDiscoveryState() was called on the next request cycle. Add isDiscovered flag to ElementReference so clear() only removes elements that were imported via setDiscoveryState(), preserving both manual and dynamic registrations.