Repository navigation
feat: serve plugin skills in chatd - #29323
jscottmiller wants to merge 2 commits into
Conversation
21b1eb2 to
57f56d9
Compare
57f56d9 to
0e5ffd0
Compare
UAT finding:
|
0e5ffd0 to
228d528
Compare
1b93d15 to
db120bb
Compare
db120bb to
571d62b
Compare
571d62b to
2557031
Compare
2557031 to
e4b94a0
Compare
e4b94a0 to
11deb02
Compare
475d72e to
fc187c2
Compare
fc187c2 to
463a03a
Compare
463a03a to
37bf56d
Compare
37bf56d to
bdd9542
Compare
bdd9542 to
f558b5c
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
This round reviews the rebased PR, which renders plugin skills in the chatd prompt, the skill tools, the chat API, and the Agents UI. Findings inline: 1 P2, 3 P3, 13 P4.
Addressed since the last round: all 25 open findings are fixed, and the site-name part of the "workspace skill" naming dispute is accepted. The P2 is new because of the rebase: this PR's base is now 7cd1e84, and #30341 is based on 1c5c928, so it is a sibling branch, not below this PR. The round 1 split required the description sanitization to merge first, and the stack no longer ensures that.
Out of scope (needs a ticket or explicit acceptance by a human):
coderd/x/chatd/chattool/skill.go:172: workspace skill descriptions also reach<available-skills>raw on the base, so any repository SKILL.md can close the block the same way. #30341 targets this.site/src/pages/AgentsPage/components/ContextUsageIndicator.tsx:303:skillBytessums the full SKILL.md, but only the name and description reach the prompt untilread_skillruns, so the Skills section overstates prompt cost.agent/agentcontext/defaults.go:9: the built-in~/.coderscan roots cannot be overridden throughagent.Options, so every test that runs a real agent reads the developer's home skills, plugins, and instruction files.
coderd/x/chatd/generation_preparer.go:531
P3 [CRF-40] The only production caller of mergeTurnSkills still names the pinned list workspaceSkills, and that list now includes plugin skills. (Netero)
The variable is used at 8 sites, including GetSkills: func() []chattool.SkillMeta { return workspaceSkills } for read_skill, so a reader concludes plugin skills are not served there. The parameter it feeds is now pinnedSkills (chatd.go:3726). Rename the variable to pinnedSkills in generation_preparer.go.
🤖
codersdk/chats.go:291
P4 [CRF-47] The ChatContext.Resources doc still says the list holds instruction files and skills that the prompt is built from, but plugin manifest rows are listed and never reach the prompt. (Mafuuu)
The kind doc on the same type was updated for this, so the field doc and the generated docs/reference/api/chats.md now disagree with it. Rewrite the sentence to match the kind doc, then run make gen.
🤖
🤖 This review was automatically generated with Coder Agents.
737f58e to
639212a
Compare
|
/coder-agents-review |
639212a to
6917a33
Compare
There was a problem hiding this comment.
This round reviews the fixes to the plugin skill rendering in chatd, the chat API, and the Agents UI. Findings inline: 2 P3, 11 P4, 5 Nit.
Addressed since the last round: 16 findings are fixed. The raw plugin description finding is deferred to CODAGT-1337 now that #30341 is closed. Most new findings are about the round 3 fixes. The ResolveAlias removal still leaves two copies of the merge recipe and some unreachable not-found branches, and some reworded comments are still wrong.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/modules/roles/RoleSelector.tsx:58:Object.groupBy(Chrome 117, Safari 17.4) is called with no polyfill while browserslist declares Chrome 111 and Safari 16.4, so the role selector throwsTypeErrorin those supported browsers.
Notes:
coderd/x/chatd/context_prompt.go:438: an OK plugin row whose body fails to decode is dropped from the listing with no log line, unlike the prompt path's malformed-row log. This can happen only after a schema change.
coderd/x/chatd/chattool/skill.go:531
P4 [CRF-61] The failed to resolve skill %q branch of skillResolveErrorResponse can no longer run. (Netero)
Lookup returns only ErrSkillNotFound or ErrSkillAmbiguous, both handled above, and the PR deleted ResolveAliasErrorIsSanitized along with the injected resolver. Coverage reports skill.go:531.2,533.3 at 0. Remove the fallback, or keep it with a comment that no current resolver returns another error.
🤖
🤖 This review was automatically generated with Coder Agents.
6917a33 to
1d5ef96
Compare
| // decodeContextBody decodes a protojson resource body into the proto type T. | ||
| // ok is false when the body cannot be decoded, letting callers count it as | ||
| // malformed rather than silently treating it as empty. | ||
| func decodeContextBody[T any, P interface { | ||
| *T | ||
| proto.Message | ||
| }](body json.RawMessage) (P, bool) { | ||
| decoded := P(new(T)) | ||
| if err := contextBodyUnmarshalOptions.Unmarshal(body, decoded); err != nil { | ||
| return nil, false | ||
| } | ||
| return &decoded, true | ||
| return decoded, true | ||
| } |
There was a problem hiding this comment.
I'm not sure I see the value in this helper?
At call sites we're going from
var decoded agentproto.MCPServerBody
if err := contextBodyUnmarshalOptions.Unmarshal(body, &decoded); err != nil {
return nil
}
// Use 'decoded' hereTo
decoded, ok := decodeContextBody[agentproto.MCPServerBody](body)
if !ok {
return nil
}
// Use 'decoded' hereMaybe this is just a completely useless nit and you can definitely ignore me but it just feels unneeded (in my opinion, feel free to completely disagree with me).
There was a problem hiding this comment.
Nah, agreed. Inlined.
| ##### Skill index | ||
|
|
||
| Each turn, chatd merges the user's personal skills with the skills the workspace agent pushed and inserts an `<available-skills>` block into the system prompt, after the chat instruction and before the user prompt. Each entry is a skill alias and its description. A name held by one skill is listed bare; a name held by several is listed as `personal/<name>`, `workspace/<name>`, or `plugin/<plugin>/<name>`. The model passes an alias to `read_skill` to load the body, and to `read_skill_file` to read a supporting file of a workspace or plugin skill. | ||
|
|
||
| A pinned skill row with a plugin name is a plugin skill: a skill shipped in an Agent Plugin's `skills/` directory. chatd keeps plugin skills apart from plain workspace skills and identifies each by (plugin, name), so two plugins can ship skills with the same name. Plugin skills follow personal and workspace skills when a name is shared, ordered by plugin name. Each plugin skill entry carries a `(plugin: <name>)` label after its alias, and the qualified-alias hint lists the `plugin/<plugin>/<name>` form only when a plugin skill is listed that way. `read_skill` and `read_skill_file` serve a plugin skill from its own pinned directory and never fall back to a same-named workspace skill. |
There was a problem hiding this comment.
We try to keep changes to ARCHITECTURE.md as human as possible. I'm just checking that you've given this a thorough read through and verify that it is readable to a human and is as accurate as possible.
There was a problem hiding this comment.
Did an update pass on this to tighten it up, clarify a few items, and add a little detail on Mike's recent org plugin work.
| runPluginTurn := func(t *testing.T, enabled bool) turnResult { | ||
| t.Helper() | ||
|
|
||
| ctx := testutil.Context(t, testutil.WaitSuperLong) |
There was a problem hiding this comment.
ctx should be declared as close to first use as possible in tests
| // PluginName is the owning Agent Plugin's name, empty for a plain | ||
| // workspace skill. Together with Name it identifies the skill. | ||
| PluginName string |
There was a problem hiding this comment.
Yeaaah that's some slop. I did a pass over the whole stack to clean things up; we may want to apply the frontend comment logic to the go code as well, though my personal instructions have something similar and the model gleefully ignores it.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
Documentation CheckUpdates Needed
Automated review via Coder Agents |
Fixed: (Coder Agents generated) |
Skills pushed by the workspace agent with a plugin_name now reach the chat
as plugin-source skills. The prompt index lists them as
"- <alias> (plugin: <name>): <description>", collisions qualify them as
plugin/<plugin>/<name>, and read_skill and read_skill_file serve them from
the pinned SKILL.md and the skill directory like workspace skills.
The skill tools resolve a name to its pinned row by (source, plugin,
name), so two plugins shipping the same skill name resolve to their own
bodies.
Pinned skill names must match the kebab-case skill grammar; rows that
fail are dropped and logged with their sources. Plugin names are not
re-validated on read, since the Agent API rejects a push carrying an
invalid one.
Plugin skill descriptions reach <available-skills> unmodified, as
workspace skill descriptions do on main; sanitizing descriptions from
every source is tracked in CODAGT-1337.
The chatd architecture document gains a Skill index section covering the
block, bare and qualified aliases, the skill tools and plugin skills.
codersdk.ChatContextResource gains PluginName and the plugin kind. The
listing sets PluginName on non-OK skill and mcp_server rows as well as
OK ones, and copies the agent's Error onto OK rows so spec-mandated
manifest warnings (unknown fields, non-object extensions, an ignored
mcp.json or skills directory) reach the API instead of only the
database.
The extending-agents docs gain a Plugin skills section covering the
skills/ directory, the plugin label in the skill list, and the
plugin/<plugin>/<name> alias, and note that an invalid plugin appears in
the chat's context list.
Skill-invocation parts accept skill_source "plugin" with a required
skill_plugin_name. chatd loads the body from the pinned plugin skill
matching both names, and a workspace invocation no longer matches a plugin
skill that shares its name. A workspace or plugin skill that is not
available is reported as missing from the chat's workspace context.
The frontend gains only a "plugin manifest" label for the new kind; the
composer and context indicator changes ship separately.
An integration test runs a real workspace agent against a scan root with
a plugin skill and a plain skill, and checks the pinned rows, the
GET /chats/{id} context listing, and the system prompt, with the
agent-plugins experiment on and off.
… index Rewrite the Skill index section of the chatd architecture doc to cover all four skill sources: personal, organization, workspace, and plugin. It now lists them in collision order and documents the org/<name> alias, which resolves the organization skills TODO. Define pinned skills as the chat's copy of each pushed SKILL.md. A later push only marks the chat dirty; the copy changes when the chat is refreshed or bound to a different agent. Describe how read_skill and read_skill_file resolve each source. Personal and organization bodies are read from the database on every call. Workspace and plugin bodies come from the pinned SKILL.md, and their supporting files are read over a live workspace connection.
tl;dr: Plugin skills reach the model's prompt, the skill tools, skill invocations, and the chat API with their plugin attribution.
chatd lists plugin skills in
<available-skills>as- <alias> (plugin: <name>): <description>, andread_skillandread_skill_fileserve them like workspace skills, matching on(source, plugin, name). Plugin skill descriptions reach the index unmodified, as workspace skill descriptions do on main; sanitizing descriptions from every source is tracked in CODAGT-1337. The chatd architecture document describes plugin skills in its Skill index section. The extending-agents docs describe plugin skills, add theirplugin/<plugin>/<name>alias to the alias table, and say the/menu lists them under Workspace skills.codersdk.ChatContextResourcegainsPluginNameand thepluginkind.GET /api/v2/chats/{chat}setsplugin_nameon skill and MCP server rows in every status and includes agent warnings on OK rows. The frontend gets only aplugin manifestlabel for the new kind; the composer and context indicator changes are in #30577.Skill-invocation parts (#30861) accept
skill_source: "plugin"with a requiredskill_plugin_name, checked withworkspacesdk.ValidatePluginName; other sources reject the field. chatd loads an invoked plugin skill from the pinned skill matching both names, so aworkspaceinvocation no longer matches a same-named plugin skill, and a part whose source and plugin name disagree loads nothing. An unavailable workspace or plugin skill returns a 400 whose detail names the chat's workspace context.TestChatContextPluginSkillsFromAgentPushruns a real agent against coderd with the experiment on and off and checks the pinned rows, the API listing, and the system prompt.How plugin skills appear
With a workspace skill
deployand a pluginacmethat shipsdeployandrelease-notes, the system prompt's skill index reads:GET /api/v2/chats/{chat}lists the plugin and its skills as context resources:[ { "source": "/home/coder/.coder/plugins/acme", "kind": "plugin", "size_bytes": 312, "plugin_name": "acme", "status": "ok" }, { "source": "/home/coder/.coder/plugins/acme/skills/deploy", "kind": "skill", "size_bytes": 540, "skill_name": "deploy", "skill_description": "Deploy through Acme", "plugin_name": "acme", "status": "ok" } ]PR map
scott/agent-plugins/3-skills..bda3ca2a4ccoderd/x/chatd/chattool/skill.go(+98/-84):MergePinnedSkillsmerges personal, organization, and project skills with the pinned skills, split into workspace and plugin skills.ReadSkillOptionstakes those skill lists in place ofResolveAlias, andread_skillandread_skill_fileresolve an alias to its own pinned skill, with no fallback to a same-named workspace skill.coderd/x/chatd/context_prompt.go(+90/-83): decodes each pinned body inline, drops OK skills with invalid names (pinnedSkillMeta), keeps warnings on OK rows, and reports plugin rows andPluginName.coderd/x/chatd/turn_environment.go(+9/-42): the per-turn merge goes throughMergePinnedSkills.coderd/x/chatd/skill_invocation.go(+22/-10): plugin invocations resolve by plugin and skill name; workspace invocations skip plugin skills.codersdk/chats.go(+24/-2, API):ChatContextResourceKindPlugin,ChatContextResource.PluginName,ChatSkillSourcePlugin, andSkillPluginNameonChatInputPartandChatMessagePart.coderd/exp_chats.go(+21/-2, API): validatesskill_plugin_nameagainst the source and words the unavailable-skill detail by source.This PR was generated by Coder Agents on behalf of @jscottmiller.