feat(mcp): always-explicit retryable on structured errors + transient inference (P1-1) (#875)

Agents could not distinguish 'keep retrying' from 'stop, this is
broken' (agent.feedback): retryable was emitted only when a registry
entry declared true — absent otherwise, including for genuinely
transient DB/network failures whose SQLSTATE is lost when tools wrap
them as Error('Database error: ...').

- StructuredError.retryable is now a required boolean. Registry
  declaration wins; otherwise isTransientFailure() infers from Postgres
  SQLSTATEs (40001/40P01/57014/08xxx/53xxx/55P03), upstream HTTP
  statuses (408/429/5xx), and message signatures that survive wrapping
  (deadlock, serialization, statement timeout, fetch/socket failures).
- Unclassified transient failures surface as stable code
  TRANSIENT_ERROR (new registry entry, retryable: true).
- categorize_transaction accepts idempotency_key — the tool agents
  blind-retry after client-side approval-elicitation drops; the key
  makes that retry replay-safe instead of double-staging.
- Contract documented in .claude/rules/mcp-server.md. The planned
  'kind' field was dropped: the code registry already encodes it;
  retryable is the agent-actionable bit.

Every tool error already flows through the single dispatch point
(toToolError -> getStructuredError), so coverage is universal without
per-tool migration. Full unit suite: 6575 tests green.

Part of dev_docs/mcp_optimization_plan.md (P1-1).

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

.claude/rules/mcp-server.md

@@ -22,3 +22,4 @@ 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.
+- 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__/staging.test.ts

@@ -134,14 +134,20 @@ describe('toToolError: retryable propagation', () => {
     expect(result.error.retryable).toBe(true)
   })
 
-  it('omits retryable on permanent validation/period errors', () => {
+  it('marks permanent validation/period errors retryable:false — explicitly, never absent', () => {
     const periodLocked = toToolError(new Error('locked/closed fiscal period'))
     expect(periodLocked.error.code).toBe('PERIOD_LOCKED')
-    expect(periodLocked.error.retryable).toBeUndefined()
+    expect(periodLocked.error.retryable).toBe(false)
 
     const tagged = Object.assign(new Error('validation'), { code: 'VALIDATION_ERROR' })
     const validation = toToolError(tagged)
-    expect(validation.error.retryable).toBeUndefined()
+    expect(validation.error.retryable).toBe(false)
+  })
+
+  it('categorize_transaction accepts idempotency_key so blind retries are replay-safe', () => {
+    const tool = tools.find((t) => t.name === 'gnubok_categorize_transaction')!
+    const schema = tool.inputSchema as { properties: Record<string, unknown> }
+    expect(schema.properties.idempotency_key).toBeDefined()
   })
 
   it('PERIOD_LOCKED carries a remediation pointing at gnubok_unlock_period', () => {

extensions/general/mcp-server/server.ts

@@ -2837,6 +2837,7 @@ export const tools: McpTool[] = [
           description: 'Dims bag {sie_dim_no: kod eller namn}, e.g. {"1":"KS01","6":"P001"}. Tags the expense/business lines of the generated voucher — never the bank or VAT lines. Unknown values rejected, never auto-created.',
         },
         allow_duplicate: { type: 'boolean', description: 'Override the duplicate-booking guard (default false). Set true ONLY after the user confirms this bank line is a genuinely separate event — the guard blocks a second verifikat for an event already booked (e.g. a paid invoice or a salary payout).' },
+        idempotency_key: { type: 'string', description: 'Optional UUID to dedupe retries — a replayed call returns the already-staged operation instead of staging twice.' },
       },
       required: ['transaction_id', 'category'],
     },
@@ -2953,7 +2954,13 @@ export const tools: McpTool[] = [
           description: 'Once approved, the journal entry is posted. Continue with gnubok_list_uncategorized_transactions to keep clearing the backlog, or lock the period once it is empty.',
           tool: 'gnubok_list_uncategorized_transactions',
         },
-        tx?.date ? { dateForPeriodCheck: tx.date } : {},
+        {
+          ...(tx?.date ? { dateForPeriodCheck: tx.date } : {}),
+          // Categorize is the tool agents blind-retry after ambiguous
+          // client-side failures (approval elicitation drops) — the key makes
+          // that retry safe instead of double-staging.
+          idempotencyKey: typeof args.idempotency_key === 'string' ? args.idempotency_key : undefined,
+        },
       )
     },
   },

lib/errors/__tests__/get-structured-error.test.ts

@@ -84,3 +84,50 @@ describe('getStructuredError', () => {
     expect(result.message_sv.length).toBeGreaterThan(0)
   })
 })
+
+describe('retryable contract (always present, transient inference)', () => {
+  it('is explicitly false for a plain unclassified error', () => {
+    const result = getStructuredError(new Error('nope'))
+    expect(result.code).toBe('UNKNOWN_ERROR')
+    expect(result.retryable).toBe(false)
+  })
+
+  it('classifies a wrapped deadlock message as TRANSIENT_ERROR, retryable', () => {
+    // Tools wrap DB errors as plain strings, losing the SQLSTATE — the
+    // message pattern must survive that wrapping.
+    const result = getStructuredError(new Error('Database error: deadlock detected'))
+    expect(result.code).toBe('TRANSIENT_ERROR')
+    expect(result.retryable).toBe(true)
+  })
+
+  it('classifies a Postgres serialization-failure SQLSTATE as transient', () => {
+    const err = Object.assign(new Error('could not complete'), { code: '40001' })
+    const result = getStructuredError(err)
+    expect(result.code).toBe('TRANSIENT_ERROR')
+    expect(result.retryable).toBe(true)
+  })
+
+  it('classifies upstream 429/503 statuses as transient', () => {
+    expect(getStructuredError(Object.assign(new Error('slow down'), { status: 429 })).retryable).toBe(true)
+    expect(getStructuredError(Object.assign(new Error('bad gateway'), { statusCode: 502 })).retryable).toBe(true)
+  })
+
+  it('classifies fetch/socket failures as transient', () => {
+    expect(getStructuredError(new Error('fetch failed')).code).toBe('TRANSIENT_ERROR')
+    expect(getStructuredError(new Error('socket hang up')).retryable).toBe(true)
+    expect(getStructuredError(new Error('connect ECONNRESET 1.2.3.4:443')).retryable).toBe(true)
+  })
+
+  it('keeps a specific inferred code but still computes retryable from the failure', () => {
+    // NOT_FOUND is permanent even though nothing in the registry says so explicitly.
+    const result = getStructuredError(new Error('Transaction not found'))
+    expect(result.code).toBe('NOT_FOUND')
+    expect(result.retryable).toBe(false)
+  })
+
+  it('lets an explicit registry retryable:true win over inference', () => {
+    const tagged = Object.assign(new Error('over the cap'), { code: 'RATE_LIMITED' })
+    const result = getStructuredError(tagged)
+    expect(result.retryable).toBe(true)
+  })
+})

lib/errors/get-structured-error.ts

@@ -51,11 +51,60 @@ export interface StructuredError {
   message_en: string
   remediation?: StructuredErrorRemediation
   /**
-   * Present (true) only when the failure is transient. Agents may retry the
-   * same request after a short backoff. Absent or false means the request
-   * will fail the same way until inputs or system state change.
+   * ALWAYS present. `true` → transient failure: the identical request may
+   * succeed after a short backoff (pair with idempotency_key on staging
+   * tools). `false` → permanent for these inputs: retrying is wasted work
+   * until arguments or system state change. From the registry entry when the
+   * code declares it; otherwise inferred by isTransientFailure().
    */
-  retryable?: boolean
+  retryable: boolean
+}
+
+// Postgres SQLSTATEs that indicate a transient condition — the same statement
+// can succeed on retry without any input change.
+const TRANSIENT_SQLSTATES = new Set([
+  '40001', // serialization_failure
+  '40P01', // deadlock_detected
+  '57014', // query_canceled (statement timeout)
+  '57P03', // cannot_connect_now
+  '53300', // too_many_connections
+  '53400', // configuration_limit_exceeded
+  '55P03', // lock_not_available
+  '08000', // connection_exception
+  '08003', // connection_does_not_exist
+  '08006', // connection_failure
+])
+
+const TRANSIENT_HTTP_STATUSES = new Set([408, 429, 502, 503, 504, 522, 524])
+
+// Message-level signatures for transient failures. Tools commonly wrap DB
+// errors as plain `Error(\`Database error: ${message}\`)`, losing the
+// SQLSTATE — these patterns survive that wrapping.
+const TRANSIENT_MESSAGE_PATTERNS = [
+  /fetch failed/i,
+  /ECONNRESET|ECONNREFUSED|ETIMEDOUT|EPIPE|EAI_AGAIN/,
+  /socket hang up/i,
+  /deadlock detected/i,
+  /could not serialize access/i,
+  /canceling statement due to statement timeout/i,
+  /connection terminated/i,
+  /too many clients/i,
+  /rate limit/i,
+  /timed out/i,
+]
+
+function isTransientFailure(error: unknown, message: string): boolean {
+  if (typeof error === 'object' && error !== null) {
+    const obj = error as Record<string, unknown>
+    const inner = typeof obj.error === 'object' && obj.error !== null ? (obj.error as Record<string, unknown>) : undefined
+    for (const c of [obj.code, inner?.code]) {
+      if (typeof c === 'string' && TRANSIENT_SQLSTATES.has(c)) return true
+    }
+    for (const s of [obj.status, obj.statusCode]) {
+      if (typeof s === 'number' && TRANSIENT_HTTP_STATUSES.has(s)) return true
+    }
+  }
+  return TRANSIENT_MESSAGE_PATTERNS.some((re) => re.test(message))
 }
 
 interface StructuredErrorOptions {
@@ -138,7 +187,11 @@ export function getStructuredError(
   const message_en = extractEnglishMessage(error)
   const message_sv = getErrorMessage(error)
 
-  const code = extractCode(error) ?? inferCode(message_en) ?? 'UNKNOWN_ERROR'
+  const transient = isTransientFailure(error, message_en)
+  let code = extractCode(error) ?? inferCode(message_en) ?? 'UNKNOWN_ERROR'
+  // Nothing more specific matched but the failure is transient — surface the
+  // stable TRANSIENT_ERROR code so agents can dispatch on it.
+  if (code === 'UNKNOWN_ERROR' && transient) code = 'TRANSIENT_ERROR'
 
   const entry = getErrorEntry(code)
   let remediation = entry?.remediation
@@ -156,7 +209,9 @@ export function getStructuredError(
     message_sv,
     message_en,
     ...(remediation ? { remediation } : {}),
-    ...(entry?.retryable ? { retryable: true } : {}),
+    // Registry declaration wins; otherwise the transient inference decides.
+    // Always explicit — agents must never have to distinguish absent from false.
+    retryable: entry?.retryable ?? transient,
   }
 }
 

lib/errors/structured-errors.ts

@@ -47,6 +47,15 @@ const GENERIC: Record<string, StructuredErrorEntry> = {
     message_sv: 'Något gick fel. Försök igen.',
     message_en: 'An unexpected error occurred.',
   },
+  // Unclassified-but-transient failures (DB deadlock/timeout, connection
+  // drop, upstream 5xx/429) inferred by isTransientFailure() when no
+  // specific code applies. Stable code so agents can dispatch on it.
+  TRANSIENT_ERROR: {
+    httpStatus: 503,
+    message_sv: 'Tillfälligt fel — försök igen om en stund.',
+    message_en: 'Transient failure — retry the same request after a short backoff.',
+    retryable: true,
+  },
   INTERNAL_ERROR: {
     httpStatus: 500,
     message_sv: 'Ett oväntat serverfel uppstod. Försök igen senare.',