Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ The board write needs `organization projects: write`, which `GITHUB_TOKEN` canno

**`.github/workflows/sdk-watch.yml` → `scripts/sdk-watch.mjs` runs nightly (#1063) and files one issue per MCP SDK release we are behind**, labeled `v2` + `chore` + `dependencies`. It is not a Dependabot replacement — it exists because SDK churn, OAuth especially, was being tracked by habit rather than by mechanism — but it obeys the same rule as the two above: **it files an issue, never a PR.**

- **Two upstreams, two issues.** `client`/`core`/`server`/`server-legacy` ship from `modelcontextprotocol/typescript-sdk` in lockstep and share one issue; `ext-apps` ships from its own repo and gets its own. A fifth `@modelcontextprotocol/*` package added to the root manifest and not added to `SDK_GROUPS` **fails the sweep loudly** rather than going unwatched — that guard is the point, since a hardcoded group table is otherwise a silent blind spot.
- **One issue per upstream.** `client`/`core`/`server`/`server-legacy` ship from `modelcontextprotocol/typescript-sdk` in lockstep and share one issue; `ext-apps` and `ext-tasks` each ship from their own repo and get their own. A `@modelcontextprotocol/*` package added to the root manifest and not added to `SDK_GROUPS` **fails the sweep loudly** rather than going unwatched — that guard is the point, since a hardcoded group table is otherwise a silent blind spot.
- **It compares the INSTALLED version, not the declared range.** The four SDK packages are pinned exactly, so the two agree for them; `ext-apps` is a caret range whose lockfile already resolves higher, and comparing the declared string would file an issue for a bump `npm install` has already taken.
- **The target is the LOWEST `latest` across a group — the version the whole group has reached — not the highest.** npm publishes a lockstep release one package at a time, so a sweep landing mid-publish sees one package ahead of its three siblings. Targeting the highest would name a version three of them do not have _and_ write a marker that suppresses the real filing once the publication completes, so the release would never be tracked at all. Taking the minimum keeps the issue actionable and lets the completed release file its own.
- **It never boards, like the monthly sweep** — no `PROJECT_TOKEN` exists in this org — so the issue arrives labeled and milestoned and `/issue-triage` places it.
Expand Down Expand Up @@ -386,7 +386,7 @@ diagnose a failing gate — is the `testing` skill. These are the rules.
- **`clients/web`**: `<Name>.test.tsx` **next to the source** — components, hooks, `lib/`, `utils/`. A web-owned test living under `src/test/` instead is a bug. `src/test/` is for the three things that cannot be co-located: tests of the repo-root **`core/`** package (`src/test/core/…`, mirroring the `core/` layout — it lives outside `clients/web/` and has no harness of its own); the **`integration`** project (`src/test/integration/…` — _placement is the manifest_, picked up by a folder glob, with no enumeration to keep in sync); and **shared test infrastructure** (`renderWithMantine.tsx`, `setup.ts`, `fixtures/`).
- **`clients/cli`, `clients/tui`, `clients/launcher`**: **all** tests in a top-level **`__tests__/`**, not beside their source. Their `tsconfig.json` excludes `**/*.test.*`, so a co-located test lands in **no** tsconfig project and fails `npm run verify:typecheck-coverage`.
- **Root tooling**: a `scripts/*.mjs` helper with pure logic gets a sibling `*.test.mjs`. Keep that exact filename — `node --test` silently _skips_ a file its glob misses and still exits 0.
- **Render Ink components through the TUI's own `render`** (`clients/tui/__tests__/helpers/renderTui.tsx`), never `ink-testing-library`'s directly. It is the same function with every frame ANSI-stripped, which is what keeps an assertion on styled text from depending on the ambient environment: Ink writes styling *inside* the styled run, so `<Text underline>I</Text>nfo` reaches the frame buffer with escapes between `I` and `nfo` and `toContain("Info")` fails. It only bites where chalk emits color — a developer whose shell exports `FORCE_COLOR` — so CI is green on a suite that is broken for them (#2207). A test that genuinely needs the raw bytes reads `stdout.lastFrame()` off the returned instance.
- **Render Ink components through the TUI's own `render`** (`clients/tui/__tests__/helpers/renderTui.tsx`), never `ink-testing-library`'s directly. It is the same function with every frame ANSI-stripped, which is what keeps an assertion on styled text from depending on the ambient environment: Ink writes styling _inside_ the styled run, so `<Text underline>I</Text>nfo` reaches the frame buffer with escapes between `I` and `nfo` and `toContain("Info")` fails. It only bites where chalk emits color — a developer whose shell exports `FORCE_COLOR` — so CI is green on a suite that is broken for them (#2207). A test that genuinely needs the raw bytes reads `stdout.lastFrame()` off the returned instance.
- **Render React components through `renderWithMantine`** (`src/test/renderWithMantine.tsx`); do not hand-roll a bare `MantineProvider`, which skips the project theme and the helper's options and drifts from every other test. Pass the `colorScheme` option to exercise a forced scheme rather than hand-rolling `defaultColorScheme`. Use `renderWithMantineTransitions` **only** when a test must assert mid-flight transition state, and read the long comment on the helper before changing anything about it.
- **The web coverage `include` is a whitelist.** It names `components`/`hooks`/`theme`/`lib`/`utils`/`server` plus the browser-consumed `core/*` runtime, so a module placed **outside** those directories falls out of the gate entirely, silently. Place new modules inside a gated directory. The documented exceptions — `src/App.tsx` and the `src/main.tsx` / `src/index.ts` bootstraps — are called out in a comment on the `include` array itself.

Expand Down
1 change: 1 addition & 0 deletions clients/cli/tsup.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export default defineConfig({
// zod-to-json-schema in with it — which the #2067 guard surfaced. ESM, so it
// was not failing the way `undici` did; the rule is what it violated.
"@modelcontextprotocol/ext-apps",
"@modelcontextprotocol/ext-tasks",
"commander",
"pino",
// Consolidated to the ROOT manifest by #2195, along with every other
Expand Down
1 change: 1 addition & 0 deletions clients/tui/tsup.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export default defineConfig({
// client's own code, so all three lists carry it (AGENTS.md). The CLI was
// inlining it; the #2067 guard surfaced that.
"@modelcontextprotocol/ext-apps",
"@modelcontextprotocol/ext-tasks",
"@napi-rs/keyring",
// Root-declared (see the repo's dependency-placement rule) and CJS, which
// is the combination that bites: tsup externalizes what the *client's*
Expand Down
35 changes: 29 additions & 6 deletions clients/web/src/test/core/mcp/inspectorClient-list-cursor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
* pins is that `""` survives and that a genuinely absent cursor still sends no
* `cursor` key at all. The SDK client is stubbed rather than connected — the
* decision under test is made entirely in `InspectorClient`.
*
* `listRequestorTasks` is asserted against the ext-tasks session instead of
* the SDK client because this branch routes it through
* `runTaskSessionOperation`; the session owns the wire params, and its own
* suite pins the verbatim-cursor behavior there. The Inspector-side property
* is that the cursor reaches `session.listTasks` unchanged.
*/
describe("InspectorClient list cursor handling (#2220)", () => {
/**
Expand Down Expand Up @@ -106,12 +112,6 @@ describe("InspectorClient list cursor handling (#2220)", () => {
result: { resourceTemplates: [] },
call: (client, cursor) => client.listResourceTemplates(cursor),
},
{
name: "listRequestorTasks",
method: "tasks/list",
result: { tasks: [] },
call: (client, cursor) => client.listRequestorTasks(cursor),
},
];

it.each(ADAPTERS)(
Expand Down Expand Up @@ -154,4 +154,27 @@ describe("InspectorClient list cursor handling (#2220)", () => {
expect(sent.params.cursor).toBe("page-2");
},
);

it.each([[""], [undefined], ["page-2"]])(
"listRequestorTasks forwards cursor %j to the ext-tasks session unchanged",
async (cursor) => {
const client = makeClient();
const listTasks = vi.fn(async (received?: string) => {
// The session owns the wire params; the Inspector-side property is
// that the cursor arrives here verbatim.
void received;
return { tasks: [] };
});
(
client as unknown as {
taskSession: { listTasks: typeof listTasks } | null;
}
).taskSession = { listTasks };

await client.listRequestorTasks(cursor);

expect(listTasks).toHaveBeenCalledTimes(1);
expect(listTasks.mock.calls[0][0]).toBe(cursor);
},
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -790,21 +790,15 @@ describe("InspectorClient peer-handler timing (#1797)", () => {
);
await client.connect();

// Seeded directly: subscribing for real needs a server that answers
// `resources/subscribe` and cancelling needs a live task, neither of which
// adds to what is under test — that a new session starts empty. The cast is
// the only route to `cancelledTaskIds`, which has no public reader.
const internals = client as unknown as {
subscribedResources: Set<string>;
cancelledTaskIds: Set<string>;
modernStreamState: {
active: boolean;
status: string;
honoredUris: string[];
};
};
internals.subscribedResources.add("file:///watched");
internals.cancelledTaskIds.add("task-1");
// The stream state a live modern subscription would have left behind.
internals.modernStreamState = {
active: true,
Expand All @@ -829,7 +823,6 @@ describe("InspectorClient peer-handler timing (#1797)", () => {
await client.connect();

expect(client.getSubscribedResources()).toEqual([]);
expect(internals.cancelledTaskIds.size).toBe(0);
// Cleared with the set it is derived from, not left reading `active` for
// an empty one.
expect(client.getResourceSubscriptionStreamState()).toMatchObject({
Expand Down Expand Up @@ -880,35 +873,6 @@ describe("InspectorClient peer-handler timing (#1797)", () => {
await client.disconnect();
});

it("aborts a paused task-input wait when the session ends", async () => {
// The bounded-window member: both registration sites release in a
// `finally`, so nothing leaks permanently — this closes the gap between a
// crash and the loop unwinding on its own.
const transport = new SampleAfterConnectTransport();
const client = new InspectorClient(
{ type: "stdio", command: "noop", args: [] },
{ environment: { transport: () => ({ transport }) } },
);
await client.connect();

// Seeded directly: reaching this map for real needs a modern task paused at
// `input_required`, which adds nothing to what is under test. No public
// reader, hence the cast.
const controller = new AbortController();
(
client as unknown as {
taskInputAbortControllers: Map<string, AbortController>;
}
).taskInputAbortControllers.set("task-1", controller);

transport.onclose?.();
await client.connect();

expect(controller.signal.aborted).toBe(true);

await client.disconnect();
});

it("closes a live listen stream the next connect drops", async () => {
// An `onerror` without an `onclose` leaves the transport up, and `connect()`
// reuses it — so the reference the reset drops can be the last one to a
Expand Down Expand Up @@ -1001,25 +965,15 @@ describe("InspectorClient peer-handler timing (#1797)", () => {
);
await client.connect();

// Seeded directly, for the reasons `closes a live listen stream the
// next connect drops` and `aborts a paused task-input wait when the
// session ends` give. No public writer for either, hence the casts.
// Seed the live subscription directly; it has no public writer.
(
client as unknown as {
modernSubscription: { close: () => Promise<void> } | null;
}
).modernSubscription = { close };

// A downstream teardown step, to witness that teardown continued.
const controller = new AbortController();
(
client as unknown as {
taskInputAbortControllers: Map<string, AbortController>;
}
).taskInputAbortControllers.set("task-1", controller);

await expect(client.disconnect()).resolves.toBeUndefined();
expect(controller.signal.aborted).toBe(true);
expect(client.getStatus()).toBe("disconnected");

// Node reports an unhandled rejection after the microtask checkpoint,
// so yield to the macrotask queue before reading the listener — nothing
Expand Down
Loading