fix(app): refuse money amounts the field cannot carry faithfully - #110
Merged
Conversation
The amount field's validator guaranteed several things about typed text — digits only, no sign, no exponent, at most two decimals, no trailing garbage — but the amountValid predicate reproduced exactly one of them, the sign, via a bare "> 0" magnitude check. The validator gates keystrokes only, and a property write bypasses it entirely, which is how every spec and every host sets the amount. So each unreproduced guarantee was a live hole rather than a theoretical one. Verified before the fix, not assumed: "12.345" passed validation and recorded $12.35, an amount the user never typed; "1e400" passed and produced Infinity on its way into a 64-bit sink; "1e3" was accepted though the validator refuses that text; "12.34abc" recorded $12.34. Nothing below the control catches any of them — neither the daemon's RecordTransaction nor core validates amount at all. The shape now lives in one pattern that both enforcement points read, so the predicate cannot drift from the validator again. Two properties of that literal are correctness-critical and commented as such: no g flag, since a global regex carries lastIndex across .test() calls and would make consecutive evaluations alternate; and the anchors, since .test() substring-searches unless anchored, which would reopen the trailing-garbage hole while the validator still rejected it. The pattern also caps integer digits at 12, just under $1 trillion, keeping cents inside the range where a JS number represents integers exactly. That cap is the only magnitude bound — a second numeric check beside it would be two different limits wearing one name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le path has recordTransaction took a positional qlonglong amount, so there was no QVariant to range-check and the transaction path carried no client-side amount guard at all — the only structural reason the two money paths differed. Converting the call to the params object createRecurringRule already uses removes that objection instead of working around it, and leaves both paths with symmetric defense in depth rather than one guarded and the other trusting a predicate inside a UI control. Defense in depth is the honest framing: the preceding commit closed the reachable hole in the control itself, so these guards exist for a non-UI caller or a future host. The comment on the rule path's amount guard claimed the failure was reachable from the UI; that is no longer true, so it says what it now guards instead. Its threshold claim is left alone — verifying what toLongLong actually range-checks needs Qt private headers that are not installed here, and swapping one unverified number for another is not an improvement. The mock takes the map and populates the same lastRecord* properties from it, so every existing assertion in the suite is untouched; a new spec pins the key set, which is the only assertion able to reach the QML/C++ key contract given the suite drives a mock rather than the real client. Separate commit from the predicate fix because the two are independent — one repairs a live defect, this one adds a guard against a caller that does not exist yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iters Converting recordTransaction to a params object left the present-AND-valid key loop duplicated verbatim between the two writers — debt this branch introduced, so it is cleaned up in it. The helper returns the bad-key list rather than emitting, because the callers differ in exactly the two ways it cannot absorb: which failure signal carries the message, and how the message names the request. The explanation of *why* presence alone is insufficient moves to the helper, since it is now one fact stated once; each call site keeps only what is specific to it, which is which of its fields are legitimately empty. No test covers the bad-key path either before or after: it lives in the real gRPC client, which the suite cannot reach because it drives a mock instead — the gap already tracked separately. So this rests on the compile plus the unchanged happy-path assertions, and is a behavior-preserving extraction rather than a change a test pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…guard comment
Pre-PR audit found the changelog entry — the only user-facing artifact in this
branch — wrong in two independent ways, and both were verified numerically rather
than argued.
Its example was not user-reachable. The previous validator already capped decimals
at two, so "12.345" was refused as you typed it, both before this change and
after; it could reach the field only from code. Presenting it as an amount a user
entered and had silently rounded would have sent people auditing data that was
never at risk. The commit message for the fix itself said this correctly ("an
amount the user never typed"); only the changelog overstated.
Its stated reason for the cap was also false. Cents remain exactly representable
far above $1 trillion — the actual limit is 2^53 cents, about $90 trillion, and
measurement puts the first inexact result at 14 integer digits. The entry claimed
the cap *was* that boundary rather than a conservative margin below it, which
would leave a maintainer refusing to raise it, or matching the daemon to a limit
that is not one.
What the entry now describes is the defect that was genuinely reachable: the old
integer part was unbounded, so a typed amount around $90 trillion or more recorded
with silent precision loss on both money paths.
The same false-threshold arithmetic appeared in the amount guard's comment, which
this branch had just promoted from background detail into the justification for
the new cap — a magnitude past a double's exact-integer ceiling fits an int64
comfortably. Correcting it needed no Qt internals, only arithmetic, so the plan's
instruction to leave the threshold claim alone was itself the defect; the
unverifiable half (what toLongLong range-checks) stays unasserted.
Three stale comments the audit caught in the same class this branch exists to fix,
including one the branch itself broadened wrongly: the shared control claimed
nothing below it validates any amount, but a transfer's is checked by both the
daemon and core.
The rejection specs now assert that the write landed before asserting it was
refused. Every other assertion in those specs holds equally if the property write
had silently failed, so they would have passed for the wrong reason had a future
change routed host writes through a validating setter — the vacuous-spec trap this
session documented elsewhere, reproduced here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
toLongLong fails identically for a non-numeric amount and an oversized one, so "That amount is too large. Please enter a smaller amount." fired for a caller that passed a string or a null — advice to fix input that was never the problem. Both money paths now document this guard as unreachable from the UI, since the amount control caps digits and scale itself, so a user-phrased message no longer fits either call site. Both say "internal error" instead, matching the sibling zero-amount guards a few lines below each of them. Changed on both paths rather than only the new one: it is one guard against one condition, and leaving the two worded differently would invite a reader to look for a distinction that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two corrections to the entry rewritten in the previous commit, both about it overstating again in smaller ways. "Was recorded with silent precision loss" claimed more than the arithmetic supports: above the exact-integer limit, values that happen to be representable still record exactly, so the honest statement is that an amount there could be recorded imprecisely, not that it always was. Correcting one imprecise claim with another would have been a poor trade. Also dropped the sentence noting the cap sits below the representational limit rather than at it. It existed to prevent the misreading the original entry caused, but with both numbers now stated plainly a reader can see they differ, so it told a user nothing they could act on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the Qt app’s money input handling so programmatic writes can’t bypass validation and silently record amounts the UI can’t represent faithfully, and it standardizes the transaction-recording call to use a params object (matching the recurring-rule path).
Changes:
- Centralize the accepted money-text shape into a single regex used by both the keystroke validator and the submit predicate, including a 12-integer-digit cap to stay within safe integer cents for the QML/JS number boundary.
- Change
recordTransactionfrom positional args to aQVariantMapparams object and add client-side “presence/readability + numeric conversion integrity” guards for amount/status. - Add/extend Qt Quick Tests to assert rejected shapes, cap behavior, and the QML-side key contract for the params object.
PR Description Checks (per repo PR conventions):
- Overview section: Pass
- Scope cohesion: Pass
- No private tooling references in tracked changes: Pass
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the new amount-field cap and stricter rejection of unfaithful shapes. |
| app/tests/recurringruleform/tst_CreateRecurringRuleForm.qml | Adds coverage for amount-shape rejection and integer-digit cap behavior on recurring rules. |
| app/tests/recordtransactionform/tst_RecordTransactionForm.qml | Adds coverage for amount-shape rejection, cap behavior, and pins the QML param-key contract. |
| app/tests/MockFinchClient.qml | Updates mock recordTransaction to accept params and captures key set for contract tests. |
| app/src/finchclient.h | Updates the recordTransaction Q_INVOKABLE signature to take a params map. |
| app/src/finchclient.cpp | Implements params-based recordTransaction, shared required-key validation helper, and conversion guards. |
| app/qml/SignedAmountField.qml | Introduces a shared amount pattern for validator + predicate and enforces the 12-digit integer cap. |
| app/qml/RecordTransactionForm.qml | Switches transaction submit to pass a params object to the client. |
Comment on lines
+26
to
+27
| // the request. Emptiness is not checked here — each caller has its own legitimately-empty fields | ||
| // (an open-ended end date, a not-a-transfer destination, an absent description). |
Owner
Author
There was a problem hiding this comment.
Fixed in ee79d20 — description must be present and may be blank, so calling it absent named the one condition the helper rejects.
The shared key helper listed "an absent description" as an example of a field that is legitimately empty — contradicting itself, since the helper's whole job is to reject a key that is absent. Description must be present; it may be blank. Its two siblings in that list were already framed correctly as empty, so this was the one term out of place, and it named the exact condition the code treats as a failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Overview
The Qt app's shared money-input control enforced the shape of an amount in two places that did not agree. A
RegularExpressionValidatorgated typed keystrokes — digits only, no sign, no exponent, at most two decimals, nothing trailing — while a separate predicate decided whether the amount could be submitted, and that predicate reproduced exactly one of those guarantees. A validator gates typed input only, so a property write goes straight past it, and a property write is how every spec and every host sets the amount. Each unreproduced guarantee was a live hole rather than a theoretical one — reachable from code though not from typing, since the validator refused these keystrokes: written into the field,12.345passed and recorded $12.35,1e400passed and producedInfinityon its way into a 64-bit sink, and12.34abcrecorded $12.34. Nothing below the control caught any of them, because neither the daemon'sRecordTransactionnor core validates a transaction amount at all.The shape now lives in one pattern that both enforcement points read, so the two cannot drift apart again. That pattern also caps the integer part, which was previously unbounded — the one hole a user could reach by typing, since past roughly $90 trillion the app stops representing that many cents exactly and the recorded amount could drift without warning.
Separately, the transaction-recording path carried no client-side amount guard at all, and the reason was structural rather than an oversight:
recordTransactiontook a positionalqlonglong, so there was noQVariantto interrogate before use. It now takes the same params objectcreateRecurringRulealready used, which removes that objection rather than working around it, and leaves both money paths with the same guard instead of one guarded and the other trusting a predicate inside a UI control.How it works
Two properties of the shared pattern are correctness-critical, and commented as such in the file. It carries no
gflag: a global JavaScript regex keepslastIndexacross.test()calls, so consecutive evaluations of the same text would alternate — and only the predicate side would be affected, presenting as an inexplicable binding bug rather than a regex one. The^…$anchors are equally load-bearing, becauseRegularExpressionValidatormatches against the whole input while JavaScript's.test()substring-searches unless anchored. Dropping them would have the predicate accept12.34abcwhile the validator still rejected it, reopening precisely the hole this closes.The digit cap sits deliberately below the representational limit rather than at it. Twelve integer digits holds cents near 1.0e14, while exactness runs to 2^53, about 9.0e15. The margin is the point: the cap is a round, explainable figure under a limit that is neither.
The rejection specs assert the write landed before asserting it was refused. Every other assertion in them would hold equally if the property write had silently failed, so without that line they could pass for a reason other than the one they name — and the premise they would be failing to test is the one the whole guard rests on.
The QML-to-C++ key contract is the one seam the suite cannot reach, since it drives a mock rather than the real client. A spec pins the key set, and the path was verified manually against a live daemon: an ordinary amount records as exactly the cents entered, and the four rejected shapes never leave the form.
Related to #108, related to #109. This closes the app-side hole, which is what makes both of those visible, but resolves neither: #108 covers a transaction amount being unvalidated below the app, and #109 covers three day-of-month fields carrying the same validator-versus-predicate mismatch this fixes for the amount.