From a8b03463e568b1089896fa8a9ef6027bec82c68f Mon Sep 17 00:00:00 2001 From: Christoph Dyllick-Brenzinger Date: Fri, 4 Sep 2026 16:17:12 +0200 Subject: [PATCH] Fail loudly when the base context is read outside its scope (v1.6.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5 fixed the one place that read the client after runWithBase() had returned. This makes the next such mistake announce itself instead of hiding. ContextualClient resolved through registry.resolve(getStore()), which could not tell "no scope at all" from "in a scope, no base named" — both arrived as undefined. With a single base the out-of-scope read then quietly succeeded via the registry default; with two or more it threw `Multiple bases available … Specify "base" parameter`, blaming the caller for omitting an argument they had supplied. That combination is what let the defect survive five months: the configuration that would have exposed it is not the one we run. The store now carries { base } instead of the bare name, so an absent store is detectable, and reading outside the scope throws a message that names the actual error — the same way regardless of how many bases are configured. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tn4FT2sVqoFz1QdpNDNAy7 --- CLAUDE.md | 6 ++++++ package.json | 2 +- src/seatable/contextualClient.ts | 26 +++++++++++++++++++++++--- tests/contextualClient.spec.ts | 31 +++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3e3386a..2c609e9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,6 +88,12 @@ The server adapter in `server.ts` collects these registrations into an internal `ClientRegistry` (`src/seatable/clientRegistry.ts`) manages multiple `SeaTableClient` instances keyed by base name. `ContextualClient` (`src/seatable/contextualClient.ts`) implements `ClientLike` and proxies calls to the right client based on a `base` parameter. In multi-base mode, `handleCallTool()` extracts the `base` arg and `handleListTools()` injects it into every tool schema dynamically — no changes needed in individual tool files. +### Multi-base and the base context + +`ContextualClient` binds the target base to an `AsyncLocalStorage` scope opened by `runWithBase()`, which `handleCallTool()` wraps around the tool handler. **Everything that needs the client must read it inside that scope.** Reading after it returns throws `ContextualClient was used outside runWithBase()` — deliberately, and identically whether one base or ten are configured. + +That guard exists because the opposite behaviour hid a bug for five months (fixed in #5): the post-handler log line called `getBaseInfo()` after the scope had closed. With a single base it silently succeeded via the registry default; with two or more `resolve(undefined)` threw `Multiple bases available … Specify "base" parameter`, which reads like a caller mistake and discarded a result the handler had already produced correctly. Coverage is in `tests/contextualClient.spec.ts` and `tests/server.spec.ts`. + ### Schema Utilities - `src/schema/map.ts` — converts SeaTable metadata to `GenericSchema` format diff --git a/package.json b/package.json index 0798628..60ddeeb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@seatable/mcp-seatable", - "version": "1.6.3", + "version": "1.6.4", "type": "module", "license": "MIT", "mcpName": "io.github.seatable/seatable", diff --git a/src/seatable/contextualClient.ts b/src/seatable/contextualClient.ts index f8bd93d..8f43762 100644 --- a/src/seatable/contextualClient.ts +++ b/src/seatable/contextualClient.ts @@ -3,7 +3,12 @@ import { AsyncLocalStorage } from 'node:async_hooks' import type { ClientLike } from '../mcp/tools/types.js' import type { ClientRegistry } from './clientRegistry.js' -const baseContext = new AsyncLocalStorage() +/** + * The store is an object rather than the bare name so that "no scope at all" + * (getStore() === undefined) stays distinguishable from "in a scope, no base + * named" ({ base: undefined }). Those two need very different errors. + */ +const baseContext = new AsyncLocalStorage<{ base?: string }>() /** * A ClientLike proxy that delegates to a specific base client @@ -24,11 +29,26 @@ export class ContextualClient implements ClientLike { * will route to the specified base. */ runWithBase(name: string | undefined, fn: () => T): T { - return baseContext.run(name, fn) + return baseContext.run({ base: name }, fn) } + /** + * Resolving outside a runWithBase() scope is always a programming error, and + * it used to hide well: with a single base it quietly succeeded via the + * registry default, and only with two or more did it surface — as "Specify + * base parameter", which blames the caller for omitting an argument they + * did supply. It cost five months to find that way once, so it is named + * here and fails the same way whatever the base count. + */ private get client(): ClientLike { - return this.registry.resolve(baseContext.getStore()) + const store = baseContext.getStore() + if (!store) { + throw new Error( + 'ContextualClient was used outside runWithBase(). The base context only exists for the ' + + 'duration of that scope — read what you need inside it rather than after it returns.' + ) + } + return this.registry.resolve(store.base) } // Base info diff --git a/tests/contextualClient.spec.ts b/tests/contextualClient.spec.ts index 5639292..fce57bc 100644 --- a/tests/contextualClient.spec.ts +++ b/tests/contextualClient.spec.ts @@ -74,6 +74,37 @@ describe('ContextualClient', () => { expect(() => ctx.runWithBase(undefined, () => ctx.listTables())).toThrow('Specify "base" parameter') }) + /* + * The failure mode this guards against cost five months to find: reading the + * client after the runWithBase() scope returned. With one base it silently + * worked (resolve(undefined) fell back to the default), with two it failed as + * "Specify base parameter" — which reads like the caller forgot an argument + * they had in fact supplied. Both halves are wrong, so the escape is named. + */ + it('throws a diagnostic error when used outside runWithBase()', () => { + const registry = createMockRegistry(['A', 'B']) + const ctx = new ContextualClient(registry as any) + + expect(() => ctx.getBaseInfo()).toThrow(/outside runWithBase/) + }) + + it('throws the same diagnostic outside the scope even with a single base', () => { + // The single-base case used to succeed here, which is exactly what kept + // the defect invisible until someone configured a second base. + const registry = createMockRegistry(['OnlyBase']) + const ctx = new ContextualClient(registry as any) + + expect(() => ctx.listTables()).toThrow(/outside runWithBase/) + }) + + it('still reports a genuinely missing base argument inside the scope', () => { + const registry = createMockRegistry(['A', 'B']) + const ctx = new ContextualClient(registry as any) + + // Inside the scope with no name, the caller really did omit "base". + expect(() => ctx.runWithBase(undefined, () => ctx.listTables())).toThrow('Specify "base" parameter') + }) + it('throws for unknown base name', () => { const registry = createMockRegistry(['CRM']) const ctx = new ContextualClient(registry as any)