fix(mcp): Stop read_skill serving YAML front matter to the agent - #9318
fix(mcp): Stop read_skill serving YAML front matter to the agent#9318Lagmator22 wants to merge 2 commits into
Conversation
read_skill returns skill files verbatim, so the YAML front matter lands in the model's context along with the procedure. That block is 26% of the built-in skill bytes, 30% for the two sub-skills, and none of it is instruction. license and metadata are provenance, and name/description already did their job in the root catalog that routed the agent here. allowed-tools sits in the same block, but nothing in the package reads it, so dropping it from the payload changes no behaviour. Files on disk are unchanged, so the metadata stays where humans and tooling read it. A file with no front matter, or with an unterminated block, is served as-is. Signed-off-by: Lagmator22 <ndgurman@gmail.com>
|
@SoumyaRaikwar thoughts? |
github-actions
Bot
added
the
waiting-for-author
|
@yurishkuro I'm doing some skill evaluation and hit a problemagents don't discover skills unless the prompt names one. Against unmodified main, driving a real ACP agent (Goose), read_skill was called in 0 of 20 runs when the prompt didn't name a skill. It went straight to the telemetry tools every time. The root SKILL.md can't route an agent that never opens it. Didn't work: rewriting the root index as symptom triage (0/5), making INSTRUCTIONS.md skills-first (0/5), a generic "call this before analysing a trace, start with SKILL.md" trigger in the tool description (0/10). The instructions do reach the model, and it quoted them back verbatim, but it still ignored them. What moved it: putting the skill names and what each is for into the read_skill tool description, so they're visible without a call. An agent picks a tool by matching its situation against the description, and "read a skill file" gives it nothing to match. A pointer to SKILL.md is still one hop away, which is the 0/10 above. Two honest numbers. Hand-writing that list got 5/5, but the wording was imperative enough that the agent opened the skill before looking at the trace. That's compliance rather than judgement, and not the behaviour we want. Generating the list from each skill's front matter description got 2/5, and in those runs the agent investigated first, saw errors, then fetched the error skill. Right shape, not yet reliable. Which is where front matter comes in: generate the catalog at startup from each skill's name/description instead of hard-coding it. One source of truth, nothing to keep in step with the root index, and it covers skills_dir, so an operator's skill becomes discoverable without hand-editing anything. No new tool and no protocol change, just the same 9 tools with one description generated instead of literal. So on this PR: allowed-tools has no consumer and can go. name/description are worth keeping, since that's the routing signal, and it's the consumer that was missing when #9195 was closed. Compatible with stripping front matter from what read_skill returns: it becomes the source of the catalog rather than payload. |
|
@SoumyaRaikwar did you try asking the agent why it did not try to read the skills? I thought about auto-generating the index from front matter as you describe, but I think it's curing a symptom, not the root cause. Please take this discussion to a separate issue, this PR is about whether front-matter needs to be returned to the agent, and it does not seem like it's needed - front matter exists for discovery, not when already reading the full skill. |
|
@yurishkuro Thanks. To confirm the scope, this actually only changes what read_skill returns. The files on disk keep their front matter, so it stays available as a source for any catalog or discovery work in a separate issue. |
github-actions
Bot
removed
the
waiting-for-author
|
@yurishkuro |
| // description the root catalog already used to route the agent here, so | ||
| // serving it again only spends context. A file with no block, or with an | ||
| // unterminated one, is returned unchanged rather than guessed at. | ||
| func stripFrontMatter(content string) string { |
There was a problem hiding this comment.
didn't we already have code for parsing front matter? we need to reuse that and the code should return a struct with fields for front matter attributes and the skill content
Which problem is this PR solving?
read_skillreturns skill files verbatim, so the YAML front matter lands in the model's context along with the procedure. Measured on the embedded skills, that block is 26% of the built-in skill bytes and 30% for the two sub-skills.None of it is instruction.
licenseandmetadataare provenance.nameanddescriptionalready did their job in the root catalog that routed the agent to the file.allowed-toolsis declared but no Go file reads it, so dropping it from the payload changes no behaviour.Description of the changes
read_skillstrips a leading YAML front matter block before returning the content.How was this change tested?
TestStripFrontMattercovers the block being removed, no block, an unterminated block, an empty body, a horizontal rule in the body, and a delimiter that is not at the start.TestReadSkillHandler_StripsFrontMatterdrives the real handler and assertslicenseandallowed-toolsno longer reach the caller.go test ./cmd/jaeger/internal/extension/jaegerquery/...passes.Checklist