Repository navigation
fix(expenses): guard aggregate amount ranges - #1160
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptile review |
 [Critical impact] Changes expense amount validation and data model. The frontend must handle the new null amounts before this is safe to merge. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
…e bounds and 422 response
…tive expense amounts as null"}
|
@greptileai review |
…ount projection and format
b978f7c to
2ec1348
Compare
|
@greptileai review |
1 similar comment
|
@greptileai review |
Backend projects oversized legacy amounts as null; type the frontend fields as number | null and render common.notAvailable instead of formatting null as $0.00.
|
Greptile P1 addressed in d0b370e: |
1 participant
Intent
PR 8 - Bound expense aggregates safely (Fixes #1059)
Branch: codex/expense-aggregate-bounds
Title: fix(expenses): guard aggregate amount ranges
Preserve the public numeric minor-unit fields; do not invent a merchant monetary cap.
Requirements:
What Changed
better-sqlite3safeIntegers(true)(BigInt), validating that totals fit withinNumber.MAX_SAFE_INTEGERbefore converting to JavaScript numbers and returning422 amount_range_exceededon out-of-range totals or SQLite integer overflow.createExpenseandreplaceExpensetransactions (subtracting the replaced active amount before validating replacement totals) while preserving idempotent replay behavior.amount_minoranddue_minorasnullwhen a legacy row's stored amount exceedsNumber.MAX_SAFE_INTEGERso list, detail, void, and replace recovery operations remain usable without precision loss, and expand unit/API test coverage and API reference documentation.Risk Assessment
✅ Low: The change cleanly enforces BigInt-backed safe-integer aggregate bounds on expense totals, handles SQLite int64 overflow and legacy oversized rows with controlled 422 responses, and projects oversized individual row amounts as null so list, detail, payment reversal, void, and replacement recovery paths remain usable without precision loss.
Testing
Ran the targeted expense test suites (
npm run test:expenses, coveringexpense-attempt.test.ts,expense-records.test.ts,expense-api.test.ts, andexpense-payments.test.ts) and stood up an isolated live Express + SQLite server to drive all five boundary, adversarial overflow, replacement, and legacy recovery scenarios end-to-end over HTTP, recording the responses and persisted SQLite state in~/.no-mistakes/evidence/01M4YJM5EGVR5ANVQWZ26DAF3Q/expense-aggregate-bounds-live-verification.json; all checks and live scenarios passed. This is a backend service and HTTP API change with no UI/visual surface change, so HTTP response and database state artifacts were captured instead of screenshots.npm run test:expensesnpm run test:expensesnpm run test:expensesnpm run test:expensesnpm run test:expensesEvidence: Live HTTP & SQLite Expense Aggregate Bounds Verification Evidence
{ "scenario_1_boundary_and_replay": { "expBoundary": { "status": 201, "amount_minor": 9007199254690991 }, "expBoundaryReplay": { "status": 201, "idempotentReplay": "true" }, "summaryAtBoundary": { "status": 200, "totals": [{ "currency_code": "INR", "expense_count": 2, "incurred_minor": 9007199254740991, "net_paid_minor": 20000, "due_minor": 9007199254720991 }] } }, "scenario_2_adversarial_create_overflow": { "over1": { "status": 422, "body": { "error": "Expense incurred total exceeds the supported amount range", "code": "amount_range_exceeded" } }, "over100": { "status": 422, "body": { "error": "Expense incurred total exceeds the supported amount range", "code": "amount_range_exceeded" } }, "rowsCommitted": 0, "receiptsCommitted": 0 }, "scenario_3_replacement_bounds": { "replaceOver": { "status": 422, "body": { "error": "Expense incurred total exceeds the supported amount range", "code": "amount_range_exceeded" } }, "replaceExact": { "status": 201 }, "replaceExactReplay": { "status": 201, "idempotentReplay": "true" } }, "scenario_4_legacy_aggregate_and_recovery": { "summaryLegacyOver": { "status": 422, "body": { "error": "Expense incurred total exceeds the supported amount range", "code": "amount_range_exceeded" } }, "listWhileOverStatus": 200, "detailWhileOverStatus": 200, "voidLegacy100Status": 200, "summaryAfterVoid100": { "status": 200, "totals": [{ "currency_code": "INR", "incurred_minor": 9007199254740991, "net_paid_minor": 20000, "due_minor": 9007199254720991 }] } }, "scenario_5_single_int64_max_row_recovery": { "summaryInt64Overflow": { "status": 422, "body": { "error": "Expense incurred total exceeds the supported amount range", "code": "amount_range_exceeded" } }, "detailInt64Active": { "status": 200, "amount_minor": null, "paid_minor": 500, "due_minor": null }, "reverseInt64Pay": { "status": 201, "paid_minor": 0 }, "voidInt64": { "status": 200, "status_field": "voided", "amount_minor": null, "due_minor": null }, "summaryAfterInt64Void": { "status": 200, "totals": [{ "currency_code": "INR", "incurred_minor": 9007199254740991 }] }, "persistedSqliteRow": { "amount_minor": "9223372036854775807", "void_reason": "Direct void of INT64_MAX legacy row" } } }Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
main/services/expenses.ts:514-projectExpenseRowonly maps out-of-rangeamountRawanddueRawtonullwhenrow.voided_at !== null(allowOversizedInactive), and otherwise callsrequireSafeTotalwhich throws422 amount_range_exceeded. While an oversized legacy expense row (amount_minor > Number.MAX_SAFE_INTEGER) is still active (voided_at === null), every read and pre-void recovery path that projects the row fails with 422 instead of staying usable:listExpenses(main/services/expenses.ts:645) fails for the entire page whenever the active oversized row is in the page results;getExpense(main/services/expenses.ts:661) fails on the active row;reverseExpensePayment(main/services/expenses.ts:1347,main/services/expenses.ts:1390viarequireActiveExpenseatmain/services/expenses.ts:948) fails with 422 so any recorded payment on an oversized legacy row cannot be reversed to satisfyrequireUnpaidOpenExpense(main/services/expenses.ts:984) before voiding or replacing; andrecordExpensePayment(main/services/expenses.ts:1264) throws 422 ingetExpensebeforerunExpenseMutation, blocking idempotent replay and making theexpense.due_minor === nullcheck atmain/services/expenses.ts:1281unreachable. Map out-of-rangeamountRawanddueRawtonullinprojectExpenseRowregardless ofrow.voided_at(new and replaced rows are already constrained to safe integers on write, andsummarizeExpensesstill enforcesrequireSafeTotalon active aggregates).🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
npm run test:expensesnpm run test:expensesnpm run test:expensesnpm run test:expensesnpm run test:expensesnpm run test:expensesnode tests/run-electron-node-test.cjs tests/expense-records.test.tsnode tests/run-electron-node-test.cjs tests/expense-api.test.tsLive HTTP scenario verification against an isolated Express + SQLite server instance recording~/.no-mistakes/evidence/01M4YJM5EGVR5ANVQWZ26DAF3Q/expense-aggregate-bounds-live-verification.json✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.