Skip to content
Merged
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
15 changes: 15 additions & 0 deletions .changeset/dev-mcp-connect-hint-origin.md
Original file line number Diff line number Diff line change
@@ -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:<port>` — 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:<boundPort>` — 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.
229 changes: 229 additions & 0 deletions packages/cli/src/commands/dev-mcp-connect-hint-origin.test.ts
Original file line number Diff line number Diff line change
@@ -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<ServerReadyOptions, 'externalBaseOrigin'> = {
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<Record<(typeof AUTH_BASE_URL_ENV_NAMES)[number], string | undefined>> = {};

let lines: string[];
let errSpy: ReturnType<typeof vi.spyOn>;
let logSpy: ReturnType<typeof vi.spyOn>;

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);
}
});
});
});
68 changes: 59 additions & 9 deletions packages/cli/src/commands/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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:<port>`).
*
* 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:<boundPort>`,
* 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';
Expand Down Expand Up @@ -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' });
}
}
});
Expand Down
Loading