feat(core): BaseContext.ext / RequestEnv.ext plugin slot (SEP-2133 mechanism)#2061
feat(core): BaseContext.ext / RequestEnv.ext plugin slot (SEP-2133 mechanism)#2061felixweinberger wants to merge 3 commits into
Conversation
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * @example | ||
| * ```ts | ||
| * // In a DispatchMiddleware: | ||
| * env.ext = { ...env.ext, 'io.modelcontextprotocol/task': { id, store } }; | ||
| * // In a handler: | ||
| * const task = taskContext(ctx); // typed read of ctx.ext['io.modelcontextprotocol/task'] | ||
| * ``` |
There was a problem hiding this comment.
🟡 nit: per CLAUDE.md "JSDoc @example Code Snippets", @example blocks should use ```ts source="./file.examples.ts#region" fences so pnpm sync:snippets keeps them type-checked — every other @example in packages/core/src follows this, but this one is an inline fence referencing symbols (env, id, store, taskContext) that do not exist yet (tasksPlugin ships in F3). Consider either rewriting the example around readExt() (which this PR does export) in a context.examples.ts //#region, or dropping the @example tag until F3 lands so the snippet can be sourced and type-checked.
Extended reasoning...
What this is
CLAUDE.md:44-48 documents the repo's @example convention:
JSDoc
@exampletags should pull type-checked code from companion.examples.tsfiles … Use```ts source="./file.examples.ts#regionName"fences referencing//#region regionNameblocks … Runpnpm sync:snippetsto sync example content into JSDoc comments and markdown files.
The new @example on BaseContext.ext (context.ts:188-194) uses a bare ```ts fence with no source= attribute and no companion packages/core/src/shared/context.examples.ts file, so sync:snippets can never validate it.
Why it matters here specifically
Grepping packages/core/src for @example shows this is the only un-sourced example in the core package — sdkErrors.ts, index.ts, protocol.ts, specTypeSchema.ts, and every validators/*.ts example all use source="./*.examples.ts#region" with a matching companion file (8 .examples.ts files exist under packages/core/src). So within core the convention is uniformly followed today, and this PR introduces the first divergence.
More substantively, the snippet references env, id, store, and taskContext(ctx). Per the PR description, tasksPlugin / taskContext ship in F3, not this PR — so the example is forward-referencing API that doesn't exist yet and cannot be type-checked today even if a .examples.ts were added. That is exactly the drift the sourced-example convention is meant to prevent: if F3's eventual API differs (different key, different accessor name), this doc comment silently rots.
Step-by-step
- CLAUDE.md:46 says
@examplefences should carrysource="./file.examples.ts#regionName". ls packages/core/src/shared/*.examples.ts→ onlyprotocol.examples.ts; nocontext.examples.ts.grep -r "@example" packages/core/src→ every hit (sdkErrors.ts:46, validators/types.ts:38, validators/cfWorkerProvider.ts:22/27, validators/fromJsonSchema.ts:18, validators/ajvProvider.ts:25/30/46+, index.ts:44/49, shared/protocol.ts) hassource=on the next line. context.ts:188 does not.- The snippet calls
taskContext(ctx);grep -r "export function taskContext" packages/→ no matches (it's slated for F3). - Therefore the example is un-type-checked pseudocode that
pnpm sync:snippetsignores, and it references not-yet-existing API.
Addressing the "there are other inline examples" objection
It's true a handful of inline @example blocks exist elsewhere in the monorepo (packages/server/src/validators/cfWorker.ts:4, packages/middleware/express/src/auth/metadataRouter.ts:143, packages/server/src/experimental/tasks/server.ts:84/181). However: (a) none of those are in packages/core, where the convention is currently 100% consistent, and (b) those stragglers reference real, in-tree symbols, whereas this snippet references API from a future PR. The combination of "first inline example in core" + "references unshipped symbols" is what makes it worth a nit — not the bare style point on its own.
Suggested fix
Either:
- Rewrite the example around what this PR actually ships, e.g. in
packages/core/src/shared/context.examples.ts:and reference it with//#region BaseContext_ext_read import { type BaseContext, readExt } from "@modelcontextprotocol/core"; declare const ctx: BaseContext; type TaskExt = { id: string }; const task = readExt<TaskExt>(ctx, "io.modelcontextprotocol/task"); //#endregion
```ts source="./context.examples.ts#BaseContext_ext_read"; or - Keep the prose description (which already explains the mechanism well) and drop the
@exampleblock until F3 landstaskContext, then add a sourced example.
Severity
nit — this is a documentation-convention issue, not a runtime or type-correctness bug. sync:snippets --check ignores fences without source=, so CI does not fail on it.
felixweinberger
force-pushed
the
fweinberger/s1-sessionid-opt
branch
from
6122f03 to
1127824
Compare
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
df45523 to
6bcdf6c
Compare
|
@claude review |
There was a problem hiding this comment.
Thanks for dropping readExt and its public export — the net diff is now a JSDoc-only expansion on the existing ext field, so LGTM. The un-sourced @example nit still stands but is non-blocking; feel free to swap it for a sourced snippet when F3 lands taskContext.
Extended reasoning...
Overview
After commit 6bcdf6c, this PR's net diff against base touches a single file (packages/core/src/shared/context.ts) and only rewrites the JSDoc comment on the pre-existing BaseContext.ext field — expanding two lines of prose into a fuller description plus an @example block. There are zero runtime or type-signature changes: ext?: Record<string, unknown> was already present on both BaseContext and RequestEnv in the base, and the readExt helper + its exports/public/index.ts re-export that I flagged in the first review round have been removed entirely (grep confirms no remaining references anywhere in packages/, and exports/public/index.ts has no net diff).
Security risks
None. This is a comment-only change with no effect on emitted JS, exported types, transport behavior, auth, or protocol handling.
Level of scrutiny
Low. My earlier 🔴 concern was about public API surface growth (readExt); the author addressed it by deleting the helper outright, which is the stronger of the two fixes I suggested. What remains is documentation on an internal mechanism that core itself never reads or writes. The repo's REVIEW.md API-surface and correctness checklists have nothing left to apply to a JSDoc-only diff.
Other factors
- The 🟡 nit (inline comment #3226617671) about the
@examplefence not usingsource="./context.examples.ts#region"and forward-referencingtaskContext(ships in F3) is still applicable, but I flagged it as a nit precisely becausesync:snippets --checkignores un-sourced fences and CI does not fail. The author has seen it and can address it when F3 introduces a realtaskContextto source from. Not re-raising. - No changeset is included, which is appropriate for a doc-only change in a stacked PR (
v2-statelesstrack, depends on S1). - Inline comment #3226617653 (the
readExtblocker) is marked resolved in the timeline, matching the code state.
felixweinberger
force-pushed
the
fweinberger/s1-sessionid-opt
branch
from
1127824 to
0a5b206
Compare
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
6bcdf6c to
1ccef83
Compare
|
@claude review |
There was a problem hiding this comment.
LGTM — thanks for dropping readExt; the net diff is now just the JSDoc on BaseContext.ext. The @example sourcing nit still stands but isn't blocking.
Extended reasoning...
Overview
After revisions, this PR's net diff against base is a single JSDoc comment change in packages/core/src/shared/context.ts: the description on the already-existing BaseContext.ext?: Record<string, unknown> field is expanded from two lines to a fuller explanation referencing SEP-2133 plus an @example block. There are no type changes, no new fields, no new exports, and no runtime code touched. The originally-added readExt helper and its exports/public/index.ts re-export — flagged in my prior review — were removed in commit 02caf53, and a follow-up commit (1ccef83) replaced a {@linkcode Protocol.use} reference (private symbol) with plain use() to keep typedoc clean. Grep confirms zero remaining readExt references in the repo.
Security risks
None. This is a comment-only edit; no executable code, schemas, transports, or auth paths are modified.
Level of scrutiny
Low. With readExt gone, what remains is documentation prose on an existing optional field that core never reads or writes. There is no public-API-surface delta to evaluate against REVIEW.md's minimalism / burden-of-proof principles, and no behavior to test.
Other factors
- My previous 🔴 concern is fully resolved (inline thread marked resolved; helper deleted rather than just un-exported, which is the cleaner outcome).
- The outstanding 🟡 nit about the un-sourced
@examplefence (referencestaskContext, which ships in F3) is still open inline. It's a documentation-convention point —sync:snippets --checkignores fences withoutsource=, so CI is unaffected — and the author has seen it across two re-review cycles. Reasonable to defer sourcing until F3 lands the referenced symbol; not a blocker. - No changeset is included, which is appropriate for a doc-comment-only change on a
v2-statelessstacked branch. - No specific CODEOWNERS beyond the default team apply to this path.
felixweinberger
force-pushed
the
fweinberger/s1-sessionid-opt
branch
from
0a5b206 to
3cbbcd0
Compare
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
1ccef83 to
06673f2
Compare
|
@claude review |
There was a problem hiding this comment.
Thanks for dropping readExt and its public export — that resolves the API-surface concern. What remains is a JSDoc-only expansion on BaseContext.ext; the inline @example nit still stands but is non-blocking. LGTM.
Extended reasoning...
Overview
After revisions (commits 6879b2f and 06673f2), the net diff is a single hunk in packages/core/src/shared/context.ts: the JSDoc on BaseContext.ext is expanded from a two-line description to a fuller one referencing SEP-2133, the use() middleware hook, the reverse-DNS key convention, and an illustrative @example. No types change, no fields are added or removed, no exports change. readExt — the one-line cast helper I flagged earlier as an unjustified public-API addition — has been deleted from both context.ts and exports/public/index.ts (grep confirms zero remaining references in the repo), and the inline comment is marked resolved.
Security risks
None. This is a comment-only change with no runtime code, no new exports, and no effect on transports, auth, or protocol handling.
Level of scrutiny
Low. The prior review's substantive concern was the speculative public export; that is now fully addressed by removal rather than demotion to internal, which is the cleaner outcome per REVIEW.md's minimalism principle. The remaining change is documentation prose on an existing optional field that core never reads or writes.
Other factors
The 🟡 nit about the @example not using a source="./context.examples.ts#region" fence (CLAUDE.md § JSDoc @example Code Snippets) and forward-referencing taskContext (which lands in F3) was not addressed. I'm treating that as the non-blocking style point I labeled it as: sync:snippets --check ignores un-sourced fences so CI is unaffected, and the prose above the example already conveys the mechanism clearly. It can be converted to a sourced snippet when F3 lands taskContext. No changeset is included, which is appropriate for a doc-only delta on a stacked feature branch.
1 participant
Adds
ext?: Record<string, unknown>toBaseContextandRequestEnv— a generic slot plugins/drivers populate and handlers read via a typed helper. Core never reads or writes it.Motivation and Context
The mechanism SEP-2133 extensions use to inject per-request context without core knowing about them.
tasksPlugin(F3) putsext.task;SessionCompatputsext.sessionId.How Has This Been Tested?
typecheck + lint clean. Type-only change.
Breaking Changes
None.
Types of changes
Checklist
Additional context
Depends on S1. Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.