Serve Supabase agent skills over MCP (SEP-2640, preview) - #437
claude[bot] wants to merge 2 commits into
Conversation
…iew) Adds a `skills` option to `createSupabaseMcpServer` that fetches Supabase's published agent-skills index (supabase.com/.well-known/agent-skills), downloads and digest-verifies each skill's tarball, unpacks it in memory, and serves it over the MCP Skills extension (SEP-2640): `skills/list`, `skills/get`, and the individual files via `resources/read`. A TTL cache (`createSkillsProvider`) avoids re-fetching/re-unpacking on every call. Built on the typescript-sdk Skills extension preview (modelcontextprotocol/typescript-sdk#2818, phase 1: schemas + `installSkills`). That phase only takes a static skill list at install time, so `installSupabaseSkills` calls it once for capability declaration and wire validation, then overrides both handlers to resolve live from the TTL cache on every call — the point of this feature, so a new skill release needs no mcp-server-supabase release. Off by default (undefined `skills` option) since it depends on an unreleased SDK preview build, pinned exactly via pkg.pr.new rather than a floating tag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REAnGWxMnmwfehoF4c4VKS
|
The failing That job regenerates Verified independently: running Not fixing it in this PR since it's an unrelated, orthogonal change (regenerated API types, no connection to the skills feature). Will retrigger the check now that this is confirmed as base-branch drift; a maintainer may want a separate PR running 🤖 Generated with Claude Code |
Coverage Report for CI Build 35396694366Coverage decreased (-0.3%) to 96.761%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
The Check Management API Types job (check_run 105714979019) failed, but it's unrelated to this PR's changes. This PR does not touch - schema_version: string;
+ schema_version?: string;I reproduced this locally on both this branch and |
Rodriguespn
left a comment
There was a problem hiding this comment.
Honestly I was expecting that the mcp ask did more heavy lifting that this preview version seems to do. Hopefully the one that ends up published can help us reduce the amount of boiler plate code in this PR.
| }, | ||
| "pnpm": { | ||
| "overrides": { | ||
| "@modelcontextprotocol/core": "https://pkg.pr.new/@modelcontextprotocol/core@b0091060d73d08211c6766990aa15656d4e03271", |
There was a problem hiding this comment.
Replace for the published version once this version of the mcp sdk lands
There was a problem hiding this comment.
Confirmed, that's already the plan — no code change here for now. Once modelcontextprotocol/typescript-sdk#2818 ships a real semver release (not just the pkg.pr.new preview), the follow-up swaps these three pnpm.overrides entries for a normal catalog: range. That's called out explicitly in the PR description's "Additional context" and "Follow-ons" sections so it doesn't get lost.
I looked for a way to add an inline comment directly in package.json noting the pin is temporary, but JSON has no comment syntax and this repo doesn't use JSON5/JSONC anywhere (package.json is also excluded from biome's formatting/linting). Since the temporary nature is already documented in the PR description rather than left implicit, I didn't add a workaround like a stray _comment key inside pnpm.overrides. Happy to add a note elsewhere (e.g. CONTRIBUTING.md) if you'd find that clearer than the PR description.
Generated by Claude Code
| import { z } from 'zod/v4'; | ||
| import { AGENT_SKILLS_INDEX_URL } from './constants.js'; | ||
|
|
||
| const SHA256_DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; |
There was a problem hiding this comment.
I think we can use the crypto package to deal with the digest instead of using regex
There was a problem hiding this comment.
Checked this against what crypto actually offers, and I don't think it's a good swap here — SHA256_DIGEST_PATTERN (and the zod .regex() built on it) is validating the string format of an untrusted, externally-supplied field (sha256: + 64 lowercase hex chars) before we do anything else with it. crypto doesn't have an API for "is this string shaped like sha256:<hex>" — Buffer.from(x, 'hex') silently drops invalid characters instead of rejecting them, so it wouldn't actually catch a malformed digest string the way the regex does, and crypto.timingSafeEqual is for constant-time comparison of two known buffers (relevant for secret/credential comparisons), not format validation — and these digests aren't secrets anyway.
The crypto module is already doing the actual digest work elsewhere in this file (sha256() at line 98 uses createHash('sha256'), and downloadTarball() compares the computed digest against the published one). That's the digest computation concern; the regex is a separate, narrower format-validation concern at the zod-schema boundary. So I kept the regex as-is rather than replacing it — let me know if you had a specific crypto API in mind that I'm missing.
Generated by Claude Code
| type Server, | ||
| } from '@modelcontextprotocol/server'; | ||
| import { installSkills } from '@modelcontextprotocol/server/ext/skills'; | ||
| import { resource, type Resource } from '@supabase/mcp-utils'; |
There was a problem hiding this comment.
Maybe the skill logic should live inside mcp-utils, just like resources does
There was a problem hiding this comment.
That's a fair question, and I don't think it's a small nit, so I'm not pushing a refactor into this PR — flagging the split for you to decide instead.
What's genuinely Supabase-specific in skills/:
- The
.well-known/agent-skillsdiscovery-index fetch and its URL (constants.ts'sAGENT_SKILLS_INDEX_URL) — that's Supabase's own published index, not a generic concept. - The fact that the content served is Supabase's own skills (docs/support workflows), as opposed to something a generic consumer would bring.
What's generic/reusable, and has nothing Supabase-specific in it:
manifest.ts: tarball download + digest verification + gunzip/untar + per-file digest/size computation — this is "given an index of{name, url, digest}entries, fetch/verify/unpack them into askills/list-shaped manifest," which has no Supabase-specific logic once the index URL is a parameter (it already is, viaFetchSkillsManifestOptions.indexUrl).provider.ts: the TTL cache + in-flight de-duplication wrapper — generic caching, no Supabase dependency at all.mcp.ts: theinstallSkills()-once-then-override-handlers SDK integration, andgetSkillsResources()'s dynamicmcp-utilsresource list — this is exactly the kind of SDK-wiringresource()/resourceTemplate()already lives inmcp-utilsfor.
Recommendation: the generic caching/manifest/SDK-wiring pieces (provider.ts, most of manifest.ts, mcp.ts) could move to @supabase/mcp-utils as a reusable installSkills-style helper parameterized by an index URL and fetch function, keeping only the Supabase-specific discovery-index constant and the "these are Supabase's skills" wiring in mcp-server-supabase. That's a genuine cross-package move though (new public API surface in mcp-utils, changed imports in mcp-server-supabase), so I'd rather do it as a focused follow-up PR than fold it into this one — happy to pick it up once this lands, if that's the direction you want.
Generated by Claude Code
| } | ||
|
|
||
| if (totalBytes > MAX_SKILL_TOTAL_BYTES) { | ||
| throw new Error( |
There was a problem hiding this comment.
Should this error or just ignore this skill? Also, the spec doesn't forbid large skills.Host may deal or not with large skills
There was a problem hiding this comment.
Good catch — you're right, and I checked the actual spec text to confirm. SEP-2640 says:
Servers SHOULD NOT exceed 512 files or 16 MiB per skill, including
SKILL.md. Hosts MUST support skills up to these limits and MAY support larger skills.
So this is a SHOULD NOT aimed at server authors, not a hard cap, and hosts are explicitly permitted to go over it. The old code (throw new Error(...exceeding the SEP-2640 limit...)) mischaracterized this as a protocol violation, and as a bonus bug it would have aborted the whole Promise.all batch — one oversized skill would have taken down every other skill in the manifest too.
Pushed a fix in 82e3482:
buildSkill()now skips an oversized skill (returnsundefined) and logs a warning viaconsole.error, instead of throwing.- Added a code comment at the check explaining this is a local practical safeguard against unbounded memory use unpacking a whole tarball at once, not a spec requirement — nothing stops us from serving larger skills if we later want to (e.g. by streaming/paginating instead of skipping).
fetchSkillsManifest()filters the skipped entries out of the manifest rather than serving a hole.- Added
manifest.test.tscoverage: an oversized skill is skipped with a logged warning, and a sibling properly-sized skill in the same index is still served.
Full unit suite (424 tests) and typecheck are green after the change.
Generated by Claude Code
SEP-2640 says servers SHOULD NOT exceed 512 files or 16 MiB per skill, but explicitly does not forbid larger ones — hosts MUST support skills up to the limit and MAY support larger ones. The prior code treated this as a hard spec-mandated error and threw, which would also abort fetching every other skill in the same Promise.all batch. buildSkill() now skips (returns undefined for) an oversized skill and logs a warning via console.error, as a local practical safeguard against unbounded memory use unpacking a tarball in memory, not a protocol violation. fetchSkillsManifest() filters skipped skills out of the manifest rather than serving a hole. Added a test covering the skip-and-warn behavior and that a sibling properly-sized skill is still served. Addresses Pedro's review comment on PR #437. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REAnGWxMnmwfehoF4c4VKS
|
Agreed — this matches what we found digging into the typescript-sdk PR. Generated by Claude Code |
Requested via Slack thread
What kind of change does this PR introduce?
Feature (draft, built on an unreleased typescript-sdk preview).
What is the current behavior?
mcp-server-supabasehas no way to expose Supabase's agent skills (supabase.com/.well-known/agent-skills) to an MCP client. The skills content only reaches agents that separately install a plugin/skill package (e.g.supabase-community/supabase-plugin); there's no MCP-native discovery path. See Linear AI-1230.What is the new behavior?
Adds an opt-in
skillsoption tocreateSupabaseMcpServer. When set, the server:supabase.com/.well-known/agent-skills..tar.gz, verifies it against the index's published sha256 digest, and unpacks it in memory (no filesystem writes)."dynamic", since Supabase's skill releases are versioned and stable per release).skills/listandskills/getfrom that manifest, and the individual files (SKILL.md,references/*.md, etc.) overresources/read.Off by default — the
skillsoption isundefinedunless a caller opts in — since this currently depends on a preview build of the typescript-sdk Skills extension (see below), not a stable release.cli.ts/http.ts/local-http-entry.tsare unchanged; wiring this on for a given deployment is a one-line follow-up once typescript-sdk publishes the real release.Existing tools (
search_docs, etc.) are unchanged.Follow-ons, explicitly out of scope here:
search_docs's embedded GraphQL schema into a served skill resource instead of always being in the tool description (saves context on everytools/list).SKILL.mdYAML frontmatter for fields beyondname/description(the discovery index already carries those two, sourced from the same frontmatter, so this PR uses them directly rather than re-parsing YAML — see the comment inmanifest.ts).pkg.pr.newpreview pin for a realcatalog:range once typescript-sdk#2818 ships stable.How to Review
Wiring / entrypoint
packages/mcp-server-supabase/src/server.ts— the newskillsoption onSupabaseMcpServerOptions, and the two call sites:resources: skillsProvider && getSkillsResources(skillsProvider)passed intocreateMcpServer, andinstallSupabaseSkills(server, skillsProvider)called after it.The SDK integration point (the part most worth scrutinizing)
packages/mcp-server-supabase/src/skills/mcp.ts—installSupabaseSkills.installSkills()(typescript-sdk#2818, phase 1) only accepts a static skill list captured at install time and can't be re-registered after the server connects. To still answer every request from the live TTL-cached manifest, this callsinstallSkills()once (for capability declaration + wire-schema validation) with an empty snapshot, then immediately replaces both handlers with versions that read through the liveSkillsProvider. This relies onServer#setRequestHandlerhaving no re-registration guard — see the docblock for the full reasoning.getSkillsResourcesis the mcp-utils-side counterpart: a dynamicresourceslist resolved fresh on everyresources/list/resources/read.Fetch / unpack / digest
packages/mcp-server-supabase/src/skills/manifest.ts—fetchSkillsManifest: fetches the index, downloads+verifies each tarball against its published digest (same check as supabase-plugin'ssync-agent-skillsGitHub Action, ported to run at request time instead of in CI), gunzips+untars viatar-stream(no disk writes), then computes per-file sha256/size.packages/mcp-server-supabase/src/skills/provider.ts—createSkillsProvider: the TTL cache, with in-flight de-duplication so concurrent callers during a refresh share one fetch.Tests
packages/mcp-server-supabase/src/skills/manifest.test.ts— digest verification, missing-SKILL.mdrejection, malformed index rejection, correct per-file digest/size/URI construction.packages/mcp-server-supabase/src/skills/provider.test.ts— TTL expiry/no-expiry,invalidate(), concurrent-call de-duplication, retry-after-failure (no caching a rejection).packages/mcp-server-supabase/src/skills/server.test.ts— end-to-end over a realClient/Serverpair (StreamTransport, same pattern asserver.test.ts): capability negotiation,skills/list,skills/get(including the-32602unknown-URI case), andresources/readserving bytes that match the published digest.Review questions
installSkills()once and then overriding its two handlers (rather than not usinginstallSkills()at all) the right call, given phase 1's static-list API?skillsdefault to enabled somewhere (e.g. the hosted HTTP entry) in a follow-up PR, or stay purely opt-in until typescript-sdk#2818 ships stable?@modelcontextprotocol/core/client/serverto an exactpkg.pr.newcommit (via a rootpnpm.overrides, to keep one canonical instance workspace-wide and avoid duplicate-package type mismatches) an acceptable temporary state for a draft PR?buildSkill)?Verification
pnpm --filter @supabase/mcp-server-supabase typecheck— clean.pnpm --filter @supabase/mcp-server-postgrest typecheck— clean (unaffected by the workspace-widepnpm.overrides).CI=true npx vitest run --project unitinpackages/mcp-server-supabase— 423 passed (408 pre-existing + 15 new), 0 failed, 0 skipped.pnpm --filter @supabase/mcp-utils test— 13 passed (unaffected).pnpm --filter @supabase/mcp-server-supabase build— tsup build succeeds; confirmed the skills module is bundled into the shared chunk bothdist/cli.jsanddist/cli.cjsimport, andSkillsProviderOptionsappears in the publisheddist/index.d.ts.pnpm biome check --write— applied, clean.Additional context
b0091060d73d08211c6766990aa15656d4e03271viapkg.pr.new(verified reachable for@modelcontextprotocol/core,/client, and/server). That PR is itself a draft; this PR will need a follow-up once it (or its stable release) ships, to swap thepkg.pr.newpins for a realcatalog:semver range.supabase-community/supabase-plugin's.github/workflows/sync-agent-skills.yml(gh release download→curlthe tarball → compare sha256 →tar -xzf), reimplemented in Node so it runs at request time in the MCP server process instead of in a GitHub Action at release time.🤖 Generated with Claude Code
https://claude.ai/code/session_01REAnGWxMnmwfehoF4c4VKS
Generated by Claude Code