fix(skills): skills__read returns full body in content, not just structuredContent#365
Merged
Merged
Conversation
…cturedContent The engine surfaces only a tool result's `content` to the model; `structuredContent` reaches /mcp clients and the UI but never the in-process agent loop. `skills__read` put the skill body and manifest only in `structuredContent` and a one-line header in `content`, so an in-agent caller saw `"<name> (L3 workspace) · loads: always"` and could not read the skill — despite the tool description promising "the markdown body plus parsed manifest fields." `summarizeList` was already corrected for this exact constraint (its comment notes structuredContent "the engine doesn't surface to the model"); `read` was the lone straggler. Sibling read tools `files__read` and `conversations__get` already embed their body in `content`. Add `renderRead`: header, then the promised manifest fields, then the full markdown body. Manifest leads and body trails so a body over MAX_TOOL_RESULT_CHARS is trimmed tail-first, keeping metadata intact (same small-first ordering as conversations__get). `structuredContent` is unchanged for typed consumers. Trim the description's field list to the fields actually returned. Regression tests assert the body and manifest reach the model-visible content, not only structuredContent.
Review follow-up. `renderRead` inherited `summarizeRead`'s status !== "active" guard, so the model-visible content omitted `status` for active skills even though the description promises it — the same description-vs-model-reality gap this PR exists to close. `status` is always populated (`buildReadResult` defaults it to "active") and is a primary audit signal, so emit it as an unconditional manifest field. Drop status from the header line to avoid double-rendering on disabled skills; add a sync-contract comment on `renderRead`'s explicit field list (deliberate subset; keep aligned with the description and SkillDetail.metadata). Test asserts `status: active` reaches the model-visible content.
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.
Problem
skills__readreturns only a one-line header to the model — e.g.<name> (L3 workspace) · loads: always— and the full skill body is missing. This blocks an in-platform agent from reading or auditing any skill it doesn't already have loaded, despite the tool description promising it "Returns the markdown body plus parsed manifest fields."Root cause
The engine surfaces only a tool result's
contentto the model (engine.ts:modelOutput ?? boundToolResultForModel(extractTextForModel(result.content), …)).structuredContentreaches/mcpclients and the UI but never the in-process agent loop.skills__readput the body + manifest only instructuredContentand a one-line summary (summarizeRead) incontent— so the body was structurally invisible to the agent.This is a stale-by-omission bug, not a design choice:
summarizeListwas already fixed for this exact constraint — its comment reads "…without depending on structuredContent (which the engine doesn't surface to the model)."readwas never given the same treatment.files__read,files__read_pdf_pages, andconversations__getalready embed their body incontent.skills__readwas the lone straggler.Fix
renderRead: header → promised manifest fields → full markdown body, incontent. Manifest leads and the body trails so a body exceedingMAX_TOOL_RESULT_CHARS(50k) is trimmed tail-first, keeping metadata intact (same small-first ordering asconversations__get).structuredContentis unchanged — typedSkillsReadOutputstill serves/mcpclients and the UI.SKILLS_READ_DESCRIPTION's field list to the fields actually returned (droppedversion,allowed_tools,requires_bundles,metadata, which aren't onSkillDetail).structuredContent.Tests
Regression assertions in both the unit and integration read tests confirm the body and manifest fields reach the model-visible
content(viaextractText/ thecontenttext blocks), not onlystructuredContent— and that manifest precedes the body. Existing structuredContent assertions are retained.bun run verifystatic gate (tsc strict, lint, format, cycles) is green; targeted skills tests pass (22/22). The one unrelated failure in the run (automations/markdown.test.ts— missingdompurifybundle-UI dep) reproduces on the untouched base and is environmental to the worktree.