Make invoice posting, cancellation and period locks atomic - #7
Merged
Conversation
A posted journal entry could be deleted outright. Cancelling an invoice removed its entry, deleting an invoice removed both, and re-posting wiped what was there first. Books you can rewrite are books an accountant has to redo, so none of the export work on top of them is worth much. A correction is now a second, mirror-image entry. The original and its Storno both stay `posted` and net to zero, so the trial balance is right without either leaving the record; the pair is linked by reverses_entry_id / reversed_by_entry_id. There is deliberately no 'reversed' status, because dropping the original out of the three `status = 'posted'` queries while adding a posted Storno subtracts the same amount twice. The document behind the entry is frozen too. The editor already hid its controls on a non-draft invoice, but the API did not, and the API is what an agent calls: an issued invoice could be silently rewritten while its journal entry stood unchanged. addLine, updateLine, deleteLine and updateInvoice now refuse on anything but a draft, and a posted invoice is cancelled rather than deleted. Months can be locked closed. Nothing posts into a locked period -- which is what stops a back-dated draft being issued into a month already reported -- and a reversal of an entry inside one is dated into the open period instead, so a closed month keeps the numbers it was reported with. Locking is one-way; an unlock would make every lock a suggestion. Every change to the books is recorded with who made it, taken from the platform's own caller identity, so the log separates a person from the org's agent acting on its own. Refusals come back as 409 with a message saying what to do instead, and the UI surfaces it rather than failing silently. Verified against an in-memory SQLite build of the real schema: 35 checks covering the reversal arithmetic, the frozen-document guards, period locking, reversal out of a locked period, and that a refused issue leaves the invoice a draft with its number sequence untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent re-review of this branch before merge surfaced real bugs the first pass's sqlite harness didn't exercise: - setStatus had no restriction on the target status, so POST /api/invoices/:id/status could set a posted invoice back to 'draft', reopening every edit path assertEditable exists to close. Restrict it to the three legitimate transitions (sent/paid/cancelled) and refuse on a still-draft invoice. - reverseEntry set the original's reversed_by_entry_id before flipping the Storno to 'posted'. A crash between those two writes left the Storno stuck pending (invisible to every report) while the original looked already reversed and unretryable -- a silent, permanent loss of the correction. Flip the Storno to posted first, and make the whole operation idempotent (reuse an already-posted Storno on retry) so a crash there can no longer double-post either. - createFromInvoice reversed the superseded entry before checking the new one would balance, have any lines, or land in an open period. A refusal there left the original reversed with nothing posted to replace it. Move the reversal after every check that can still refuse the call. - lockPeriod accepted the current or a future month. There is no unlock route, so locking a month before it's over would have permanently refused every invoice in it. Also: listAudit's limit clamp let a non-numeric ?limit= reach the query as `LIMIT NaN` and 500. Verified via the node:sqlite domain harness from the original commit (memory: template-sqlite-harness) -- all 35 existing checks still pass, plus 13 new checks targeting these 5 bugs specifically, each confirmed to fail against the pre-fix code first. tsc --noEmit and vite build both clean (same 15 pre-existing tsc errors as baseline, all in client code unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
An independent second-opinion review (a separate agent, asked to specifically try to break this before it merges) found 4 real correctness bugs the original sqlite-harness verification didn't reach, plus one crash-on-bad-input:
Fixed in 66640a4, verified against all 4 by extending the same node:sqlite domain harness from the original commit: 13 new checks, each first confirmed to fail against the pre-fix code, all passing after. The existing 35 checks, Not merging this — that's still yours to do ( |
pallaoro
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issuing, cancelling, or retrying an invoice could leave the document, journal and audit log out of sync. Repeating cancellation could restore revenue; simultaneous requests could post duplicate reversals; a failed audit write could leave a financial change committed without its record.
Invoice issuing, cancellation, status changes, draft deletion and period locking now commit their dependent writes in one SQLite statement through triggers. Issuing allocates the number and posts balanced lines together; failure leaves the invoice draft and preserves the sequence. Cancellation retains the original and one mirror entry, with actor-attributed audit records, and remains safe to retry. A correction to a locked month lands in the current open month.
Database guards validate dates, enforce closed periods, and freeze issued documents even when an edit started while they were drafts. Ordinary posting/backfill is idempotent; quotes allocate numbers without posting financial entries. Ledger refusals return 409 JSON, while existing HTTPException responses and unexpected-error behavior are preserved.
The schema changes are additive and verified against populated main data. Unique indexes refuse duplicate historical postings instead of silently repairing accounting data. New triggers are supported by both platform storage paths; future changes to installed trigger definitions require explicit migrations because the platform reconciler compares trigger names.
Validation: 56 committed regression tests, including an independently written public-HTTP suite; independent concurrency/fault tests on local Cloudflare D1 and SQLite Durable Objects; actual exported Worker smoke test; populated main-to-final schema reconciliation and repeat-apply convergence. Vite build passes. Typecheck has the same nine pre-existing client errors, with no new errors.
Scope is ledger integrity; country-specific filing and export requirements remain separate. Validation used local Cloudflare runtimes, without migrating production customer databases.