* feat(cli): lift config-file headers/timeouts/OAuth into serverSettings (#1482) The CLI resolved file-sourced servers via resolveServerConfigs(), which returns bare MCPServerConfig and dropped disk-level headers, timeouts, and OAuth — CLI connections from a --catalog/--config file silently ignored them. The TUI already lifted these via mcpConfigToServerEntries(). Extract the TUI's catalog/config -> per-server-settings resolution into a shared core module (core/mcp/node/servers.ts: loadServerEntries + selectServerEntry) and route both CLI and TUI through it, so the two share one file->settings path. --header still overrides the file's headers for the run while preserving the file's timeouts/OAuth. - core: add loadServerEntries/selectServerEntry/headersToServerSettings - tui: tui-servers.ts re-exports the shared core helpers (no logic dup) - cli: resolve via loadServerEntries + selectServerEntry; drop the CLI-only conflict check and bare-config path - tests: core servers.test.ts (settings lifting, header merge/override, stdio overrides, seed/error/conflict, selection); CLI integration tests asserting a config header is sent over HTTP and --header overrides it - docs: CLI README note on file settings being applied Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): address #1500 review — dedup overrides, drop dead wrapper, clearer error, spec refresh - Remove the now-unused resolveLaunchServerConfigs() wrapper (both CLI and TUI route through loadServerEntries now); migrate its default-catalog launch tests to compose withDefaultCatalogPath + resolveServerConfigs. - Deduplicate applyStdioOverrides against config.ts applyOverrides: export the latter (now trims cwd) and reuse it in servers.ts. - selectServerEntry's unknown-name error is source-agnostic ("not found" not "not found in config file") since entries may be a single ad-hoc default, where "config file" would mislead. - Refresh specification/ docs (v2_cli_tui_launcher.md, v2_catalog_launch_config.md): CLI/TUI resolution now via loadServerEntries/selectServerEntry; mark gap G1 resolved (#1482); withDefaultConfigPath → withDefaultCatalogPath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(cli): extend smoke:cli to cover #1482 lift→serverSettings Adds end-to-end smoke coverage for the config-file headers/timeouts/OAuth lift introduced in #1482: - unknown --server now errors source-agnostically ("not found") - multi-server catalog selection via --server (and the omit-it error) - --header merge does not break the stdio connect path - HTTP transport: a config-file `headers` object is lifted onto the wire, and a CLI --header overrides it (verified against an in-process test server's recorded requests via a non-blocking spawn) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
73b651ff21
commit
e72d4307f3
1 changed files with 1033 additions and 420 deletions
|
|
@ -14,6 +14,15 @@
|
|||
* "no servers"), rather than erroring "Config file not found".
|
||||
* 3. A read-only `--config` that is missing errors and is NOT seeded.
|
||||
* 4. `--catalog` + `--config` is rejected as mutually exclusive.
|
||||
* 5. An unknown `--server` errors with the source-agnostic "not found.
|
||||
* Available servers: …" message (#1482).
|
||||
* 6. A multi-server catalog selects via `--server` and errors (asking for a
|
||||
* selection) when it is omitted.
|
||||
* 7. `--header` merges into the resolved server's settings without breaking
|
||||
* the connect path (the file→settings lift from #1482).
|
||||
* 8. Over an HTTP transport (in-process test server), a config-file `headers`
|
||||
* object is lifted onto the wire, and a CLI `--header` overrides it — the
|
||||
* headline lift→transport path of #1482, verified end to end.
|
||||
*
|
||||
* Exits non-zero (failing CI / `npm run validate`) on any mismatch.
|
||||
*
|
||||
|
|
@ -22,10 +31,11 @@
|
|||
* (`test-servers/build`) is built on demand here if missing.
|
||||
*/
|
||||
|
||||
import { spawnSync } from "node:child_process";
|
||||
import { spawn, spawnSync } from "node:child_process";
|
||||
import { mkdtempSync, rmSync, existsSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, resolve } from "node:path";
|
||||
import { pathToFileURL } from "node:url";
|
||||
|
||||
const repoRoot = resolve(import.meta.dirname, "..");
|
||||
const launcher = join(repoRoot, "clients", "launcher", "build", "index.js");
|
||||
|
|
@ -35,24 +45,34 @@ const testServer = join(
|
|||
"build",
|
||||
"test-server-stdio.js",
|
||||
);
|
||||
const httpTestServerModule = join(
|
||||
repoRoot,
|
||||
"test-servers",
|
||||
"build",
|
||||
"test-server-http.js",
|
||||
);
|
||||
|
||||
function fail(message) {
|
||||
console.error(`smoke:cli FAILED — ${message}`);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
/** Build the bundled stdio test server if it isn't present yet. */
|
||||
/** Build the bundled test servers (stdio + http) if they aren't present yet. */
|
||||
function ensureTestServer() {
|
||||
if (existsSync(testServer)) return;
|
||||
if (existsSync(testServer) && existsSync(httpTestServerModule)) return;
|
||||
console.log("smoke:cli — building test-servers (missing build output)...");
|
||||
const r = spawnSync("npx", ["tsc", "-p", "test-servers", "--noCheck"], {
|
||||
cwd: repoRoot,
|
||||
stdio: "inherit",
|
||||
});
|
||||
if (r.status !== 0 || !existsSync(testServer)) {
|
||||
if (
|
||||
r.status !== 0 ||
|
||||
!existsSync(testServer) ||
|
||||
!existsSync(httpTestServerModule)
|
||||
) {
|
||||
fail(
|
||||
"could not build the stdio test server (test-servers/build/test-server-stdio.js). " +
|
||||
"Run `npm run test-servers:build` from clients/cli.",
|
||||
"could not build the test servers (test-servers/build/test-server-stdio.js, " +
|
||||
"test-server-http.js). Run `npm run test-servers:build` from clients/cli.",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
@ -67,6 +87,28 @@ function runCli(args, extraEnv = {}) {
|
|||
return { status: r.status, stdout: r.stdout ?? "", stderr: r.stderr ?? "" };
|
||||
}
|
||||
|
||||
/**
|
||||
* Async variant of `runCli` using non-blocking `spawn`. Required for the HTTP
|
||||
* case: the test server runs in THIS process's event loop, so a blocking
|
||||
* `spawnSync` would deadlock (the CLI child's request could never be serviced).
|
||||
* Returns { status, stdout, stderr }.
|
||||
*/
|
||||
function runCliAsync(args, extraEnv = {}) {
|
||||
return new Promise((resolveRun) => {
|
||||
const child = spawn(process.execPath, [launcher, "--cli", ...args], {
|
||||
cwd: repoRoot,
|
||||
env: { ...process.env, ...extraEnv },
|
||||
});
|
||||
let stdout = "";
|
||||
let stderr = "";
|
||||
child.stdout.setEncoding("utf-8");
|
||||
child.stderr.setEncoding("utf-8");
|
||||
child.stdout.on("data", (d) => (stdout += d));
|
||||
child.stderr.on("data", (d) => (stderr += d));
|
||||
child.on("close", (status) => resolveRun({ status, stdout, stderr }));
|
||||
});
|
||||
}
|
||||
|
||||
if (!existsSync(launcher)) {
|
||||
fail(`launcher build not found at ${launcher} — run \`npm run build\` first`);
|
||||
}
|
||||
|
|
@ -161,9 +203,212 @@ try {
|
|||
);
|
||||
}
|
||||
|
||||
// 5) Unknown --server errors with the source-agnostic message (#1482): the
|
||||
// entry may come from a file or a single ad-hoc target, so it is "not
|
||||
// found" rather than "not found in config file".
|
||||
const unknownServer = runCli([
|
||||
"--catalog",
|
||||
catalogPath,
|
||||
"--server",
|
||||
"nope",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (unknownServer.status === 0) {
|
||||
fail("--server with an unknown name should have exited non-zero");
|
||||
}
|
||||
if (
|
||||
!/Server 'nope' not found\. Available servers: test/.test(
|
||||
unknownServer.stderr,
|
||||
)
|
||||
) {
|
||||
fail(
|
||||
`expected "Server 'nope' not found. Available servers: test"; got:\n${unknownServer.stderr}`,
|
||||
);
|
||||
}
|
||||
|
||||
// 6) Multi-server catalog: --server selects one; omitting it errors asking for
|
||||
// a selection (selectServerEntry over a multi-entry source).
|
||||
const multiCatalogPath = join(work, "multi-catalog.json");
|
||||
writeFileSync(
|
||||
multiCatalogPath,
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
one: { type: "stdio", command: process.execPath, args: [testServer] },
|
||||
two: { type: "stdio", command: process.execPath, args: [testServer] },
|
||||
},
|
||||
}),
|
||||
);
|
||||
const ambiguous = runCli([
|
||||
"--catalog",
|
||||
multiCatalogPath,
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (ambiguous.status === 0) {
|
||||
fail("multi-server catalog without --server should have exited non-zero");
|
||||
}
|
||||
if (!/Multiple servers found/.test(ambiguous.stderr)) {
|
||||
fail(
|
||||
`expected "Multiple servers found" without --server; got:\n${ambiguous.stderr}`,
|
||||
);
|
||||
}
|
||||
const selected = runCli([
|
||||
"--catalog",
|
||||
multiCatalogPath,
|
||||
"--server",
|
||||
"two",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (selected.status !== 0) {
|
||||
fail(
|
||||
`--server two against a multi-server catalog exited ${selected.status}\n${selected.stderr || selected.stdout}`,
|
||||
);
|
||||
}
|
||||
let selectedParsed;
|
||||
try {
|
||||
selectedParsed = JSON.parse(selected.stdout);
|
||||
} catch {
|
||||
fail(`--server two did not return JSON on stdout:\n${selected.stdout}`);
|
||||
}
|
||||
if (!(selectedParsed.tools ?? []).some((t) => t.name === "echo")) {
|
||||
fail(`--server two tools/list missing expected "echo" tool`);
|
||||
}
|
||||
|
||||
// 7) --header is merged into the resolved server's settings and broadcast to
|
||||
// every entry (#1482). Over stdio the header is inert, so the real check is
|
||||
// that the merge path does not break the connect: tools/list still works.
|
||||
const withHeader = runCli([
|
||||
"--catalog",
|
||||
catalogPath,
|
||||
"--server",
|
||||
"test",
|
||||
"--header",
|
||||
"X-Smoke: 1",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (withHeader.status !== 0) {
|
||||
fail(
|
||||
`tools/list with --header exited ${withHeader.status}\n${withHeader.stderr || withHeader.stdout}`,
|
||||
);
|
||||
}
|
||||
let withHeaderParsed;
|
||||
try {
|
||||
withHeaderParsed = JSON.parse(withHeader.stdout);
|
||||
} catch {
|
||||
fail(
|
||||
`tools/list with --header did not return JSON on stdout:\n${withHeader.stdout}`,
|
||||
);
|
||||
}
|
||||
if (!(withHeaderParsed.tools ?? []).some((t) => t.name === "echo")) {
|
||||
fail(`tools/list with --header missing expected "echo" tool`);
|
||||
}
|
||||
|
||||
// 8) HTTP transport — the headline of #1482: a config-file `headers` object is
|
||||
// lifted into InspectorServerSettings and actually sent ON THE WIRE, and a
|
||||
// CLI `--header` overrides it. Unlike stdio (where headers are inert), this
|
||||
// proves the full lift→transport path. The HTTP test server runs IN-PROCESS
|
||||
// so we can read back the headers it recorded; the CLI connects to it as a
|
||||
// child via `runCliAsync` (a blocking spawnSync would deadlock the server's
|
||||
// event loop). `getRecordedRequests()` lowercases header names (Node http).
|
||||
const { createTestServerHttp } = await import(
|
||||
pathToFileURL(httpTestServerModule).href
|
||||
);
|
||||
const { createEchoTool, createTestServerInfo } = await import(
|
||||
pathToFileURL(
|
||||
join(repoRoot, "test-servers", "build", "test-server-fixtures.js"),
|
||||
).href
|
||||
);
|
||||
const httpServer = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo(),
|
||||
tools: [createEchoTool()],
|
||||
serverType: "streamable-http",
|
||||
});
|
||||
const httpPort = await httpServer.start();
|
||||
try {
|
||||
const httpConfigPath = join(work, "http-config.json");
|
||||
const writeHttpConfig = (headers) =>
|
||||
writeFileSync(
|
||||
httpConfigPath,
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
http: {
|
||||
type: "http",
|
||||
url: `http://localhost:${httpPort}/mcp`,
|
||||
headers,
|
||||
},
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
// 8a) File-level header is lifted onto the wire.
|
||||
writeHttpConfig({ "X-Smoke-Header": "from-config" });
|
||||
const httpList = await runCliAsync([
|
||||
"--config",
|
||||
httpConfigPath,
|
||||
"--server",
|
||||
"http",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (httpList.status !== 0) {
|
||||
fail(
|
||||
`HTTP tools/list exited ${httpList.status}\n${httpList.stderr || httpList.stdout}`,
|
||||
);
|
||||
}
|
||||
const headerValues = () =>
|
||||
httpServer
|
||||
.getRecordedRequests()
|
||||
.map((r) => r.headers?.["x-smoke-header"])
|
||||
.filter(Boolean);
|
||||
if (!headerValues().includes("from-config")) {
|
||||
fail(
|
||||
`expected config-file header "X-Smoke-Header: from-config" on the wire; ` +
|
||||
`recorded values: ${JSON.stringify(headerValues())}`,
|
||||
);
|
||||
}
|
||||
|
||||
// 8b) CLI --header overrides the file header on the wire (mergeSettings).
|
||||
httpServer.clearRecordings();
|
||||
const httpOverride = await runCliAsync([
|
||||
"--config",
|
||||
httpConfigPath,
|
||||
"--server",
|
||||
"http",
|
||||
"--header",
|
||||
"X-Smoke-Header: from-cli",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
if (httpOverride.status !== 0) {
|
||||
fail(
|
||||
`HTTP tools/list with --header override exited ${httpOverride.status}\n${httpOverride.stderr || httpOverride.stdout}`,
|
||||
);
|
||||
}
|
||||
const overrideValues = headerValues();
|
||||
if (!overrideValues.includes("from-cli")) {
|
||||
fail(
|
||||
`expected --header override "X-Smoke-Header: from-cli" on the wire; ` +
|
||||
`recorded values: ${JSON.stringify(overrideValues)}`,
|
||||
);
|
||||
}
|
||||
if (overrideValues.includes("from-config")) {
|
||||
fail(
|
||||
`--header should override the file header, but "from-config" still reached the wire; ` +
|
||||
`recorded values: ${JSON.stringify(overrideValues)}`,
|
||||
);
|
||||
}
|
||||
} finally {
|
||||
await httpServer.stop();
|
||||
}
|
||||
|
||||
console.log(
|
||||
"smoke:cli OK — tools/list over stdio via --catalog; default-catalog seed; " +
|
||||
"read-only --config error (no seed); --catalog/--config conflict",
|
||||
"read-only --config error (no seed); --catalog/--config conflict; " +
|
||||
"unknown --server error; multi-server --server selection; --header merge; " +
|
||||
"HTTP config-header lift + --header override on the wire",
|
||||
);
|
||||
} finally {
|
||||
rmSync(work, { recursive: true, force: true });
|
||||
|
|
|
|||
Loading…
Reference in a new issue