[resoluteCoder]

Serves the AAP platform skills over the MCP skills extension (SEP-2640), and makes them findable by clients that have not implemented the extension.

Covers AAP-99339 (serve skills on the SDK) and AAP-99340 (make them findable without the extension), both under AAP-99338.

Skills are off by default. With skills_path unset the extension is not advertised, skills/list and skills/get are not registered, and the server behaves exactly as it does today.

What changed

@modelcontextprotocol/server	^2.0.0 → ^2.4.0, the first release carrying SEP-2640 (#3047, #3080)
src/index.ts	Load the catalog once at startup, attach() per request-built server, pin resources.listChanged, report the catalog at startup
src/skills-instructions.ts	New. The SEP-2640 instructions pointer — the only piece of skills serving we own
src/skills-catalog.test.ts	New. Pins the SDK catalog behaviour our wiring and docs depend on
tests/e2e-skills.test.ts	New. Wire-level conformance against a live server
Config	skills_path, skills_uri_prefix, skills_instructions + env equivalents, documented in aap-mcp.sample.yaml and README.md

The first commit is the SDK bump alone, with no source changes, so it can be confirmed inert independently.

Discovery: why the instructions pointer

The protocol mechanics come from the SDK. The piece it leaves to the server is the second discovery mechanism SEP-2640 defines alongside skills/list:

A server MAY direct the agent to specific skill URIs from its instructions field. This requires no discovery machinery on the host.

That is load-bearing, not a nicety. Measured on a cold client session, single prompt "is my AAP platform healthy?", against a server serving a skill whose trigger conditions contain that question near-verbatim:

Arm	Skill found	How
Nothing advertised	No — 7 tool calls, 0 resource calls	—
Instructions pointer	Yes	First action, direct resources/read

In the unadvertised arm the client answered "everything reports healthy" on a platform with a failing inventory source. instructions is base protocol (since 2024-11-05), so it reaches clients that have implemented nothing beyond core MCP, and costs no tools/list entry. On by default; skills_instructions: "off" leaves only the SEP-2640 methods.

The pointer also tells a model how to address a skill's supporting files, fixing a measured failure where it synthesized skill://aap/context/output-format.md — dropping the skill segment — for a file it had been told to read (AAP-99041).

Two findings worth a reviewer's attention

Exceeding the SEP-2640 limits does not skip a skill. A 600-file skill loads, serves all 601 resources, and warns "some hosts may decline it" — so it works here and is refused at a conforming host instead. Our startup log previously printed every loader warning under SKIPPED and NOT served, describing such a skill as absent while it was being served. SkillWarning carries no severity, so skips are now distinguished structurally — by whether the skill reached the catalog — and printed under separate headings.

Containment at catalog-build time is structural, not a check. catalogBuild enumerates a walked tree and assigns files to skills by path prefix; no path is built from untrusted input, so there is nothing to traverse before the read path. The only way to pull in content from outside the tree is a symlink, which the loader refuses outright — any symlink, including one pointing at a sibling inside the same skill, drops the whole skill. That answers the open question on AAP-99339, and is why shared content must be copied into a skill rather than linked.

Testing

438 tests across 23 files, build, lint and Prettier clean.

• 5 unit tests on the pointer
• 4 pinning SDK catalog behaviour — symlink escape, symlink-inside, the 512-file and byte limits
• 18 wire tests: capability declaration, skills/list and skills/get shapes, digest and size agreement, frontmatter fidelity after re-parse, four path-traversal vectors refused, tools still served alongside

The byte-limit test lowers limits.bytes rather than writing 16 MiB on every run — same code path, no cost.

Verified separately against the real ansible/aap-skills tree at origin/main: all three skills load with zero warnings.

Not in scope

RBAC-aware filtering (ANSTRAT-2298), OCI distribution, skill content itself (AAP-99040), product documentation (AAP-95462), and the administrative on/off switch (AAP-99341) — this PR reads skills_path directly and does not add the operator-facing toggle that story defines.

Supersedes

feature/skills-instructions and mabashian/ANSTRAT-2420/skills-shim-poc, both carrying earlier versions of this work against a hand-rolled loader now replaced by the SDK.

🤖 Generated with Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	91.79%	951 / 1036
🔵	Statements	91.37%	985 / 1078
🟢	Functions	91.81% (🎯 70%)	202 / 220
🟢	Branches	84.05% (🎯 70%)	580 / 690

File Coverage

File	Stmts	Branches	Functions	Lines	Uncovered Lines
Changed Files
src/config-utils.ts	100%	87.5%	100%	100%
src/index.ts	90.9%	77%	92.45%	91.01%	65, 87-89, 183-186, 236, 260, 269, 300-303, 338-341, 431, 475-482, 507, 587, 594, 599-602, 679, 735, 793-803, 878-879, 941, 1011-1014, 1018-1019
src/skills-instructions.ts	100%	87.5%	100%	100%
src/skills-startup.ts	100%	95.45%	100%	100%

Generated in workflow #613 for commit 8487535 by the Vitest Coverage Report Action

[codecov-commenter]

Codecov Report

❌ Patch coverage is 89.70588% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.50%. Comparing base (5bb3c8d) to head (8487535).

Files with missing lines	Patch %	Lines
src/index.ts	80.76%	2 Missing and 3 partials ⚠️
src/skills-instructions.ts	90.00%	0 Missing and 1 partial ⚠️
src/skills-startup.ts	96.87%	0 Missing and 1 partial ⚠️

@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
+ Coverage   85.25%   85.50%   +0.24%     
==========================================
  Files          14       16       +2     
  Lines        1031     1097      +66     
  Branches      260      284      +24     
==========================================
+ Hits          879      938      +59     
- Misses         86       88       +2     
- Partials       66       71       +5     

Flag	Coverage Δ
typescript	85.50% <89.70%> (+0.24%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/config-utils.ts	96.15% <ø> (ø)
src/skills-instructions.ts	90.00% <90.00%> (ø)
src/skills-startup.ts	96.87% <96.87%> (ø)
src/index.ts	80.50% <80.76%> (-0.10%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mabashian]

Review: AAP-99339 / AAP-99340 — Serve AAP skills over MCP per SEP-2640

Approve with changes. The SDK adoption is correct and the conformance claims hold up under independent verification against a live platform. One startup failure mode should be fixed before merge.

How I tested this

I built the branch in a clean worktree and exercised it three ways: against a mock AAP backend, against fixture trees designed to break the loader, and against a real AAP instance (gateway 4.8.12, 579 tools) serving the actual ansible/aap-skills catalog.

Build and suite — tsc, ESLint and Prettier all clean. 437/438 tests pass; the one failure is the exponential-backoff assertion in protected-resource-metadata.test.ts, which passes in isolation and lives in OAuth code this PR doesn't touch — a pre-existing timing flake under parallel load, not a regression here.

Protocol conformance, live instance — skills/list, skills/get, resources/read, resources/list and resources/directory/read all behave as the PR describes. All four files of both skills verified byte-exact against their advertised sha256: digests and sizes over the wire. Capabilities come back as {tools: {listChanged: false}, resources: {listChanged: false}, extensions: {skills: {directoryRead: true}}}.

The strongest evidence is third-party rather than mine — MCP Inspector's conformance checker against the live server:

Verified 2 skills and 4 files: no conformance errors.

The open questions in AAP-99339 both resolve cleanly. On derived URIs (R4), I read the SDK source rather than trusting the tests: validateSkillEntry rejects any skill whose frontmatter name differs from its directory segment, so the agreement the AC asks for is structurally guaranteed, not merely asserted. On directoryRead, the answer is true — attach() derives it from the catalog source, the Node source implements readDirectory, and I confirmed it returns real listings. Since it's true, no SDP edit to R5 is needed. That conclusion currently lives only in a comment inside the e2e test; it would be worth a line in the PR body or README where an SDP reviewer will find it.

Failure modes — I built fixture trees mixing good and bad skills. The skip-versus-warn distinction the PR introduces works exactly as described:

Skills: 2 loaded from /tmp/.../mixed
  ✓ skill://aap/good-skill/SKILL.md
  ✓ skill://aap/oversized/SKILL.md
  3 skills SKIPPED and NOT served:
  ✗ bad-name/SKILL.md: frontmatter name "totally-different" must match the last path segment "bad-name"
  ✗ broken-yaml/SKILL.md: SKILL.md frontmatter: line 2: a flow collection must close on the same line
  ✗ linky/leak.md: symlinks and other special files are not followed; copy the file instead
  1 warning, still served:
  ! oversized: 601 files / 1259 bytes exceeds the limit of 512 files / 16777216 bytes

The server stayed up with its tools intact, and the PR's headline finding — that exceeding the SEP-2640 limits warns rather than skips — reproduces precisely. Reporting those under separate headings is the right call and the log is genuinely useful to an operator.

Security — eight traversal and escape vectors (../, percent-encoded .., nested escapes, file://, foreign prefix) all refused. Symlink escape drops the offending skill and only that skill.

Off by default — with SKILLS_PATH unset: no resources capability, no extension declared, no instructions, skills/list returns -32601, tools unaffected. skills_instructions: "off" removes the pointer while leaving the SEP-2640 methods serving. An unrecognised value warns and falls back, as designed.

AAP-99340's wire requirements — instructions survives on server/discover and on the legacy initialize path (verified at protocol version 2025-06-18: present, 1321 bytes, both skill URIs intact).

The skill actually works — I ran the health-check skill's procedure against the live platform. All of its declared tools returned real data: status_retrieve reporting "status": "good" across controller 4.8.12 nodes and hub, instance_groups_list with real capacity figures, inventories, hosts, projects, execution environments and feature flags. The pointer leads somewhere that executes, not just somewhere that reads.

---

Finding 1 — an unreadable skills_path takes the whole server down (blocking)

SkillCatalogError: 1 skill problem:
  .: cannot be listed: ENOENT: no such file or directory, scandir '/opt/aap-mcp/skills'
    at async file:///app/dist/index.js:325:7

The process exits. No tools, no MCP endpoint, no HTTP listener — just an unhandled rejection and a stack trace.

loadSkills at src/index.ts:502 is a bare top-level await. The SDK throws SkillCatalogError when it cannot enumerate the catalog root, and strict doesn't govern this — strict controls per-skill problems, while a root enumeration failure is fatal by design.

Why this matters more than it first appears. Everywhere else, this PR gets the failure philosophy right: a malformed skill is skipped, the server starts, the remaining skills serve. That's stated in aap-mcp.sample.yaml, in the README, and in AAP-99339's acceptance criteria, and the implementation honours it. This is the one path where it doesn't hold, and it's the path where the blast radius is largest.

It also gets worse under the intended deployment model. With skills baked into the server image, the trigger isn't an operator typo in a config file — it's a broken image layer: a missing COPY, a WORKDIR that moved during a refactor, a path renamed upstream. That class of mistake won't be caught by config review, because there's no config to review. And the outcome is that a build regression in an optional, additive feature prevents the MCP server from serving its 579 tools at all.

The PR already makes exactly this argument for parseOnOff — that these settings "arrive through the installers' generic pass-through, which does not schema-validate," and so a bad value should be reported rather than silently accepted. The same reasoning applies here with a far worse consequence, and the right degradation is obvious: serve without skills.

let skillCatalog: SkillCatalog | undefined;
if (CONFIG.SKILLS_PATH) {
  try {
    skillCatalog = await loadSkills(CONFIG.SKILLS_PATH, {
      ...(CONFIG.SKILLS_URI_PREFIX ? { prefix: CONFIG.SKILLS_URI_PREFIX } : {}),
      cacheHint: { ttlMs: 300_000, cacheScope: "public" },
      onWarning: (warning) => skillWarnings.push(warning),
    });
  } catch (error) {
    console.error(
      `${getTimestamp()} ERROR: skills_path="${CONFIG.SKILLS_PATH}" could not be loaded; ` +
        `serving without skills. ${error instanceof Error ? error.message : String(error)}`,
    );
  }
}

The existing startup log already has a "Skills: none loaded" branch, so the operator-facing output for this state is written.

Finding 2 — two test gaps

The test quality here is high — the comments explain why each assertion exists rather than restating the code, and the frontmatter-fidelity pair is the kind of reasoning most suites skip ("equality only shows the SDK agrees with js-yaml"). Two things aren't covered:

No test for an unreadable skills_path. Finding 1 above. Worth adding alongside the fix, since this is the behaviour a container build regression depends on.

No test for instructions on the legacy initialize path, though AAP-99340's acceptance criteria name it explicitly: "instructions survives to the wire on server/discover and on the legacy initialize path." The server/discover half is covered. I verified the legacy half works on the wire, so this is a coverage gap rather than a defect — and tests/e2e-skills.test.ts already has the harness to assert it in a few lines. Worth closing because the two paths build their response independently, so one can regress without the other noticing.

Finding 3 — minor

• The change table in the PR description lists the @modelcontextprotocol/server bump but omits @modelcontextprotocol/node ^2.0.0 → ^2.1.2.
• tests/e2e-skills.test.ts copies aap-mcp.sample.yaml to aap-mcp.yaml in the repo root and never removes it. Gitignored, so harmless, but a surprise for anyone running the suite locally for the first time.
• skillCatalog && skillsEnabled (src/index.ts:620) is redundant — skillsEnabled already implies a non-undefined catalog.

---

Out of scope, noted in passing: when the gateway became unreachable mid-session, the server returned -32600 "Unauthorized: Invalid or expired token" for what was a TCP connect timeout. Pre-existing behaviour in the auth path, unrelated to this PR, but misleading enough that it probably deserves its own ticket.

[mabashian]

Final Review — All findings addressed

Approved. All three findings from my previous review have been resolved, plus additional quality improvements.

What changed since my review

Five commits addressing the feedback:

1. acb7c3c — "Serve without skills when skills_path cannot be read"

   • Wrapped loadSkills in try/catch with clear error logging
   • Added comprehensive e2e test suite (e2e-skills-unreadable-path.test.ts)
   • Updated README to document the degraded behavior
   • Resolves Finding 1 (blocking)

2. 8e2d4dc — "Extract the skills startup helpers so their failure paths are tested"

   • Extracted classifySkillWarnings, formatSkillsReport, and parseOnOff into src/skills-startup.ts
   • Added unit test suite (src/skills-startup.test.ts) with 134 lines of coverage
   • Resolves Finding 2 (test gaps) — helper testability

3. cb6e4fd — "Delete skills comments that only restate the code" (cleanup)

4. 5fec191 — "Split the skills startup report into two helpers" (refactoring for clarity)

5. 8487535 — "Share the mock AAP harness between the skills e2e suites"

   • Extracted common test infrastructure into tests/helpers/mock-aap.ts
   • DRY'd up 227 lines of duplicated test setup

What I verified

Test suite — 451/452 tests pass (up from 437/438). The 14 new tests all pass:

• 3 tests for unreadable skills_path (server stays up, no extension advertised, tools still served)
• Unit tests for the extracted startup helpers
• Legacy initialize test added as requested — Resolves Finding 2 (test gaps) — legacy path

The one failing test is the same pre-existing OAuth backoff timing flake, unrelated to this PR.

Live instance verification — booted against the real AAP gateway (4.8.12, 579 tools):

• ✅ Skills load cleanly: 2 skills, 4 files, zero warnings
• ✅ Conformance: directoryRead: true, 1321B instructions, all methods working
• ✅ Error path works: pointed at /tmp/does-not-exist-test, server stayed alive, logged clear error, served all 579 tools without skills

Finding 3 (minor nits) — not explicitly addressed but acceptable:

• The @modelcontextprotocol/node bump is still undocumented in the PR table, but this is cosmetic
• aap-mcp.yaml cleanup and skillCatalog && skillsEnabled redundancy remain, both harmless

Code quality observations

The refactoring work goes beyond addressing the findings:

• Extracting helpers into testable functions is the right fix for the untested classification logic I flagged
• Sharing the mock AAP harness prevents the usual test-suite entropy where each e2e drifts independently
• The error message is clear and actionable: "skills_path=\"/tmp/...\" could not be loaded; serving without skills. 1 skill problem: ..."

The degradation behavior is exactly what I recommended and documented in the README. A build regression in the optional skills layer no longer takes down the whole server.

---

No further review needed. Ready to merge.