diff --git a/README.md b/README.md index bfd15b35..c01ba993 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,12 @@ A client-requested `ttl` must be a non-negative integer number of milliseconds; `ttl` is clamped between 1 second and `taskMaxTtlMs`, and the effective value is returned in the `CreateTaskResult`. +`taskDefaultTtlMs`, `taskMaxTtlMs` and `taskLeaseMs` must be positive integer numbers of +milliseconds and `taskMaxRetainedPerPrincipal` a positive integer. Registration throws a +`TypeError` naming the option otherwise, so a typo or a value read from an environment variable +as a string fails at startup rather than failing every task. A `taskDefaultTtlMs` above +`taskMaxTtlMs` is capped to it. + Tasks are stored in memory by default and in Redis when a `redis` option is given, so any instance can serve a poll for a task created on another. diff --git a/src/index.ts b/src/index.ts index a57f9b31..f255cb26 100644 --- a/src/index.ts +++ b/src/index.ts @@ -102,6 +102,14 @@ const mcpPlugin = fp(async function (app: FastifyInstance, opts: MCPPluginOption // Checked before any Redis connection is opened, so a bad value cannot leak one validateTaskLimit('taskMaxRetainedPerPrincipal', opts.taskMaxRetainedPerPrincipal) } + // A zero lease lapses at once and a NaN or negative ttl expires every task, so + // reject them (and strings read from env variables) here rather than at runtime + for (const name of ['taskLeaseMs', 'taskDefaultTtlMs', 'taskMaxTtlMs'] as const) { + const value: unknown = opts[name] + if (value !== undefined && !(Number.isSafeInteger(value) && (value as number) > 0)) { + throw new TypeError(`${name} must be a positive integer number of milliseconds, got ${String(value)}`) + } + } const tools = new Map() const resources = new Map() const prompts = new Map() diff --git a/test/task-options-validation.test.ts b/test/task-options-validation.test.ts new file mode 100644 index 00000000..2330872c --- /dev/null +++ b/test/task-options-validation.test.ts @@ -0,0 +1,83 @@ +import { test, describe } from 'node:test' +import type { TestContext } from 'node:test' +import Fastify from 'fastify' +import mcpPlugin from '../src/index.ts' +import type { MCPPluginOptions } from '../src/types.ts' + +async function assertRegistrationRejects (t: TestContext, opts: Partial, pattern: RegExp): Promise { + const app = Fastify() + try { + await t.assert.rejects( + async () => { await app.register(mcpPlugin, { enableTasks: true, ...opts } as MCPPluginOptions) }, + (err: Error) => { + t.assert.ok(err instanceof TypeError, `expected a TypeError, got ${err}`) + t.assert.match(err.message, pattern) + return true + } + ) + } finally { + await app.close() + } +} + +async function assertRegistrationAccepts (t: TestContext, opts: Partial): Promise { + const app = Fastify() + try { + await app.register(mcpPlugin, { enableTasks: true, ...opts } as MCPPluginOptions) + await app.ready() + } finally { + await app.close() + } + t.assert.ok(true) +} + +// Values that would otherwise surface only at runtime: a lease that lapses at +// once, a ttl that is NaN or negative, or a string read from an env variable +const invalidValues: unknown[] = [0, -5, NaN, Infinity, -Infinity, 1.5, '30000', null, Number.MAX_SAFE_INTEGER + 1] + +describe('task options validation (#302)', () => { + for (const option of ['taskLeaseMs', 'taskDefaultTtlMs', 'taskMaxTtlMs'] as const) { + test(`registration throws a TypeError naming ${option} for invalid values`, async (t: TestContext) => { + for (const value of invalidValues) { + await assertRegistrationRejects(t, { [option]: value as any }, new RegExp(`^${option} must be a positive integer`)) + } + }) + + test(`registration throws for an invalid ${option} even when tasks are disabled`, async (t: TestContext) => { + await assertRegistrationRejects(t, { enableTasks: false, [option]: 0 }, new RegExp(`^${option} must be`)) + }) + + test(`registration accepts a positive integer or undefined ${option}`, async (t: TestContext) => { + await assertRegistrationAccepts(t, { [option]: undefined }) + await assertRegistrationAccepts(t, { [option]: 1000 }) + }) + } + + test('registration throws for an invalid taskMaxRetainedPerPrincipal', async (t: TestContext) => { + for (const value of [0, -1, NaN, 1.5, '20', null]) { + await assertRegistrationRejects(t, { taskMaxRetainedPerPrincipal: value as any }, /^taskMaxRetainedPerPrincipal must be a positive integer/) + } + await assertRegistrationAccepts(t, { taskMaxRetainedPerPrincipal: undefined }) + await assertRegistrationAccepts(t, { taskMaxRetainedPerPrincipal: 20 }) + }) + + test('registration accepts a taskDefaultTtlMs above taskMaxTtlMs, which is capped', async (t: TestContext) => { + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 120_000, taskMaxTtlMs: 60_000 }) + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 2 * 3600_000 }) + }) + + test('registration accepts taskDefaultTtlMs equal to taskMaxTtlMs', async (t: TestContext) => { + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 60_000, taskMaxTtlMs: 60_000 }) + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 15 * 60_000, taskMaxTtlMs: 60 * 60_000 }) + }) + + test('registration accepts a taskMaxTtlMs below the default taskDefaultTtlMs', async (t: TestContext) => { + // An unset taskDefaultTtlMs is capped by the ceiling, as before + await assertRegistrationAccepts(t, { taskMaxTtlMs: 30_000 }) + }) + + test('registration fails before connecting to Redis', async (t: TestContext) => { + // An unreachable port: had the plugin connected first, registration would hang or leak + await assertRegistrationRejects(t, { redis: { host: '127.0.0.1', port: 1, lazyConnect: true }, taskLeaseMs: 0 } as any, /^taskLeaseMs must be/) + }) +})