From 59ecaee650882d278e8f127bfe0b4ba21eb9217c Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Fri, 3 Jul 2026 11:20:32 +0200 Subject: [PATCH] feat(mcp): qualified identifiers in all tool output schemas (P1-2) (#877) Agents grabbed the wrong id when list rows exposed a bare 'id' next to qualified ids (journal_entry_id) with no type distinction, got NOT_FOUND, and had to re-derive (agent.feedback). Additive sweep of all 11 bare-id sites: every output identifier now ships a fully qualified sibling (transaction_id, fact_id, atom_id, company_id, dimension_id, dimension_value_id); the bare 'id' stays as a deprecated alias for compatibility. New ratchet test (qualified-ids.test.ts): a shrinking grandfathered list carries the deprecated aliases; any NEW bare 'id' in an output schema fails CI, every remaining alias must ship alongside its qualified sibling, and stale grandfather entries must be pruned. Convention documented in .claude/rules/mcp-server.md. Full unit suite: 6,577 green. Part of dev_docs/mcp_optimization_plan.md (P1-2). Co-authored-by: Claude Fable 5 --- .claude/rules/mcp-server.md | 1 + .../__tests__/agent-briefing.test.ts | 2 + .../__tests__/qualified-ids.test.ts | 84 +++++++++++++++++++ extensions/general/mcp-server/server.ts | 84 ++++++++++++------- 4 files changed, 141 insertions(+), 30 deletions(-) create mode 100644 extensions/general/mcp-server/__tests__/qualified-ids.test.ts diff --git a/.claude/rules/mcp-server.md b/.claude/rules/mcp-server.md index cda091d4..4b977951 100644 --- a/.claude/rules/mcp-server.md +++ b/.claude/rules/mcp-server.md @@ -22,4 +22,5 @@ Accounted exposes its bookkeeping engine as an MCP server for Claude Desktop/Cod - Machine-readable staging contract: `tools/list` (and `gnubok_search_tools` detail=full) attach a derived `_meta` to staging writes so an agent knows the contract WITHOUT reading prose. `deriveToolMeta()` keys off `outputSchema === STAGED_OPERATION_SCHEMA` and emits `{ requires_approval: true, approve_tool: 'gnubok_approve_pending_operation', preflight? }`; it merges under any literal `_meta` (e.g. UI widget hints), which wins on collision. Add to `TOOL_PREFLIGHT_MAP` when a write has a genuine read-only pre-flight (e.g. `gnubok_run_year_end` → `gnubok_year_end_readiness`). A new staging tool inherits `_meta` for free — just keep its description declaring it stages (guarded by `__tests__/staging-meta.test.ts`). `confirmed=true` belongs on the APPROVE call for high-risk ops, never on the staging tool; only some tools accept `dry_run`/`idempotency_key` — never imply they are universal. - Skill/atom summaries: `gnubok_list_skills` and `gnubok_get_agent_briefing` pass registry `description` fields through `toSummary()` (`skills/atoms.ts`) — the raw SKILL.md frontmatter is a long keyword-stuffed trigger list authored for CLI matching, not display copy, and gets truncated mid-sentence otherwise. Full bodies are fetched via `gnubok_load_skill`. The local `.claude/skills/*` are the Claude-Code surface; the `agent_atom_registry` rows seeded from the same bodies are the canonical connector surface — when they overlap, the connector atom is authoritative for MCP users. - Tools that touch a fiscal-period-bound date (categorize, mark paid, create voucher, correct/reverse entry, approve supplier invoice) pass `dateForPeriodCheck` to `stagePendingOperation` so the response includes `period_status: { period_id, status: open|locked|closed, lock_date }`. Widgets and agents use this to disable writes without round-trips. +- Qualified identifiers: no bare `id` in tool OUTPUT schemas — every identifier is fully qualified (`transaction_id`, `journal_entry_id`, `fact_id`, `dimension_value_id`, …) so agents never guess which entity an id belongs to. Guarded by `__tests__/qualified-ids.test.ts` (a shrinking grandfathered list carries the deprecated `id` aliases; new tools must use qualified names only). - Error envelope: every tool failure flows through the single dispatch point (`toToolError` → `getStructuredError`) and returns `{ error: { code, message_sv, message_en, retryable, remediation? } }`. `retryable` is ALWAYS an explicit boolean — `true` means transient (back off and retry the identical call, pairing with `idempotency_key` where the tool accepts one); `false` means permanent for these inputs (fix arguments/state, never blind-retry). Unclassified transient failures (deadlock, statement timeout, connection drop, upstream 429/5xx) surface as code `TRANSIENT_ERROR`. Don't wrap errors in ad-hoc shapes inside tools — throw (typed errors or plain `Error`; SQLSTATE/message inference handles classification) and let the dispatch layer build the envelope. Client-side failures (e.g. the claude.ai approval elicitation's "No approval received") never reach this envelope — idempotency keys are what make those blind retries safe. diff --git a/extensions/general/mcp-server/__tests__/agent-briefing.test.ts b/extensions/general/mcp-server/__tests__/agent-briefing.test.ts index 7bfb4808..503b874c 100644 --- a/extensions/general/mcp-server/__tests__/agent-briefing.test.ts +++ b/extensions/general/mcp-server/__tests__/agent-briefing.test.ts @@ -234,6 +234,7 @@ describe('gnubok_get_agent_briefing tool', () => { } expect(result.company).toEqual({ id: 'company-1', + company_id: 'company-1', name: 'Acme AB', org_number: '556677-8899', entity_type: 'aktiebolag', @@ -254,6 +255,7 @@ describe('gnubok_get_agent_briefing tool', () => { company: { id: string; name: string | null; accounting_method: string | null } } expect(result.company.id).toBe('company-1') + expect((result.company as { company_id?: string }).company_id).toBe('company-1') expect(result.company.name).toBeNull() expect(result.company.accounting_method).toBeNull() }) diff --git a/extensions/general/mcp-server/__tests__/qualified-ids.test.ts b/extensions/general/mcp-server/__tests__/qualified-ids.test.ts new file mode 100644 index 00000000..1456b624 --- /dev/null +++ b/extensions/general/mcp-server/__tests__/qualified-ids.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from 'vitest' +import { tools } from '../server' + +/** + * Identifier discipline (mcp_optimization_plan P1-2): agents grabbed the + * wrong id when list rows exposed a bare `id` next to qualified ids like + * `journal_entry_id` with no type distinction, got NOT_FOUND, and had to + * re-derive. Every identifier in a tool OUTPUT schema must be fully + * qualified (`transaction_id`, `journal_entry_id`, `fact_id`, …). + * + * Bare `id` survives only as a deprecated alias at the GRANDFATHERED paths + * below, each shipping alongside its qualified sibling. This list may only + * SHRINK (remove entries as the deprecated aliases are dropped) — a new bare + * `id` anywhere fails this test; add the qualified name instead. + */ + +const GRANDFATHERED_BARE_ID_PATHS = [ + 'gnubok_forget_fact', + 'gnubok_get_agent_briefing.atoms[]', + 'gnubok_get_agent_briefing.company', + 'gnubok_get_agent_briefing.memory[]', + 'gnubok_list_dimension_values.dimension', + 'gnubok_list_dimension_values.values[]', + 'gnubok_list_dimensions.dimensions[]', + 'gnubok_list_dimensions.dimensions[].values[]', + 'gnubok_list_transactions_without_documents.transactions[]', + 'gnubok_list_uncategorized_transactions.transactions[]', + 'gnubok_remember_fact', +].sort() + +type SchemaNode = { + properties?: Record + items?: unknown +} + +function collectBareIdPaths(): { path: string; siblingKeys: string[] }[] { + const found: { path: string; siblingKeys: string[] }[] = [] + const walk = (schema: unknown, path: string) => { + if (!schema || typeof schema !== 'object') return + const s = schema as SchemaNode + if (s.properties) { + const keys = Object.keys(s.properties) + if (keys.includes('id')) found.push({ path, siblingKeys: keys }) + for (const [key, val] of Object.entries(s.properties)) walk(val, `${path}.${key}`) + } + if (s.items) walk(s.items, `${path}[]`) + } + for (const t of tools) walk(t.outputSchema, t.name) + return found +} + +describe('qualified identifiers in tool output schemas', () => { + it('no tool exposes a bare `id` outside the shrinking grandfathered list', () => { + const actual = collectBareIdPaths() + .map((f) => f.path) + .sort() + const newOffenders = actual.filter((p) => !GRANDFATHERED_BARE_ID_PATHS.includes(p)) + expect( + newOffenders, + `New bare \`id\` in an output schema — use a qualified name (transaction_id, journal_entry_id, …) instead:\n${newOffenders.join('\n')}`, + ).toEqual([]) + }) + + it('every remaining bare `id` ships alongside its qualified sibling', () => { + const missingSibling = collectBareIdPaths().filter( + (f) => !f.siblingKeys.some((k) => k !== 'id' && k.endsWith('_id')), + ) + expect( + missingSibling.map((f) => f.path), + 'bare `id` without a qualified *_id sibling — agents cannot migrate off the deprecated alias', + ).toEqual([]) + }) + + it('the grandfathered list only shrinks (entries removed when aliases are dropped)', () => { + const actual = collectBareIdPaths() + .map((f) => f.path) + .sort() + const stale = GRANDFATHERED_BARE_ID_PATHS.filter((p) => !actual.includes(p)) + expect( + stale, + `Grandfathered paths no longer exist — remove them from GRANDFATHERED_BARE_ID_PATHS:\n${stale.join('\n')}`, + ).toEqual([]) + }) +}) diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 6b98ad93..57b740d4 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -1936,12 +1936,13 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read fact_id instead' }, + fact_id: { type: 'string' }, kind: { type: 'string' }, content: { type: 'string' }, created_at: { type: 'string' }, }, - required: ['id', 'kind', 'content', 'created_at'], + required: ['id', 'fact_id', 'kind', 'content', 'created_at'], }, annotations: { readOnlyHint: false, @@ -1994,6 +1995,7 @@ export const tools: McpTool[] = [ .eq('id', dupe.id) return { id: dupe.id, + fact_id: dupe.id, kind: dupe.kind, content: dupe.content, created_at: dupe.created_at, @@ -2015,7 +2017,7 @@ export const tools: McpTool[] = [ .select('id, kind, content, created_at') .single() if (error) throw new Error(`Failed to remember fact: ${error.message}`) - return data + return { ...data, fact_id: data.id } }, }, @@ -2036,10 +2038,11 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read fact_id instead' }, + fact_id: { type: 'string' }, is_active: { type: 'boolean' }, }, - required: ['id', 'is_active'], + required: ['id', 'fact_id', 'is_active'], }, annotations: { readOnlyHint: false, @@ -2058,7 +2061,7 @@ export const tools: McpTool[] = [ .select('id, is_active') .single() if (error) throw new Error(`Failed to forget fact: ${error.message}`) - return data + return { ...data, fact_id: data.id } }, }, @@ -2180,7 +2183,8 @@ export const tools: McpTool[] = [ description: 'The single company every tool call in this session reads and writes. Confirm this is the entity the user means BEFORE any staged write — there is no per-call company switch; scope is fixed by the API key.', properties: { - id: { type: 'string', description: 'company_id this session is scoped to.' }, + id: { type: 'string', description: 'Deprecated — read company_id instead.' }, + company_id: { type: 'string', description: 'company_id this session is scoped to.' }, name: { type: ['string', 'null'] }, org_number: { type: ['string', 'null'] }, entity_type: { type: ['string', 'null'], description: 'e.g. "aktiebolag", "enskild_firma". Null if unset.' }, @@ -2190,7 +2194,7 @@ export const tools: McpTool[] = [ description: 'accrual = faktureringsmetoden: payment debits 19xx AND credits 1510 (both sides). cash = kontantmetoden: payment debits 19xx and books revenue + moms. Drives the settlement posting. Null defaults to accrual.', }, }, - required: ['id'], + required: ['id', 'company_id'], }, user_name: { type: ['string', 'null'], @@ -2208,12 +2212,13 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string', description: 'Atom id (e.g. "horizontal/swedish-vat", "vertical/konsult-it", "modifier/holding-ab"). Use as gnubok_load_skill slug.' }, + id: { type: 'string', description: 'Deprecated — read atom_id instead.' }, + atom_id: { type: 'string', description: 'Atom id (e.g. "horizontal/swedish-vat", "vertical/konsult-it", "modifier/holding-ab"). Use as gnubok_load_skill slug.' }, tier: { type: 'string', enum: ['horizontal', 'vertical', 'modifier'] }, title: { type: 'string' }, description: { type: 'string' }, }, - required: ['id', 'tier', 'title', 'description'], + required: ['id', 'atom_id', 'tier', 'title', 'description'], }, }, memory: { @@ -2223,12 +2228,13 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read fact_id instead.' }, + fact_id: { type: 'string', description: 'Pass to gnubok_forget_fact to deactivate.' }, kind: { type: 'string', enum: ['fact', 'preference', 'pattern', 'correction'] }, content: { type: 'string' }, relevance_score: { type: ['number', 'null'] }, }, - required: ['id', 'kind', 'content'], + required: ['id', 'fact_id', 'kind', 'content'], }, }, dimensions: { @@ -2373,6 +2379,7 @@ export const tools: McpTool[] = [ | null const company = { id: companyId, + company_id: companyId, name: companyRow?.name ?? null, org_number: companyRow?.org_number ?? null, entity_type: companyRow?.entity_type ?? null, @@ -2437,7 +2444,7 @@ export const tools: McpTool[] = [ ...(profile?.modifier_atoms ?? []), ] - let atoms: Array<{ id: string; tier: string; title: string; description: string }> = [] + let atoms: Array<{ id: string; atom_id: string; tier: string; title: string; description: string }> = [] if (atomIds.length > 0) { const { data: atomRows, error: atomErr } = await supabase .from('agent_atom_registry') @@ -2452,6 +2459,7 @@ export const tools: McpTool[] = [ description: string }>).map((r) => ({ id: r.id, + atom_id: r.id, tier: r.tier, title: r.title ?? r.id, // Trim the keyword-stuffed registry description to a clean one-liner — @@ -2467,6 +2475,7 @@ export const tools: McpTool[] = [ atoms, memory: memoryRows.map((m) => ({ id: m.id, + fact_id: m.id, kind: m.kind, content: m.content, relevance_score: m.relevance_score, @@ -2605,7 +2614,8 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read transaction_id instead' }, + transaction_id: { type: 'string' }, date: { type: 'string' }, description: { type: 'string' }, amount: { type: 'number' }, @@ -2647,15 +2657,16 @@ export const tools: McpTool[] = [ if (error) throw new Error(`Database error: ${error.message}`) + const rows = (data ?? []).map((t: { id: string }) => ({ ...t, transaction_id: t.id })) const total = totalCount ?? 0 - const hasMore = total > offset + (data?.length ?? 0) + const hasMore = total > offset + rows.length return { - transactions: data, - count: data?.length ?? 0, + transactions: rows, + count: rows.length, total_count: total, has_more: hasMore, - ...(hasMore ? { next_offset: offset + (data?.length ?? 0) } : {}), + ...(hasMore ? { next_offset: offset + rows.length } : {}), } }, }, @@ -4594,7 +4605,8 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read dimension_id instead' }, + dimension_id: { type: 'string' }, sie_dim_no: { type: 'number' }, name: { type: 'string' }, resets_annually: { type: 'boolean' }, @@ -4607,18 +4619,19 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read dimension_value_id instead' }, + dimension_value_id: { type: 'string' }, code: { type: 'string' }, name: { type: 'string' }, is_active: { type: 'boolean' }, start_date: { type: ['string', 'null'] }, end_date: { type: ['string', 'null'] }, }, - required: ['id', 'code', 'name', 'is_active', 'start_date', 'end_date'], + required: ['id', 'dimension_value_id', 'code', 'name', 'is_active', 'start_date', 'end_date'], }, }, }, - required: ['id', 'sie_dim_no', 'name', 'resets_annually', 'is_system', 'is_active', 'sort_order', 'values'], + required: ['id', 'dimension_id', 'sie_dim_no', 'name', 'resets_annually', 'is_system', 'is_active', 'sort_order', 'values'], }, }, }, @@ -4636,7 +4649,13 @@ export const tools: McpTool[] = [ async execute(_args, companyId, _userId, supabase) { await ensureCompanyDimensions(supabase, companyId) const dimensions = await fetchDimensionRegistry(supabase, companyId) - return { dimensions } + return { + dimensions: dimensions.map((d) => ({ + ...d, + dimension_id: d.id, + values: d.values.map((v) => ({ ...v, dimension_value_id: v.id })), + })), + } }, }, @@ -4663,13 +4682,14 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read dimension_id instead' }, + dimension_id: { type: 'string' }, sie_dim_no: { type: 'number' }, name: { type: 'string' }, resets_annually: { type: 'boolean' }, is_active: { type: 'boolean' }, }, - required: ['id', 'sie_dim_no', 'name', 'resets_annually', 'is_active'], + required: ['id', 'dimension_id', 'sie_dim_no', 'name', 'resets_annually', 'is_active'], }, values: { type: 'array', @@ -4677,7 +4697,8 @@ export const tools: McpTool[] = [ type: 'object', additionalProperties: false, properties: { - id: { type: 'string' }, + id: { type: 'string', description: 'Deprecated — read dimension_value_id instead' }, + dimension_value_id: { type: 'string' }, code: { type: 'string' }, name: { type: 'string' }, is_active: { type: 'boolean' }, @@ -4685,7 +4706,7 @@ export const tools: McpTool[] = [ end_date: { type: ['string', 'null'] }, confidence: { type: 'number', description: 'Fuzzy confidence 0–1; present only with query.' }, }, - required: ['id', 'code', 'name', 'is_active', 'start_date', 'end_date'], + required: ['id', 'dimension_value_id', 'code', 'name', 'is_active', 'start_date', 'end_date'], }, }, count: { type: 'number' }, @@ -4741,9 +4762,11 @@ export const tools: McpTool[] = [ end_date: string | null }> + const qualifiedDimension = { ...dimension, dimension_id: dimension.id } + if (!query) { - const values = all.slice(0, limit) - return { dimension, values, count: values.length } + const values = all.slice(0, limit).map((v) => ({ ...v, dimension_value_id: v.id })) + return { dimension: qualifiedDimension, values, count: values.length } } // Fuzzy ranking — same fuse.js setup as the resolve step so what this @@ -4754,9 +4777,10 @@ export const tools: McpTool[] = [ .slice(0, limit) .map((hit) => ({ ...hit.item, + dimension_value_id: hit.item.id, confidence: roundOre(1 - (hit.score ?? 1)), })) - return { dimension, values, count: values.length } + return { dimension: qualifiedDimension, values, count: values.length } }, },