From 2df0d05ed6ac05bdf107b918dd460f73ad896d35 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 06:35:27 +0000 Subject: [PATCH 1/2] fix(cli): build `os dev`'s MCP connect hint from the resolved origin, not the listen socket `os dev` printed two MCP addresses in one boot output. The ready banner's `MCP:` row goes through `resolveAuthBaseUrl` (`OS_AUTH_URL` -> legacy `BETTER_AUTH_URL` -> `OS_BASE_URL` -> `http://localhost:`); the `MCP server - connect a coding agent` block built its base from the serve child's `objectstack:listening` url, i.e. the socket the child bound. Under `OS_AUTH_URL=https://localhost:4443` with `dev -p 4001` the two disagreed, and the block's `Connect` line is a command the reader pastes: it registered an MCP entry against an origin discovery never advertises and a TLS proxy never exposes. The hint now resolves through the same function `serve` calls, with the port the child ACTUALLY bound - so the no-variable-set case still answers `http://localhost:` (auto-shifted port included) from the resolver's own tail rather than from a second fallback. When the chain yields no parseable origin the block is omitted rather than reprinting the address the banner just refused to print. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- .../dev-mcp-connect-hint-origin.test.ts | 229 ++++++++++++++++++ packages/cli/src/commands/dev.ts | 68 +++++- 2 files changed, 288 insertions(+), 9 deletions(-) create mode 100644 packages/cli/src/commands/dev-mcp-connect-hint-origin.test.ts diff --git a/packages/cli/src/commands/dev-mcp-connect-hint-origin.test.ts b/packages/cli/src/commands/dev-mcp-connect-hint-origin.test.ts new file mode 100644 index 0000000000..0ba1f1e6dd --- /dev/null +++ b/packages/cli/src/commands/dev-mcp-connect-hint-origin.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// framework#16734 — `os dev`'s MCP connect hint names the origin an MCP client +// can REACH, and the ready banner printed in the same boot agrees with it. +// +// ## Why both printers are driven into ONE buffer +// +// The defect was never visible inside either printer. `os dev` prints two MCP +// addresses from two processes: the serve child's ready banner (`➜ MCP:`, +// `printServerReady`, resolved through `resolveAuthBaseUrl`) and the parent's +// `🤖 MCP server — connect a coding agent` block, which used to be built from +// the child's `objectstack:listening` `url` — the socket it BOUND. Each was +// self-consistent; the bug was that one boot output carried both, saying +// `https://localhost:4443/…` on one row and `http://localhost:4001/…` on the +// next. A pin that reads only the block cannot see that, so every case below +// captures BOTH printers into one ordered buffer and asserts on the whole +// thing — `console.error` (the banner) and `console.log` (the hint) in call +// order, exactly as a terminal renders them. +// +// ## The chain is not restated here +// +// No case below writes the precedence order down as a literal. Both printers +// go through `resolveAuthBaseUrl`, whose own pins live in +// `serve-auth-base-url-diagnostic.test.ts`; what this file asserts is that the +// two printers consult THAT function and therefore cannot disagree. +// +// The parent resolves in its own process, which is sound because it hands the +// child its own `process.env` (plus internal keys) and both run +// `dotenvFlow.config({ node_env: 'development' })` over the same files before +// any lookup. The last case pins the half of that a runtime assertion cannot: +// that dev's child-env literal never sets a chain variable. + +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { printMcpConnectHint } from './dev.js'; +import { AUTH_BASE_URL_ENV_NAMES, resolveAuthBaseUrl } from './serve.js'; +import { printServerReady, type ServerReadyOptions } from '../utils/format.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const DEV_SOURCE = readFileSync(resolve(HERE, 'dev.ts'), 'utf8'); + +const bannerOpts: Omit = { + configFile: 'objectstack.config.ts', + isDev: true, + pluginCount: 1, + uiEnabled: true, + consolePath: '/_console', + mcpEnabled: true, +}; + +/** Every absolute MCP address anywhere in the captured output, deduplicated. */ +const mcpOrigins = (output: string): string[] => + [...new Set([...output.matchAll(/(https?:\/\/[^\s]+?)\/api\/v1\/mcp/g)].map((m) => m[1]))]; + +describe('os dev MCP connect hint — origin (#16734)', () => { + const saved: Partial> = {}; + + let lines: string[]; + let errSpy: ReturnType; + let logSpy: ReturnType; + + const record = (...args: unknown[]) => { + lines.push(args.join(' ').replace(/\u001b\[[0-9;]*m/g, '')); + }; + + beforeEach(() => { + for (const n of AUTH_BASE_URL_ENV_NAMES) { + saved[n] = process.env[n]; + delete process.env[n]; + } + lines = []; + errSpy = vi.spyOn(console, 'error').mockImplementation(record); + logSpy = vi.spyOn(console, 'log').mockImplementation(record); + }); + + afterEach(() => { + errSpy.mockRestore(); + logSpy.mockRestore(); + for (const n of AUTH_BASE_URL_ENV_NAMES) { + if (saved[n] === undefined) delete process.env[n]; + else process.env[n] = saved[n]; + } + }); + + /** + * One `os dev` boot, both printers, in the order a reader sees them: the + * child's banner (its own call site's expression, verbatim) and then the + * parent's connect hint, both told the port the server ACTUALLY bound. + */ + const boot = (boundPort: number, name = 'hotcrm') => { + printServerReady({ ...bannerOpts, externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin }); + printMcpConnectHint({ boundPort, name }); + return lines.join('\n'); + }; + + // ── Acceptance 1 ──────────────────────────────────────────────────────── + describe('with OS_AUTH_URL set, all three hint lines and the banner row agree', () => { + it('reproduces #16530: dev -p 4001 behind OS_AUTH_URL=https://localhost:4443', () => { + process.env.OS_AUTH_URL = 'https://localhost:4443'; + + const output = boot(4001); + + // The banner row and each of the three block lines, from ONE capture. + expect(output).toContain('MCP: https://localhost:4443/api/v1/mcp'); + expect(output).toContain('Endpoint https://localhost:4443/api/v1/mcp'); + expect(output).toContain('Skill https://localhost:4443/api/v1/mcp/skill'); + expect(output).toContain( + 'Connect claude mcp add --transport http hotcrm https://localhost:4443/api/v1/mcp', + ); + + // The reported symptom, stated as the absence it is: the bound socket + // must not appear anywhere in the boot output that advertises MCP. + expect(output).not.toContain('localhost:4001'); + // And the agreement itself, independent of which rows were named above. + expect(mcpOrigins(output)).toEqual(['https://localhost:4443']); + }); + + it('follows the deployment onto its public origin, port and all', () => { + process.env.OS_AUTH_URL = 'https://app.example.com'; + expect(mcpOrigins(boot(3000))).toEqual(['https://app.example.com']); + + lines.length = 0; + process.env.OS_AUTH_URL = 'https://app.example.com:8443'; + expect(mcpOrigins(boot(3000))).toEqual(['https://app.example.com:8443']); + }); + + it('honours the rest of the chain — the legacy name, then OS_BASE_URL', () => { + process.env.BETTER_AUTH_URL = 'https://legacy.example.com'; + process.env.OS_BASE_URL = 'https://base.example.com'; + expect(mcpOrigins(boot(3000))).toEqual(['https://legacy.example.com']); + + lines.length = 0; + delete process.env.BETTER_AUTH_URL; + expect(mcpOrigins(boot(3000))).toEqual(['https://base.example.com']); + }); + }); + + // ── Acceptance 2 — the negative control ───────────────────────────────── + describe('with OS_AUTH_URL unset, the hint still prints the LISTEN origin', () => { + it('names the bound port on an ordinary local boot', () => { + const output = boot(3000, 'my-app'); + + expect(output).toContain('MCP: http://localhost:3000/api/v1/mcp'); + expect(output).toContain('Endpoint http://localhost:3000/api/v1/mcp'); + expect(output).toContain('Skill http://localhost:3000/api/v1/mcp/skill'); + expect(output).toContain( + 'Connect claude mcp add --transport http my-app http://localhost:3000/api/v1/mcp', + ); + expect(mcpOrigins(output)).toEqual(['http://localhost:3000']); + }); + + it("follows dev's auto-shifted port — 3000 busy, bound 3001", () => { + // The case the surrounding code exists to handle. A fix that reached for + // a canonical origin instead of the resolver would print :3000 here, or + // nothing at all; both are worse than the behaviour being repaired. + const output = boot(3001, 'my-app'); + + expect(output).toContain('Endpoint http://localhost:3001/api/v1/mcp'); + expect(output).toContain( + 'Connect claude mcp add --transport http my-app http://localhost:3001/api/v1/mcp', + ); + expect(output).not.toContain('3000'); + expect(mcpOrigins(output)).toEqual(['http://localhost:3001']); + }); + + it('names an ephemeral bound port, never the 0 that was requested', () => { + expect(mcpOrigins(boot(45064))).toEqual(['http://localhost:45064']); + }); + }); + + // ── Acceptance 3 — the unusable-value cases stay the resolver's, unchanged ─ + describe('an unusable base URL prints no connect command, and no guess', () => { + it('set-but-empty OS_AUTH_URL: banner prints paths only, the hint prints nothing', () => { + // Empty is not unset — the chain stops there, so neither OS_BASE_URL nor + // the localhost tail is consulted. `resolveAuthBaseUrl` reports that as + // `baseOrigin: null` (its own pins own that behaviour); what this asserts + // is that BOTH printers obey it. A `claude mcp add` line has no + // paths-only form, so the block is omitted rather than fabricated. + process.env.OS_AUTH_URL = ''; + process.env.OS_BASE_URL = 'https://never-consulted.example.com'; + + const output = boot(3000); + + expect(output).toContain('/api/v1/mcp'); + expect(mcpOrigins(output)).toEqual([]); + expect(output).not.toContain('http://localhost:3000'); + expect(output).not.toContain('never-consulted'); + expect(output).not.toContain('claude mcp add'); + expect(output).toContain('OS_AUTH_URL'); + }); + + it('a value with no scheme is not smuggled in as an origin either', () => { + process.env.OS_AUTH_URL = 'app.example.com'; + + const output = boot(3000); + + expect(mcpOrigins(output)).toEqual([]); + expect(output).not.toContain('claude mcp add'); + expect(output).not.toContain('http://localhost:3000'); + expect(output).not.toContain('app.example.com/api/v1/mcp'); + }); + }); + + // ── What only the source can say ──────────────────────────────────────── + describe('the call site feeds the printer the bound port, and nothing else', () => { + it('hands `printMcpConnectHint` the ACTUALLY BOUND port', () => { + expect(DEV_SOURCE).toContain('printMcpConnectHint({ boundPort: actual,'); + }); + + it('builds no address out of the listening message any more', () => { + // The defect in one line: `base` came from `msg.url`, the bound socket. + expect(DEV_SOURCE).not.toMatch(/msg\.url/); + }); + + it('never sets a base-URL chain variable in the child env it spawns', () => { + // The parent resolves the chain in ITS process and the child resolves it + // again in its own; the two answers are the same value only while dev + // passes these variables through untouched. + const start = DEV_SOURCE.indexOf('const localEnv: NodeJS.ProcessEnv = {'); + expect(start).toBeGreaterThan(-1); + const childEnvLiteral = DEV_SOURCE.slice(start, DEV_SOURCE.indexOf('\n };', start)); + for (const name of AUTH_BASE_URL_ENV_NAMES) { + expect(childEnvLiteral).not.toContain(name); + } + }); + }); +}); diff --git a/packages/cli/src/commands/dev.ts b/packages/cli/src/commands/dev.ts index 77e5c2b428..8bb27f19db 100644 --- a/packages/cli/src/commands/dev.ts +++ b/packages/cli/src/commands/dev.ts @@ -24,6 +24,9 @@ import { readEnvWithDeprecation, isMcpServerEnabled } from '@objectstack/types'; // no range, no reader, no wording; a second copy of the bound is exactly what // #12620 and #12662 protected against. import { describePortSource, parseRequestedPort, formatInvalidPortNotice } from '../utils/port-contract.js'; +// The auth base-URL precedence chain, borrowed from the command that owns it +// (#16734). ⛔ `dev` declares no chain of its own — see printMcpConnectHint. +import { resolveAuthBaseUrl } from './serve.js'; import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime'; /** @@ -63,6 +66,57 @@ export async function resolveDevDatabase(opts: { }); } +/** + * The `os dev` MCP connect hint — the three lines a reader PASTES (#3167, + * #16734). + * + * ## The origin these lines carry is the one an MCP client can REACH + * + * `Connect` is a COMMAND, not documentation: whatever address it names is the + * address `claude mcp add` registers. So the block has to print the origin the + * deployment is reachable on — which is exactly what the ready banner the + * serve child prints in the same boot output already carries, resolved through + * {@link resolveAuthBaseUrl} (`OS_AUTH_URL` → legacy `BETTER_AUTH_URL` → + * `OS_BASE_URL` → `http://localhost:`). + * + * It used to be built from the child's `objectstack:listening` `url`, which is + * the socket the child BOUND, by construction. `OS_AUTH_URL` never entered + * that expression, so behind anything at all — a TLS reverse proxy, a compose + * stack that only `expose`s the app port — one boot printed two origins. + * MEASURED with `objectstack dev -p 4001` under + * `OS_AUTH_URL=https://localhost:4443`: the banner's `➜ MCP:` row said + * `https://localhost:4443/…` and this block said `http://localhost:4001/…`. + * Pasting the latter registers an MCP entry against an origin discovery never + * advertises and the proxy never exposes — and makes the correct row look + * like the typo. + * + * ⛔ The chain is NOT re-derived here: this calls the same function `serve` + * calls, with the port the child ACTUALLY bound. That is also why the ordinary + * local case needs no fallback of its own — with none of the three variables + * set, the resolver's own built-in tail answers `http://localhost:`, + * dev's auto-shifted port (3000 busy → 3001) included. + * + * ## `null` means print nothing, never guess + * + * `baseOrigin` is `null` when the chain produced a value that will not parse — + * a set-but-empty `OS_AUTH_URL=` (which does NOT fall through to the rest of + * the chain), or a value with no scheme. The banner's rule for that case is to + * print the paths with no origin in front of them and name the variable that + * fixes it. A connect COMMAND has no paths-only form, so the honest output + * here is no block at all: falling back to the bound socket would reprint, on + * the same screen, the exact address the banner just refused to print. + */ +export function printMcpConnectHint(opts: { boundPort: number | string; name: string }): void { + const { baseOrigin } = resolveAuthBaseUrl(opts.boundPort); + if (baseOrigin === null) return; + console.log(); + console.log(chalk.cyan(' 🤖 MCP server — connect a coding agent:')); + console.log(` Endpoint ${baseOrigin}/api/v1/mcp`); + console.log(` Skill ${baseOrigin}/api/v1/mcp/skill`); + console.log(chalk.dim(` Connect claude mcp add --transport http ${opts.name} ${baseOrigin}/api/v1/mcp`)); + console.log(chalk.dim(' Disable OS_MCP_SERVER_ENABLED=false')); +} + export default class Dev extends Command { static override description = 'Start development mode — watch sources, rebuild the artifact, and restart the server on change'; @@ -497,15 +551,11 @@ export default class Dev extends Command { // (OS_MCP_SERVER_ENABLED=false) advertises nothing, mirroring the // connect-UI / discovery gates that follow the same switch. if (isMcpServerEnabled()) { - const base = - typeof msg.url === 'string' && msg.url ? msg.url.replace(/\/+$/, '') : `http://localhost:${actual}`; - const name = path.basename(process.cwd()) || 'objectstack'; - console.log(); - console.log(chalk.cyan(' 🤖 MCP server — connect a coding agent:')); - console.log(` Endpoint ${base}/api/v1/mcp`); - console.log(` Skill ${base}/api/v1/mcp/skill`); - console.log(chalk.dim(` Connect claude mcp add --transport http ${name} ${base}/api/v1/mcp`)); - console.log(chalk.dim(' Disable OS_MCP_SERVER_ENABLED=false')); + // ⛔ The ACTUALLY BOUND port, never the child's listen URL: the + // origin these lines carry is resolved from the runtime's own + // precedence chain inside the printer, so the banner the child + // prints and this block cannot name two different deployments. + printMcpConnectHint({ boundPort: actual, name: path.basename(process.cwd()) || 'objectstack' }); } } }); From b466fc5d90ec4621d2e5ced3bd9b004a92563389 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 07:01:39 +0000 Subject: [PATCH 2/2] chore(changeset): patch @objectstack/cli for the dev MCP connect-hint origin fix Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- .changeset/dev-mcp-connect-hint-origin.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .changeset/dev-mcp-connect-hint-origin.md diff --git a/.changeset/dev-mcp-connect-hint-origin.md b/.changeset/dev-mcp-connect-hint-origin.md new file mode 100644 index 0000000000..631f7e5583 --- /dev/null +++ b/.changeset/dev-mcp-connect-hint-origin.md @@ -0,0 +1,15 @@ +--- +"@objectstack/cli": patch +--- + +`os dev`'s MCP connect hint is built from the origin the deployment is REACHABLE on, not from the socket the serve child bound. + +A dev boot printed two MCP addresses. The ready banner's `➜ MCP:` row goes through `resolveAuthBaseUrl` — `OS_AUTH_URL` → legacy `BETTER_AUTH_URL` → `OS_BASE_URL` → `http://localhost:` — while the `🤖 MCP server — connect a coding agent` block below it derived its base from the child's `objectstack:listening` `url`, which is the bound socket by construction. `OS_AUTH_URL` never entered that expression, so anything sitting in front of the app split the two apart: measured on `objectstack dev -p 4001` under `OS_AUTH_URL=https://localhost:4443` behind a TLS reverse proxy, the banner said `https://localhost:4443/…` and the block said `http://localhost:4001/…` in the same output. + +That block's `Connect` line is a command the reader pastes, so the wrong origin was not cosmetic: `claude mcp add` registered an entry against an address discovery never advertises and, behind the proxy, nothing can reach — and the two rows disagreeing made the correct one look like the typo. + +- **One resolver, not two.** The hint now calls the same `resolveAuthBaseUrl` the banner's call site does, with the port the child ACTUALLY bound. The precedence chain is not restated anywhere in `dev`. +- **The ordinary local boot is unchanged, by the resolver's own tail.** With none of the three variables set the chain answers `http://localhost:` — including dev's auto-shift (`3000` busy → `3001`) and an ephemeral port — so the local case needs no second fallback and cannot be broken by a canonical origin being hardcoded in front of it. +- **An unusable base URL now prints no block at all.** When the chain yields nothing parseable — a set-but-empty `OS_AUTH_URL=`, which does not fall through to the rest of the chain, or a value with no scheme — the banner's rule is to print paths with no origin and name the variable that fixes it. A `claude mcp add` line has no paths-only form, so the block is omitted instead of reprinting, on the same screen, the exact address the banner just refused to print. + +`resolveAuthBaseUrl` itself is untouched, including its set-but-empty behaviour; the `Endpoint` / `Skill` / `Connect` wording is unchanged.