fix: follow-up review rounds on #50 — quantity precision, Incoterm recording - #51
Merged
Conversation
A follow-up review of #50 found thirteen issues. Two of them defeated the precision fix that PR shipped, which is the reason for this one. **The quantity never reached the form at full precision.** `roundScaled` chose a precision to suit each conversion, and then both adapters rounded to exactly three decimals on the way into the box. A commodity number reported in tonnes — 247 of them in the shipped Census file — printed `0.004` against a 4.263 kg row, under-declaring by 6%, and a 0.4 kg row printed `0.000`. The keying sheet kept the full figure, so the SLI and the sheet for one shipment stated different quantities: exactly the disagreement the feature exists to prevent. One `formatQuantity` in `form-utils` now serves both forms, treating three decimals as the floor rather than the ceiling. **Named places were being thrown away whole.** `isNamedPlace` rejected the entire remainder when a freight qualifier appeared anywhere in it, so `CIF Rotterdam Prepaid` left box 15 empty with no warning. Replaced by `namedPlaceFrom`, which truncates at the qualifier and keeps `Rotterdam`. It also drops punctuation-only remainders, so `EXW.` no longer writes `.` into a form box. **CEVA dropped an operator's named place** whenever the term itself did not parse (`Ex Factory` + `SFO` wrote the term alone), and raised a conflict warning against freight wording that was never a competing place. The term written to special instructions is now built from the rule and the place that applies, and the comparison goes through `namedPlaceFrom`. **The keying sheet read the wrong SLI row.** `aggregateLines` splits rows on the canonical unit as well as D/F and the export-control triplet, so one commodity number can be two rows filing two units. Keyed on the code alone, the kilogram group was told it should be filing NO and the sheet asserted it had no figure for it — about goods the SLI files in KG. The lookup is now keyed on code and unit, with the code alone as a fallback. Also: box 24 appended the unit to plain piece counts under the 51 commodity numbers the Census file reports in `PCS`; the grid blanked its quantity total whenever two groups spelled one unit differently (`PCS` beside `EA`); the unit-of-quantity failure message hard-coded `KG` and so named the wrong remedy for tonne and gram codes; `basisNote` compared raw spellings and reported a row as deviating from a unit it was actually filing; `parseIncoterm` ran twice per fill; and `keyedQuantity` re-implemented the identity test `restateQuantity` already performs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013x3f6KRnFFe1R83BSTZewU
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Status
FindingsNo issues found in changed scope. Proposed patchesNone CI failure mappingCI is still running (pending). Diagnostic plannpm ci
npm run lint
npm test -- --watch=false
npm run buildNext actions
MetaProcessed at 2026-08-20 19:03:01.901 UTC |
… wording Twelve more findings, one of which is a regression introduced by the previous commit. **The regression.** Rebuilding the CEVA special-instruction line from the parsed rule and place discarded everything else the document said. `CIF Rotterdam Prepaid` came out as `CIF Rotterdam`, and box 20 is filled from `header.freightTerms` — null on a document that states its freight terms only inside the Incoterm, so the box falls back to this adapter's COLLECT default. The form said COLLECT over an invoice saying Prepaid, with nothing anywhere to show it. The line is now the document's own words, with the operator's place beside them rather than substituted into them, and a contradiction between the term's payment wording and the freight box is warned about. **Named places were still being lost, for the other word order.** `namedPlaceFrom` truncated at the first freight qualifier, so `FOB Freight Prepaid, Chicago` and `DDP Duty Paid Rotterdam` — ordinary wordings — yielded nothing. Qualifiers are now removed wherever they sit, along with the `by <party>` tail that belongs to them. The trailing-separator strip no longer eats the full stop of `Washington, D.C.` **`formatQuantity` derived precision from the float's own decimal expansion**, which is whatever the shortest-round-trip printer chose. It wrote `4.263e-7` into a PDF box in scientific notation, `0.1 + 0.2` as seventeen digits, and `NaN` as the word. Fixed notation to nine places, trimmed back to a floor of three. **The keying sheet's quantity total was still clamped to three decimals** while its rows carried more, so the grid's last row no longer equalled the column above it — the same under-declaration, one layer down. It is now trimmed by precision rather than by `roundTo`, whose epsilon nudge scales with the number of places and at nine is larger than the precision it protects. Also: box 24 wrote a bare count on a row that had fallen back off the required unit, leaving nothing on the paper to say the box was not in the Schedule B unit; the unit-of-quantity remedy was chosen from the unit's family rather than from whether the required unit is actually reachable, so a row invoiced in kilograms under a tonne code was told to go and supply weights it does not need; and the "how many distinct units" rule existed twice in one function, sixty lines apart, with the grid and the notes written from different copies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013x3f6KRnFFe1R83BSTZewU
…he symptom Fourteen findings. The important one identifies the defect the previous commit had only worked around. **`roundTo`'s epsilon nudge scaled with the decimal count, not the value.** At three places `EPSILON * 1000` is negligible; at nine it is 2.2e-7, larger than the precision it exists to protect — `roundTo(2, 9)` returned 2.000000222. The last commit diagnosed that correctly and then routed only the keying-sheet total around it, leaving `roundScaled` calling into it, so a gram-to-tonne restatement produced 2.000000222 tonnes and carried it onto the form and into every sheet row. The nudge is now proportional to the figure, and the workaround is gone. **Regressions from the previous commit.** The `BY <party>` tail consumed a single word, so `DUTY PAID BY ULTIMATE CONSIGNEE` left `CONSIGNEE` to be written into a box captioned NAMED PLACE/PORT. Dropping `.` from the trailing-separator class fixed `Washington, D.C.` and broke `Rotterdam Prepaid.`, which became `Rotterdam .` and then failed to match an operator-typed `Rotterdam`, raising a spurious conflict. And gating the special-instruction line on "the text differs from the code" fired on every rule spelled out in words, so `Ex Works` wrote an instruction repeating what the ticked box already said — against the rule stated eight lines above it. Also: a non-finite quantity was swallowed into an empty string, so CEVA box 24 printed a leading space and the keying sheet could still render the word NaN — both now carry the unit alone and say so in the warnings; the freight-contradiction warning asserted the document states no freight terms, which the adapter cannot know, and now states the disagreement without diagnosing it; `requiredReachable` was computed across every accepted unit while the advice named the first, so a filer could be told to restore a unit that cannot be produced; `keyedUnits` was called twice in the function extracted to make it one definition; and the `formatQuantity` tests were importing `src/carriers` from a `src/domain` test file, inverting the boundary `incoterms.ts` states. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013x3f6KRnFFe1R83BSTZewU
…oney rounding out of it Two of these matter more than the rest, and both are about the previous commit reaching further than it said it did. **`roundTo` is restored exactly as it was.** Changing its nudge fixed the nine-place case and silently moved money: `roundTo(256.025, 2)` went from 256.02 to 256.03, and that function rounds every customs value and every reconciled total in the application — enough to put a line a cent away from the total printed on the document it is proved against. The quantity restatement now has its own `roundPrecise`, whose nudge is relative to the figure and capped so it cannot reach a rounding boundary at any magnitude. The previous version also still leaked at nine places for figures above ~5.6e5, which the new one does not. **The named-place heuristic is withdrawn rather than tuned again.** Three commits, three attempts at taking the place out of a composite trade-terms line, three new holes: truncating at the qualifier lost `Prepaid, Long Beach`; removing qualifiers wherever they sat turned `DUTY PAID BY ULTIMATE CONSIGNEE` into a port called `CONSIGNEE`; bounding the party clause swallowed the `Hamburg` in `Collect by Shipper Hamburg`. The document does not mark which words are the place, so the app no longer guesses: a remainder carrying freight wording supplies no place, the box is left for the operator, and the adapter says why. That is the trade the destination-country box already makes — a blank box a reviewer is told about beats a wrong one they are not. Also: a non-finite quantity is now a blocking reconciliation check rather than a warning in one of the two adapters and nothing at all for the keying sheet, where blanking the cell had turned a visible failure into a TOTAL that was quietly short; the review screen renders its figure through the same formatter the forms use, having shown `4.263e-7` and the word `NaN` raw; the unit-of-quantity advice now names a unit the row can actually state, instead of sending a filer back to the code's first unit when only its second is reachable; and the keying sheet's quantity total and its notes label are derived from one test, so a sheet whose rows print no unit cannot get a total and a contradicting caption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013x3f6KRnFFe1R83BSTZewU
JoelA510
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request: Follow-up review rounds on #50
📋 Summary
#50 shipped the unit-of-quantity and Incoterm work after one review round. Four further review rounds followed, and each found real defects — including, twice, regressions introduced by the round before it. This PR is the result of all four.
Read the "Review history" section before merging. The headline finding is that #50's precision fix never reached the printed form, and the round that fixed that introduced a money-rounding change that has since been withdrawn.
✨ Highlights
0.004against a 4.263 kg row, under-declaring by 6%. The keying sheet kept the full figure, so the SLI and the sheet stated different quantities for one shipment.roundTois untouched, androundPrecisecarries the quantity work. An earlier round fixed the precision bug insideroundToitself and silently moved money with it —roundTo(256.025, 2)went 256.02 → 256.03, and that function rounds every customs value and reconciled total in the app.PrepaidfromCIF Rotterdam Prepaid, while box 20 falls back to aCOLLECTdefault — a form contradicting its own invoice.🗺️ Roadmap Progress
🔬 Review history
Five rounds in total, counting the one on #50. Recorded because the trend is part of what a reviewer needs to judge this by.
PCScodes filed asNOroundTo's nudge; two regressions from round 3 (BY <party>over-consumption, trailing period)roundTochange moved two-decimal money. Withdrawn.Severity is declining and round 5's changes were mostly withdrawals of over-clever code rather than new mechanism — but three of five rounds found a regression from the round before, so a sixth round finding something is not unlikely.
🏗️ Architecture Decisions
roundTorounds money; quantity restatement got its own function. Changing how a shared rounding helper breaks a tie is not a local change.resolveDestinationCountryalready makes: a blank box a reviewer is told about beats a wrong one they are not.NOandPCS, and 51 codes are reported inPCS.🔍 Review Guide
🚨 High Risk / Compliance Sensitive
src/domain/units.ts—roundTo(unchanged, and why),roundPrecise,roundScaled,restateQuantity.src/carriers/form-utils.ts—formatQuantity, which decides what number appears on a signed declaration.src/carriers/ceva/adapter.ts— box 24, the instruction line, the freight-contradiction warning.src/domain/incoterms.ts—isNamedPlaceand the reasoning for not extracting.🧠 Medium Complexity
src/carriers/keying-sheet/index.ts— the code-and-unit lookup, the mixed-unit total, the notes.src/domain/schedule-b/index.ts— the unit check naming a reachable unit.src/domain/reconcile/index.ts— thequantities-usablegate and reachability.🟢 Low Risk
src/features/review.tsx,src/carriers/nippon-express/adapter.ts,src/domain/reconcile/lines.ts.🧪 Verification Plan
1. Environment Setup
npm install— no new dependencies.2. Manual Verification
2523.10.0000) with 4.263 kg net weight — box 24 reads0.004263 T, not0.004.roundTo(256.025, 2)is still256.02; customs values and totals reconcile exactly as before.CIF Rotterdamfills Nippon box 15;FOB Origin - Collectleaves it blank and warns.CIF Rotterdam PrepaidwritesIncoterm: CIF Rotterdam Prepaidand warns if the freight box disagrees.3. Automated Tests
npm run check # typecheck, lint, 778 tests, production buildThe fixture-backed regression suite has never run. It is gated on real shipment documents that are not committed, and this repo's own standard is that those expectations are the arbiter — "if a change breaks one, the change is wrong until proven otherwise". Twenty of the assertions in
carriers.test.tsand the whole ofreconcile.test.tsskipped on every run behind this work.That matters most for the rounding: round 5's money finding was exactly the kind of thing the fixtures exist to catch, and it was found by reading rather than by a failing test. Please run
npm run checkwhere the fixtures are present before relying on this.One expectation was changed deliberately in #50 and still stands:
carriers.test.tsnow asserts box 24 of the filed vendorA3 SLI as10\r75\r4.263 KGrather than10\r75\r12, because its third row is9031.90.0000— reported in kilograms.🤖 Generated with Claude Code
https://claude.ai/code/session_013x3f6KRnFFe1R83BSTZewU