Skip to content

fix(plugins): skip invalid plugin components instead of failing - #309

Merged
danielkov merged 3 commits into
mainfrom
fix/plugin-diagnostics-nonfatal
Oct 25, 2026
Merged

danielkov merged 3 commits into
mainfrom
fix/plugin-diagnostics-nonfatal

Conversation

@danielkov

@danielkov danielkov commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Invalid plugin components no longer abort session startup. Plugin diagnostics and per-server MCP preparation errors are logged as warnings and only the affected skill or MCP server is skipped.

Motivation

A single non-conforming SKILL.md or a duplicate MCP server name across two marketplace plugins made the agent exit before the session opened:

plugin … (SkillSkipped) at …/SKILL.md: skill `requesting-infra-access` does not conform to the Agent Skills specification
MCP server "Datadog" is declared by both plugins "…engineering…" and "…kitchen-sink…"

agentkit-plugins reports these as non-fatal diagnostics scoped to the narrowest component; kit was escalating them to hard errors.

Impact

  • Plugins with a broken skill, skills directory, mcp.json, MCP server entry, or extensions field still load their valid parts. Each skipped component is logged via tracing::warn!, which the CLI does not currently surface; feat: model-accessible diagnostics search for skill and MCP loading issues #310 tracks a model-accessible diagnostics search.
  • A malformed mcp.json edit during hot-reload now publishes a generation without that plugin's MCP servers, instead of rejecting the reload and retaining the previous generation. Fatal load errors (e.g. invalid plugin.json) still retain the last valid generation.

Technical details

Duplicate plugin MCP server names

The first plugin in alias order keeps the server name; later declarations are skipped with a warning. The name is reserved before preparation, so an invalid first declaration leaves the server unavailable rather than handing it to a later plugin. This mirrors the existing precedence where user-config MCP servers silently override plugin servers.

Per-server MCP preparation

Server preparation moved into prepare_plugin_server, so an unsupported SSE transport, invalid cwd, escaping command path, or invalid header skips only that server. Preparation errors are split into Invalid (skipped) and Unavailable (propagated). Disk-availability and filesystem failures stay fatal so a transient failure cannot publish a configuration that unregisters a working server. Non-UTF-8 plugin root or data paths also remain fatal.

Revalidation passes

The diagnostic checks in the staging revalidation and immutable skill snapshot passes were removed. The existing semantic-key and skill-set comparisons still reject plugins that change mid-staging.

Plugin diagnostics and per-server MCP preparation errors now log a
warning and skip the affected skill or server. Duplicate plugin MCP
server names keep the first plugin in alias order.

@kit-code-agent kit-code-agent 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.

Plugin component isolation currently loses user-visible skip diagnostics, converts transient disk failures into server removals, and allows failed declarations to transfer duplicate-name ownership. The documented rejection behavior also needs updating.

Took 15 min

Comment thread src/tools/mcp.rs Outdated
Comment thread src/tools/mcp.rs
Comment thread src/tools/mcp.rs Outdated
Comment thread src/tools/mcp.rs
Unavailable plugin files or data directories still fail preparation
instead of silently removing a server. A plugin MCP name is reserved
before preparation so an invalid first declaration cannot transfer
ownership to a later plugin. Docs describe component-level skipping.

@kit-code-agent kit-code-agent 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.

Malformed plugin package paths can still abort startup or reject an entire reload instead of skipping only the affected server.

Took 9 min 22 sec

Comment thread src/tools/mcp.rs Outdated
A plugin command or cwd whose ancestor is a regular file is a package
defect and skips only that server.

@kit-code-agent kit-code-agent 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.

No issues found. The changes look good to merge.

Took 10 min

@danielkov
danielkov merged commit dff1545 into main Oct 25, 2026
5 checks passed
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.

1 participant