Some checks failed
* feat(runtime): add `disableOAuth` connect option (cache-friendly OAuth suppression) Closes #197. Long-running headless callers (daemons, scheduled jobs, CI workers) need to suppress the interactive OAuth flow without losing connection caching. The only existing knob — `maxOAuthAttempts: 0` — couples those two concerns because `useCache` is gated on `options.maxOAuthAttempts === undefined`. Daemons that wrap `connect` to force `maxOAuthAttempts: 0` end up spawning a fresh transport per `callTool`/`listTools` and `runtime.close()` cannot reap any of them. Add an additive `disableOAuth: boolean` option that suppresses OAuth at the transport layer (short-circuits `shouldEstablishOAuth` and `maybePromoteHttpDefinition`) but preserves caching. The cache entry metadata gains a `disableOAuth` field so connections established with the flag don't share a slot with connections that could refresh into an OAuth flow — switching the flag between calls evicts and re-establishes, mirroring the existing `allowCachedAuth` mismatch path. Backward compatibility: * `maxOAuthAttempts: 0` keeps its legacy escape-the-cache contract unchanged. Existing callers see no behavior change. * `skipCache: true` keeps its behavior unchanged. * `disableOAuth` defaults to undefined; only opt-in changes behavior. Also export `ConnectOptions` from `runtime.ts` and add the parameter to the `Runtime.connect` interface signature — the implementation already accepted options at runtime but the interface only exposed `connect(server)`, so callers couldn't pass options through the type system. (Pre-existing gap surfaced by adding the new test coverage.) Tests added to `tests/runtime-integration.test.ts`: * `reuses cached connection when disableOAuth: true is passed` — two calls return the same ClientContext, `close()` reaps it. * `maxOAuthAttempts: 0 still bypasses the cache (existing contract preserved)` — regression guard. * `evicts and re-establishes the cached client when disableOAuth flag changes` — the core eviction semantic. `pnpm test` (709 pass / 3 skip), `pnpm lint`, `pnpm typecheck` all green. * fix(runtime): preserve disableOAuth across helper calls * fix(daemon): forward disableOAuth through keep-alive paths * feat(cli): expose disableOAuth for headless commands * fix(runtime): preserve cached slot across connect(disableOAuth) → callTool/listTools Addresses PR #198 review comment r3366238654. The documented headless setup is: await runtime.connect(server, { disableOAuth: true }); await runtime.callTool(server, 'foo', { ... }); The first call stored the cache slot with `allowCachedAuth: undefined`, but `callTool()` internally calls `this.connect(server, { allowCachedAuth: true, disableOAuth: <effective>: true })` and the cache-match check treated the two options shapes as structurally different: existing.allowCachedAuth (undefined) !== options.allowCachedAuth (true) && options.allowCachedAuth !== undefined => MISMATCH => evict + reopen transport Every first callTool / listTools after a pre-connect spawned a fresh transport, defeating the pooling guarantee that motivated the disableOAuth option in the first place. Same shape affected `listTools` (which defaults `allowCachedAuth: options.allowCachedAuth ?? true`). Fix: normalize at the connect() entrypoint. A `disableOAuth: true` caller has no path to interactive OAuth, so cached-token application is the only auth they can ever use — default `allowCachedAuth: true` when the caller didn't pick a side. Explicit `false` is honored (header-only / anonymous callers). The normalized value flows through both the cache lookup and the cache write so subsequent internal callers compose without eviction. Two regression tests added to `tests/runtime-integration.test.ts`: - `preserves the cached client across connect(disableOAuth:true) → callTool() (no implicit eviction)` - `preserves the cached client across connect(disableOAuth:true) → listTools() (no implicit eviction)` Both call `runtime.connect(disableOAuth:true)`, then invoke the internal-cached path (callTool or listTools), then re-call `runtime.connect(disableOAuth:true)` and assert the resulting ClientContext is `=== ` the first one. Both tests fail without this fix (the second connect returns a new ClientContext because the first was evicted). `pnpm test` 723 pass / 3 skip / 0 fail. `pnpm lint` + `pnpm typecheck` clean. No push. * docs(examples): add headless-pooling-demo for disableOAuth verification Demonstrates the three patterns under the new `disableOAuth` option against a local mock MCP server (no real auth). Reproducible artifact for PR #198 review proof. Patterns demonstrated: * Legacy `maxOAuthAttempts: 0` (uncached): 5 connect() calls produce 5 distinct ClientContexts. Existing contract preserved. * `disableOAuth: true` on every connect: 5 calls produce 1 ClientContext. Cache reuse under cache-friendly suppression. * Documented headless setup — pre-connect(disableOAuth: true) + 5 callTool() — proves the pre-connected slot survives the implicit internal connect path. Directly demonstrates the fix from b0e3e2e. Run: `pnpm tsx examples/headless-pooling-demo.ts` Sample output is intentionally redacted to no PII / no secrets: a local http://127.0.0.1:<random-port>/mcp server with a public `add` tool. * style(examples): oxfmt headless-pooling-demo (CI fix) * fix(server-proxy): thread disableOAuth through schema-discovery listTools Addresses PR #198 review comment r3366307210 (clawsweeper proxy gap). The Proxy returned by `createServerProxy` calls `ensureMetadata()` on every tool invocation, which fires `runtime.listTools(server, { includeSchema: true })` for schema discovery. That call ran BEFORE the proxy parsed the caller's options bag, so a `proxy.tool({ ... }, { disableOAuth: true })` invocation on an OAuth server with no cached schema could still trigger an interactive OAuth flow during metadata fetch — defeating the no-browser guarantee the option was meant to provide. Fix: * Pre-scan callArgs once for `disableOAuth: true` before invoking `ensureMetadata`. The scan is a single linear pass over the already-present argument list and short-circuits on the first match. * Extend `ensureMetadata(toolName, { disableOAuth? })` and forward the flag to the underlying `runtime.listTools(serverName, { includeSchema: true, disableOAuth: true })` call. * The schema-fetch path that was vulnerable now inherits the same no-OAuth posture as the eventual `runtime.callTool` invocation. End- to-end no-browser guarantee is preserved across the proxy interface. Regression test in `tests/server-proxy.test.ts`: > threads disableOAuth through schema discovery so > proxy.tool({disableOAuth:true}) cannot trigger OAuth during > metadata fetch Asserts BOTH: - `runtime.listTools` called with `{ includeSchema: true, disableOAuth: true }` - `runtime.callTool` called with the eventual tool args and `disableOAuth: true` Locks the contract on both halves so a future refactor that re-introduces the gap on either side will fail loudly. Full suite: 724 pass / 3 skipped / 0 fail. `pnpm check` (format + lint + typecheck) clean. * refactor(cli): drop --disable-oauth alias; keep only --no-oauth The PR originally exposed two CLI names for the same intent: --disable-oauth (mirroring the JS option `disableOAuth: true`) and --no-oauth (the GNU-style boolean opt-out). Two names for one behavior is noise — documentation has to mention both, users have to learn both, and they invite drift. --no-oauth is the right shape for a per-invocation boolean opt-out: - Matches the dominant unix convention (git --no-verify, npm --no-save, bun --no-cache, curl --no-progress-meter). - Shorter to type. - Composes naturally with other flags in scripts. The JS option name stays `disableOAuth: boolean` — that's the right shape for a JS option (verb+noun, no Boolean-negation prefix ambiguity), and the JS and CLI naming conventions are genuinely different domains. Removed CLI registrations + help text + internal forwarding for --disable-oauth across: - src/cli/call-arguments.ts (FLAG_HANDLERS registration) - src/cli/call-command.ts (internal listArgs forwarding, 2 sites) - src/cli/call-help.ts (help text) - src/cli/list-command.ts (help text) - src/cli/list-flags.ts (token check) - src/cli/resource-command.ts (token check + help text) - docs/cli-reference.md (3 references) Renamed test cases that exclusively exercised --disable-oauth to exercise --no-oauth instead, preserving regression coverage: - tests/call-arguments.test.ts - tests/cli-list-flags.test.ts - tests/cli-resource-command.test.ts The internal cache-key fragment `disable-oauth:` in src/cli/tool-cache.ts is kept — it mirrors the JS option name (which stays `disableOAuth`), not the CLI flag. Tests: 724 passed, 3 skipped, 0 failed. Lint: 0 warnings, 0 errors. Typecheck: clean. * fix(runtime): forward disableOAuth through callOnce * chore: update dependencies * fix(server-proxy): preserve schema-owned option fields * fix(runtime): isolate OAuth cache variants safely * fix(server-proxy): isolate schema discovery posture * fix(server-proxy): preserve OAuth posture during discovery --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
286 lines
10 KiB
TypeScript
286 lines
10 KiB
TypeScript
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js';
|
|
import { describe, expect, it, vi } from 'vitest';
|
|
import type { ServerDefinition } from '../src/config.js';
|
|
import { createKeepAliveRuntime } from '../src/daemon/runtime-wrapper.js';
|
|
import type { CallOptions, ConnectOptions, ListToolsOptions, Runtime } from '../src/runtime.js';
|
|
|
|
class FakeRuntime implements Runtime {
|
|
private readonly definitions: ServerDefinition[];
|
|
public readonly callToolMock = vi.fn().mockResolvedValue('local-call');
|
|
public readonly listToolsMock = vi.fn().mockResolvedValue([{ name: 'local-tool' }]);
|
|
public readonly listResourcesMock = vi.fn().mockResolvedValue([]);
|
|
public readonly readResourceMock = vi.fn().mockResolvedValue({ contents: [] });
|
|
public readonly connectMock = vi.fn().mockResolvedValue({ client: {}, transport: {}, definition: {} });
|
|
public readonly closeMock = vi.fn().mockResolvedValue(undefined);
|
|
|
|
constructor(definitions: ServerDefinition[]) {
|
|
this.definitions = definitions;
|
|
}
|
|
|
|
listServers(): string[] {
|
|
return this.definitions.map((definition) => definition.name);
|
|
}
|
|
|
|
getDefinitions(): ServerDefinition[] {
|
|
return this.definitions;
|
|
}
|
|
|
|
getDefinition(server: string): ServerDefinition {
|
|
const definition = this.definitions.find((entry) => entry.name === server);
|
|
if (!definition) {
|
|
throw new Error(`Unknown server ${server}`);
|
|
}
|
|
return definition;
|
|
}
|
|
|
|
registerDefinition(): void {
|
|
// no-op for tests
|
|
}
|
|
|
|
async getInstructions(): Promise<string | undefined> {
|
|
return undefined;
|
|
}
|
|
|
|
async listTools(server: string, options?: ListToolsOptions): Promise<Awaited<ReturnType<Runtime['listTools']>>> {
|
|
return await this.listToolsMock(server, options);
|
|
}
|
|
|
|
async callTool(server: string, toolName: string, options?: CallOptions): Promise<unknown> {
|
|
return await this.callToolMock(server, toolName, options);
|
|
}
|
|
|
|
async listResources(server: string, options?: unknown): Promise<unknown> {
|
|
return await this.listResourcesMock(server, options);
|
|
}
|
|
|
|
async readResource(server: string, uri: string): Promise<unknown> {
|
|
return await this.readResourceMock(server, uri);
|
|
}
|
|
|
|
async connect(server: string, options?: ConnectOptions): Promise<Awaited<ReturnType<Runtime['connect']>>> {
|
|
return await this.connectMock(server, options);
|
|
}
|
|
|
|
async close(server?: string): Promise<void> {
|
|
await this.closeMock(server);
|
|
}
|
|
}
|
|
|
|
describe('createKeepAliveRuntime', () => {
|
|
const definitions: ServerDefinition[] = [
|
|
{
|
|
name: 'alpha',
|
|
description: 'keep alive server',
|
|
command: { kind: 'http', url: new URL('https://alpha.example.com') },
|
|
lifecycle: { mode: 'keep-alive' },
|
|
source: { kind: 'local', path: '/tmp' },
|
|
},
|
|
{
|
|
name: 'beta',
|
|
description: 'ephemeral server',
|
|
command: { kind: 'http', url: new URL('https://beta.example.com') },
|
|
source: { kind: 'local', path: '/tmp' },
|
|
},
|
|
];
|
|
|
|
it('routes keep-alive servers through the daemon client', async () => {
|
|
const runtime = new FakeRuntime(definitions);
|
|
const daemon = {
|
|
callTool: vi.fn().mockResolvedValue('daemon-call'),
|
|
listTools: vi.fn().mockResolvedValue([{ name: 'remote-tool' }]),
|
|
listResources: vi.fn().mockResolvedValue(['resource']),
|
|
readResource: vi.fn().mockResolvedValue({ contents: [{ uri: 'memo://1', text: 'daemon-resource' }] }),
|
|
closeServer: vi.fn().mockResolvedValue(undefined),
|
|
};
|
|
const keepAliveRuntime = createKeepAliveRuntime(runtime as unknown as Runtime, {
|
|
daemonClient: daemon as never,
|
|
keepAliveServers: new Set(['alpha']),
|
|
});
|
|
|
|
await keepAliveRuntime.callTool('alpha', 'ping', { args: { value: 1 }, timeoutMs: 4_200 });
|
|
expect(daemon.callTool).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
tool: 'ping',
|
|
args: { value: 1 },
|
|
timeoutMs: 4_200,
|
|
disableOAuth: undefined,
|
|
});
|
|
|
|
await keepAliveRuntime.listTools('alpha', { includeSchema: true });
|
|
expect(daemon.listTools).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
includeSchema: true,
|
|
autoAuthorize: undefined,
|
|
allowCachedAuth: true,
|
|
disableOAuth: undefined,
|
|
});
|
|
|
|
await keepAliveRuntime.listTools('alpha', { allowCachedAuth: false });
|
|
expect(daemon.listTools).toHaveBeenLastCalledWith({
|
|
server: 'alpha',
|
|
includeSchema: undefined,
|
|
autoAuthorize: undefined,
|
|
allowCachedAuth: false,
|
|
disableOAuth: undefined,
|
|
});
|
|
|
|
await keepAliveRuntime.listResources('alpha', { cursor: '1' });
|
|
expect(daemon.listResources).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
params: { cursor: '1' },
|
|
allowCachedAuth: undefined,
|
|
disableOAuth: undefined,
|
|
});
|
|
|
|
await expect(keepAliveRuntime.readResource('alpha', 'memo://1')).resolves.toEqual({
|
|
contents: [{ uri: 'memo://1', text: 'daemon-resource' }],
|
|
});
|
|
expect(daemon.readResource).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
uri: 'memo://1',
|
|
allowCachedAuth: undefined,
|
|
disableOAuth: undefined,
|
|
});
|
|
|
|
await keepAliveRuntime.close('alpha');
|
|
expect(daemon.closeServer).toHaveBeenCalledWith({ server: 'alpha' });
|
|
|
|
await keepAliveRuntime.callTool('beta', 'pong', {});
|
|
expect(runtime.callToolMock).toHaveBeenCalledWith('beta', 'pong', {});
|
|
|
|
await keepAliveRuntime.close();
|
|
expect(runtime.closeMock).toHaveBeenCalledWith(undefined);
|
|
});
|
|
|
|
it('forwards disableOAuth through daemon requests and connect wrappers', async () => {
|
|
const runtime = new FakeRuntime(definitions);
|
|
const daemon = {
|
|
callTool: vi.fn().mockResolvedValue('daemon-call'),
|
|
listTools: vi.fn().mockResolvedValue([{ name: 'remote-tool' }]),
|
|
listResources: vi.fn().mockResolvedValue(['resource']),
|
|
readResource: vi.fn().mockResolvedValue({ contents: [] }),
|
|
closeServer: vi.fn().mockResolvedValue(undefined),
|
|
};
|
|
const keepAliveRuntime = createKeepAliveRuntime(runtime as unknown as Runtime, {
|
|
daemonClient: daemon as never,
|
|
keepAliveServers: new Set(['alpha']),
|
|
});
|
|
|
|
await keepAliveRuntime.callTool('alpha', 'ping', { disableOAuth: true });
|
|
expect(daemon.callTool).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
tool: 'ping',
|
|
args: undefined,
|
|
timeoutMs: undefined,
|
|
disableOAuth: true,
|
|
});
|
|
|
|
await keepAliveRuntime.listTools('alpha', { disableOAuth: true });
|
|
expect(daemon.listTools).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
includeSchema: undefined,
|
|
autoAuthorize: undefined,
|
|
allowCachedAuth: true,
|
|
disableOAuth: true,
|
|
});
|
|
|
|
await keepAliveRuntime.listResources('alpha', { cursor: '1', disableOAuth: true });
|
|
expect(daemon.listResources).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
params: { cursor: '1' },
|
|
allowCachedAuth: undefined,
|
|
disableOAuth: true,
|
|
});
|
|
|
|
await keepAliveRuntime.readResource('alpha', 'memo://1', { disableOAuth: true });
|
|
expect(daemon.readResource).toHaveBeenCalledWith({
|
|
server: 'alpha',
|
|
uri: 'memo://1',
|
|
allowCachedAuth: undefined,
|
|
disableOAuth: true,
|
|
});
|
|
|
|
await keepAliveRuntime.connect('alpha', { disableOAuth: true });
|
|
expect(runtime.connectMock).toHaveBeenCalledWith('alpha', { disableOAuth: true });
|
|
});
|
|
|
|
it('restarts daemon servers after fatal errors and retries the operation', async () => {
|
|
const runtime = new FakeRuntime(definitions);
|
|
const daemon = {
|
|
callTool: vi.fn().mockRejectedValueOnce(new Error('transport hung up')).mockResolvedValueOnce('daemon-call'),
|
|
closeServer: vi.fn().mockResolvedValue(undefined),
|
|
listTools: vi.fn(),
|
|
listResources: vi.fn(),
|
|
readResource: vi.fn(),
|
|
};
|
|
const keepAliveRuntime = createKeepAliveRuntime(runtime as unknown as Runtime, {
|
|
daemonClient: daemon as never,
|
|
keepAliveServers: new Set(['alpha']),
|
|
});
|
|
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
|
|
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
|
|
|
|
await expect(keepAliveRuntime.callTool('alpha', 'ping', {})).resolves.toBe('daemon-call');
|
|
expect(daemon.callTool).toHaveBeenCalledTimes(2);
|
|
expect(daemon.closeServer).toHaveBeenCalledWith({ server: 'alpha' });
|
|
expect(logSpy).not.toHaveBeenCalled();
|
|
expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining("Restarting 'alpha'"));
|
|
logSpy.mockRestore();
|
|
errorSpy.mockRestore();
|
|
});
|
|
|
|
it('deduplicates concurrent restarts for the same server', async () => {
|
|
const runtime = new FakeRuntime(definitions);
|
|
let releaseClose!: () => void;
|
|
const closePromise = new Promise<void>((resolve) => {
|
|
releaseClose = resolve;
|
|
});
|
|
const daemon = {
|
|
callTool: vi
|
|
.fn()
|
|
.mockRejectedValueOnce(new Error('transport hung up'))
|
|
.mockRejectedValueOnce(new Error('transport hung up'))
|
|
.mockResolvedValue('daemon-call'),
|
|
closeServer: vi.fn().mockImplementation(async () => {
|
|
await closePromise;
|
|
}),
|
|
listTools: vi.fn(),
|
|
listResources: vi.fn(),
|
|
readResource: vi.fn(),
|
|
};
|
|
const keepAliveRuntime = createKeepAliveRuntime(runtime as unknown as Runtime, {
|
|
daemonClient: daemon as never,
|
|
keepAliveServers: new Set(['alpha']),
|
|
});
|
|
|
|
const first = keepAliveRuntime.callTool('alpha', 'ping', {});
|
|
const second = keepAliveRuntime.callTool('alpha', 'pong', {});
|
|
await Promise.resolve();
|
|
expect(daemon.closeServer).toHaveBeenCalledTimes(1);
|
|
releaseClose();
|
|
|
|
await expect(Promise.all([first, second])).resolves.toEqual(['daemon-call', 'daemon-call']);
|
|
expect(daemon.closeServer).toHaveBeenCalledTimes(1);
|
|
});
|
|
|
|
it('does not restart daemon servers for InvalidParams errors', async () => {
|
|
const runtime = new FakeRuntime(definitions);
|
|
const error = new McpError(ErrorCode.InvalidParams, 'Tool not found');
|
|
const daemon = {
|
|
callTool: vi.fn().mockRejectedValue(error),
|
|
closeServer: vi.fn().mockResolvedValue(undefined),
|
|
listTools: vi.fn(),
|
|
listResources: vi.fn(),
|
|
readResource: vi.fn(),
|
|
};
|
|
const keepAliveRuntime = createKeepAliveRuntime(runtime as unknown as Runtime, {
|
|
daemonClient: daemon as never,
|
|
keepAliveServers: new Set(['alpha']),
|
|
});
|
|
|
|
await expect(keepAliveRuntime.callTool('alpha', 'ping', {})).rejects.toThrow('Tool not found');
|
|
expect(daemon.callTool).toHaveBeenCalledTimes(1);
|
|
expect(daemon.closeServer).not.toHaveBeenCalled();
|
|
});
|
|
});
|