From a52aebf0a3b38c90b318f21d50d9d71ca6186607 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Mon, 24 Aug 2026 09:17:51 +0000 Subject: [PATCH 1/2] fix(mcp): read stdio localization after the settings engine bind Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../plugin-settings-bind-window.test.ts | 375 ++++++++++++++++++ packages/mcp/src/plugin.ts | 67 +++- 2 files changed, 428 insertions(+), 14 deletions(-) create mode 100644 packages/mcp/src/__tests__/plugin-settings-bind-window.test.ts diff --git a/packages/mcp/src/__tests__/plugin-settings-bind-window.test.ts b/packages/mcp/src/__tests__/plugin-settings-bind-window.test.ts new file mode 100644 index 0000000000..98eab8dff3 --- /dev/null +++ b/packages/mcp/src/__tests__/plugin-settings-bind-window.test.ts @@ -0,0 +1,375 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ── #11580 — the stdio transport's localization is READ AFTER THE BIND ────── +// +// ## The defect this file pins +// +// `MCPServerPlugin.start()` used to resolve the workspace's localization +// in-line, on the stdio auto-start path: +// +// settingsService = ctx.getService('settings'); +// const localization = await resolveLocalizationContext({ settings, … }); +// +// `SettingsServicePlugin` REGISTERS its service in `init()` but binds its DATA +// ENGINE from a `kernel:ready` hook it registers in its own `start()`. Every +// plugin's `start()` body runs strictly before the first `kernel:ready` +// handler, so that read was inside the bind window under EVERY composition +// order — ordering this plugin after the settings plugin does not help, and the +// `optionalDependencies` edge that repairs the #10250 class would not move it +// (that is exactly why `check:settings-bind-window` ledgered this site as +// `unfixable-by-declaration` rather than `undeclared`). +// +// In the window the read does not FAIL — it succeeds with the wrong answer: +// the empty in-memory fallback plus the manifest defaults answer with +// `source: 'default'`, so `resolveLocalizationContext` returns `UTC` / `en-US`, +// reports no failure, and never reaches its direct `sys_setting` fallback. +// #7279 then holds that value for the life of the transport by design, so a +// long-lived stdio MCP server served every call with the manifest defaults on a +// workspace whose persisted `localization` rows said otherwise, forever. +// +// ## What is asserted, and why it is the CONFIGURED value +// +// A test that only asserted "a settings read happened" would stay green on the +// defect — the defect IS a read, at the wrong time, with a plausible answer. So +// every case below asserts the value that reaches the data engine: the +// `ExecutionContext` carried by `ql.find` must hold the CONFIGURED locale +// (`zh-CN` / `Asia/Shanghai` / `CNY`), which in this fixture exists ONLY behind +// the bind. `sys_setting` answers empty here on purpose, so the settings +// service after its bind is the single possible source of those values. +// +// ## Why the sweep, rather than one arrangement +// +// The card's claim is an ORDERING claim ("under every composition order"), and +// a pin that booted one arrangement would leave exactly that claim unpinned. +// So the sweep boots all 24 permutations of the four plugins through the REAL +// `LiteKernel` — real `resolvePluginOrder`, real phase sequencing, real hook +// dispatch — and each case additionally asserts the arrangement it MEASURED +// (`startOrder`), so a kernel that silently normalized the order into one shape +// would fail here instead of quietly collapsing 24 cases into one. +// +// ## Why a settings DOUBLE and not `SettingsServicePlugin` +// +// `@objectstack/mcp` does not depend on `@objectstack/service-settings` and +// must not grow that edge for a test. The double reproduces the two facts this +// card turns on, and nothing else: the service is registered in `init()`, and +// its engine binds from a `kernel:ready` hook registered in `start()`. The +// direction of any drift is safe: if the real plugin ever bound EARLIER, this +// double would merely be a stricter window than production has, so the pin +// would still be sound. `check:settings-bind-window` is the gate that watches +// the real provider's shape. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; + +import { MCPServerPlugin } from '../plugin.js'; +import { MCPServerRuntime } from '../mcp-server-runtime.js'; + +/** What the workspace has PERSISTED — readable only once the engine is bound. */ +const CONFIGURED: Record = { + timezone: 'Asia/Shanghai', + locale: 'zh-CN', + currency: 'CNY', +}; + +/** + * What the settings service answers INSIDE the bind window: the manifest + * defaults, `source: 'default'`, non-empty — which is why the defect is silent + * (`resolveLocalizationContext` takes this branch and returns successfully). + */ +const MANIFEST_DEFAULTS: Record = { timezone: 'UTC', locale: 'en-US' }; + +type ReadPhase = 'pre-bind' | 'bound'; + +interface SettingsDouble { + service: Record; + /** Every namespace read, tagged with the phase it landed in. */ + reads: Array<{ namespace: string; keys: string[]; phase: ReadPhase }>; + /** Reads already taken at the moment the engine bound (the pre-bind count). */ + readsAtBind: number | undefined; + bind(): void; +} + +function createSettingsDouble(): SettingsDouble { + let bound = false; + const double: SettingsDouble = { + reads: [], + readsAtBind: undefined, + bind() { + double.readsAtBind = double.reads.length; + bound = true; + }, + service: {}, + }; + const answer = (keys: string[]) => { + const source = bound ? CONFIGURED : MANIFEST_DEFAULTS; + const out: Record = {}; + for (const key of keys) { + const value = source[key]; + out[key] = value === undefined ? undefined : { value, source: bound ? 'tenant' : 'default' }; + } + return out; + }; + double.service = { + get: vi.fn(async (namespace: string, key: string) => { + double.reads.push({ namespace, keys: [key], phase: bound ? 'bound' : 'pre-bind' }); + return answer([key])[key]; + }), + getMany: vi.fn(async (namespace: string, keys: string[]) => { + double.reads.push({ namespace, keys, phase: bound ? 'bound' : 'pre-bind' }); + return answer(keys); + }), + }; + return double; +} + +/** + * `SettingsServicePlugin`'s lifecycle shape, and only that: register in + * `init()`, bind the engine from a `kernel:ready` hook registered in `start()`. + */ +function settingsProviderPlugin(double: SettingsDouble): Plugin { + return { + name: 'com.objectstack.service.settings', + providesServices: ['settings'], + async init(ctx: PluginContext) { + ctx.registerService('settings', double.service); + }, + async start(ctx: PluginContext) { + ctx.hook('kernel:ready', async () => { + double.bind(); + }); + }, + }; +} + +interface FindCall { + object: string; + options: { context?: ExecutionContext }; +} + +/** + * The `objectql` service, faked at the one seam this path uses. `sys_api_key` + * feeds the REAL `resolveAuthzContext` chain (same fixture as + * `plugin.record-resource-exposure.test.ts`); `sys_setting` answers EMPTY so + * the configured locale can only come from the bound settings service. + */ +function fakeObjectQL(finds: FindCall[]) { + return { + find: vi.fn(async (object: string, options: FindCall['options']) => { + finds.push({ object, options }); + if (object === 'sys_api_key') return [{ id: 'k1', user_id: 'usr_stdio', revoked: false }]; + if (object === 'sys_setting') return []; + return [{ id: 'r_1', name: 'Acme' }]; + }), + findOne: vi.fn(async () => null), + count: vi.fn(async () => 0), + }; +} + +function fakeMetadata() { + return { + listObjects: vi.fn(async () => []), + getObject: vi.fn(async () => null), + get: vi.fn(async () => null), + list: vi.fn(async () => []), + exists: vi.fn(async () => false), + getRegisteredTypes: vi.fn(async () => ['object']), + register: vi.fn(), + unregister: vi.fn(), + }; +} + +function servicePlugin(name: string, serviceName: string, service: unknown): Plugin { + return { + name, + providesServices: [serviceName], + async init(ctx: PluginContext) { + ctx.registerService(serviceName, service); + }, + async start() {}, + }; +} + +interface BootResult { + /** The `ExecutionContext` the last row read carried into the engine. */ + context: ExecutionContext; + settings: SettingsDouble; + /** Plugin `start()` order as the KERNEL actually ran it. */ + startOrder: string[]; +} + +/** + * Boot the real plugin set through the real kernel in `order`, then drive + * `reads` record reads through the principal-bound reader the plugin built. + * + * Only three `MCPServerRuntime` seams are stubbed, all for the same reason — + * `start()` would claim this test process's real stdin/stdout, and the prompt / + * resource bridges have nothing to say here. Everything this card is about + * (when the settings handle is taken, what it answers, what reaches the engine) + * runs for real. + */ +async function bootAndRead(orderKeys: readonly PluginKey[], reads = 1): Promise { + const finds: FindCall[] = []; + const settings = createSettingsDouble(); + const startOrder: string[] = []; + + const plugins: Record = { + settings: settingsProviderPlugin(settings), + objectql: servicePlugin('com.objectstack.engine.objectql', 'objectql', fakeObjectQL(finds)), + metadata: servicePlugin('com.objectstack.service.metadata', 'metadata', fakeMetadata()), + mcp: new MCPServerPlugin({ autoStart: true }), + }; + for (const key of ['settings', 'objectql', 'metadata'] as const) { + const plugin = plugins[key]; + const inner = plugin.start!.bind(plugin); + plugin.start = async (ctx: PluginContext) => { + startOrder.push(plugin.name); + await inner(ctx); + }; + } + + let getRecord: ((object: string, id: string) => Promise) | undefined; + const bridgeResources = vi + .spyOn(MCPServerRuntime.prototype, 'bridgeResources') + .mockImplementation((_meta: unknown, reader?: unknown) => { + // Also the observation point for MCP's own position in the start order: + // this runs inside `MCPServerPlugin.start()`. + startOrder.push('com.objectstack.mcp'); + getRecord = reader as typeof getRecord; + }); + const bridgePrompts = vi + .spyOn(MCPServerRuntime.prototype, 'bridgePrompts') + .mockImplementation(async () => {}); + const transportStart = vi + .spyOn(MCPServerRuntime.prototype, 'start') + .mockImplementation(async () => {}); + + try { + const kernel = new LiteKernel({ logger: { level: 'silent' } }); + for (const key of orderKeys) kernel.use(plugins[key]); + await kernel.bootstrap(); + } finally { + bridgeResources.mockRestore(); + bridgePrompts.mockRestore(); + transportStart.mockRestore(); + } + + if (!getRecord) throw new Error('stdio start registered no record reader'); + for (let i = 0; i < reads; i++) await getRecord('crm_account', `r_${i}`); + + const rowRead = finds.filter((f) => f.object === 'crm_account').pop(); + if (!rowRead?.options.context) throw new Error('no row read carried an ExecutionContext'); + return { context: rowRead.options.context, settings, startOrder }; +} + +type PluginKey = 'settings' | 'objectql' | 'metadata' | 'mcp'; + +const PLUGIN_KEYS: readonly PluginKey[] = ['settings', 'objectql', 'metadata', 'mcp']; + +function permutations(items: readonly T[]): T[][] { + if (items.length <= 1) return [[...items]]; + const out: T[][] = []; + for (let i = 0; i < items.length; i++) { + const rest = [...items.slice(0, i), ...items.slice(i + 1)]; + for (const tail of permutations(rest)) out.push([items[i]!, ...tail]); + } + return out; +} + +const ORDERS = permutations(PLUGIN_KEYS); + +/** Plugin name → the sweep key that composed it. */ +const KEY_OF: Record = { + 'com.objectstack.service.settings': 'settings', + 'com.objectstack.engine.objectql': 'objectql', + 'com.objectstack.service.metadata': 'metadata', + 'com.objectstack.mcp': 'mcp', +}; + +describe('#11580 — the stdio transport reads localization after the settings bind', () => { + const originalEnv = { ...process.env }; + + beforeEach(() => { + process.env = { ...originalEnv }; + delete process.env.OS_MCP_SERVER_TRANSPORT; + delete process.env.OS_MCP_STDIO_ENABLED; + process.env.OS_MCP_STDIO_API_KEY = 'osk_settings_bind_window_pin'; + }); + + afterEach(() => { + process.env = { ...originalEnv }; + vi.restoreAllMocks(); + }); + + // ── The fixture is falsifiable ──────────────────────────────────────────── + // Without this, every green below could mean "the double always answers + // zh-CN" rather than "the read landed after the bind". + it('the settings double answers the MANIFEST DEFAULTS before its engine binds', async () => { + const double = createSettingsDouble(); + const before = await (double.service.getMany as (n: string, k: string[]) => Promise>)( + 'localization', + ['timezone', 'locale', 'currency'], + ); + expect(before.timezone?.value).toBe('UTC'); + expect(before.locale?.value).toBe('en-US'); + expect(before.timezone?.source).toBe('default'); + expect(before.currency).toBeUndefined(); + + double.bind(); + const after = await (double.service.getMany as (n: string, k: string[]) => Promise>)( + 'localization', + ['timezone', 'locale', 'currency'], + ); + expect(after.locale?.value).toBe('zh-CN'); + expect(after.timezone?.value).toBe('Asia/Shanghai'); + }); + + // ── The ordering sweep ──────────────────────────────────────────────────── + it.each(ORDERS.map((order) => [order.join(' → '), order] as const))( + 'composition %s: the CONFIGURED locale reaches the transport', + async (_name, order) => { + const { context, settings, startOrder } = await bootAndRead(order); + + // The card's payload: the persisted value, not the manifest default. + expect(context.locale).toBe('zh-CN'); + expect(context.timezone).toBe('Asia/Shanghai'); + expect(context.currency).toBe('CNY'); + + // …and it got there by reading AFTER the bind, not by luck. + expect(settings.reads.length).toBeGreaterThan(0); + expect(settings.reads.map((r) => r.phase)).not.toContain('pre-bind'); + expect(settings.readsAtBind).toBe(0); + + // The arrangement this case actually exercised — measured, so a kernel + // that normalized every permutation into one order fails here rather + // than collapsing the sweep into a single repeated case. + const realized = startOrder.filter((name) => order.some((k) => KEY_OF[name] === k)); + expect(realized.map((name) => KEY_OF[name])).toEqual([...order]); + }, + ); + + // ── The read's PHASE, stated directly ───────────────────────────────────── + it('takes no settings read in start() or in any kernel:ready handler', async () => { + const { settings } = await bootAndRead(['mcp', 'objectql', 'metadata', 'settings']); + + // `readsAtBind` is the count at the moment the settings engine bound — i.e. + // everything `init()`, every `start()` body, and every `kernel:ready` + // handler registered before the provider's own had taken by then. The + // defect's signature is this number being 1. + expect(settings.readsAtBind).toBe(0); + expect(settings.reads).toHaveLength(1); + expect(settings.reads[0]!.phase).toBe('bound'); + expect(settings.reads[0]!.namespace).toBe('localization'); + }); + + // ── #7279's property survives the move ──────────────────────────────────── + it('still resolves localization ONCE for the life of the transport', async () => { + const { settings, context } = await bootAndRead(['settings', 'objectql', 'metadata', 'mcp'], 5); + + // Moving the read must not turn it into a per-call cost on a long-lived + // process — the counterweight to the fix (#7279). + expect(settings.reads).toHaveLength(1); + expect(context.locale).toBe('zh-CN'); + }); +}); diff --git a/packages/mcp/src/plugin.ts b/packages/mcp/src/plugin.ts index 7fd4a17b05..b4b75196f7 100644 --- a/packages/mcp/src/plugin.ts +++ b/packages/mcp/src/plugin.ts @@ -37,8 +37,10 @@ import { CONNECT_AGENT_UI_BUNDLE } from './connect-ui.js'; * than merely moved. Re-run per read, so revocation of a key takes effect on * the next call of a live stdio session (ADR-0101 D1). * - * @param localization Resolved ONCE by `start()` and threaded in — see the - * hoist there for why this function must not resolve it itself. + * @param localization Resolved ONCE for the life of the transport and threaded + * in — see the hoist in `start()` for why this function must not resolve it + * itself, and [#11580] for why that resolution happens at `kernel:bootstrapped` + * rather than in the `start()` body. */ async function resolveStdioExecutionContext( ql: { find: (object: string, opts: unknown) => Promise }, @@ -296,21 +298,58 @@ export class MCPServerPlugin implements Plugin { // service falls back to the direct `sys_setting` read and then to the // built-ins (`UTC` / `en-US`), i.e. exactly the values this face used to // get by carrying nothing. - let settingsService: unknown; - try { - settingsService = ctx.getService('settings'); - } catch { - settingsService = undefined; - } - const localization: EntryLocalization = await resolveLocalizationContext({ - ql: scopedQl, - settings: settingsService, - tenantId: initial.tenantId, - userId: initial.userId, + // + // [#11580] Resolved from a `kernel:bootstrapped` hook, NOT in this + // `start()` body. `SettingsServicePlugin` registers the settings service + // in `init()` but binds its DATA ENGINE from a hook IT registers in its + // own `start()` (`settings-service-plugin.ts`, `kernel:ready`). Every + // plugin's `start()` body runs strictly before the first `kernel:ready` + // handler, so a read taken here is inside that bind window under EVERY + // composition order — being ordered after the settings plugin does not + // help, and an `optionalDependencies` edge (the #10250 shape) would not + // move it. In the window `getMany('localization', …)` is answered by the + // empty in-memory fallback plus the manifest defaults with + // `source: 'default'`; it does NOT throw, so the direct `sys_setting` + // fallback inside `resolveLocalizationContext` never runs either. The + // wrong answer (`UTC` / `en-US`) is indistinguishable from a right one, + // and the hoist above then holds it for the life of the process: a + // long-lived stdio server served every call with the manifest defaults + // on a workspace whose persisted `localization` rows said otherwise, and + // never self-corrected. + // + // `kernel:bootstrapped` is the earliest phase strictly after that bind + // (`kernel:ready` → `kernel:bootstrapped` → `kernel:listening`) and the + // one `SettingsService.reportPreBindRead` names as the remedy. The memo + // is what keeps #7279's property across both entry points: whoever gets + // there first pays for the single resolution, everyone else awaits it. + // The lazy entry is not decoration — it is why this cannot deadlock a + // host that never fires the boot hooks (a bare kernel, a test harness): + // such a host resolves at first use instead, which is still later than + // `start()` and still once for the life of the transport. + let localizationOnce: Promise | undefined; + const resolveLocalizationOnce = (): Promise => { + localizationOnce ??= (async () => { + let settingsService: unknown; + try { + settingsService = ctx.getService('settings'); + } catch { + settingsService = undefined; + } + return resolveLocalizationContext({ + ql: scopedQl, + settings: settingsService, + tenantId: initial.tenantId, + userId: initial.userId, + }); + })(); + return localizationOnce; + }; + ctx.hook('kernel:bootstrapped', async () => { + await resolveLocalizationOnce(); }); // Re-resolve per call so a revoked/expired key stops working on the next read. const resolvePrincipal = async (): Promise => { - const ec = await resolveStdioExecutionContext(scopedQl, apiKey, localization); + const ec = await resolveStdioExecutionContext(scopedQl, apiKey, await resolveLocalizationOnce()); if (!ec) throw new Error('MCP stdio identity is no longer valid (key revoked or expired)'); return ec; }; From 6ef6b911e227fbc857bc272a00b98bdecdafaffb Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Mon, 24 Aug 2026 09:34:26 +0000 Subject: [PATCH 2/2] chore(devx): delete the com.objectstack.mcp pre-bind ledger entry The site it recorded is repaired, and the ledger is shrink-only: an entry that outlives its defect holds a ceiling for a leak that no longer exists. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- ...-stdio-localization-after-settings-bind.md | 36 +++++++++++++++++++ scripts/check-settings-bind-window.mjs | 10 ------ 2 files changed, 36 insertions(+), 10 deletions(-) create mode 100644 .changeset/mcp-stdio-localization-after-settings-bind.md diff --git a/.changeset/mcp-stdio-localization-after-settings-bind.md b/.changeset/mcp-stdio-localization-after-settings-bind.md new file mode 100644 index 0000000000..4a67551268 --- /dev/null +++ b/.changeset/mcp-stdio-localization-after-settings-bind.md @@ -0,0 +1,36 @@ +--- +'@objectstack/mcp': patch +--- + +MCP stdio transport now serves the workspace's CONFIGURED timezone/locale +instead of the manifest defaults + +The stdio transport resolved its localization inside `MCPServerPlugin.start()`. +`SettingsServicePlugin` registers its service in `init()` but binds its data +engine from a `kernel:ready` hook registered in its own `start()`, and every +plugin's `start()` body runs strictly before the first `kernel:ready` handler — +so that read was inside the settings bind window under **every** composition +order. Being ordered after the settings plugin did not help, and the +`optionalDependencies` edge that repairs the neighbouring ordering defects would +not have moved it either. + +In that window the read does not fail: the empty in-memory fallback plus the +manifest defaults answer with `source: 'default'`, so `resolveLocalizationContext` +returned `UTC` / `en-US` and reported success, never reaching its direct +`sys_setting` fallback. The value is then held for the life of the transport by +design, so a long-lived stdio MCP server served every call with `UTC` / `en-US` +on a workspace whose persisted `localization` settings said otherwise, and never +self-corrected. + +The resolution now happens from a `kernel:bootstrapped` hook — the earliest phase +strictly after the bind, and the one `SettingsService.reportPreBindRead` names as +the remedy — memoized so it stays one resolution for the life of the transport +rather than a per-call settings read. A host that never fires the boot hooks +resolves it lazily at first use instead, so nothing can deadlock on a hook that +never arrives. + +**Behaviour change on a declared setting**: a deployment that has configured +`localization.timezone` / `localization.locale` / `localization.currency` will +see those values take effect on the stdio MCP surface, where it previously +always received the platform defaults. Formula evaluation (`ctx.timezone`) and +message localization on that surface change accordingly. diff --git a/scripts/check-settings-bind-window.mjs b/scripts/check-settings-bind-window.mjs index 62bf3208fe..b7779b0d92 100644 --- a/scripts/check-settings-bind-window.mjs +++ b/scripts/check-settings-bind-window.mjs @@ -161,16 +161,6 @@ const KNOWN_PRE_BIND_READS = [ 'composition AuthPlugin is used() before the capability loop registers ' + 'SettingsServicePlugin, so its hooks fire first. Repair is the #10250 declaration.', }, - { - plugin: 'com.objectstack.mcp', - verdict: 'unfixable-by-declaration', - issue: '#11580', - note: - 'MCPServerPlugin.start() resolves settings on the stdio auto-start path and immediately ' + - 'awaits resolveLocalizationContext with it, once for the life of the transport (#7279). ' + - 'A start()-body read is inside the window under EVERY composition order, so no ' + - 'declaration repairs it — the read has to move or become lazy.', - }, ]; // ── Discovery ────────────────────────────────────────────────────────────────