fix(pending-ops): MCP approval of bulk-book works and failed approvals no longer consume the op (#1852)

* fix(pending-ops): MCP approval of bulk-book works and failed approvals no longer consume the op Feedback seq 261545 (deepCFO): approving a bulk_book_transactions op over MCP returned BULK_BOOK_UNAUTHORIZED, yet the op vanished from /pending with nothing booked; the user believed it had been approved. Two defects: 1. The bulk_book_transactions RPC gates on auth.uid(), which is NULL on the cookieless service client every MCP approval runs on, so EVERY API-key approval of a samlingsverifikat was refused. New migration 20260824170000 adds p_user_id, honored only for service_role callers (same gate as match_batch_allocate 20260817150000 and undo_sie_import); the executor passes the approving user, who is now also the actor stamped on the verifikat. pg-real test covers member/spoof/no-JWT/ grants like the precedent. 2. The dispatcher consumed the op on ANY executor error other than 404/ 409. An authorization refusal happens before any side-effect and says nothing about the op, so 401/403 now release the claim back to 'pending'. The executor maps RPC codes through the structured-error registry so 403/404/409 are distinguishable from 400. Every CommitResult carries operation_status (pending | committed | rejected | failed_partial), exposed on gnubok_approve_pending_operation, so agents stop inferring consumption from status 'failed'. Catalog token ceiling 59.95K -> 60K per the documented ratchet protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ScVhg6XsDtNXkiEQNV7LaZ * fix(pending-ops): revoke anon explicitly on the service-actor bulk_book signature Default privileges grant EXECUTE on new functions to anon; the pg-real grants test (mirroring match_batch_allocate) caught it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ScVhg6XsDtNXkiEQNV7LaZ --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-25 10:32:12 +02:00
parent e35714518f
commit dc92fb5c0c
7 changed files with 801 additions and 4 deletions
@@ -0,0 +1,121 @@
+/**
+ * Authorization refusals must not consume a pending operation.
+ *
+ * Feedback seq 261545: an API-key approve of a bulk_book_transactions op hit
+ * BULK_BOOK_UNAUTHORIZED (the RPC saw auth.uid() = NULL on the service
+ * client), and the dispatcher landed the op as 'rejected'. It vanished from
+ * the /pending queue with nothing booked, and the user believed it had been
+ * approved. A 401/403 happens before any side-effect and says nothing about
+ * the op's content, so the claim is released back to 'pending' and the
+ * result says so explicitly (operation_status) instead of leaving agents to
+ * infer consumption from status 'failed'.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { eventBus } from '@/lib/events/bus'
+import { createQueuedMockSupabase } from '@/tests/helpers'
+import type { PendingOperation } from '@/types'
+
+import { commitPendingOperation } from '../commit'
+
+function makeBulkBookOp(): PendingOperation {
+  return {
+    id: 'op-bulk-1',
+    user_id: 'user-1',
+    company_id: 'company-1',
+    operation_type: 'bulk_book_transactions',
+    status: 'pending',
+    title: 'Samlingsverifikation: 3 transaktioner 2026-07-22',
+    params: {
+      tx_ids: ['tx-1', 'tx-2', 'tx-3'],
+      existing_journal_entry_id: null,
+      new_entry: { description: 'Dagskassa', lines: [] },
+    },
+    preview_data: {},
+    result_data: null,
+    actor_type: 'api_key',
+    actor_id: 'key-1',
+    actor_label: 'deepCFO',
+    risk_level: 'medium',
+    created_at: '2026-08-24T00:00:00Z',
+    resolved_at: null,
+    updated_at: '2026-08-24T00:00:00Z',
+  } as PendingOperation
+}
+
+describe('commitPendingOperation: authorization refusal is recoverable', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    eventBus.clear()
+  })
+
+  it('releases the claim back to pending on BULK_BOOK_UNAUTHORIZED and reports operation_status', async () => {
+    const { supabase, enqueueMany, findCalls } = createQueuedMockSupabase()
+    enqueueMany([
+      { data: { id: 'op-bulk-1' }, error: null }, // atomic claim pending -> committing
+      { data: { ok: false, code: 'BULK_BOOK_UNAUTHORIZED' }, error: null }, // RPC refusal
+      { data: null, error: null }, // release claim back to pending
+    ])
+
+    const result = await commitPendingOperation(
+      supabase as never,
+      'user-1',
+      'company-1',
+      makeBulkBookOp(),
+    )
+
+    expect(result.status).toBe('failed')
+    expect(result.http_status).toBe(403)
+    expect(result.code).toBe('BULK_BOOK_UNAUTHORIZED')
+    expect(result.operation_status).toBe('pending')
+
+    const updates = findCalls('pending_operations', 'update')
+    expect(updates).toContainEqual([{ status: 'committing' }])
+    expect(updates).toContainEqual([{ status: 'pending' }])
+    expect(updates.some((args) => (args[0] as { status?: string }).status === 'rejected')).toBe(false)
+  })
+
+  it('passes the approving user as p_user_id so the service client is attributed', async () => {
+    const { supabase, enqueueMany } = createQueuedMockSupabase()
+    enqueueMany([
+      { data: { id: 'op-bulk-1' }, error: null },
+      { data: { ok: true, journal_entry_id: 'je-1', mode: 'create_new', linked_tx_count: 3 }, error: null },
+      { data: null, error: null }, // finalize committed
+    ])
+
+    const result = await commitPendingOperation(
+      supabase as never,
+      'user-1',
+      'company-1',
+      makeBulkBookOp(),
+    )
+
+    expect(result.status).toBe('committed')
+    expect(result.operation_status).toBe('committed')
+    expect(supabase.rpc).toHaveBeenCalledTimes(1)
+    expect(supabase.rpc).toHaveBeenCalledWith(
+      'bulk_book_transactions',
+      expect.objectContaining({ p_user_id: 'user-1', p_company_id: 'company-1' }),
+    )
+  })
+
+  it('still consumes the op as rejected on a genuine input error (400)', async () => {
+    const { supabase, enqueueMany, findCalls } = createQueuedMockSupabase()
+    enqueueMany([
+      { data: { id: 'op-bulk-1' }, error: null },
+      { data: { ok: false, code: 'BULK_BOOK_INVALID_PAYLOAD' }, error: null },
+      { data: null, error: null }, // rejected update
+    ])
+
+    const result = await commitPendingOperation(
+      supabase as never,
+      'user-1',
+      'company-1',
+      makeBulkBookOp(),
+    )
+
+    expect(result.status).toBe('failed')
+    expect(result.operation_status).toBe('rejected')
+    const updates = findCalls('pending_operations', 'update')
+    expect(updates.some((args) => (args[0] as { status?: string }).status === 'rejected')).toBe(true)
+  })
+})
@@ -208,6 +208,12 @@ export interface CommitResult {
  // callers that do not recognize the code fall back to `error`.
  code?: string
  account_numbers?: string[]
+  // Where the pending_operations row landed, independent of `status`:
+  // 'pending' means the op was NOT consumed and can be approved again
+  // (recoverable refusal: capability, chart accounts, Skatteverket, or an
+  // authorization failure that happened before any side-effect). Agents
+  // used to infer "consumed" from status 'failed' and were wrong both ways.
+  operation_status?: 'pending' | 'committed' | 'rejected' | 'failed_partial'
 }

 export interface CommitOptions {
@@ -5874,6 +5880,7 @@ async function commitMatchBatchAllocate(

 async function commitBulkBookTransactions(
  supabase: SupabaseClient,
+  userId: string,
  companyId: string,
  params: Record<string, unknown>
 ): Promise<ExecutorResult> {
@@ -5910,6 +5917,10 @@ async function commitBulkBookTransactions(
    p_existing_journal_entry_id: existingJeId,
    p_new_entry: newEntry,
    p_company_id: companyId,
+    // This path runs on the cookieless service client where auth.uid() is
+    // NULL; the RPC honors p_user_id only for service_role callers
+    // (migration 20260824170000), so the approving human is the actor.
+    p_user_id: userId,
  })
  if (error) {
    // Sanitised log (A.8.11, CC7.2): only error code + message.
@@ -5921,9 +5932,15 @@ async function commitBulkBookTransactions(
  }
  const result = data as { ok: boolean; code?: string; details?: unknown; journal_entry_id?: string; mode?: string; linked_tx_count?: number; docs_linked?: number }
  if (!result || !result.ok) {
+    const code = result?.code
+    const entry = code ? getErrorEntry(code) : undefined
    return {
-      error: result?.code || 'bulk_book_transactions failed',
-      status: 400,
+      error: code || 'bulk_book_transactions failed',
+      // Registry httpStatus so the dispatcher can tell an authorization
+      // refusal (403: nothing posted, op must stay pending) from bad input
+      // (400) or a vanished/already-booked tx (404/409: auto-reject).
+      status: entry?.httpStatus ?? 400,
+      ...(code ? { errorCode: code } : {}),
      data: result?.details as Record<string, unknown> | undefined,
    }
  }
@@ -6257,6 +6274,7 @@ async function commitPendingOperationInner(
      error: CAPABILITY_BLOCKED_MESSAGE_SV,
      http_status: 403,
      code: 'capability_blocked',
+      operation_status: 'pending',
    }
  }

@@ -6472,7 +6490,7 @@ async function commitPendingOperationInner(
        result = await commitMatchBatchAllocate(supabase, userId, companyId, pendingOp.params)
        break
      case 'bulk_book_transactions':
-        result = await commitBulkBookTransactions(supabase, companyId, pendingOp.params)
+        result = await commitBulkBookTransactions(supabase, userId, companyId, pendingOp.params)
        break
      case 'bulk_book_inbox_items':
        result = await commitBulkBookInboxItems(supabase, userId, companyId, pendingOp.params)
@@ -6528,6 +6546,7 @@ async function commitPendingOperationInner(
        http_status: 500,
        code: 'partial_commit',
        data: { posted_ids: err.postedIds },
+        operation_status: 'failed_partial',
      }
    }
    // Accounts-not-in-chart is RECOVERABLE: the booking itself is valid; the
@@ -6546,6 +6565,7 @@ async function commitPendingOperationInner(
        http_status: 400,
        code: ACCOUNTS_NOT_IN_CHART,
        account_numbers: err.accountNumbers,
+        operation_status: 'pending',
      }
    }
    // Recoverable Skatteverket failure (extension disabled, no connection,
@@ -6562,6 +6582,7 @@ async function commitPendingOperationInner(
        error: err.message,
        http_status: err.httpStatus,
        code: err.code,
+        operation_status: 'pending',
      }
    }
    const isBkErr = isBookkeepingError(err)
@@ -6581,6 +6602,7 @@ async function commitPendingOperationInner(
      status: 'failed',
      error: message,
      http_status: isBkErr ? 400 : 500,
+      operation_status: 'rejected',
    }
  }

@@ -6613,6 +6635,27 @@ async function commitPendingOperationInner(
        http_status: result.status ?? 500,
        code: 'partial_commit',
        data: { posted_ids: partialPostedIds },
+        operation_status: 'failed_partial',
+      }
+    }
+    // Authorization refusals (401/403) happen BEFORE any side-effect and say
+    // nothing about the op's content: the credential, not the booking, was
+    // wrong. Release the claim back to 'pending' so the op survives for a
+    // caller that IS authorized (the /pending UI, or a key with the scope),
+    // instead of vanishing as 'rejected'. Feedback seq 261545: three
+    // samlingsverifikat were consumed this way and the user believed they
+    // had been approved.
+    if (result.status === 401 || result.status === 403) {
+      await supabase
+        .from('pending_operations')
+        .update({ status: 'pending' })
+        .eq('id', pendingOp.id)
+      return {
+        status: 'failed',
+        error: result.error,
+        http_status: result.status,
+        ...(result.errorCode ? { code: result.errorCode } : {}),
+        operation_status: 'pending',
      }
    }
    const isAutoReject = result.status === 404 || result.status === 409
@@ -6662,6 +6705,7 @@ async function commitPendingOperationInner(
        http_status: result.status,
        ...(result.errorCode ? { code: result.errorCode } : {}),
        ...(failureDetails ? { data: failureDetails } : {}),
+        operation_status: 'rejected',
      }
    }
    return {
@@ -6670,6 +6714,7 @@ async function commitPendingOperationInner(
      http_status: result.status ?? 500,
      ...(result.errorCode ? { code: result.errorCode } : {}),
      ...(failureDetails ? { data: failureDetails } : {}),
+      operation_status: 'rejected',
    }
  }

@@ -6706,5 +6751,6 @@ async function commitPendingOperationInner(
  return {
    status: 'committed',
    data: result.data,
+    operation_status: 'committed',
  }
 }