fix(skills): skills__read returns full body in content, not just structuredContent

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<string, unknown>,
             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";
@@ -1036,10 +1047,45 @@ 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(" · ");
 }
 
+// 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.
+//
+// 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) {
+    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);

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(

test/unit/tools/platform/skills-tools.test.ts

@@ -407,6 +407,22 @@ 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");
+    // `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."));
   });
 
   test("skill:// URI → resolves the authoring guide", async () => {