Skip to content

Fix crash when MCP plugins are persisted with Guid.Empty as Id#397

Open
CowboyHat-exe wants to merge 1 commit into
Sylinko:mainfrom
CowboyHat-exe:fix/mcp-plugin-zero-guid-crash
Open

Fix crash when MCP plugins are persisted with Guid.Empty as Id#397
CowboyHat-exe wants to merge 1 commit into
Sylinko:mainfrom
CowboyHat-exe:fix/mcp-plugin-zero-guid-crash

Conversation

@CowboyHat-exe
Copy link
Copy Markdown

Problem

KernelPluginCollection uses the plugin Name (derived from Id.ToString("N")) as its dictionary key. When two or more McpChatPluginEntity records in settings have Id == Guid.Empty, the second Add call throws:

System.ArgumentException: Argument_AddingDuplicateWithKey, 00000000000000000000000000000000
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at Microsoft.SemanticKernel.KernelPluginCollection.Add(KernelPlugin plugin)
   at Everywhere.Chat.ChatService.BuildKernelAsync(...)

Every chat request fails with "An unknown error occurred" until the app is restarted with healed settings.

Root cause

McpChatPluginEntity.ToMcpChatPlugin() passes Id directly to the McpChatPlugin constructor without checking for Guid.Empty. If settings were ever written with un-initialised IDs (e.g. by a migration or external edit), all such entries produce the same plugin name and the second one crashes the kernel builder.

Fix

Self-heal on load: if Id == Guid.Empty, assign Guid.CreateVersion7() before constructing the plugin. The healed value is written back to settings on the next save (via the existing SourceList → McpChatPluginEntity binding in ChatPluginManager), so the repair is permanent.

public McpChatPlugin? ToMcpChatPlugin()
{
    var transportConfiguration = Stdio as McpTransportConfiguration ?? Http;
    if (transportConfiguration == null) return null;

    // Self-heal persisted Guid.Empty IDs; they cause KernelPluginCollection to throw
    // a duplicate-key ArgumentException when two or more zero-ID plugins are loaded.
    if (Id == Guid.Empty)
        Id = Guid.CreateVersion7();

    return new McpChatPlugin(Id, transportConfiguration);
}

Not changed: the [JsonConstructor] path — initialising Id there would assign a fresh GUID to every record on every deserialization, breaking round-trips for records that already have a valid Id.

KernelPluginCollection uses the plugin Id (formatted as 'N') as its
dictionary key. When two or more McpChatPluginEntity entries are loaded
from settings with Id == Guid.Empty, KernelPluginCollection.Add throws:

  ArgumentException: Argument_AddingDuplicateWithKey,
  00000000000000000000000000000000

ToMcpChatPlugin() now assigns a fresh Guid.CreateVersion7() whenever it
encounters a zero Id, matching the behaviour of the McpChatPlugin primary
constructor.  On next save the healed Id is written back to settings so the
self-repair is permanent.

Constraint: KernelPluginCollection uses the plugin Name as the dict key; Name is set from Id in the ctor so Guid.Empty produces the same key for every affected plugin
Rejected: Initialize Id in [JsonConstructor] McpChatPluginEntity() | breaks round-trip: the ctor is called for every existing record including ones that already have a good Id
Confidence: high
Scope-risk: narrow
Directive: Do not change the key used by KernelPluginCollection without verifying the dedup logic in ChatPluginManager
Tested: manual restart with settings containing 8 Guid.Empty plugin entries; error no longer thrown
Not-tested: automated
@github-actions
Copy link
Copy Markdown

Please read the following Contributor License Agreement (CLA). If you agree with the CLA, please comment with the following:


I have read the CLA Document and I hereby sign the CLA


CowboyHat-exe seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@DearVa
Copy link
Copy Markdown
Member

DearVa commented May 31, 2026

Hey @CowboyHat-exe, thank you very much for this PR!

The root cause, however, actually stems from PluginSettings.McpChatPlugins using an inappropriate data type: it should be an ObservableDictionary<string, McpChatPluginEntity> rather than a list. That would eliminate duplicate keys at the source, while also allowing us to drop the Id field from McpChatPluginEntity entirely. Additionally, ChatPluginSnapshot itself needs name deduplication to prevent this exception from surfacing further downstream.

The refactoring involved is fairly extensive — it includes converting both ChatPluginManager._builtInPluginsSource and ChatPluginManager._mcpPluginsSource to SourceCache with custom mapping strategies. I'm planning to tackle these refactorings in the near future, along with removing the ObjectObserver-based change tracking.

DearVa added a commit that referenced this pull request May 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants