From 28df5d851e24623c3f6a1a8fe78c721506fb1736 Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Tue, 21 Apr 2026 17:36:09 +0200 Subject: [PATCH] fix: cancel orphan draft when commitEntry fails + add compliance review CI (#302) createJournalEntry now cancels the draft with a CAS guard (status='draft') if commitEntry throws, so callers don't leave undeletable stuck drafts when the commit RPC rejects (balance trigger, period lock, overload ambiguity). Also adds a PR-triggered GitHub Actions workflow that runs Claude against the diff using the swedish-* skills as authoritative references and posts advisory compliance feedback as a PR comment. Co-authored-by: Claude Opus 4.7 (1M context) --- .../workflows/swedish-compliance-review.yml | 44 ++++ .../__tests__/voucher-atomicity.test.ts | 194 +++++++++++++++++- lib/bookkeeping/engine.ts | 22 +- scripts/swedish-compliance-review.mjs | 184 +++++++++++++++++ 4 files changed, 442 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/swedish-compliance-review.yml create mode 100644 scripts/swedish-compliance-review.mjs diff --git a/.github/workflows/swedish-compliance-review.yml b/.github/workflows/swedish-compliance-review.yml new file mode 100644 index 00000000..c1fd4a77 --- /dev/null +++ b/.github/workflows/swedish-compliance-review.yml @@ -0,0 +1,44 @@ +name: Swedish Accounting Compliance Review + +on: + pull_request: + types: [opened, synchronize, reopened] + +permissions: + contents: read + pull-requests: write + +jobs: + review: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - uses: actions/setup-node@v4 + with: + node-version: 20 + - name: Install Anthropic Bedrock SDK + run: npm install --no-save --no-package-lock @anthropic-ai/bedrock-sdk@latest + - name: Run compliance review + env: + AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} + AWS_REGION: ${{ secrets.AWS_REGION || 'eu-north-1' }} + GITHUB_BASE_REF: ${{ github.base_ref }} + REVIEW_MODEL: eu.anthropic.claude-sonnet-4-6 + run: node scripts/swedish-compliance-review.mjs + - name: Find previous compliance comment + uses: peter-evans/find-comment@v3 + id: find-comment + with: + issue-number: ${{ github.event.pull_request.number }} + comment-author: 'github-actions[bot]' + body-includes: '' + - name: Post or update PR comment + uses: peter-evans/create-or-update-comment@v4 + with: + issue-number: ${{ github.event.pull_request.number }} + comment-id: ${{ steps.find-comment.outputs.comment-id }} + body-path: review.md + edit-mode: replace diff --git a/lib/bookkeeping/__tests__/voucher-atomicity.test.ts b/lib/bookkeeping/__tests__/voucher-atomicity.test.ts index 5a31aa70..7a52df88 100644 --- a/lib/bookkeeping/__tests__/voucher-atomicity.test.ts +++ b/lib/bookkeeping/__tests__/voucher-atomicity.test.ts @@ -6,7 +6,7 @@ vi.mock('@/lib/events', () => ({ eventBus: { emit: vi.fn().mockResolvedValue([]) }, })) -import { commitEntry, getNextVoucherNumber } from '../engine' +import { commitEntry, getNextVoucherNumber, createJournalEntry } from '../engine' describe('voucher number atomicity', () => { beforeEach(() => { @@ -133,3 +133,195 @@ describe('voucher number atomicity', () => { }) }) }) + +describe('createJournalEntry orphan draft cleanup', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + /** + * Regression test for #292: when commit_journal_entry RPC fails (e.g. overload + * ambiguity, balance trigger, period lock), the draft created by createDraftEntry + * must be cancelled so it doesn't linger as an undeletable stuck draft. + */ + it('cancels the draft when commit RPC fails', async () => { + const draftId = 'entry-1' + const cancelUpdate = vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockResolvedValue({ error: null }), + }), + }) + + const supabase = { + from: vi.fn().mockImplementation((table: string) => { + if (table === 'fiscal_periods') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { name: 'FY 2025', period_start: '2025-01-01', period_end: '2025-12-31' }, + error: null, + }), + }), + }), + }), + } + } + if (table === 'chart_of_accounts') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + in: vi.fn().mockResolvedValue({ + data: [ + { account_number: '1930', id: 'acc-1930' }, + { account_number: '1510', id: 'acc-1510' }, + ], + error: null, + }), + }), + }), + } + } + if (table === 'journal_entries') { + return { + insert: vi.fn().mockReturnValue({ + select: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { id: draftId, status: 'draft' as JournalEntryStatus }, + error: null, + }), + }), + }), + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { id: draftId, status: 'draft', lines: [] }, + error: null, + }), + }), + }), + update: cancelUpdate, + } + } + if (table === 'journal_entry_lines') { + return { + insert: vi.fn().mockResolvedValue({ error: null }), + } + } + return {} + }), + // commit_journal_entry RPC fails — simulates overload ambiguity or balance error + rpc: vi.fn().mockResolvedValue({ + data: null, + error: { message: 'Could not choose the best candidate function' }, + }), + } + + await expect( + createJournalEntry(supabase as never, 'co-1', 'user-1', { + fiscal_period_id: 'fp-1', + entry_date: '2025-06-15', + description: 'Payment', + source_type: 'invoice_paid', + lines: [ + { account_number: '1930', debit_amount: 1000, credit_amount: 0 }, + { account_number: '1510', debit_amount: 0, credit_amount: 1000 }, + ], + }) + ).rejects.toThrow('Failed to commit journal entry') + + // The orphan draft must have been cancelled with CAS guard (status='draft') + expect(cancelUpdate).toHaveBeenCalledWith({ status: 'cancelled' }) + const firstEq = cancelUpdate.mock.results[0].value.eq + expect(firstEq).toHaveBeenCalledWith('id', draftId) + const secondEq = firstEq.mock.results[0].value.eq + expect(secondEq).toHaveBeenCalledWith('status', 'draft') + }) + + it('surfaces original commit error even if cleanup update fails', async () => { + const draftId = 'entry-1' + + const supabase = { + from: vi.fn().mockImplementation((table: string) => { + if (table === 'fiscal_periods') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { name: 'FY 2025', period_start: '2025-01-01', period_end: '2025-12-31' }, + error: null, + }), + }), + }), + }), + } + } + if (table === 'chart_of_accounts') { + return { + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + in: vi.fn().mockResolvedValue({ + data: [ + { account_number: '1930', id: 'acc-1930' }, + { account_number: '1510', id: 'acc-1510' }, + ], + error: null, + }), + }), + }), + } + } + if (table === 'journal_entries') { + return { + insert: vi.fn().mockReturnValue({ + select: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { id: draftId, status: 'draft' as JournalEntryStatus }, + error: null, + }), + }), + }), + select: vi.fn().mockReturnValue({ + eq: vi.fn().mockReturnValue({ + single: vi.fn().mockResolvedValue({ + data: { id: draftId, status: 'draft', lines: [] }, + error: null, + }), + }), + }), + // Cleanup throws — original error should still propagate + update: vi.fn().mockImplementation(() => { + throw new Error('Network error during rollback') + }), + } + } + if (table === 'journal_entry_lines') { + return { + insert: vi.fn().mockResolvedValue({ error: null }), + } + } + return {} + }), + rpc: vi.fn().mockResolvedValue({ + data: null, + error: { message: 'Period is locked' }, + }), + } + + await expect( + createJournalEntry(supabase as never, 'co-1', 'user-1', { + fiscal_period_id: 'fp-1', + entry_date: '2025-06-15', + description: 'Payment', + source_type: 'invoice_paid', + lines: [ + { account_number: '1930', debit_amount: 1000, credit_amount: 0 }, + { account_number: '1510', debit_amount: 0, credit_amount: 1000 }, + ], + }) + // Original commit error surfaces, not the cleanup error + ).rejects.toThrow('Period is locked') + }) +}) diff --git a/lib/bookkeeping/engine.ts b/lib/bookkeeping/engine.ts index 78768901..b0886135 100644 --- a/lib/bookkeeping/engine.ts +++ b/lib/bookkeeping/engine.ts @@ -285,6 +285,10 @@ export async function commitEntry( * Convenience wrapper: creates draft + commits in one step. * The voucher number is only assigned after lines are successfully inserted, * preventing gaps in the voucher sequence (BFL 5 kap. 7§). + * + * If commitEntry fails (e.g. balance trigger rejection, period lock, RPC error), + * the orphan draft is cancelled so callers don't leave an undeletable stuck draft. + * The commit RPC is atomic — no voucher number is burned on failure. */ export async function createJournalEntry( supabase: SupabaseClient, @@ -295,7 +299,23 @@ export async function createJournalEntry( rubricVersion?: string ): Promise { const draft = await createDraftEntry(supabase, companyId, userId, input) - return commitEntry(supabase, companyId, userId, draft.id, commitMethod, rubricVersion) + try { + return await commitEntry(supabase, companyId, userId, draft.id, commitMethod, rubricVersion) + } catch (commitError) { + // CAS guard: only cancel if still in draft. If the RPC actually posted + // before failing downstream, immutability trigger blocks draft→cancelled + // on a posted row anyway — the filter just avoids firing the trigger. + try { + await supabase + .from('journal_entries') + .update({ status: 'cancelled' }) + .eq('id', draft.id) + .eq('status', 'draft') + } catch { + // Swallow rollback failure — surface the original commit error + } + throw commitError + } } /** diff --git a/scripts/swedish-compliance-review.mjs b/scripts/swedish-compliance-review.mjs new file mode 100644 index 00000000..1c7dc945 --- /dev/null +++ b/scripts/swedish-compliance-review.mjs @@ -0,0 +1,184 @@ +#!/usr/bin/env node +// Runs Claude against the PR diff using all swedish-* skills under +// .claude/skills/ as the authoritative reference. Writes advisory +// feedback to review.md for the workflow to post as a PR comment. + +import AnthropicBedrock from '@anthropic-ai/bedrock-sdk'; +import { readFileSync, readdirSync, writeFileSync } from 'node:fs'; +import { execSync } from 'node:child_process'; +import path from 'node:path'; + +const SKILLS_DIR = '.claude/skills'; +const ALWAYS_LOAD = 'swedish-accounting-compliance'; +const MODEL = process.env.REVIEW_MODEL || 'eu.anthropic.claude-sonnet-4-6'; +const MAX_DIFF_CHARS = 180_000; +const OUTPUT_FILE = 'review.md'; +const COMMENT_MARKER = ''; + +function loadSkills() { + const ids = readdirSync(SKILLS_DIR).filter((n) => n.startsWith('swedish-')); + if (!ids.includes(ALWAYS_LOAD)) { + throw new Error(`Required skill missing: ${ALWAYS_LOAD}`); + } + const primary = readFileSync(path.join(SKILLS_DIR, ALWAYS_LOAD, 'SKILL.md'), 'utf8'); + const others = ids + .filter((id) => id !== ALWAYS_LOAD) + .map((id) => ({ + id, + content: readFileSync(path.join(SKILLS_DIR, id, 'SKILL.md'), 'utf8'), + })); + return { primary: { id: ALWAYS_LOAD, content: primary }, others }; +} + +function getDiff() { + const baseRef = process.env.GITHUB_BASE_REF || 'main'; + execSync(`git fetch origin ${baseRef} --depth=1`, { stdio: 'ignore' }); + const mergeBase = execSync(`git merge-base origin/${baseRef} HEAD`).toString().trim(); + const files = execSync(`git diff --name-only ${mergeBase} HEAD`).toString().trim(); + let diff = execSync(`git diff ${mergeBase} HEAD`).toString(); + let truncated = false; + if (diff.length > MAX_DIFF_CHARS) { + diff = diff.slice(0, MAX_DIFF_CHARS); + truncated = true; + } + return { files, diff, truncated }; +} + +function buildSystemPrompt({ primary, others }) { + const otherBlocks = others + .map((s) => `### Skill: ${s.id}\n\n${s.content}`) + .join('\n\n---\n\n'); + + return `You are reviewing a pull request in **gnubok**, a Swedish accounting SaaS built in Next.js + TypeScript on top of Supabase. Your job is to flag compliance risks against Swedish accounting law (Bokföringslagen / BFL), BFNAR, tax law, BAS 2026 chart, and VAT rules (ML 2023:200). + +You have been given a corpus of compliance skills below. Use them as your authoritative source — prefer them over your training data whenever they conflict. + +## Primary skill (ALWAYS consult) + +### Skill: ${primary.id} + +${primary.content} + +--- + +## Topic-specific skills (consult when relevant) + +${otherBlocks} + +--- + +## Your task + +1. **Route**: identify which topic-specific skills are relevant to the diff (in addition to the primary skill). State them upfront. +2. **Review**: produce advisory findings for compliance risks only. Examples of in-scope findings: + - BAS account misuse (wrong account number for the purpose, wrong VAT account) + - VAT errors (wrong rate, missing reverse charge marker, incorrect ruta mapping, representation moms > 300 SEK deduction) + - Accounting guard-rail violations (editing posted entries, direct inserts into journal tables, non-storno corrections) + - Retention / WORM violations (deleting documents linked to posted entries, mutable audit rows) + - Period-lock bypasses + - SIE/SRU encoding or field errors + - Year-end / tax calculation errors (periodiseringsfond, överavskrivningar, bolagsskatt, egenavgifter) + - Payroll errors (arbetsgivaravgifter rate, skatteavdrag, förmånsbeskattning, semesterlöneskuld) + - Invoice field requirements (ML 17 kap 24§), kreditfaktura handling, Peppol/e-faktura +3. **Cite**: for each finding, cite the specific skill and section that supports it. +4. **Be concise**: use short bullets. No restating the diff. No style/formatting/naming comments. No praise. +5. **Allow the empty case**: if nothing in the diff touches compliance (pure UI tweak, refactor of non-accounting code, docs, tests), say so in one line and stop. +6. **Never fabricate a rule**: if uncertain, mark as "unsure" rather than asserting. + +## Output format + +Start with the marker literal ${COMMENT_MARKER} on its own line. + +Then: + +\`\`\` +## Swedish Accounting Compliance Review + +**Skills consulted**: + +### Findings + +- **[SKILL_ID] ** — <1–3 sentence explanation with file:line references and the fix>. + +(or: "No compliance concerns in this diff — changes are outside the scope of the Swedish accounting skills.") + +### Notes (optional) + + +\`\`\` + +Render no emojis. Do not wrap the final output in a code fence.`; +} + +function buildUserMessage({ files, diff, truncated }) { + const note = truncated + ? `\n\n> Note: diff exceeded ${MAX_DIFF_CHARS} chars and was truncated. Review is based on the first ${MAX_DIFF_CHARS} chars only.` + : ''; + return `## Changed files + +\`\`\` +${files} +\`\`\` + +## Diff + +\`\`\`diff +${diff} +\`\`\`${note}`; +} + +async function main() { + if (!process.env.AWS_ACCESS_KEY_ID || !process.env.AWS_SECRET_ACCESS_KEY) { + writeFileSync( + OUTPUT_FILE, + `${COMMENT_MARKER}\n\n## Swedish Accounting Compliance Review\n\nSkipped: AWS Bedrock credentials (\`AWS_ACCESS_KEY_ID\` / \`AWS_SECRET_ACCESS_KEY\`) are not set.\n`, + ); + console.warn('AWS credentials missing — wrote skip notice and exiting 0.'); + return; + } + + const skills = loadSkills(); + const { files, diff, truncated } = getDiff(); + + if (!diff.trim()) { + writeFileSync( + OUTPUT_FILE, + `${COMMENT_MARKER}\n\n## Swedish Accounting Compliance Review\n\nNo diff detected against the base branch.\n`, + ); + return; + } + + const client = new AnthropicBedrock({ + awsRegion: process.env.AWS_REGION || 'eu-north-1', + awsAccessKey: process.env.AWS_ACCESS_KEY_ID, + awsSecretKey: process.env.AWS_SECRET_ACCESS_KEY, + }); + const system = buildSystemPrompt(skills); + const user = buildUserMessage({ files, diff, truncated }); + + const resp = await client.messages.create({ + model: MODEL, + max_tokens: 4096, + system, + messages: [{ role: 'user', content: user }], + }); + + const text = resp.content + .filter((b) => b.type === 'text') + .map((b) => b.text) + .join('\n') + .trim(); + + const body = text.startsWith(COMMENT_MARKER) ? text : `${COMMENT_MARKER}\n\n${text}`; + writeFileSync(OUTPUT_FILE, body + '\n'); + console.log(`Wrote ${OUTPUT_FILE} (${body.length} chars, model=${MODEL}).`); +} + +main().catch((err) => { + console.error('Compliance review failed:', err); + writeFileSync( + OUTPUT_FILE, + `${COMMENT_MARKER}\n\n## Swedish Accounting Compliance Review\n\nReview failed: \`${String(err.message || err)}\`. This is advisory only — the PR is not blocked.\n`, + ); + process.exit(0); +});