Skip to content

fix(reports): a zero credit balance renders as $0.00, not -$0.00 - #1306

Merged
jfrench9 merged 1 commit into
mainfrom
bugfix/negative-zero-statement-rows
Aug 29, 2026
Merged

fix(reports): a zero credit balance renders as $0.00, not -$0.00#1306
jfrench9 merged 1 commit into
mainfrom
bugfix/negative-zero-statement-rows

Conversation

@jfrench9

@jfrench9 jfrench9 commented Aug 29, 2026

Copy link
Copy Markdown
Member

Summary

On a live balance sheet, Accounts Payable, Current rendered as −$0.00 when the account's debits exactly equalled its credits. The value leaving the API was a literal -0.0: _natural_sign negates credit-normal balances, and -(0.0) is -0.0 — equal to zero for every comparison and filter in the renderer, but formatted with a sign by the app. Subtotals happened to come out clean because their rollup starts from an integer accumulator; leaf rows carried the raw value through.

Changes

  • operations/roboledger/reports/fact_grid.py
    • _natural_sign returns an unsigned 0.0 for a zero credit balance instead of negating it.
    • New _unsigned_zero applied where rows leave _build_rows, so no other sign-flipping path (cash-flow deltas, equity reducers) can reintroduce -0.0 on a rendered row.
  • tests/operations/roboledger/reports/test_fact_grid.py — two regressions that check the sign with math.copysign (the existing test_zero passed on -0.0, since -0.0 == 0.0): the helper directly, and a _build_rows case with a zero-balance credit leaf that stays visible because its prior period is non-zero — the exact shape seen on prod.

Breaking Changes

None. Response shapes are unchanged; only a signed zero becomes an unsigned zero.

Testing

  • just test — full unit suite: 14,193 passed, 42 skipped (223 in the reports/reads modules specifically).
  • just test-code — clean; re-run by the pre-commit hook.
  • Reproduced from prod: live-financial-statement (balance sheet, YTD) on the Harbinger ledger returned "values": [-0.0, 195.94] for AP before the fix.

Certification

  • I have the right to submit this work under the Apache 2.0 license, and do so. Where any part of it is owned by my employer, I have their permission.

_natural_sign negates credit-normal balances, so an account whose debits
exactly equal its credits came out as -0.0 — equal to zero everywhere
except the formatter, which printed "-$0.00" for Accounts Payable on a
live balance sheet. Return an unsigned zero from the sign flip, and
normalize -0.0 where rows leave the renderer so no other sign-flipping
path can reintroduce it.
@jfrench9

Copy link
Copy Markdown
Member Author

@claude please review this PR

@claude

claude Bot commented Aug 29, 2026

Copy link
Copy Markdown

Claude finished @jfrench9's task in 2m 54s —— View job


Review: fix(reports) — negative-zero credit balances

  • Gather context (diff, changed files)
  • Review fact_grid.py changes
  • Review test changes
  • Post review feedback

Summary: Small, well-targeted fix. _natural_sign (robosystems/operations/roboledger/reports/fact_grid.py:2862) now returns an explicit 0.0 when net_balance is falsy instead of -net_balance, which eliminates -0.0 at its source. _unsigned_zero (fact_grid.py:2879) is applied at the single choke point where FactRow.values are populated in _build_rows (fact_grid.py:2814), which is good defense in depth — every rendered statement goes through _build_rows, so this catches any -0.0 regardless of which upstream path produced it (cash-flow deltas, equity reducers, calc-DAG rollups, etc.), not just the credit-balance case described in the bug report.

I verified the fix is sound:

  • _natural_sign can never return -0.0 post-fix: the falsy branch is hardcoded to 0.0, and the non-falsy branch negates a non-zero value, which is also non-zero.
  • _unsigned_zero(value) correctly no-ops on None (None == 0.0 is False) and on non-zero floats/NaN, only normalizing when value == 0.0 (true for both 0.0 and -0.0).
  • The calc-DAG summation path (resolve_calc_dag in calc_dag.py:149, and the equivalent inline sum in _build_rows at fact_grid.py:2769) happens to already be safe from -0.0 because Python's sum() starts its accumulator at int 0, and 0.0 + -0.0 is IEEE-754 +0.0 — so a weighted sum over falsy sources can't produce -0.0 even before this fix.

Minor, non-blocking observations:

  1. _unsigned_zero is applied only at the _build_rows render boundary, not at fact-generation/persistence time (generate_report_factsReportFact.value, e.g. in _derive_cash_flow_facts / _reconcile_operating_to_cash sign-flip paths). Every current render path (render_structure_view, and the report-snapshot read path in reads/reports.py:868) funnels through _build_rows, so this doesn't reopen the reported bug — but a persisted Fact.value could technically still carry -0.0 if something ever reads it directly instead of going through render_structure_view. Worth a quick grep if there's a "raw facts" API surface, otherwise not worth acting on now.
  2. The two new tests (test_zero_credit_balance_is_not_negative_zero, test_zero_credit_leaf_carries_no_sign) both exercise the _natural_sign fix (directly, and indirectly through _build_rows's non-pre-signed path). Neither imports or directly exercises _unsigned_zero, so the "belt-and-suspenders" half of the fix (catching a -0.0 that arrives at _build_rows via the pre_signed=True path, e.g. simulating a stale persisted fact) isn't independently regression-tested. Not required given the analysis above, but a one-line assert math.copysign(1.0, _unsigned_zero(-0.0)) == 1.0 plus a pre_signed=True case would close the loop.

Nothing here blocks merging — the fix correctly resolves the reported -$0.00 display bug, the reasoning in the PR description matches the code, and the regression tests correctly use math.copysign to catch what a plain == comparison would miss.

Note: I wasn't able to execute just test / pytest in this sandboxed review environment (command execution requires interactive approval that isn't available here), so this review is based on static analysis of the diff plus the surrounding call graph, not a live test run.

@jfrench9
jfrench9 merged commit 5a2a774 into main Aug 29, 2026
7 checks passed
@jfrench9
jfrench9 deleted the bugfix/negative-zero-statement-rows branch August 29, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant