feat(schema): add phase 22 calculation engine - #23
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sourcePath: input.sourcePath, | ||
| valuePath: input.valuePath, | ||
| }); | ||
|
|
||
| return { | ||
| diagnostics: aggregate.diagnostics, | ||
| key: groupKey, | ||
| label: groupKey, | ||
| total: | ||
| aggregate.value ?? createDecimalValue(zeroBigInt, precision.scale, 0), | ||
| }; | ||
| }); | ||
| const grandAggregate = calculateRowsAggregate({ | ||
| operation: 'sum', | ||
| precision, | ||
| rows: rowsResult.rows, | ||
| sourcePath: input.sourcePath, | ||
| valuePath: input.valuePath, |
There was a problem hiding this comment.
sourceIndex in group diagnostics is group-relative, not source-array-relative
calculateRowsAggregate is called with the per-group row subset, so the sourceIndex emitted in each group.diagnostics[i] is the index of the row within that group's slice, not within the original sourcePath array. Any caller that reads group.diagnostics[].sourceIndex to identify which source record caused the error will get the wrong row for every group that does not happen to start at position 0 in the original array.
Concretely: if the source array is [A, B, C, D] and group 2 contains [C, D] (original indices 2 and 3), a non-numeric value on D yields sourceIndex: 1 in the group diagnostic, not sourceIndex: 3. The test for the "Outreach" group deliberately omits the sourceIndex assertion, confirming the value is incorrect.
Fix: track original indices when building group buckets and pass them through to calculateRowsAggregate so diagnostics carry the source-array position.
| }); | ||
|
|
||
| diagnostics.push(...grandAggregate.diagnostics); | ||
|
|
||
| return { | ||
| diagnostics, | ||
| grandTotal: | ||
| grandAggregate.value ?? | ||
| createDecimalValue(zeroBigInt, precision.scale, 0), | ||
| groups: calculatedGroups, | ||
| }; | ||
| } | ||
|
|
||
| export function calculateInvoiceTotals( | ||
| input: CalculateInvoiceTotalsInput, | ||
| ): CalculateInvoiceTotalsResult { | ||
| const precision = normalizePrecision(input.precision); |
There was a problem hiding this comment.
grandTotal silently includes amounts from rows excluded from all groups
Rows whose groupPath value is null, undefined, or missing are skipped during group assignment (a missing_calculation_field diagnostic is pushed and return is called), so they contribute to no group's total. However, grandAggregate runs unconditionally on rowsResult.rows — the full unfiltered slice — meaning those same rows' valuePath amounts are summed into grandTotal.
The result: when any row lacks a group key, grandTotal > sum(group.total). A caller doing financial reconciliation will silently find a discrepancy without any diagnostic pointing at the mismatch.
Fix options:
- Build
grandAggregatefrom only the rows successfully assigned to a group. - Document and test the current semantics explicitly and add a test asserting the discrepancy when a group-key is missing.
2 participants
Summary
@asym/pdf-template-schema.Validation
corepack pnpm --filter @asym/pdf-template-schema test- passed, 64 tests.corepack pnpm --filter @asym/pdf-template-schema typecheck- passed.corepack pnpm --filter @asym/pdf-template-schema build- passed.corepack pnpm lint- passed with one existing warning in untouchedapps/web/src/app/editor/editor-overrides.css.corepack pnpm dlx @fission-ai/openspec@latest validate build-pdf-document-builder- passed.corepack pnpm dlx @fission-ai/openspec@latest validate --all- passed.git diff --check- passed with Windows LF-to-CRLF working-copy warnings only.Earlier Phase 22 validation also passed renderer/editor package tests, typechecks, builds,
@react-email/editortests, export smoke, and fullpnpm testbefore this PR branch was created.Known gaps
Rollback
Revert this PR to remove the calculation source and tests, restore the schema export/maturity metadata, and undo the Phase 22 documentation/OpenSpec updates. Then rerun schema, renderer, editor, lint, and OpenSpec validation.
OpenSpec
Phase 22: Build Calculation Engine for Totals, Subtotals, and Grouping. Next phase entry point: Phase 23 exposes summary blocks and total rows.
Greptile Summary
This PR introduces the Phase 22 deterministic calculation engine in
@asym/pdf-template-schema, adding BigInt-backed decimal helpers for numeric aggregates, table totals, grouped subtotals, invoice totals, financial totals, and tax-deductible amounts — all exported from the schema root with no new runtime dependencies.Two correctness issues in
calculateGroupedTableTotalsneed attention before callers rely on the public API:sourceIndexis group-relative, not source-array-relative. Diagnostics emitted inside a per-groupcalculateRowsAggregatecall carry the row's index within the group slice, not within the original source array. Any UI or tool that readsgroup.diagnostics[].sourceIndexto highlight the offending row will silently point to the wrong row for any group that does not start at position 0.grandTotalsilently includes amounts from rows excluded from all groups. Rows with a missing/null group key are skipped during group assignment but are still passed to the grand-aggregate call, sograndTotalcan exceed the sum of allgroup.totalvalues without any diagnostic flagging the discrepancy.Confidence Score: 4/5
Safe to merge with the two P1 issues tracked — the calculation engine is not yet wired into rendered output (Phase 23), but the public API contract should be correct before consumers depend on it.
Two P1 correctness issues in
calculateGroupedTableTotals(wrongsourceIndexin group diagnostics;grandTotalmay not equal sum of group totals when group keys are missing) cap the score at 4. No P0 issues found. The rest of the arithmetic logic, rounding, and edge-case handling is solid and well-tested.packages/pdf-template-schema/src/calculations.ts — specifically the
calculateGroupedTableTotalsfunction (lines 348–382).Important Files Changed
calculateGroupedTableTotals: group diagnostics report group-relativesourceIndex(not source-array-relative), andgrandTotalcan silently include amounts from rows excluded from all groups../calculations; bumps maturity tag tophase-22-calculation-engine.sourceIndexassertion for non-first groups, leaving the group-relative index bug untested.calculateNumericAggregateis exported.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[calculateGroupedTableTotals] --> B[resolveCalculationRows\nfull source array] B --> C{each row} C -->|groupPath found| D[assign row to group bucket] C -->|groupPath missing| E[push missing_calculation_field diagnostic\nskip row] D --> F[calculateRowsAggregate\nper group subset] F --> G[group.diagnostics\nsourceIndex = group-relative ⚠️] B --> H[calculateRowsAggregate\nGRAND — ALL rows including skipped ⚠️] H --> I[grandTotal\nmay include skipped-row amounts] G --> J[CalculateGroupedTableTotalsResult] I --> JReviews (2): Last reviewed commit: "fix(schema): harden calculation diagnost..." | Re-trigger Greptile