Support agent-plugins-spec v1.0.0 format - #135
Merged
Merged
Conversation
Consume plugin.json manifests from external_skills sources for richer metadata (name, version, author, license) and opt-in mcp.json MCP server discovery. Ship a skills-only plugin.json at the repo root so dotagents is itself consumable by any Agent Plugins v1 client. New config fields: mcp (bool, opt-in) and mcp_agents on external_skills entries. Plugin-mode sources with default skill_dir tolerate missing skills/ (MCP-only plugins). Lock file gains plugin_name/plugin_version.
Reviewer's GuideAdds support for Agent Plugins v1.0.0 plugin.json and mcp.json manifests, including parsing/validation, MCP server discovery/injection, relaxed external skill discovery semantics for plugin-only sources, and embedding a plugin.json so dotagents itself can be consumed as a plugin. Sequence diagram for plugin-based MCP server injection during config loadsequenceDiagram
participant CLI
participant loadContext
participant injectPluginMCPServers
participant parsePluginManifest
participant discoverPluginMCPServers
CLI->>loadContext: loadContext(opts)
loadContext->>injectPluginMCPServers: injectPluginMCPServers(&cfg, home)
injectPluginMCPServers->>injectPluginMCPServers: externalCacheDir(home), externalDataDir(home)
injectPluginMCPServers->>injectPluginMCPServers: build existingNames from cfg.MCPServers
loop each cfg.ExternalSkills src
injectPluginMCPServers->>injectPluginMCPServers: check src.MCP
injectPluginMCPServers->>injectPluginMCPServers: repoName(src.URL), hasDir(cachePath+"/.git")
injectPluginMCPServers->>injectPluginMCPServers: hasPluginManifest(cachePath)
injectPluginMCPServers->>parsePluginManifest: parsePluginManifest(cachePath)
parsePluginManifest-->>injectPluginMCPServers: manifest, ok
injectPluginMCPServers->>injectPluginMCPServers: os.MkdirAll(pluginDataPath)
injectPluginMCPServers->>discoverPluginMCPServers: discoverPluginMCPServers(cachePath, manifest.Schema, pluginDataPath)
discoverPluginMCPServers-->>injectPluginMCPServers: []mcpServerConfig, problems
injectPluginMCPServers->>injectPluginMCPServers: apply src.MCPAgents to servers
injectPluginMCPServers->>injectPluginMCPServers: collect servers by name
end
injectPluginMCPServers->>injectPluginMCPServers: skip servers conflicting with existingNames
injectPluginMCPServers->>injectPluginMCPServers: skip servers declared by multiple sources
injectPluginMCPServers->>injectPluginMCPServers: append remaining servers to cfg.MCPServers
loadContext-->>CLI: home, contextDir, cfg, agents
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- isPluginRelaxedSource relies on the literal "skills" for the default skill directory, which could silently diverge from future config defaults; consider centralizing this default in one place or reusing existing helpers/constants.
- parsePluginManifest treats any unknown field in the author object as a hard failure while unknown top-level fields are tolerated, which may be too strict for real-world manifests; consider aligning author handling with the more permissive top-level unknown-field behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- isPluginRelaxedSource relies on the literal "skills" for the default skill directory, which could silently diverge from future config defaults; consider centralizing this default in one place or reusing existing helpers/constants.
- parsePluginManifest treats any unknown field in the author object as a hard failure while unknown top-level fields are tolerated, which may be too strict for real-world manifests; consider aligning author handling with the more permissive top-level unknown-field behavior.
## Individual Comments
### Comment 1
<location path="cmd/dotagents/config.go" line_range="189-198" />
<code_context>
}
seenExt[name] = struct{}{}
+
+ if src.MCP && len(src.MCPAgents) > 0 {
+ kept := src.MCPAgents[:0]
+ for _, agentName := range src.MCPAgents {
+ agentName = normalizeAgentName(agentName)
+ if agentName == "" {
+ continue
+ }
+ if len(seen) > 0 {
+ if _, ok := seen[agentName]; !ok {
+ return fmt.Errorf("config external_skills repo %q mcp_agents targets unknown agent %q", name, agentName)
+ }
+ }
+ if !hasMCPSupport(agentName) {
+ fmt.Fprintf(os.Stderr, "warning: config external_skills repo %q mcp_agents targets agent %q without MCP support; target ignored\n", name, agentName)
+ continue
+ }
+ kept = append(kept, agentName)
+ }
+ src.MCPAgents = kept
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Filtering MCPAgents on the range variable does not update cfg.ExternalSkills
Because the loop ranges over `cfg.ExternalSkills` by value (`for _, src := range cfg.ExternalSkills`), `src` is a copy and `src.MCPAgents = kept` does not update `cfg.ExternalSkills`. The MCPAgents filtering/normalization is therefore discarded and `cfg.ExternalSkills[i].MCPAgents` remains unvalidated. To persist the changes, range by index and update the slice element (e.g. `for i := range cfg.ExternalSkills { src := &cfg.ExternalSkills[i]; src.MCPAgents = kept }`) or otherwise assign back into `cfg.ExternalSkills`.
</issue_to_address>
### Comment 2
<location path="cmd/dotagents/pluginmcp_test.go" line_range="311-320" />
<code_context>
+func TestInjectPluginMCPServersWithOptIn(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to verify MCPAgents targeting is correctly applied when injecting plugin MCP servers.
Currently the new `MCPAgents` field on `externalSkillSource` is only exercised indirectly. Please add a variant of this test where `src.MCPAgents` is set (e.g. `[]string{"claude-code", "codex"}`) and assert that the injected `mcpServerConfig.Agents` is populated with that filtered list, to validate end-to-end agent targeting for plugin-derived servers.
Suggested implementation:
```golang
func TestInjectPluginMCPServersWithOptIn(t *testing.T) {
home := t.TempDir()
cacheRoot := filepath.Join(home, ".agents", "external")
pluginDir := filepath.Join(cacheRoot, "test-plugin")
makeGitDir(t, pluginDir)
writePluginJSON(t, pluginDir, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json",
"name": "test-plugin"
}`)
writeMCPJSON(t, pluginDir, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json",
"name": "test-plugin-mcp",
"command": "test-plugin-mcp",
"args": ["run"],
"env": {
"FOO": "BAR"
}
}`)
// Base config with plugin opt-in enabled.
cfg := Config{
MCP: true,
ExternalSkills: []externalSkillSource{
{
Directory: cacheRoot,
OptIn: []string{"test-plugin"},
},
},
}
// First scenario: no MCPAgents specified, servers should be injected but with no agent targeting.
injectPluginMCPServers(&cfg, home)
if len(cfg.MCPServers) != 1 {
t.Fatalf("expected 1 MCP server when mcp: true and plugin opted in, got %d", len(cfg.MCPServers))
}
server := cfg.MCPServers[0]
if server.Name != "test-plugin-mcp" {
t.Fatalf("expected MCP server name %q, got %q", "test-plugin-mcp", server.Name)
}
if len(server.Agents) != 0 {
t.Fatalf("expected injected MCP server to have no agent targeting by default, got %v", server.Agents)
}
// Second scenario: specify MCPAgents on the externalSkillSource and verify end-to-end agent targeting.
cfg = Config{
MCP: true,
ExternalSkills: []externalSkillSource{
{
Directory: cacheRoot,
OptIn: []string{"test-plugin"},
MCPAgents: []string{"claude-code", "codex"},
PluginName: "test-plugin",
},
},
}
injectPluginMCPServers(&cfg, home)
if len(cfg.MCPServers) != 1 {
t.Fatalf("expected 1 MCP server when mcp: true and plugin opted in with MCPAgents, got %d", len(cfg.MCPServers))
}
server = cfg.MCPServers[0]
expectedAgents := []string{"claude-code", "codex"}
if !reflect.DeepEqual(server.Agents, expectedAgents) {
t.Fatalf("expected injected MCP server Agents %v, got %v", expectedAgents, server.Agents)
}
```
The above edit assumes the following, which you should align with your actual codebase:
1. `Config` has fields `MCP bool`, `ExternalSkills []externalSkillSource`, and `MCPServers []mcpServerConfig` (or similar), and the test file already imports `reflect`. If not, add `import "reflect"` at the top of `cmd/dotagents/pluginmcp_test.go`.
2. `externalSkillSource` has fields `Directory`, `OptIn`, `MCPAgents`, and possibly `PluginName`. If `PluginName` is not needed in your implementation, you can remove it from the test setup; if it has a different name, adjust accordingly.
3. `mcpServerConfig` (or the type used for `cfg.MCPServers`) has fields `Name string` and `Agents []string`. If naming differs, update the test accordingly.
4. If the existing `TestInjectPluginMCPServersWithOptIn` already contains assertions, merge the new scenarios rather than overwriting. The intent is to:
- Keep the current “opt-in only” behavior assertion.
- Add a second scenario where `src.MCPAgents` is populated and assert that the resulting `cfg.MCPServers[0].Agents` matches the given list.
</issue_to_address>
### Comment 3
<location path="cmd/dotagents/pluginmcp_test.go" line_range="345-354" />
<code_context>
+func TestInjectPluginMCPServersUserWins(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the case where multiple plugins declare the same MCP server name.
There’s another collision path where two external sources declare the same MCP server name and are skipped with a warning. Please add a test that configures two `ExternalSkills` entries with `MCP: true`, each plugin exposing an `mcp.json` with the same `mcpServers` key, and assert that no additional servers are added to `cfg.MCPServers`. This will cover the multi-source conflict resolution behavior.
Suggested implementation:
```golang
func TestInjectPluginMCPServersUserWins(t *testing.T) {
home := t.TempDir()
cacheRoot := filepath.Join(home, ".agents", "external")
// First plugin exposing an MCP server named "test-server"
pluginDir1 := filepath.Join(cacheRoot, "test-plugin-1")
makeGitDir(t, pluginDir1)
writePluginJSON(t, pluginDir1, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json",
"name": "test-plugin-1"
}`)
writeMCPJSON(t, pluginDir1, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json",
"mcpServers": {
"test-server": {
"command": ["echo", "plugin-1"],
"args": [],
"env": {}
}
}
}`)
// Second plugin also exposing an MCP server with the same name "test-server"
pluginDir2 := filepath.Join(cacheRoot, "test-plugin-2")
makeGitDir(t, pluginDir2)
writePluginJSON(t, pluginDir2, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/plugin.schema.json",
"name": "test-plugin-2"
}`)
writeMCPJSON(t, pluginDir2, `{
"$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json",
"mcpServers": {
"test-server": {
"command": ["echo", "plugin-2"],
"args": [],
"env": {}
}
}
}`)
// User configuration already declares an MCP server named "test-server"
cfg := Config{
MCPServers: []MCPServer{
{
Name: "test-server",
// other fields (e.g. Command, Args, Env) can be populated as needed
},
},
ExternalSkills: []ExternalSkillConfig{
{
Name: "test-plugin-1",
MCP: true,
// fields pointing to pluginDir1 as appropriate for your config type
},
{
Name: "test-plugin-2",
MCP: true,
// fields pointing to pluginDir2 as appropriate for your config type
},
},
}
injectPluginMCPServers(&cfg, home)
// Assert that no additional servers are added from the two plugins.
// The user-configured server should "win" and remain the only entry.
if len(cfg.MCPServers) != 1 {
t.Fatalf("expected 1 MCP server when user config declares %q and multiple plugins collide, got %d", "test-server", len(cfg.MCPServers))
}
if cfg.MCPServers[0].Name != "test-server" {
t.Errorf("server name = %q, want %q", cfg.MCPServers[0].Name, "test-server")
}
}
```
To integrate this test with the rest of the codebase, you will likely need to:
1. Adjust the `Config`, `MCPServer`, and `ExternalSkillConfig` types to match your actual configuration structures:
- Replace `Config` with the real root config type used by `injectPluginMCPServers`.
- Replace `MCPServer` with the actual type of entries in `cfg.MCPServers` (and set any required fields beyond `Name`).
- Replace `ExternalSkillConfig` and its fields (`Name`, `MCP`, etc.) with the correct type/field names for external skills.
2. Ensure that each `ExternalSkills` entry correctly points at `pluginDir1` and `pluginDir2`:
- Depending on your existing tests, you may need fields like `Path`, `CacheDir`, `Source`, or similar to associate the config entry with the plugin directory created under `cacheRoot`.
3. Confirm that `injectPluginMCPServers` is using the same collision semantics this test expects:
- When a user-configured MCP server exists with a given name, any MCP servers with the same name coming from *any* plugins (including multiple plugins) should be skipped, leaving `cfg.MCPServers` unchanged except for the already-configured entry.
4. If you’d prefer not to repurpose `TestInjectPluginMCPServersUserWins` for the multi-source collision case, you can instead move this new test body into a separate function, e.g. `TestInjectPluginMCPServersMultiSourceCollisionUserWins`, and restore the original single-plugin behavior in `TestInjectPluginMCPServersUserWins`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c1f6f9e34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
yourconscience
force-pushed
the
plugin-support
branch
2 times, most recently
from
August 8, 2026 12:15
ea264c9 to
fbb5d52
Compare
yourconscience
force-pushed
the
plugin-support
branch
from
August 8, 2026 12:17
fbb5d52 to
dae325f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dae325f767
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9766803c93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - dotagents syncs exactly four surfaces: skills, MCP servers, hooks, and agent roles. | ||
| - Plugin packaging, plugin delivery modes, and native-plugin projection are intentionally unsupported. | ||
| - dotagents syncs exactly five surfaces: skills, MCP servers, hooks, agent roles, and plugins. | ||
| - Plugins follow the agent-plugins-spec v1.0.0 format. For harnesses with native plugin systems (Codex), dotagents projects spec-format plugins into the native format. Claude Code plugins (`.claude/plugins/*.ts`) are a planned addition. |
There was a problem hiding this comment.
For any spec-format external plugin used with Codex, a repo-wide search finds no .codex-plugin writer or adapter; this change only extracts skills and MCP servers, while the README explicitly calls native projection planned. Consequently dotagents does not sync the declared plugin surface for the repository's native-plugin harness, so either implement the projection or keep the invariant marked as planned. Fresh evidence relative to the earlier comment is that the final tree now explicitly requires Codex native projection rather than prohibiting it.
AGENTS.md reference: AGENTS.md:L15-L16
Useful? React with 👍 / 👎.
| } | ||
| kept = append(kept, agentName) | ||
| } | ||
| src.MCPAgents = kept |
There was a problem hiding this comment.
When every requested target is filtered out, such as mcp_agents: [pi], this assignment leaves MCPAgents empty; injectPluginMCPServers then treats the empty slice as the default and leaves each derived server's Agents empty, which desiredMCPServersForAgent interprets as all MCP-capable harnesses. A configuration intended only for an unsupported target therefore installs the plugin server into unrelated Codex, Claude, or other harness configurations despite warning that the target was ignored.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
| // Re-inject after clone so first sync picks up newly fetched plugins. | ||
| injectPluginMCPServers(&cfg, home) |
There was a problem hiding this comment.
When refreshing a materialized external source fails, syncExternalRepos may continue after warning that it is keeping committed skill copies; if the existing cache is at a different commit, this unconditional injection then reads that off-pin cache's plugin.json and mcp.json and writes its commands into native harness configuration. This lets an ordinary sync expose unreviewed external executable configuration instead of limiting agents to the lock-selected tree, so unavailable sources must be excluded from MCP injection or their cache HEAD must be verified against the prior pin first.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| | Hermes | yes | -- | yes | yes | | ||
| | OpenCode | yes† | yes | yes | -- | | ||
| | Pi* | yes | --* | --* | -- | | ||
| 5. **Plugins** — external sources with an [agent-plugins-spec](https://agent-plugins.org) `plugin.json` get their skills and MCP servers discovered automatically. Native plugin projection (Codex `.codex-plugin/`) is planned. |
There was a problem hiding this comment.
For users following this new description, MCP discovery is not automatic: MCP defaults to false and injectPluginMCPServers skips the source unless its YAML entry includes mcp: true, but neither this README section nor the shipped skills/dotagents/SKILL.md documents that option or mcp_agents (the skill still describes four surfaces). Such users will configure a valid plugin source and never receive its MCP servers; document the opt-in and keep the starter skill and release-site copy consistent.
AGENTS.md reference: AGENTS.md:L48-L50
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
plugin.jsonmanifests fromexternal_skillssources for richer metadata (name, version, author, license) and opt-inmcp.jsonMCP server discoveryplugin.jsonat the repo root so dotagents is itself consumable by any Agent Plugins v1 client.codex-plugin/) tracked as planned for follow-upplugin.jsondisables metadata/MCP only, skill discovery is unaffectedDetails
New config fields on
external_skillsentries:mcp: true-- opt-in to discover stdio MCP servers from the source'smcp.jsonmcp_agents: [claude-code, codex]-- optional target subset for plugin-derived MCP serversPlugin consumer behavior:
plugin.jsonat external source root, parses/validates per spec v1.0.0stdioMCP servers frommcp.jsonwith${PLUGIN_ROOT}/${PLUGIN_DATA}placeholder expansionstreamable-http,sse) and servers withcwd(not representable in dotagents' MCP config)repoName(url)dotagents as a plugin:
plugin.jsonat repo root makes~/.agents/skills/*discoverable by any Agent Plugins v1 clientgo:embedDocumentation updates:
Test plan