From d626967e6862d40f8b18783175d88f94ff6163ca Mon Sep 17 00:00:00 2001 From: Christoph Dyllick-Brenzinger Date: Fri, 4 Sep 2026 15:17:33 +0200 Subject: [PATCH] Fix single-select write validation rejecting every option (v1.6.2) Every row-write tool (add_row, append_rows, update_rows, upsert_rows) failed for single-select and multi-select columns with: Column "Statut": unknown option "En cours". Valid options: mapMetadataToGeneric flattens select options to a string array ({ options: ["En cours"] }), but getSelectOptions still expected the raw SeaTable object form and did opts.map(o => o.name), producing Set{undefined}. That set is non-empty, so validation ran and rejected every value, and [undefined].join(', ') rendered an empty option list. Broken since v1.3.0 (312daac), which changed the mapping without updating the validator. The unit tests missed it because their fixtures use the object form that no longer reaches production. getSelectOptions now accepts both shapes. Also stop add_select_options from creating duplicates: it now reads the column's current options and skips the ones that already exist, which is what users hit while working around the bug above. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q94wosDRhUCAx7HDAk4HU9 --- README.md | 2 +- package.json | 2 +- src/mcp/tools/addSelectOption.ts | 49 +++++++++++- src/schema/validate.ts | 9 ++- tests/addSelectOptions.spec.ts | 131 +++++++++++++++++++++++++++++++ tests/validate.spec.ts | 38 +++++++++ 6 files changed, 224 insertions(+), 7 deletions(-) create mode 100644 tests/addSelectOptions.spec.ts diff --git a/README.md b/README.md index f4f030d..2f5d9f5 100644 --- a/README.md +++ b/README.md @@ -300,7 +300,7 @@ The metrics server only starts in HTTP mode (not stdio) and binds to `0.0.0.0` - **`get_row_activities`** — Get change history of a row (who changed what, when, old/new values) - **`create_snapshot`** — Create a snapshot of the current base (10 min cooldown) -- **`add_select_options`** — Add new options to single-select or multi-select columns +- **`add_select_options`** — Add new options to single-select or multi-select columns (existing options are skipped, no duplicates) - **`ping_seatable`** — Health check with latency monitoring ## Supported Column Types diff --git a/package.json b/package.json index 513d3c5..aa3fa84 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@seatable/mcp-seatable", - "version": "1.6.1", + "version": "1.6.2", "type": "module", "license": "MIT", "mcpName": "io.github.seatable/seatable", diff --git a/src/mcp/tools/addSelectOption.ts b/src/mcp/tools/addSelectOption.ts index 14a46f2..68d0a29 100644 --- a/src/mcp/tools/addSelectOption.ts +++ b/src/mcp/tools/addSelectOption.ts @@ -1,5 +1,6 @@ import { z } from 'zod' +import { makeError } from '../../errors.js' import { ToolRegistrar } from './types.js' const InputSchema = z.object({ @@ -12,25 +13,67 @@ const InputSchema = z.object({ })).min(1).describe('Array of options to add'), }) +/** Names of the options a select column already has. Empty if the column has none yet. */ +function existingOptionNames(metadata: any, table: string, column: string): Set { + const tableObj = (metadata?.tables ?? []).find((t: any) => t.name === table) + if (!tableObj) { + throw makeError('ERR_SCHEMA_UNKNOWN_TABLE', `Table "${table}" not found`, { table }) + } + const columnObj = (tableObj.columns ?? []).find((c: any) => c.name === column) + if (!columnObj) { + throw makeError('ERR_SCHEMA_UNKNOWN_COLUMN', `Column "${column}" not found in table "${table}"`, { + table, + column, + }) + } + const opts = columnObj.data?.options + if (!Array.isArray(opts)) return new Set() + return new Set( + opts.map((o: any) => o?.name).filter((n: unknown): n is string => typeof n === 'string') + ) +} + export const registerAddSelectOptions: ToolRegistrar = (server, { client, getInputSchema }) => { server.registerTool( 'add_select_options', { title: 'Add Select Options', - description: 'Add new options to a single-select or multi-select column. Use this before writing rows with option values that do not exist yet.', + description: 'Add new options to a single-select or multi-select column. Use this before writing rows with option values that do not exist yet. Options that already exist are skipped, so this will not create duplicates.', inputSchema: getInputSchema(InputSchema), annotations: { readOnlyHint: false, destructiveHint: false, idempotentHint: true, openWorldHint: false }, }, async (args: unknown) => { const parsed = InputSchema.parse(args) + // Skip options the column already has — the SeaTable API happily creates + // a second option with the same name, which is never what the caller wants. + const existing = existingOptionNames(await client.getMetadata(), parsed.table, parsed.column) + const skipped: string[] = [] + const seen = new Set() + const toAdd: typeof parsed.options = [] + for (const opt of parsed.options) { + if (existing.has(opt.name)) { + skipped.push(opt.name) + } else if (!seen.has(opt.name)) { + seen.add(opt.name) + toAdd.push(opt) + } + } + + const added = toAdd.map((o) => o.name) + if (toAdd.length === 0) { + return { + content: [{ type: 'text', text: JSON.stringify({ added, skipped, success: true }) }], + } + } + const result = await client.addColumnOptions({ table: parsed.table, column: parsed.column, - options: parsed.options, + options: toAdd, }) - return { content: [{ type: 'text', text: JSON.stringify(result) }] } + return { content: [{ type: 'text', text: JSON.stringify({ added, skipped, ...result }) }] } } ) } diff --git a/src/schema/validate.ts b/src/schema/validate.ts index 0eebb71..2a58d36 100644 --- a/src/schema/validate.ts +++ b/src/schema/validate.ts @@ -132,9 +132,14 @@ function validateDate(colName: string, value: unknown): void { } function getSelectOptions(col: GenericColumn): Set { - const opts = col.options?.options as Array<{ name: string }> | undefined + // Options arrive in two shapes: the raw SeaTable form ([{ name, id, color }]) + // and the flattened form produced by mapMetadataToGeneric (["open", "closed"]). + const opts = col.options?.options as Array<{ name?: unknown } | string> | undefined if (!Array.isArray(opts)) return new Set() - return new Set(opts.map((o) => o.name)) + const names = opts + .map((o) => (typeof o === 'string' ? o : (o as { name?: unknown })?.name)) + .filter((n): n is string => typeof n === 'string' && n !== '') + return new Set(names) } function validateSingleSelect(colName: string, value: unknown, col: GenericColumn): void { diff --git a/tests/addSelectOptions.spec.ts b/tests/addSelectOptions.spec.ts new file mode 100644 index 0000000..2ee39f9 --- /dev/null +++ b/tests/addSelectOptions.spec.ts @@ -0,0 +1,131 @@ +import { describe, expect, it } from 'vitest' + +import { registerAddSelectOptions } from '../src/mcp/tools/addSelectOption.js' +import type { ClientLike } from '../src/mcp/tools/types.js' + +type Handler = (args: unknown) => Promise + +/** Metadata with one single-select column that already has two options. */ +function metadataWithOptions(names: string[]) { + return { + tables: [ + { + _id: 'tbl1', + name: 'Tasks', + columns: [ + { key: 'col1', name: 'Title', type: 'text' }, + { + key: 'col2', + name: 'Statut', + type: 'single-select', + data: { options: names.map((n, i) => ({ id: `o${i}`, name: n, color: '#fff' })) }, + }, + ], + }, + ], + } +} + +/** Wire the registrar to a fake client and return the handler plus recorded calls. */ +function setup(existing: string[]) { + const calls: Array<{ table: string; column: string; options: Array<{ name: string }> }> = [] + const client = { + getMetadata: async () => metadataWithOptions(existing), + addColumnOptions: async (args: any) => { + calls.push(args) + return { success: true } + }, + } as unknown as ClientLike + + let handler: Handler | undefined + const server = { + registerTool: (_name: string, _cfg: unknown, h: Handler) => { + handler = h + }, + } + + registerAddSelectOptions(server as any, { + client, + env: {} as any, + getInputSchema: (s: any) => s, + }) + + return { handler: handler as Handler, calls } +} + +function parse(result: any): any { + return JSON.parse(result.content[0].text) +} + +describe('add_select_options', () => { + it('adds options that do not exist yet', async () => { + const { handler, calls } = setup(['En cours']) + const result = await handler({ + table: 'Tasks', + column: 'Statut', + options: [{ name: 'Reconnu' }], + }) + + expect(calls).toHaveLength(1) + expect(calls[0].options.map((o) => o.name)).toEqual(['Reconnu']) + expect(parse(result).added).toEqual(['Reconnu']) + }) + + it('skips options that already exist instead of duplicating them', async () => { + const { handler, calls } = setup(['En cours', 'Reconnu']) + const result = await handler({ + table: 'Tasks', + column: 'Statut', + options: [{ name: 'En cours' }, { name: 'Non reconnu' }], + }) + + expect(calls).toHaveLength(1) + expect(calls[0].options.map((o) => o.name)).toEqual(['Non reconnu']) + const data = parse(result) + expect(data.added).toEqual(['Non reconnu']) + expect(data.skipped).toEqual(['En cours']) + }) + + it('makes no API call when every option already exists', async () => { + const { handler, calls } = setup(['En cours', 'Reconnu']) + const result = await handler({ + table: 'Tasks', + column: 'Statut', + options: [{ name: 'En cours' }, { name: 'Reconnu' }], + }) + + expect(calls).toHaveLength(0) + const data = parse(result) + expect(data.added).toEqual([]) + expect(data.skipped).toEqual(['En cours', 'Reconnu']) + }) + + it('collapses duplicates within a single request', async () => { + const { handler, calls } = setup([]) + await handler({ + table: 'Tasks', + column: 'Statut', + options: [{ name: 'Nouveau' }, { name: 'Nouveau' }], + }) + + expect(calls[0].options.map((o) => o.name)).toEqual(['Nouveau']) + }) + + it('treats option names as case-sensitive, like SeaTable does', async () => { + const { handler, calls } = setup(['En cours']) + await handler({ + table: 'Tasks', + column: 'Statut', + options: [{ name: 'EN COURS' }], + }) + + expect(calls[0].options.map((o) => o.name)).toEqual(['EN COURS']) + }) + + it('fails clearly when the column does not exist', async () => { + const { handler } = setup(['En cours']) + await expect( + handler({ table: 'Tasks', column: 'Nope', options: [{ name: 'x' }] }) + ).rejects.toThrow(/Nope/) + }) +}) diff --git a/tests/validate.spec.ts b/tests/validate.spec.ts index 94083ed..5689d85 100644 --- a/tests/validate.spec.ts +++ b/tests/validate.spec.ts @@ -311,3 +311,41 @@ describe('validateRowsAgainstSchema', () => { }) }) }) + +// Regression: the shape produced by mapMetadataToGeneric must validate correctly. +// mapMetadataToGeneric flattens select options to a string array ({ options: ['open'] }), +// while the fixtures above use the raw SeaTable object form ({ options: [{ name: 'open' }] }). +// Both must be accepted — see ticket "unknown option ... Valid options: ". +describe('select validation against mapped schema shape', () => { + const mappedSchema: GenericSchema = { + base_id: 'base1', + tables: [ + { + id: 'tbl1', + name: 'Tasks', + columns: [ + { id: 'col1', name: 'Title', type: 'text' }, + { id: 'col2', name: 'Statut', type: 'single_select', options: { options: ['En cours', 'Reconnu'] } }, + { id: 'col3', name: 'Tags', type: 'multi_select', options: { options: ['urgent', 'low'] } }, + ], + }, + ], + } + + it('accepts a valid single-select option', () => { + const rows = [{ Title: 'A', Statut: 'En cours' }] + validateRowsAgainstSchema(mappedSchema, 'Tasks', rows) + }) + + it('accepts valid multi-select options', () => { + const rows = [{ Title: 'A', Tags: ['urgent'] }] + validateRowsAgainstSchema(mappedSchema, 'Tasks', rows) + }) + + it('still rejects an unknown option and lists the valid ones', () => { + const rows = [{ Title: 'A', Statut: 'Nope' }] + expect(() => validateRowsAgainstSchema(mappedSchema, 'Tasks', rows)).toThrowError( + 'unknown option "Nope". Valid options: En cours, Reconnu' + ) + }) +})