From 91ee5b4c95e68680ab06e04afc27ab7089543cf0 Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Tue, 2 Jun 2026 07:54:43 -1000 Subject: [PATCH 1/2] fix(skills): skills__read returns full body in content, not just structuredContent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `" (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. --- src/tools/platform/skills.ts | 57 +++++++++++++++---- test/integration/skills-read-tools.test.ts | 10 ++++ test/unit/tools/platform/skills-tools.test.ts | 13 +++++ 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/src/tools/platform/skills.ts b/src/tools/platform/skills.ts index 7153370d..60ca19de 100644 --- a/src/tools/platform/skills.ts +++ b/src/tools/platform/skills.ts @@ -78,10 +78,10 @@ const SKILLS_LIST_DESCRIPTION = const SKILLS_READ_DESCRIPTION = "Read one skill by id. The `id` is either a filesystem path (returned by `skills__list`) " + - "or a bundle skill:// URI. Returns the markdown body plus parsed manifest fields (name, " + - "description, version, type, priority, scope, layer, loading_strategy, applies_to_tools, " + - "status, allowed_tools, requires_bundles, metadata). Always call `skills__list` first to " + - "discover ids — bare names and scope-prefixed forms (e.g. `org/foo`) are NOT valid input."; + "or a bundle skill:// URI. Returns the full markdown body plus parsed manifest fields (name, " + + "description, type, priority, scope, layer, loading_strategy, applies_to_tools, status). " + + "Always call `skills__list` first to discover ids — bare names and scope-prefixed forms " + + "(e.g. `org/foo`) are NOT valid input."; const SKILLS_ACTIVE_FOR_DESCRIPTION = "Show which Layer 3 skills are currently loaded for a conversation. " + @@ -263,7 +263,11 @@ export function createSkillsSource(runtime: Runtime, eventSink: EventSink): McpS // the same shim explained in the `list` handler. const out: SkillsReadOutput = result; return { - content: textContent(summarizeRead(result)), + // `content` carries the full body + manifest because the + // engine never surfaces `structuredContent` to the model; + // `structuredContent` keeps the typed copy for `/mcp` clients + // and the UI. See `renderRead`. + content: textContent(renderRead(result)), structuredContent: out as unknown as Record, isError: false, }; @@ -1000,12 +1004,19 @@ function errorResult(err: unknown): ToolResult { }; } -// ── Human-readable summaries for tool `content` field ────────────────── +// ── Human-readable renderings for tool `content` field ───────────────── // -// Per AGENTS.md, `content` carries a short human-readable summary; the -// full structured payload lives only in `structuredContent`. Each summary -// is one or two short lines optimized for a debug log or a CLI render — -// agents and UI clients consume the structured form. +// The engine surfaces only `content` to the model — `structuredContent` +// reaches `/mcp` clients and the UI but never the in-process agent loop. +// So `content` must carry everything the model needs to act: +// +// - For *status/enumeration* tools (`active_for`, `loading_log`) a short +// summary line is sufficient; the model only needs the gist. +// - For *enumeration the model must read* (`list`) and *document fetch* +// (`read`) the payload itself goes in `content` — IDs for `list`, the +// full body + manifest for `read` — because the structured copy is +// invisible to the model. `files__read` and `conversations__get` embed +// their bodies the same way. function summarizeList(skills: ListedSkill[]): string { if (skills.length === 0) return "0 skills"; @@ -1040,6 +1051,32 @@ function summarizeRead(skill: ReadResult): string { return parts.join(" · "); } +// Render the full skill for the model-visible `content` field: header, +// then the parsed manifest fields the description promises, then the +// markdown body. `read`'s entire purpose is to deliver the skill to the +// caller, and the body lives only here for the in-process agent (the +// structured copy never reaches the model). Manifest leads and the body +// trails so that if `boundToolResultForModel` trims at +// MAX_TOOL_RESULT_CHARS the metadata survives and only the body tail is +// cut — the same small-first ordering `conversations__get` uses. +function renderRead(skill: ReadResult): string { + const m = skill.metadata; + const fields = [summarizeRead(skill), `id: ${skill.id}`, `source: ${skill.source}`]; + if (m.description) fields.push(`description: ${m.description}`); + if (m.type) fields.push(`type: ${m.type}`); + if (m.priority != null) fields.push(`priority: ${m.priority}`); + if (m.appliesToTools?.length) fields.push(`applies_to_tools: ${m.appliesToTools.join(", ")}`); + if (m.derivedFrom) fields.push(`derived_from: ${m.derivedFrom}`); + if (m.overrides?.length) { + const overrides = m.overrides + .map((o) => `${o.bundle ?? "?"}/${o.skill ?? "?"} — ${o.reason}`) + .join("; "); + fields.push(`overrides: ${overrides}`); + } + if (skill.modifiedAt) fields.push(`modified: ${skill.modifiedAt}`); + return `${fields.join("\n")}\n\n---\n\n${skill.content}`; +} + function summarizeActive(active: ActiveForEntry[]): string { if (active.length === 0) return "No skills loaded for this conversation yet."; const totalTokens = active.reduce((sum, a) => sum + a.tokens, 0); diff --git a/test/integration/skills-read-tools.test.ts b/test/integration/skills-read-tools.test.ts index 4b587a70..b1faacaf 100644 --- a/test/integration/skills-read-tools.test.ts +++ b/test/integration/skills-read-tools.test.ts @@ -159,6 +159,16 @@ describe("skills read tools — end-to-end", () => { expect(readSC.metadata.loadingStrategy).toBe("always"); expect(readSC.content).toContain("Speak plainly"); + // Regression (skills__read body-in-content): the engine surfaces only + // `content` to the model, never `structuredContent`. The body and the + // promised manifest fields must reach the MODEL-visible text, or an + // in-agent caller sees only the one-line header and cannot read the + // skill. `read.content` here is `extractText(result.content)` — exactly + // what the engine feeds the model. + expect(read.content).toContain("Speak plainly"); + expect(read.content).toContain("voice-rules"); + expect(read.content).toContain("loads: always"); + // skills__active_for — the most recent skills.loaded for the conv must // include voice-rules (loading_strategy: always). const active = await callTool( diff --git a/test/unit/tools/platform/skills-tools.test.ts b/test/unit/tools/platform/skills-tools.test.ts index c3de59e0..fd080949 100644 --- a/test/unit/tools/platform/skills-tools.test.ts +++ b/test/unit/tools/platform/skills-tools.test.ts @@ -407,6 +407,19 @@ describe("skills__read", () => { expect(metadata.loadingStrategy).toBe("always"); expect(sc.scope).toBe("org"); expect(sc.layer).toBe(3); + + // The model-visible `content` text (not just `structuredContent`, which + // the engine never surfaces to the model) must carry the body and the + // promised manifest fields. Manifest leads, body trails after `---`. + const text = (result.content as Array<{ type: string; text?: string }>) + .filter((b) => b.type === "text") + .map((b) => b.text ?? "") + .join("\n"); + expect(text).toContain("voice-rules"); + expect(text).toContain("loads: always"); + expect(text).toContain("priority: 25"); + expect(text).toContain("Speak plainly."); + expect(text.indexOf("priority: 25")).toBeLessThan(text.indexOf("Speak plainly.")); }); test("skill:// URI → resolves the authoring guide", async () => { From 091b6efdbf041d1b744e5849c4930e2dba34fda0 Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Tue, 2 Jun 2026 21:35:24 -1000 Subject: [PATCH 2/2] fix(skills): always render status in skills__read content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/tools/platform/skills.ts | 11 ++++++++++- test/unit/tools/platform/skills-tools.test.ts | 3 +++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/tools/platform/skills.ts b/src/tools/platform/skills.ts index 60ca19de..ac247f11 100644 --- a/src/tools/platform/skills.ts +++ b/src/tools/platform/skills.ts @@ -1047,7 +1047,6 @@ function summarizeRead(skill: ReadResult): string { const m = skill.metadata; const parts = [`${m.name} (L${skill.layer} ${skill.scope})`]; if (m.loadingStrategy) parts.push(`loads: ${m.loadingStrategy}`); - if (m.status && m.status !== "active") parts.push(`status: ${m.status}`); return parts.join(" · "); } @@ -1059,12 +1058,22 @@ function summarizeRead(skill: ReadResult): string { // trails so that if `boundToolResultForModel` trims at // MAX_TOOL_RESULT_CHARS the metadata survives and only the body tail is // cut — the same small-first ordering `conversations__get` uses. +// +// The field list is explicit (not a spread of `metadata`) so it stays a +// deliberate subset — the same minimum-sufficient-surface contract the +// schema uses. Keep it in sync with `SKILLS_READ_DESCRIPTION` and +// `SkillDetail.metadata` when adding a field: type, description, renderer, +// and tests move together. function renderRead(skill: ReadResult): string { const m = skill.metadata; const fields = [summarizeRead(skill), `id: ${skill.id}`, `source: ${skill.source}`]; if (m.description) fields.push(`description: ${m.description}`); if (m.type) fields.push(`type: ${m.type}`); if (m.priority != null) fields.push(`priority: ${m.priority}`); + // `status` always renders — `buildReadResult` defaults it to "active", so + // the description's promise of a `status` field holds for every skill + // (an audit's first question is "active or disabled?"). + if (m.status) fields.push(`status: ${m.status}`); if (m.appliesToTools?.length) fields.push(`applies_to_tools: ${m.appliesToTools.join(", ")}`); if (m.derivedFrom) fields.push(`derived_from: ${m.derivedFrom}`); if (m.overrides?.length) { diff --git a/test/unit/tools/platform/skills-tools.test.ts b/test/unit/tools/platform/skills-tools.test.ts index fd080949..520ba999 100644 --- a/test/unit/tools/platform/skills-tools.test.ts +++ b/test/unit/tools/platform/skills-tools.test.ts @@ -418,6 +418,9 @@ describe("skills__read", () => { expect(text).toContain("voice-rules"); expect(text).toContain("loads: always"); expect(text).toContain("priority: 25"); + // `status` is promised by the description and must render even for an + // active skill (the default) — not only when non-active. + expect(text).toContain("status: active"); expect(text).toContain("Speak plainly."); expect(text.indexOf("priority: 25")).toBeLessThan(text.indexOf("Speak plainly.")); });