diff --git a/src/tools/platform/skills.ts b/src/tools/platform/skills.ts index 7153370d..ac247f11 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"; @@ -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); 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..520ba999 100644 --- a/test/unit/tools/platform/skills-tools.test.ts +++ b/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 () => {