mcporter/tests/cli-list-classification.test.ts
Sebastian B Otaegui 3e27b64021
Some checks failed
CI / build (${{ matrix.os }}) (ubuntu-latest) (push) Has been cancelled
CI / build (${{ matrix.os }}) (macos-15) (push) Has been cancelled
CI / build (${{ matrix.os }}) (windows-latest) (push) Has been cancelled
pages / Deploy docs (push) Has been cancelled
fix(runtime): preserve disableOAuth across headless paths (#198)
* 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>
2026-06-08 16:11:23 -07:00

363 lines
14 KiB
TypeScript

import fs from 'node:fs/promises';
import os from 'node:os';
import path from 'node:path';
import { describe, expect, it, vi } from 'vitest';
import type { ServerDefinition } from '../src/config.js';
import { cliModulePromise, linearDefinition } from './fixtures/cli-list-fixtures.js';
describe('CLI list classification and routing', () => {
it('identifies auth and offline failures and suggests remediation', async () => {
const originalCI = process.env.CI;
process.env.CI = '1';
const { handleList } = await cliModulePromise;
const definitions: ServerDefinition[] = [
{
name: 'healthy',
command: { kind: 'stdio', command: 'noop', args: [], cwd: process.cwd() },
source: { kind: 'local', path: '/tmp/config.json' },
},
{
name: 'vercel',
description: 'Vercel MCP',
command: { kind: 'http', url: new URL('https://example.com') },
},
{
name: 'github',
command: { kind: 'http', url: new URL('https://example.com') },
source: { kind: 'import', path: '/tmp/import.json' },
},
{
name: 'next-devtools',
command: { kind: 'http', url: new URL('https://localhost') },
},
{
name: 'obsidian',
command: { kind: 'http', url: new URL('https://localhost') },
},
];
const runtime = {
getDefinitions: () => definitions,
listTools: (name: string) => {
switch (name) {
case 'healthy':
return Promise.resolve([{ name: 'ok' }]);
case 'vercel':
return Promise.reject(new Error('SSE error: Non-200 status code (401)'));
case 'github':
return Promise.reject(new Error('SSE error: Non-200 status code (405)'));
case 'next-devtools':
return Promise.reject(new Error('SSE error: fetch failed: connect ECONNREFUSED 127.0.0.1:3000'));
case 'obsidian':
return Promise.reject(new Error('MCP error -32000: Connection closed'));
default:
return Promise.resolve([]);
}
},
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
await handleList(runtime, []);
const logLines = logSpy.mock.calls.map((call) => call.join(' '));
expect(
logLines.some((line) => line.includes("vercel — Vercel MCP (auth required — run 'mcporter auth vercel'"))
).toBe(true);
expect(logLines.some((line) => line.includes('github') && line.includes('HTTP 405'))).toBe(true);
const nextDevtoolsLineFound = logLines.some(
(line) => line.startsWith('- next-devtools') && line.includes('offline — unable to reach server')
);
expect(nextDevtoolsLineFound).toBe(true);
expect(
logLines.some((line) => line.includes('obsidian') && line.includes('offline — unable to reach server'))
).toBe(true);
const summaryLine = logLines.find((line) => line.startsWith('✔ Listed'));
expect(summaryLine).toBeDefined();
expect(summaryLine).toContain('auth required');
expect(summaryLine).toContain('http errors');
expect(summaryLine).toContain('offline');
logSpy.mockRestore();
warnSpy.mockRestore();
process.env.CI = originalCI;
});
it('suggests URL-based auth for ad-hoc HTTP servers', async () => {
const { handleList } = await cliModulePromise;
const definitions = new Map<string, ServerDefinition>();
const runtime = {
registerDefinition: vi.fn((definition: ServerDefinition) => {
definitions.set(definition.name, definition);
}),
getDefinition: vi.fn((name: string) => {
const entry = definitions.get(name);
if (!entry) {
throw new Error(`Unknown MCP server '${name}'.`);
}
return entry;
}),
getDefinitions: () => Array.from(definitions.values()),
listTools: vi.fn().mockRejectedValue(new Error('SSE error: Non-200 status code (401)')),
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
await handleList(runtime, ['https://mcp.supabase.com/mcp']);
const hinted = warnSpy.mock.calls.some((call) =>
(call[0]?.toString() ?? '').includes("Next: run 'mcporter auth https://mcp.supabase.com/mcp'")
);
expect(hinted).toBe(true);
expect(warnSpy.mock.calls.map((call) => call.join(' '))).toContain(' Tools: <unavailable>');
warnSpy.mockRestore();
logSpy.mockRestore();
});
it('persists OAuth promotion for ad-hoc HTTP servers', async () => {
const { handleList } = await cliModulePromise;
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-persist-oauth-'));
const persistPath = path.join(tempDir, 'mcporter.json');
const definitions = new Map<string, ServerDefinition>();
const runtime = {
registerDefinition: vi.fn((definition: ServerDefinition) => {
definitions.set(definition.name, definition);
}),
getDefinition: vi.fn((name: string) => {
const entry = definitions.get(name);
if (!entry) {
throw new Error(`Unknown MCP server '${name}'.`);
}
return entry;
}),
getDefinitions: () => Array.from(definitions.values()),
listTools: vi.fn(async (name: string) => {
const entry = definitions.get(name);
if (!entry) {
throw new Error(`Unknown MCP server '${name}'.`);
}
definitions.set(name, { ...entry, auth: 'oauth' });
return [{ name: 'ok' }];
}),
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
try {
await handleList(runtime, ['https://mcp.granola.ai/mcp', '--persist', persistPath]);
const parsed = JSON.parse(await fs.readFile(persistPath, 'utf8')) as {
mcpServers: Record<string, { auth?: string; baseUrl?: string }>;
};
expect(parsed.mcpServers['mcp-granola-ai-mcp']).toMatchObject({
baseUrl: 'https://mcp.granola.ai/mcp',
auth: 'oauth',
});
} finally {
logSpy.mockRestore();
await fs.rm(tempDir, { recursive: true, force: true });
}
});
it('reuses configured servers when listing by URL', async () => {
const { handleList } = await cliModulePromise;
const definition: ServerDefinition = {
name: 'vercel',
description: 'Vercel MCP',
command: { kind: 'http', url: new URL('https://mcp.vercel.com') },
source: { kind: 'local', path: '/tmp/config.json' },
};
const registerDefinition = vi.fn();
const listTools = vi.fn().mockResolvedValue([{ name: 'ok' }]);
const runtime = {
getDefinitions: () => [definition],
registerDefinition,
getDefinition: () => definition,
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
await handleList(runtime, ['https://mcp.vercel.com']);
expect(listTools).toHaveBeenCalledWith(
'vercel',
expect.objectContaining({ includeSchema: true, autoAuthorize: false, allowCachedAuth: true })
);
expect(registerDefinition).not.toHaveBeenCalled();
});
it('reuses configured servers when listing by HTTP tool selector', async () => {
const { handleList } = await cliModulePromise;
const definition: ServerDefinition = {
name: 'shadcn',
description: 'shadcn/ui registry MCP',
command: { kind: 'http', url: new URL('https://shadcn.io/api/mcp') },
source: { kind: 'local', path: '/tmp/config.json' },
};
const registerDefinition = vi.fn();
const listTools = vi.fn().mockResolvedValue([{ name: 'getComponents' }]);
const runtime = {
getDefinitions: () => [definition],
registerDefinition,
getDefinition: () => definition,
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
await handleList(runtime, ['https://www.shadcn.io/api/mcp.getComponents']);
expect(listTools).toHaveBeenCalledWith(
'shadcn',
expect.objectContaining({ includeSchema: true, autoAuthorize: false, allowCachedAuth: true })
);
expect(registerDefinition).not.toHaveBeenCalled();
});
it('reuses configured servers for scheme-less HTTP tool selectors', async () => {
const { handleList } = await cliModulePromise;
const definition: ServerDefinition = {
name: 'shadcn',
description: 'shadcn/ui registry MCP',
command: { kind: 'http', url: new URL('https://shadcn.io/api/mcp') },
source: { kind: 'local', path: '/tmp/config.json' },
};
const listTools = vi.fn().mockResolvedValue([{ name: 'getComponents' }]);
const runtime = {
getDefinitions: () => [definition],
registerDefinition: vi.fn(),
getDefinition: () => definition,
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
await handleList(runtime, ['shadcn.io/api/mcp.getComponents']);
expect(listTools).toHaveBeenCalledWith(
'shadcn',
expect.objectContaining({ includeSchema: true, autoAuthorize: false, allowCachedAuth: true })
);
});
it('enables cached OAuth when listing all servers', async () => {
const { handleList } = await cliModulePromise;
const definition: ServerDefinition = {
name: 'linear',
description: 'Linear MCP',
auth: 'oauth',
command: { kind: 'http', url: new URL('https://mcp.linear.app/sse') },
source: { kind: 'local', path: '/tmp/config.json' },
};
const listTools = vi.fn().mockResolvedValue([{ name: 'ok' }]);
const runtime = {
getDefinitions: () => [definition],
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
await handleList(runtime, []);
expect(listTools).toHaveBeenCalledWith('linear', {
autoAuthorize: false,
allowCachedAuth: true,
disableOAuth: false,
});
});
it('registers an ad-hoc HTTP server when URL is provided', async () => {
const { handleList } = await cliModulePromise;
const definitions = new Map<string, ServerDefinition>();
const registerDefinition = vi.fn((definition: ServerDefinition) => {
definitions.set(definition.name, definition);
});
const listTools = vi.fn(() => Promise.resolve([]));
const runtime = {
getDefinitions: () => Array.from(definitions.values()),
getDefinition: (name: string) => {
const definition = definitions.get(name);
if (!definition) {
throw new Error('missing');
}
return definition;
},
listTools,
registerDefinition,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
await handleList(runtime, ['https://mcp.example.com/mcp']);
expect(registerDefinition).toHaveBeenCalled();
expect(definitions.get('mcp-example-com-mcp')).toBeDefined();
expect(listTools).toHaveBeenCalledWith(
'mcp-example-com-mcp',
expect.objectContaining({ includeSchema: true, autoAuthorize: false, allowCachedAuth: true })
);
logSpy.mockRestore();
});
it('auto-corrects unknown server names when the edit distance is small', async () => {
const { handleList } = await cliModulePromise;
const definition = linearDefinition;
const getDefinition = vi.fn().mockImplementation((name: string) => {
if (name === 'linear') {
return definition;
}
throw new Error(`Unknown MCP server '${name}'.`);
});
const listTools = vi.fn(() => Promise.resolve([]));
const runtime = {
getDefinition,
getDefinitions: () => [definition],
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
await handleList(runtime, ['linera']);
expect(getDefinition).toHaveBeenCalledTimes(2);
expect(listTools).toHaveBeenCalledWith(
'linear',
expect.objectContaining({ includeSchema: true, autoAuthorize: false, allowCachedAuth: true })
);
const messages = logSpy.mock.calls.map((call) => call.join(' '));
expect(messages.some((line) => line.includes('Auto-corrected server name to linear'))).toBe(true);
logSpy.mockRestore();
});
it('suggests a server name when the typo is large', async () => {
const { handleList } = await cliModulePromise;
const previousExitCode = process.exitCode;
process.exitCode = undefined;
const definition = linearDefinition;
const listTools = vi.fn();
const runtime = {
getDefinition: () => {
throw new Error("Unknown MCP server 'zzz'");
},
getDefinitions: () => [definition],
listTools,
} as unknown as Awaited<ReturnType<(typeof import('../src/runtime.js'))['createRuntime']>>;
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
try {
await handleList(runtime, ['zzz']);
const errorLines = errorSpy.mock.calls.map((call) => call.join(' '));
expect(errorLines.some((line) => line.includes('Did you mean linear?'))).toBe(true);
expect(listTools).not.toHaveBeenCalled();
expect(process.exitCode).toBe(1);
} finally {
errorSpy.mockRestore();
logSpy.mockRestore();
process.exitCode = previousExitCode;
}
});
});