Full audit: fix every confirmed issue#127
Conversation
…dpyright - Add ClassVar annotations to class attributes in tags.py (pattern, name, slug, logger, status, RE_FLAGS, scope) - Fix multiple inheritance type compatibility in models.py with proper type: ignore comments - Add pyrightconfig.json to disable reportImportCycles (which pyright already ignores) These changes ensure basedpyright shows 0 errors, matching the strict pyright configuration.
- Delete pyrightconfig.json: its presence made pyright ignore [tool.pyright]
in pyproject.toml entirely, creating two sources of truth and an IDE-level
regression ('mt940 is not defined' in parser.py).
- Move reportImportCycles = false into the existing [tool.pyright] block with
a comment: mt940/parser.py imports the package root by design. basedpyright
reads [tool.pyright] too.
- Parameterize Tag.RE_FLAGS as ClassVar[re.RegexFlag] instead of bare ClassVar.
The :13:/:13D: offset subfield is a signed HHMM value (+0130 = 1h30m) but was passed raw to models.DateTime, which expects minutes (+0100 became UTC+1:40). A negative sign was never matched and a missing offset passed offset=None, both crashing DateTime with TypeError. Capture the sign, convert HHMM to signed minutes in DateTimeIndication.__call__, and drop the offset keys entirely when absent. Regenerate the mBank mt942 golden (stored instant was wrong by 40 minutes).
…hunks The TransactionDetails pattern capped the capture at ~593 characters (eight 65-char chunks plus one), silently dropping the rest of longer multi-line details. Real banks exceed the 6x65 SWIFT cap routinely -- the cap was already raised once in 4575222 for exactly that reason, and the multiline.sta fixture was still losing its ?61-?65 subfields (visible in the regenerated golden: additional_purpose now carries the full text). Capture is now unbounded; Transactions.parse already limits the value to the text between this tag and the next one.
…tent The NonSwift pattern only matched values that were either a single line or consisted entirely of two-digit-prefixed lines; any other multi-line value (NS is explicitly bank specific free-form data) failed to match and aborted the whole parse. On top of that, the failure surfaced as re.PatternError because the debug helper re-compiles unbalanced pattern fragments. Accept any content in the pattern and keep per-line 2!n35x extraction in __call__. Also reorder the __call__ branches so free-form lines keep their content in non_swift_text instead of being replaced by an empty line (the raphaelm golden: bare "32" sub-tag line is now kept, consistent with the "33" line right after it).
The pattern accepts '[DC ]?' for the D/C mark (the space was added in d36c51b for Fiducia/Volksbank ':34F:EUR 999999999999,99' input), but __call__ treated the space as a real status and produced a bogus ' _floor_limit' key instead of the d_/c_floor_limit pair an absent mark yields. A lowercase mark (matched via re.IGNORECASE) produced the right key but skipped Amount's debit negation ('d' != 'D'). Strip and uppercase the mark before dispatching.
…values Tag.parse documents RuntimeError for unparseable values, but before raising it calls _debug_partial_match, which re-compiles the pattern one line at a time. For any pattern with a group spread over several lines the fragments are unbalanced, so re.match raised re.error (re.PatternError) and masked both the RuntimeError and the diagnostics it was supposed to produce. Skip uncompilable fragments. The failure branch and the helper are now exercised by a test, so their no-cover pragmas are removed.
Strict xfail documenting that a reversal-of-credit (RC) mark leaves the amount positive although funds leave the account (needs a user decision: flipping the sign would change the betterplace fixture goldens and any downstream consumer relying on status+positive amount). Passing edge coverage: RD/RC mark parsing, 15d amount without fraction digits (10,), a second // kept inside the bank reference, and a balance on 29 February in a leap year.
The tag patterns are compiled with re.IGNORECASE, so a bank sending a lowercase 'd' debit mark in :61: or a balance is accepted by the regex, but Amount only negated on an exact 'D' -- a debit was silently stored as a positive amount. Compare the mark case-insensitively (status.upper() == 'D'). Reversal marks RD/RC are unchanged (still positive).
The existing pickle test only exercises default (empty) state. Guard the newer opt-in transaction_boundary attribute (issue #110) against a future __getstate__/__setstate__ regression that could drop it.
The existing test_json_dump only calls json.dumps over .sta fixtures with no value assertions; add full dumps->loads round-trips asserting the string forms of nested Balance/Amount/Date, the SumAmount entry number, the :13D: DateTime FixedOffset, and the opt-in StatementASNB .txt path. Also cover the previously unexercised non-leap-year February clamp in date_fixup_pre_processor.
_parse_mt940_gvcodes sliced purpose[index - 4:index] with no lower bound, so a '+' before index 4 produced a wrapped/empty slice matching the empty-string GVC key. A literal '+' in the leading free text of a structured :86: (e.g. 'AB+EREF', 'A+B SVWZ TEXT') was thus treated as a KEYWORD+ separator, truncating the purpose. GVC keywords are four characters wide, so guard the match on index >= 4. The previously unreachable else branch is now covered; drop its pragmas.
A structured :86: emits every DETAIL_KEYS value including None for absent sub-fields. A second structured :86: on the same :61: then hit 'None += str' in _update_transaction (TypeError aborting the whole file) in either tag order. Append now requires BOTH sides to be strings; any other combination assigns the incoming value, so a later string replaces an existing None. Current clobber semantics are unchanged (an incoming None still overwrites -- the behavior the real-bank goldens encode); the preservation question is pinned as a strict xfail pending decision.
A UTF-8/UTF-16 byte-order mark decodes to U+FEFF, which is not whitespace and so survives Transactions.strip. It pushed the first :20: off the start-of-line tag anchor, so transaction_reference was silently lost on BOM-prefixed files (common from Windows tools/banks). Strip a leading BOM in _read for both bytes and str input.
…dent Regression guards for previously-untested parser edges verified correct during the audit: a :86: line starting with an unknown tag-lookalike (:12:) stays in details; parse_statements drops a leading SWIFT header and absorbs lone `-` terminators; :86: continuation-line leading whitespace is kept.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class TransactionsAndTransaction( # type: ignore[misc] # pyright: ignore[reportUnsafeMultipleInheritance] | ||
| Transactions, Transaction | ||
| ): |
| logger: logging.Logger | ||
| RE_FLAGS: ClassVar[re.RegexFlag] = re.IGNORECASE | re.VERBOSE | re.UNICODE | ||
| scope: ClassVar[type[models.Transactions | models.Transaction]] = ( | ||
| models.Transactions |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness improvements and bug fixes to the MT940 parser, including case-insensitive handling of debit/credit status marks, stripping of leading byte-order marks (BOM), safe GVC code parsing with index guards, relaxed length limits for transaction details and non-SWIFT tags, and timezone offset conversions for date-time indications. Feedback on the changes suggests preventing silent data loss during transaction updates by not clobbering existing values with None from subsequent structured tags, and safely handling potential None values for the status field in the floor limit indicator to avoid AttributeError.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| else: | ||
| transaction.data[k] = v |
There was a problem hiding this comment.
Overwriting existing non-None values with None from subsequent structured :86: tags causes silent data loss (e.g., clobbering the customer_reference set by :61:). We should only update the transaction data if the incoming value v is not None, or if the key does not exist in the transaction data yet. This resolves the clobbering issue and allows the pending test_repeated_structured_86_preserves_real_values test to pass successfully.
| else: | |
| transaction.data[k] = v | |
| elif v is not None or k not in transaction.data: | |
| transaction.data[k] = v |
There was a problem hiding this comment.
Agreed this is the more intuitive semantics, but it's deliberately quarantined rather than fixed in this PR: the suggested rule (elif v is not None or k not in transaction.data) changes parse output on 5 real-bank fixtures (betterplace sepa_mt9401/sepa_snippet, cmxl statement_details, sparkassen, wrapped_timestamp), where a structured :86: currently nulls the customer_reference set by :61: (e.g. NONREF) and the goldens pin customer_reference: null. Whether that null-out is a bug or intended ('structured details are authoritative') is a maintainer decision — it's tracked by the strict xfail test_repeated_structured_86_preserves_real_values and the PR description's Needs decision section. This PR ships only the non-debatable part: the TypeError crash fix.
| # d36c51b) means "both", like an absent mark, and a lowercase mark | ||
| # (the patterns match case-insensitively) must behave like its | ||
| # uppercase form so the debit amount is negated consistently. | ||
| status: str = data['status'].strip().upper() |
There was a problem hiding this comment.
To enforce defensive programming and prevent potential AttributeError if status is ever None (e.g., if the group is somehow missing or modified in custom subclasses), we should safely handle potential None values before calling .strip().
| status: str = data['status'].strip().upper() | |
| status: str = (data.get('status') or '').strip().upper() |
There was a problem hiding this comment.
The status group can't be None here: the shipped pattern uses (?P<status>[DC ]?), which always participates in the match (empty string when the mark is absent), so data['status'] is always a str. A None would require a custom Tag subclass whose pattern drops the group — and in that case a loud AttributeError at the call site is preferable to silently coercing to '' (errors shouldn't pass silently). Leaving as-is per the audit's no-speculative-guards rule.
Full audit of mt940 (spec: 2026-07-06)
Full-codebase audit: baseline type-check hygiene, every open/recently-closed issue re-triaged against current
develop, and a spec-driven latent-bug hunt acrosstags.py,models.py,processors.py/json.py, andparser.py/utils.py. Full log:docs/superpowers/plans/2026-07-06-audit-findings.md.Fixed
ClassVarannotations intags.pyand single-sourced the pyright config inpyproject.toml(2d37679,ab773c6):13:/:13D:timezone offset subfield was read as raw HHMM digits and passed straight tomodels.DateTime, whoseoffsetkwarg is minutes (+0100became UTC+1:40); a negative or missing offset crashed withTypeError. Convert HHMM to signed minutes inDateTimeIndication.__call__, drop the offset keys when absent (36b5e66):86:transaction details were silently truncated after nine 65-char chunks (~593 chars) — capture is now unbounded (5735f2d):NS:(non-SWIFT) multi-line free-form values aborted the entire parse whenever a line didn't start with\d{2}— pattern now accepts any content,__call__line-keeping order fixed (427cf06)Tag.parsedocumentsRuntimeErrorbut raised a rawre.errorfor any tag whose pattern spread a group over several lines, masking the real diagnostic — fixed in_debug_partial_match(f8704dd):34F:floor limit with a blank or lowercase D/C mark produced a bogus' _floor_limit'key and skipped debit negation — mark is now stripped/uppercased before dispatch (243d387)Amountonly treated an exact-case'D'as debit even though the tag patterns are case-insensitive — a lowercasedmark stored a debit as a positive amount (silent wrong sign). Comparison is now case-insensitive (ff3d24a)+inside German structured:86:(GVC) free text truncatedpurpose/details before the real key (:86:020?20AB+EREF→'EREF'instead of'AB+EREF') — index-guard fix in_parse_mt940_gvcodes(3c0dbfe):86:tags on one transaction crashed withTypeError: NoneType += strwhen merging sibling sub-fields (or silently clobbered a value withNone, depending on order) — now appends only when both sides are strings, otherwise assigns (0ef2211)Transactions.strip(rstrip-only) and shifted the file's first:20:line off the^:start-of-line anchor, droppingtransaction_reference— BOM is now stripped in_read(9b2e094)Investigated, not a bug
Issues re-triaged (30 most recently closed, plus #105):
:28C:09/2021/Mnon-numeric statement number: parser correctly rejects the out-of-spec value by default; the documentedtags=custom-tag kwarg already provides a fully-supported overrideparse_statements()(How to read closing, opening and available balances #107) — reproduced against the issue's own attachment, values now correctscopeclass-attribute override still works todayTransactionto the originating tag: acknowledged API limitation, unchanged since it was wontfix'dSpec-driven latent-bug hunt (tags.py):
:61:amount-without-decimals,RD/RCmark parsing, and a second//in the bank reference all parse correctly1!a6!n3!a15d: 29 Feb in a leap year parses; non-leap 29 Feb is correctly rejected bydatetime:28C:optional/2nsequence number parses correctly (sequence_number=Nonewhen absent); an over-long unslashed statement number splits across both fields — longstanding documented leniency, same override path as Non-numeric statement number #92models.py semantics:
+2000rule (no 49/50-style sliding pivot), documented and total for MT940's 2-digitYYMMDD:61:entry-date year-rollover inference (forward/backward/same-year) is correct on all branchesBalance.__eq__excludesdatewhile__str__/__repr__include it — a defensible API-design choice, not a correctness defectTransaction.datainheritance fromTransactions.datais only "ordering sensitive" for malformed input where a statement tag arrives after its transactions, which is not plausible bank outputTransactionsSequencecontract (__getitem__/__len__/iteration/reversed) and pickle round-trip (__getstate__/__setstate__, includingtransaction_boundary) both work correctly; coverage addedprocessors.py / json.py:
mt940.JSONEncoderround-trips every model type that reaches.data(Decimal,Date,DateTime+FixedOffset,Balance,Amount,SumAmount) across all fixtures; coverage added?or duplicated sub-field code inside a structured:86:value is malformed input (?is the format's own delimiter) — deliberate leniency, not a bugdate_fixup_pre_processor's Feb-only clamp cannot over-fire on other months and is leap-consistent across the full 2-digit year range; coverage added for the non-leap 29→28 pathutils.coalescecorrectly treats falsy-but-valid values ('',0,False) as present via an explicitis not Nonecheckparser.py / utils.py:
parse_statements()'s^:20:block splitting correctly drops a SWIFT{1:}{2:}{4:header, absorbs-terminators, and never produces an empty trailing block; coverage added:86:continuation line starting with an unknown tag-lookalike (e.g.:12:) correctly stays inside the:86:slice with no structural corruption; coverage addedTransactions.stripnever lstrips, so significant leading whitespace on:86:continuations is preserved; coverage addedencoding=follows the documented, deliberate fallback-chain order (cp852 before latin1); a raw no-newline MT940 string is not plausible as a real file path, so the path-vs-data dispatch ambiguity is inherent, not a bugNeeds decision
Three items, each pinned by a strict (
xfail(strict=True)) regression test so a future accidental fix is caught rather than silently changing behavior::20:block, multiple:28C:pages —PAGED_SINGLE_BLOCK):parse_statements()splits only on^:20:, so a paged-single-block file becomes oneTransactions, andself.data.update(result)(models.py:600) lets the second page's:60M:/:25:overwrite the first page's values. Two fix shapes, each with a real tradeoff: B1 — also splitparse_statements()on:28C:page boundaries (matches the existing one-Transactions-per-statement model, nomodels.pychange needed, but multiplies the returned list beyond one-per-:20:and needs a page-boundary heuristic for banks using a different paging tag). B2 — flipmodels.py:600to first-wins (no new splitting heuristic, but:62M:/:62F:/:64:/:65:closing/available/forward balances share the exact same unscoped code path as the opening-type tags, so a blanket first-wins would make the FIRST page's closing balance win instead of the true final one — a real regression for genuine multi-page statements; only safe if scoped per-tag, which is a judgment call). Strict xfail:test_paged_single_block_first_intermediate_balance_accessible(mt940_tests/test_issues/test_issue105_first_balance.py).:61:RC(reversal of credit) amount sign:RCleaves the amount positive (RDcorrectly stays positive too); a credit reversal arguably removes funds and should negate, but realRCtransactions inmt940_tests/betterplace/sepa_mt9401.sta(lines 19/101) have goldens with positive amounts, so flipping the sign is a compatibility break for any consumer keying offstatus='RC'+ positive amount. Strict xfail:test_statement_rc_reversal_amount_is_negative(mt940_tests/test_tags.py).:86:None-clobber semantics: the crash on two structured:86:tags per transaction is fixed (0ef2211), but an incomingNonesub-field still overwrites an earlier real value (last:86:wins per key). "Preserve non-None" would be more correct but can't distinguish a value set by a prior:86:from one set by:61:, so applying it changescustomer_referencefromNoneback to:61:'s value on 5 real-bank goldens:betterplace/sepa_mt9401.sta,betterplace/sepa_snippet.sta,cmxl/statement_details.sta,self-provided/sparkassen.sta,self-provided/wrapped_timestamp.sta. Strict xfail:test_repeated_structured_86_preserves_real_values(mt940_tests/test_processors.py).Release notes
User-visible behavior changes worth calling out in the changelog. Recommend releasing this as 5.1.0 rather than 5.0.1 — these correct previously-wrong output, not pure patches. No version bump included in this PR; release is out of scope here.
:13:/:13D:timezone offsets were misparsed as minutes instead of signed HHMM (off by up to ~7x) and crashed on negative/missing offsets; also changesFixedOffset.tzname()'s string form from raw HHMM digits (e.g.'0100') to signed minutes (e.g.'60'):86:transaction details longer than ~593 characters are no longer truncated:NS:(non-SWIFT) multi-line free-form values that previously aborted the entire parse now parse successfullyTag.parsenow raises the documentedRuntimeErrorinstead of a rawre.errorfor certain unmatched multi-line patterns:34F:floor limit with a blank or lowercase D/C mark now produces the correct key and sign instead of a bogus' _floor_limit'keyddebit marks (already accepted leniently by the case-insensitive tag patterns) are now correctly negated instead of silently stored as a positive amount+in German structured:86:(GVC) free text no longer truncates the purpose/details text before the real key:86:tags on the same transaction no longer crash withTypeError:20:tag dataVerification
fef884f, coverage 100% (637 stmts / 176 branches)basedpyright: 0 errors, 257 warnings (baseline before this audit: 0 errors, 255 warnings)uv run pytest -q: 193 passed, 3 xfailed (strict) — the three needs-decision items above