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 <noreply@anthropic.com>

.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.

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()
   })

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<string, unknown>
+  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([])
+  })
+})

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 }
     },
   },