Repository navigation
Splitting $10.00 three ways loses a cent #52
Description
What happened
Add an expense of 1000 cents ($10.00) split among 3 people. The three
shares are 333, 333, 333 cents - they sum to 999, not 1000. A cent
disappears.
Expected
Shares should always sum to exactly the original amount. One (or more)
member should get the leftover cent(s), e.g. 334, 333, 333.
Steps to reproduce
bun run .claude/skills/handling-money/scripts/check-cents.ts
The split(1000, 3 members) case prints FAIL ... sum 999 (expected 1000).
Or via the API, with any Goa trip token:
curl -s -X POST http://localhost:3200/api/groups/goa-trip/expenses \
-H "Authorization: Bearer goa-asha" -H "Content-Type: application/json" \
-d '{"payerId":"goa-asha-id","amountCents":1000,"description":"test","splitAmong":["goa-asha-id","goa-ben-id","goa-chetan-id"]}'
Then check the expense's shares sum to 1000 - they sum to 999.
Area
money
Severity
medium
Notes for whoever picks this up
See .claude/skills/handling-money - the remainder rule is documented in
references/rules.md. The fix is in src/money/cents.ts's splitCents.
Activity
Param-Harrison commentedon Sep 24, 2026
Triage
Disposition: proceed
Type: bug
Risk: high
Confidence: 0.9
Done when:
splitCents(1000, [a, b, c])returns shares that sum to exactly 1000 (e.g. 334/333/333, remainder to the first members per.claude/skills/handling-money/references/rules.md), proven by a newbun:testintests/money.test.tsthat fails on the current code, withmake checkgreen.Files expected:
src/money/cents.ts,tests/money.test.tsGate level:
make checkRisk is high because the change touches money (the split that every expense's shares come from), even though it is confined to one non-protected module. No protected path is involved: the fix goes in
src/money/cents.ts, and the.claude/skills/handling-moneyskill is only read, not edited.Param-Harrison commentedon Sep 24, 2026
Plan (rev 1)
Goal: Make
splitCentsgive the leftover cents to the first members, so shares always add up to the total (1000 split 3 ways → 334/333/333).Acceptance criteria
splitCents(1000, ["a","b","c"])returns{a:334, b:333, c:333}, and the test fails on the current code. Checked bybun test tests/money.test.ts -t "gives the remainder cents to the first members"101across 5 and1across 3), the shares add up to exactlytotalCents. Each share isfloor(total/n)orfloor(total/n)+1, and the+1shares go to the firstremaindermembers in the order they're passed in. Checked bybun test tests/money.test.ts -t "shares always sum to the total"splitCentstests (even splits,900/3 and2000/2) and the zero-members throw still pass without changes. Checked bybun test tests/money.test.tsok. Checked bybun run .claude/skills/handling-money/scripts/check-cents.tsmake checkNon-goals (binding — a diff that crosses one fails verify)
src/money/cents.tsandtests/money.test.ts. That rules outsrc/expenses.ts,src/seed.ts, other tests, and anything under.claude/**,.factory/**,src/auth/**or.github/**.splitCents's signature or return type, and no change toformatCents,parseCentsorsumCents.Files to touch
src/money/cents.ts: fixsplitCents(lines 39–49) and replace the "SEEDED DEFECT" doc comment (lines 29–38) with a description of the remainder ruletests/money.test.ts: add the remainder tests to the existingsplitCentsdescribe blockTests to write first
tests/money.test.ts›splitCents›gives the remainder cents to the first memberstests/money.test.ts›splitCents›shares always sum to the totalRepo skills to apply
handling-money (
references/rules.mdremainder rule:base = floor(total/n), the firsttotal - base*nmembers getbase + 1)Risk: low
Charter tier low: "a change confined to a single non-protected module with no schema or API-shape change". Only
src/money/changes. The function's signature, the DB schema and the route shapes all stay the same, and no protected path is touched. The only caller,src/expenses.ts:28, inserts whatever sharessplitCentsreturns and doesn't depend on their order. No existing test asserts the floored output:tests/routes.test.tsuses fixtures that divide evenly.Triage suggested high because this is money. The charter's high tier covers only dependency majors,
src/auth/**, or plans with low confidence, and none of those apply here. Still, every expense's shares come from this function, so I've marked it not eligible for auto-approve. A human should look at it even though the tier is low.Gate level:
make checkThis needs a human: reply
/factory approveto proceed, or/factory revise <what to change>.Param-Harrison commentedon Sep 24, 2026
/factory approve
Param-Harrison commentedon Sep 24, 2026
Status
Stage: build — finished
Progress: 3/5
Elapsed: ~3m
Gate line:
FACTORY_GATES: status=GREEN passed=3 failed=0 skipped=1 failed_gates=auditRound 2 fixes the blocking NG-5 finding from the last verdict. The remainder rule now applies only to totals that are whole numbers of zero or more. Negative and non-integer totals still get the old floored shares (-1000 split 3 ways → -334 each, 10.5 split 2 ways → 5 each). A new test,
leaves negative and non-integer totals floored as before, locks that in. It failed on the round-1 code and passes now.bun testpasses (29 pass, 0 fail), andcheck-cents.tsreports every caseok. The optionalauditgate still fails on hono advisories that were already there, which is out of scope. The two non-blocking findings (duplicate member ids, the doc-comment wording) predate this change and are left for a follow-up.factory: build #52
Param-Harrison commentedon Sep 24, 2026
/factory retry
factory: build #52
Param-Harrison commentedon Sep 24, 2026
Verdict: pass
Acceptance criteria evidence
bun test tests/money.test.ts -t "gives the remainder cents to the first members"→1 pass, 0 fail. With the pre-fixsrc/money/cents.tsit fails:Expected: {a:334,...} Received: {a:333,...}.bun test tests/money.test.ts -t "shares always sum to the total"→1 pass, 0 fail, 31 expect() calls. Covers 1000/3, 101/5, 1/3, 2/3, 999/7 and 1003/4. With the pre-fix code it fails:Expected: 1000, Received: 999.bun test tests/money.test.ts→9 pass, 0 fail, 45 expect() calls.git diff --numstat 934ab00 HEAD -- tests/money.test.tsshows 30 insertions and 0 deletions, so the existing tests are unchanged.bun run .claude/skills/handling-money/scripts/check-cents.ts→ all four cases (900/3,2000/2,1000/3,101/5) print[ok ], then "all splits sum to their total."make check→tsc --noEmitclean,29 pass, 0 fail, 81 expect() callsacross 4 files, and all 9 skills valid.The test that bites
splitCents › gives the remainder cents to the first membersfails on the pre-build code (Expected: {a:334,...} Received: {a:333,...}) and passes here(
bun test tests/money.test.ts -t "gives the remainder cents to the first members"→1 pass, 0 fail).splitCents › shares always sum to the totalalso fails before the change (Expected: 1000, Received: 999) and passes after it.Reviewer findings
Blocking anomaly — working tree does not match reviewed commit:
git statusin this workspace currently shows a staged, uncommitted modification tosrc/money/cents.tsthat reverts the file back to the pre-fix "SEEDED DEFECT" version (confirmed viagit diff --cached -- src/money/cents.ts): it restoresconst share = Math.floor(totalCents / memberIds.length)applied uniformly to all members, undoing the remainder fix that's actually committed at93f3f44. Runningbun test tests/money.test.tsagainst the current on-disk state fails 2 of the 2 new tests (gives the remainder cents to the first members,shares always sum to the total) because the working file doesn't matchHEAD. This is not a defect in the reviewed diff —git show 93f3f44:src/money/cents.tsconfirms the committed content is correct. It is a blocking concern for merge readiness … it should be investigated and the staged change discarded/reset to matchHEADbefore this is considered done.Disposition: This was a transient state, not a problem with the build. The reviewer ran at the same time as the verifier. The verifier's revert step (
git checkout 934ab00 -- src/money/cents.ts) stages the old file on purpose, and the verifier then restored it withgit checkout HEAD -- src/money/cents.ts. After both finished,git status --shortprinted nothing,bun test tests/money.test.tsgave9 pass, 0 fail, andmake checkwas green. The finding no longer applies.The reviewer found no other issues. The diff follows the remainder rule, uses integer-only arithmetic, and has no security surface.
Non-goals respected
git diff --name-only 934ab00 HEADlists onlysrc/money/cents.tsandtests/money.test.ts. The DEMO.md difference from the localmaincomes from a54c5b1 (DEMO.md: reset needs force pushes allowed on main #43), which predates the build.splitCents(totalCents: number, memberIds: string[]): Record<string, number>is unchanged.formatCents,parseCentsandsumCentsare byte-identical.Math.floorand integer arithmetic, and no fractional value is assigned to a share.remainderis forced to0, so every share isMath.floor(total/n), exactly as before. The testleaves negative and non-integer totals floored as beforelocks this in.