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)