[Bug]: [mcp] read_skill returns the skill body twice, in two different encodings

[dev-aditya-hub]

What happened?

read_skill is the only one of the nine MCP tools that sets CallToolResult.Content itself:

return &mcp.CallToolResult{
    Content: []mcp.Content{&mcp.TextContent{Text: content}},
}, types.ReadSkillOutput{Instructions: content}, nil

The SDK then unconditionally populates StructuredContent from the typed output, so the response carries the skill body twice — once as raw markdown in content[0].text, once JSON-escaped inside structuredContent.instructions.

The other eight handlers return nil, output, nil and also carry their payload twice, but that is the spec's backward-compat duplicate and both copies are identical JSON. read_skill is different: the duplication is hand-written, and the two copies are different encodings of the same bytes.

Measured against the embedded skills, wire bytes per call:

path	file	wire	ratio
SKILL.md	800 B	1723 B	2.15×
error-root-cause/SKILL.md	1427 B	3021 B	2.12×
detect-n-plus-one/SKILL.md	1535 B	3243 B	2.11×

INSTRUCTIONS.md points agents at SKILL.md first, so a skill-driven run pays this on at least two calls before it touches any telemetry.

Steps to reproduce

1. Call read_skill with path: "SKILL.md" over the MCP endpoint.
2. Compare content[0].text with structuredContent.instructions — same body, two encodings.

Expected behavior

One copy. Which one is a design call, so I'd rather ask than guess. Measured on a 347-byte skill:

option	wire	note
today	797 B	—
drop the hand-set Content	832 B	worse — content becomes the JSON copy
Content: []mcp.Content{}	413 B	one copy, but no content block for clients that only read content
markdown in Content, metadata (path, truncated) in the typed output	473 B	keeps readable markdown and a content block

The last one looks best to me — it keeps the agent-facing markdown unescaped and drops the body from the structured half — but it changes ReadSkillOutput's schema, so it's your call. Happy to implement whichever you prefer, with a test pinning the wire shape (the current tests assert output.Instructions only, which is why this went unnoticed).

Worth noting the same 2× applies to the other eight tools; that half is inherited from the spec's backward-compat guidance, so I've left it out of this issue and raised the measurement side in #9135 instead.

Relevant log output

No response

Screenshot

No response

Additional context

No response

Jaeger backend version

main (2487d69)

SDK

Not applicable — this is in the MCP server handler layer (mcptools), not ingestion.

Pipeline

Not applicable.

Stogage backend

Not applicable.

[kashvipeehu24]

Hi @dev-aditya-hub , I’d be happy to work on this issue and submit a PR.
My approach would be to keep the skill body in Content, avoid duplicating it in structuredContent, and add regression tests to verify the expected response shape.

[aryangorde8]

One small correction, since it changes which option is cheapest to implement.

the current tests assert output.Instructions only, which is why this went unnoticed

TestReadSkillHandler_RawTextInContent already pins the Content half, and pins the two copies as equal:

tc, ok := result.Content[0].(*mcp.TextContent)
require.True(t, ok)
assert.Contains(t, tc.Text, "# Skills")
assert.Equal(t, tc.Text, output.Instructions)

It is the only test in read_skill_test.go that touches result.Content (checked against main @ 2487d69).

That matters for your fourth option. Moving the body out of the typed output breaks the assert.Equal, so that test has to be rewritten rather than extended — and the fact that someone named a test RawTextInContent and pinned the raw-markdown copy on purpose is probably the strongest argument for that option: the unescaped markdown in Content reads like a deliberate choice, and the structured copy is the accident.

[dev-aditya-hub]

Thanks @aryangorde8 — good catch, you're right. TestReadSkillHandler_RawTextInContent does pin the Content half and asserts the two copies are equal, so my line about the tests only covering output.Instructions was wrong. Agreed it argues for option 4: that test looks like a deliberate choice to keep the raw markdown, which makes the structured copy the accident. It'll need rewriting rather than extending. Appreciate you digging into it.

@kashvipeehu24 thanks for offering — I'm already partway through this one and would like to see it through since I filed it. Happy to tag you on the PR for review if you'd like.

[kashvipeehu24]

Sure, I’d be happy to review it. Please tag me once the PR is ready. Thanks!

[aryangorde8]

@dev-aditya-hub @kashvipeehu24 — flagging something before effort gets duplicated: there is already an open PR implementing option 4, #9299, opened 2026-08-11 21:19 UTC.

Its diff is the option-4 change:

• read_skill.go keeps Content: []mcp.Content{&mcp.TextContent{Text: content}} and returns types.ReadSkillOutput{Path: input.Path, Truncated: n > int(h.maxFileSize)}
• types/read_skill.go replaces Instructions string with Path string + Truncated bool
• read_skill_test.go drops assert.Equal(t, tc.Text, output.Instructions) from TestReadSkillHandler_RawTextInContent, which is the rewrite that option 4 forces

It does not reference this issue, so it does not show up as a linked PR here — which is probably why it has not come up in the thread.

Worth sorting out who carries it before more time goes in. One thing that would help either way: #9299's description is about routing custom/<skill-name>/... paths between the custom and built-in skill trees, which is not what its diff does. A reviewer reading the description will be looking for the wrong change.

Also still open, and worth settling before either version lands: none of us in this thread is a maintainer, so option 4 is still an unratified choice. It changes ReadSkillOutput's public schema, dropping instructions in favour of path + truncated, and any client reading structuredContent.instructions breaks. That seems worth an explicit maintainer ack rather than inferring it from a test name — including mine, since I am the one who argued the test name implies intent.

[dev-aditya-hub]

Thanks @aryangorde8 — useful catch on #9299, and I agree on the ack. Dropping instructions for path + truncated changes the advertised output schema, so it shouldn't land on a test name and three non-maintainers agreeing. @yurishkuro @jkowall, could you confirm option 4 is the shape you want before either version goes further?

@kashvipeehu24 — heads up that #9299 doesn't build against main. Applying its diff and running the package:

read_skill_test.go:91:10:  declared and not used: output
read_skill_test.go:129:41: out.Instructions undefined
read_skill_test.go:135:45: out.Instructions undefined
read_skill_test.go:141:53: out.Instructions undefined
FAIL  .../internal/handlers [build failed]

TestReadSkillHandler_DispatchesByPrefix still reads out.Instructions in three subtests, and removing the last use of output from RawTextInContent leaves it unused. A local go test on that package will show it.

One other thing while you're in there: the rewritten RootSkillMD and SubSkillMD assert only output.Path and output.Truncated, so they'd pass if the handler returned an empty file. Those body assertions are worth keeping, just moved onto result.Content — and something that fails if the body reappears in the structured output would be worth adding, since that's the actual bug.

On who carries it: I filed this issue and had a finished branch before #9299 opened, and said upthread I wanted to see it through, so I've put mine up as #9307. It keeps the body assertions, adds the regression test, and holds 100% coverage on the changed file. I'd rather not spend the thread on precedence though — happy either way as long as one correct version lands, and glad to review yours.