From 250cc7c450b3232f2ceebabe4564592cc506dab2 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Fri, 3 Jul 2026 10:29:58 +0200 Subject: [PATCH] feat(mcp): always-explicit retryable on structured errors + transient inference (P1-1) (#875) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .claude/rules/mcp-server.md | 1 + .../mcp-server/__tests__/staging.test.ts | 12 +++- extensions/general/mcp-server/server.ts | 9 ++- .../__tests__/get-structured-error.test.ts | 47 +++++++++++++ lib/errors/get-structured-error.ts | 67 +++++++++++++++++-- lib/errors/structured-errors.ts | 9 +++ 6 files changed, 135 insertions(+), 10 deletions(-) diff --git a/.claude/rules/mcp-server.md b/.claude/rules/mcp-server.md index d3a016cb..cda091d4 100644 --- a/.claude/rules/mcp-server.md +++ b/.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. diff --git a/extensions/general/mcp-server/__tests__/staging.test.ts b/extensions/general/mcp-server/__tests__/staging.test.ts index 00eec82d..9295dd0e 100644 --- a/extensions/general/mcp-server/__tests__/staging.test.ts +++ b/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 } + expect(schema.properties.idempotency_key).toBeDefined() }) it('PERIOD_LOCKED carries a remediation pointing at gnubok_unlock_period', () => { diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 7f1a604a..c5f4dffa 100644 --- a/extensions/general/mcp-server/server.ts +++ b/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, + }, ) }, }, diff --git a/lib/errors/__tests__/get-structured-error.test.ts b/lib/errors/__tests__/get-structured-error.test.ts index debf4fb1..6310cb37 100644 --- a/lib/errors/__tests__/get-structured-error.test.ts +++ b/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) + }) +}) diff --git a/lib/errors/get-structured-error.ts b/lib/errors/get-structured-error.ts index 2659e8c9..3724caa9 100644 --- a/lib/errors/get-structured-error.ts +++ b/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 + const inner = typeof obj.error === 'object' && obj.error !== null ? (obj.error as Record) : 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, } } diff --git a/lib/errors/structured-errors.ts b/lib/errors/structured-errors.ts index e6521d7a..421f3567 100644 --- a/lib/errors/structured-errors.ts +++ b/lib/errors/structured-errors.ts @@ -47,6 +47,15 @@ const GENERIC: Record = { 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.',