fix(workbook): show the decimals a value actually has - #213
Conversation
Every numeric cell was written with a fixed `#,##0.00`, so a stored `0.001` displayed `0.00` and `1.234` displayed `1.23`. The exact value was in the cell and the sheet showed a different one, with nothing on the page saying so -- and a non-zero amount could read as zero on a working paper someone files from. Two mandatory decimal places keep the ordinary case looking like currency; the rest render only when the value has them, so `12.50` is unchanged while `0.001` shows in full. Widening the format alone would have moved the boundary rather than removed it, because significant digits do not bound decimal places: `0.0000000000000001` has one significant digit and sixteen decimals, and the old rule admitted it. So the writer's format and the caller's acceptance rule are now derived from one constant, `XLSX_NUMBER_DECIMAL_PLACES`, and a value with more decimals than a cell can display is refused rather than rounded silently -- taking the same `Precision limit` treatment an unrepresentable value already gets. Displayed and stored now agree by construction rather than by the data being well-behaved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rolled byte-identical digest carries its evidence in a comment: the regenerated workbook was unzipped and diffed against the previously validated one, and `xl/styles.xml` differed in exactly one attribute while every sheet, cell value and shared string stayed byte-identical. That diff is what shows totals, the `Precision limit` marker and the exact-decimal path are untouched -- a rolled digest asserts nothing by itself. Both halves mutation-checked. Reverting the format to two fixed places fails the format-shape test and the digest; dropping the decimal-place rejection fails the ceiling test alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17a4407300
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A zero past the last significant digit cannot change what a cell displays, so `1.23` and `1.2300000000000000` are the same value and must reach the same answer. Counting characters refused the second, and the refusal direction is the dangerous one: it reports as the guard working while withholding correct output. The blast radius was not one cell. `filed-returns-gstr2b-workbook.ts` treats a null from `exactSpreadsheetNumber` as unrepresentable and throws, so a single padded token anywhere in a year would have refused the entire GSTR-2B workbook rather than marking the value it came from. The tell was in the two lines: `significantDigits` directly above already strips trailing zeros, and the new count did not. One value, two treatments, inside one function. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two levels, because the unit answer and the consequence are different failures: `exactSpreadsheetNumber` must accept a padded value, and the GSTR-2B builder must still produce a workbook when a raw amount carries insignificant zeros. Reverting the trim fails exactly those two and nothing else. The artifact-level test asserts the workbook is not null before its length -- the defect refused it outright rather than producing a smaller one, and `tsc` caught that the original assertion would have passed through a null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it The decimal count reads the digits after the point, and an exponent token has none: `1.5e-20` counts zero decimals, passes every check, and then displays as `0.000000000000000`. Two spellings of one value would get two answers, and the one that slips through is the defect this function exists to prevent. Not reachable today, and that is the reason to pin it rather than to skip it. Every caller already converts -- the flattener runs `jsonNumberTokenToPlainDecimal` as it parses, and the GSTR-2B scan passes the converted form -- so the guard rejects nothing that occurs now. What it removes is an unstated precondition on an exported function, where the next caller's mistake would be a silent zero in a filed working paper rather than a visible refusal. Refused rather than converted, because converting here would put a second exponent parser beside the real one and the two would drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b02260a25a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The precision scan covered the whole of `docdata`, so a value in a section this build does not recognise reached it. Those sections are excluded from the workbook and named in the coverage footer -- none of their values become cells -- and with the new decimal ceiling one such figure would refuse the entire artifact over a number that is never displayed. This is #204's shape again at one level down, and worth naming as such: that issue was "precision refusal rejects on values the workbook never renders", and scoping the scan to `docdata` and `itcsumm` fixed it for the siblings of those keys while leaving the same problem inside one of them. A tighter rule made the remaining gap reachable rather than creating it. The scan now descends into `SECTION_ORDER` -- the same list the sheets are built from, so a section cannot become renderable without the scan following it -- and `itcsumm` stays whole because it is rendered whole. Written as a table of how far to descend per subtree rather than a special case for `docdata`, so a third rendered subtree has to state its own answer. The fail-closed rule for a key the parser can reach and the scan cannot now applies at the section level too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two cases, because the narrowing must not become a hole: an unrecognised section carrying an unrepresentable value still builds, and an unrepresentable value inside a rendered section is still refused. Restoring the whole-subtree scan fails the first and only the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1439f5628
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`String(value)` is the shortest decimal that round-trips to the same double, and below `1e-6` JavaScript writes that in exponent form. So `0.0000001` stringified as `1e-7`, the lexical comparison against the plain `exactText` failed, and the total reported itself unavailable at spreadsheet precision for a value the double holds exactly and the widened format now displays. The month showed the figure and the total said it could not. That is the same stored-versus-shown disagreement this branch exists to remove, one column over, and this change is what made it reachable: seven decimal places only became displayable here. Compared as plain decimals through `jsonNumberTokenToPlainDecimal` rather than as strings, so two spellings of one value stop giving two answers -- the same correction as the trailing-zero round, in the other direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Recording the audit gap for the merged head, per
Verified on merged master A correction to how that was measured, because the first attempt was wrong. I ran those gates in the primary checkout, which turned out to hold ~900 lines of another session's uncommitted work across 17 files. It reported Not covered by any of this. No live authenticated run was performed for #213, and the reasoning is in the PR body rather than assumed: the regenerated workbook was diffed entry by entry against the previously validated artifact, and every sheet, cell value and shared string was byte-identical — only The rejected finding is recorded in its thread with the probe that refuted it, not as an assertion. |
1 participant
Summary
Closes #167. Every numeric cell in the consolidated workbook was written with a
fixed
#,##0.00, so the stored value and the displayed value could disagree.Root Cause / Decision Record
What failed.
0.001displayed as0.00and1.234as1.23. The exactvalue was in the cell and the sheet showed a different one, with nothing on the
page saying so. For an artifact a practitioner reads as a working paper and
files from, a non-zero amount that renders as zero is the worst shape of defect
available: silent, wrong, and in the one output the product exists to produce.
Why not just widen the format. That moves the boundary instead of removing
it. Significant digits do not bound decimal places —
0.0000000000000001hasone significant digit and sixteen decimals, and
exactSpreadsheetNumberadmitted it on the 15-significant-digit rule alone. A wider format would render
that as zero just as the old one did, only for rarer inputs, and the defect
would be harder to find for having been "fixed".
So the writer's format and the caller's acceptance rule are derived from one
constant,
XLSX_NUMBER_DECIMAL_PLACES. A value with more decimal places than acell can display is refused rather than rounded, taking the same
Precision limittreatment an unrepresentable value already gets. Displayed andstored agree by construction rather than by the data being well-behaved.
Why two mandatory places.
#,##0.00#############keeps the common caselooking like currency —
12.50stays12.50— while#renders a digit onlywhen the value has one. An optional place before a mandatory one would drop the
currency look from every ordinary amount.
Scope
src/core/xlsx.ts(the constant, the derived format code) andsrc/connectors/gst/filed-returns-full-year-workbook.ts(the decimal-placerule in
exactSpreadsheetNumber).tests/core/xlsx.test.ts; the decimalceiling and the ordinary-amount cases in the workbook tests; the rolled
byte-identical digest with its diff evidence.
exact-total string path, and Deferred review findings from the overnight run #178's deferred findings.
Pack Workflow Preflight
pnpm workflow:preflightwas run before editing/push, or the skip reason is documented.Sanchika Adoption Gate
@sanchika/*packages or copied Sanchika guidance, Iread
sanchika/docs/adoption-pack.mdin the coordinated parent worktree.and records the Sanchika commit or copied guidance used.
../sanchika,sanchika/packages/*/src, or parentsource paths.
Privacy And Data-Flow Impact
Sensitive Surface Review
Chrome Web Store Impact
docs/PUBLICATION_READINESS.mdis checked.Verification
pnpm install --frozen-lockfilepnpm audit --audit-level highpnpm exec wxt preparepnpm exec prettier --check .pnpm exec eslint . --max-warnings 0pnpm exec tsc --noEmitpnpm exec vitest run—Tests 2077 passed (2077),Test Files 124 passed (124)pnpm exec wxt buildnode scripts/verify-extension-package.mjs .output/chrome-mv3pnpm exec wxt zip(release-only; not run)node scripts/verify-extension-zip.mjs(release-only; not run)node scripts/write-release-provenance.mjs(release-only; not run)node scripts/verify-github-release-assets.mjs --tag <tag> --zip <zip> --checksum <sha256> --provenance <json>when release assets existnode scripts/publish-chrome-web-store.mjs --zip .output/<zip> --provenance .output/pack-release-provenance.v1.json --publisher-id <id> --dry-run truegit diff --checkpnpm review:gate -- --strict-head-review --required-review-author chatgpt-codex-connectorbefore merge, with a missing Codex review recorded as an audit gap if reported.--allow-missing-head-reviewis dropped deliberately — see Documented review-gate commands are satisfiable without a reviewer #211.Regenerated-workbook diff, which is the verification #167 asked for. The
workbook was built on
masterand on this branch from the same fixture, bothunzipped, and compared entry by entry:
xl/styles.xml— differs in exactly one attribute,formatCode="#,##0.00"becoming
"#,##0.00#############".xl/worksheets/sheet1.xml,sheet2.xml,sharedStrings.xml,workbook.xml, the rels and content types — byteidentical.
That is the evidence that totals, the
Precision limitmarker and theexact-decimal path are unchanged. The byte-identical digest was rolled once as a
consequence, and its new value is only meaningful alongside this diff.
Live run not required for this change, and here is the reasoning rather than
an assumption. The cell values are byte-identical to the artifact already
validated against real portal data; only the display format differs, and the
format string is asserted directly. A fresh authenticated run would exercise the
same bytes. #167 offered the diff as an acceptable alternative to a fresh run,
and this is that diff.
Artifact Evidence
PR Review Follow-Up
SECTION_ORDER, pinned e1439f5itcsummmembersReview round one found a defect in the rule this PR added, and it is the more
serious of the two. The decimal ceiling counted decimal characters, so
1.2300000000000000— the same value as1.23, displaying identically — wasrefused. Because
filed-returns-gstr2b-workbook.tstreats a null fromexactSpreadsheetNumberas unrepresentable and throws, one padded token anywherein a year would have refused the entire GSTR-2B workbook rather than marking
one cell. A rule added to stop one number displaying wrongly would have destroyed
the whole artifact.
The tell was four lines up:
significantDigitsalready strips trailing zeros,and the count I added did not — one value with two treatments inside one
function, which is the duplicate-of-a-canonical-fact shape this repo warns about.
Pinned at both levels, since the unit answer and the consequence are different
failures.
Both halves mutation-checked: reverting the format to two fixed places fails the
format-shape test and the digest; dropping the decimal-place rejection fails the
ceiling test alone.
Screenshots
None. The change is a number format inside an
.xlsxpart; the format string isasserted directly in
tests/core/xlsx.test.tsand the surrounding bytes areproven unchanged by the diff above.
DCO
Signed-off-by:trailers; the trailer isapplied at squash merge, as every commit on master shows. Left unchecked
rather than claimed — chore(governance): make the DCO checkbox mechanically satisfiable, or drop it #109 tracks making this box mechanically satisfiable
or dropping it.