feat(reports): add splitKind to distinguish deposit-driven splits from line splits (#1911) - #2015
Conversation
…m line splits
Adds SourceReportInvoice.splitKind ('lines' | 'deposits' | 'both' | null),
derived alongside isSplit in the existing step-f UNION at zero extra query
cost, so the report legend can distinguish a genuine line split from an
invoice whose lines all sit in one source but carries a deposit tagged to
another. Previously both cases were byte-identical in the response and
collapsed to the generic split label instead of the deposit-reduced one.
Also removes the unsound budgetLines.length > 0 gate on isDepositReduced,
and fixes a second, previously unreported defect: isDepositReduced was
over-inclusive, firing on line-split invoices carrying an untagged deposit
even though untagged deposits are apportioned pro-rata rather than claimed
separately.
Fixes #1911
Co-Authored-By: Claude dev-team-lead <noreply@anthropic.com>
Co-Authored-By: Claude backend-developer <noreply@anthropic.com>
Co-Authored-By: Claude frontend-developer <noreply@anthropic.com>
Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com>
Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com>
Co-Authored-By: Claude product-owner <noreply@anthropic.com>
|
[security-engineer] Security review of #2015 ( Verdict: no findings. Approve. This PR was worth pulling in even against the "touches no auth/Dockerfile/dependency" trigger — a SQL rewrite plus a new field that encodes information about other budget sources is a legitimate information-disclosure shape to check independently rather than take on trust. Here's what I verified directly in 1. SQL injection surface — clean.
2. Information disclosure across budget sources — premise verified independently, not taken on trust.
3. What actually leaves the system — matches AC 1.10.
4. Performance/DoS — no complexity-class change. Correctness note (non-security, flagging since you called it out): the On the trigger call: this was a reasonable ask despite missing the literal auth/Dockerfile/dependency triggers — a SQL rewrite plus a field that could have carried cross-source identity is exactly the shape that "looks harmless in a diff." Calibration for next time: this pattern (new field derived from data outside the request scope) is worth a security pass whenever the there is no auth boundary here premise hasn't been independently checked recently — which is what made this one worth doing rather than skippable. |
|
[product-architect] Architecture review of #1911's Verdict: CHANGES REQUESTED. The production design is sound — I verified the query, the derivation, the invariant, and the wiki, and I agree with the behaviour ruling. What blocks is that two E2E assertions this PR itself added/re-seeded are failing reproducibly on CI, and they are the browser-level verification of two of this story's own acceptance criteria (3.4 and 3.6). Neither needs a production-code change. Details in H1/H2. BlockingH1 — Scenario 17's contiguous-substring assertion can never match (
|
| fixture | residualFraction | depositFractions | line 75 → contribution |
|---|---|---|---|
| deposit untagged (old) | (200−50)/200 = 0.75 |
[0.25], emitted (untagged) |
75×0.75 + 75×0.25 = 75.00 |
| deposit tagged to other (new) | (200−50)/200 = 0.75 |
[] — tagged rows filtered out |
75×0.75 = 56.25 |
Fix: change the expectation to €56.25. AC 3.4's mixed-case browser verification is red until then.
This table is also the arithmetic proof that AC 3.2 is right in both directions, which I think is worth recording on the issue because until now it has only been argued in prose. A foreign-tagged deposit shrinks the residual and its fraction is never returned to this source — the allocation genuinely drops, so "claimed separately" is a true statement. An untagged deposit shrinks the residual by exactly the fraction it hands back (0.75 + 0.25 = 1.0) — net zero, nothing is claimed elsewhere, so the pre-#1911 (less deposit) was a literally false statement to a bank. The derivation isn't just better-typed than the old gate; it is the arithmetically correct predicate and the old one was not.
On merging anyway: E2E Gates is main-only, so this can squash to beta red. Please don't. This repo has a documented incident of exactly one consistently-failing E2E test riding nine consecutive beta PRs because each one read "beta doesn't gate on E2E". Both fixes are one line.
1. The UNION dedup hazard — your reasoning holds
Confirmed. COUNT(DISTINCT split_data.source_id) and both MAX(CASE …) aggregates are multiplicity-insensitive, so adding origin cannot move isSplit. I also checked the NULL edge: both arms filter source_id IS NOT NULL before the outer aggregate (:287, :294), so source_id != :sourceId never evaluates to SQL NULL and never silently falls through the CASE to ELSE 0 for the wrong reason. The derivation is correct.
On robustness, one thing genuinely changed that the code does not say out loud. Pre-#1911, cross-arm dedup made COUNT(*) and COUNT(DISTINCT source_id) equivalent for every invoice. Post-#1911 they are not. So a future edit that reaches for COUNT(*) > 1, COUNT(source_id) > 1, or any row-counting aggregate — which reads as an obvious simplification of a query that visibly says UNION — silently makes every invoice with both a line and a tagged deposit in the same source isSplit: true. That is precisely the AC 1.9 fixture.
Two mitigations, one present, one missing:
-
Present and good: the AC 1.9 test (
sourceReportService.test.ts) is the right guard, its name states the mechanism, and it genuinely flips if the basis changes. This is the strongest form of protection available and I'd have asked for it if it weren't there. -
M2 (medium, non-blocking): the query itself is silent. The step-f comment at
sourceReportService.ts:262-263still describes onlyisSplitand says nothing aboutorigin, the defeated dedup, or whyDISTINCTis load-bearing. The person who breaks this will be reading line 276, not the test file. Please add two lines there — something like "originis a discriminating column: it defeats UNION's cross-arm row dedup, so a source present in both arms now yields two rows. Every aggregate here must be multiplicity-insensitive —COUNT(DISTINCT source_id), notCOUNT(*). See AC 1.9 regression fixture." Document the invariant at the place where it can be violated, not only where it is checked. -
L1 (low, optional): given that all three aggregates ignore multiplicity, the
UNIONdedup is now entirely non-load-bearing (it can only collapse rows identical in all three columns).UNION ALLis semantically identical here, cheaper (no dedup sort), and — the real argument — honest: it makes row multiplicity visible at a glance, soCOUNT(*)looks obviously wrong instead of obviously fine.UNIONnow reads as a guarantee it no longer provides. Your call; M2's comment covers the hazard either way.
2. Inferring domain facts from response shape — the general answer
splitKind is the right correction, and the principle it establishes is worth naming explicitly because it will recur:
When the server ships a source-scoped projection of an entity, any domain predicate over the unscoped entity must be shipped as its own field. The client cannot recover it from the projection — not by cardinality, not by summing, not by filtering.
budgetLines[] and deposits[] are projections; isSplit/splitKind are unscoped predicates. This PR applies the principle correctly.
But the API still invites the inference, in three places, and the reason is structural rather than local:
(a) The scoping lives entirely in prose. budgetLines: SourceReportBudgetLine[] and deposits: SourceReportDeposit[] are named exactly as an unscoped inventory would be. The entire warning is carried by a JSDoc line and the wiki's "Budget Line Scope" note. Every instance of this bug — #1898's legend, #1902's footnotes, this one, and (most tellingly) the PO addendum's finding that the same false invariant was independently encoded in the if/else, in two page-object JSDoc blocks, and in a Scenario 18 assertion — came from a reader who did not have that prose in front of them. Four readers believed it; none of them were looking at the doc comment.
The cheap structural fix is to encode the scope in the names: budgetLinesForSource / depositsVisibleToSource, or nest both under scopedDetail: { budgetLines, deposits }. A name is re-read at every call site; a JSDoc is read once, by the person who least needs it. This is a mechanical rename with a compiler-enumerated call-site list, and it is the highest-leverage remaining change. Worth its own issue.
(b) One live inference remains, and it is one filter change from being the next bug. buildReportContent.ts:173:
const hasOwnTaggedDeposit = invoice.deposits.some((d) => d.budgetSourceId === report.source.id);This is the same shape as the defect this story fixes: a domain fact recovered by filtering a source-scoped projection. It is sound today — S-tagged deposits are never filtered out by step i — but sound by the accidental direction of that filter, not by contract, and nothing pins the dependency. Narrow deposits[] for any reason (drop untagged rows, paginate it, scope it to the status slice) and isDeposit silently goes false, the (Deposit) badge silently disappears from bank reports, and every current test still passes because they all construct deposits[] by hand. Note that AC 3.3 deliberately froze this trigger, which is a reasonable scope call — but it means the story fixed two of the three inference sites and left the third, undocumented. At minimum, add a comment on the step-i filter in sourceReportService.ts recording that deposits[] must continue to include this-source-tagged rows because the client's constituted-deposit badge depends on it. Better, follow (a) and eventually ship hasOwnTaggedDeposit as a field too.
(c) sum(allocatedPortion) < invoiceAmount remains available as a split proxy. The wiki already warns against it, and #1965's rationale (the reader sees the discrepancy anyway) means it isn't worth new machinery. Noting for completeness only.
3. The invariant, and whether isSplit should be deprecated
Not runtime-asserting is the right call. Three reasons, in order of weight:
- It is an output-shape property of one function, not input validation. A runtime assertion has only two available behaviours: throw — converting a cosmetic grey-label defect into a 500 on a bank report a user is trying to generate — or log, which is a test in the wrong place. Neither is better than the fixture-level assertions you already have.
- It is pinned at the level where it can actually break. The invariant is a property of the SQL, and the eight AC 1.6 fixtures assert it against real seeded rows. A runtime check in the mapping loop would be checking two values the same loop just wrote, downstream of where any real divergence originates.
- The failure mode degrades gracefully. A violation produces a missing or extra grey label. No money moves.
isSplit should be retained, and I want to give you a better reason than AC 2.2's. "Every existing consumer compiles unchanged" is a reason with an expiry date — it stops applying the moment consumers migrate. The durable reason is:
isSplitandsplitKindare computed by two independent expressions over the same rows —COUNT(DISTINCT source_id) > 1versus twoMAX(CASE …)aggregates. That redundancy is what makesexpect(inv.splitKind !== null).toBe(inv.isSplit)a meaningful test rather than a tautology.
Collapse isSplit into a derived alias for splitKind !== null and AC 1.6 becomes expect(x).toBe(x) — you lose the only cross-check that the two derivations agree, which is exactly the check that would catch the COUNT(*) regression from §1 from the other direction. Keeping two independently-derived values that must agree, with an assertion that they do, is a deliberately useful redundancy, not accidental denormalization. I'd record that reasoning in the type's doc comment so the next person to propose deprecating it finds the argument instead of re-deriving it.
isSplit also remains a live input, not just a legacy field: it drives the AC 3.3 constituted trigger (buildReportContent.ts:174) and the wizard list badge (ReportInvoiceList.tsx:245). Deprecating it isn't a doc change, it's a rewrite of both.
4. Wiki — complete and correct
Verified published: branch records 1f3eb7c, git -C wiki ls-remote origin master = 1f3eb7c. (Worth noting for everyone that no workflow checks out submodules, so an unpushed ref merges fully green and then breaks git submodule update on beta — this one is fine.)
The predicate is stated correctly, in the form the AC required:
The predicate per arm is "does this arm contain a source ≠ the requested
sourceId" — not "does this arm contain ≥2 distinct sources".
It also carries the AC 1.5 trap case explicitly ("A deposit-only foreign source paired with exactly one line-source (the requested source itself) still yields 'deposits'"), the structural invariant with its non-asserted status, the AC 1.10 non-disclosure property, the JSON example, both forward-references retired from "planned", and a Deviation Log row naming the source of truth. Nothing to add.
5. ADR / Schema — neither needed
Schema page: no change, correctly. splitKind is derived at query time; no column, no DDL, no migration. Nothing on that page describes it.
No ADR for splitKind itself. It is a field addition following an already-documented pattern, and the API-Contract entry plus Deviation Log is the right weight. An ADR here would be ceremony.
There is an ADR-shaped thing nearby, and it is the §2 principle rather than this field. "Source-scoped projections must not be the basis for unscoped predicates — ship the predicate" has now generated #1898, #1902, #1911, and a four-reader shared misconception. That's a decision worth recording once, either as a short ADR or as a page-level convention on API-Contract generalizing the existing "Budget Line Scope" note beyond budgetLines[]. I'll take it as a follow-up rather than ask this PR to carry it.
Non-blocking findings
M1 — stale and inverted comments in client/src/lib/reportContent/types.ts:16-17. Both are now wrong, and one of them is wrong in a way that would cause the bug to be re-introduced:
isSplit: boolean; // split invoice with budget lines → inline "partial" label
isDepositReduced: boolean; // split invoice reduced by untagged deposits → inline labelLine 16's "with budget lines" names precisely the gate AC 3.1 removed — a reader trusting it reinstates budgetLines.length > 0, which AC 3.7 explicitly forbids. Line 17 is now inverted: after AC 3.2 an untagged deposit is exactly the case that does not set the flag. It restates, in a comment on the shared type both the PDF and the preview consume, the same false claim ("untagged deposits reduce this source") that this story removed from the bank-facing legend. Suggest: // splitKind ∈ {lines, both} → inline "(partial)" label and // splitKind ∈ {deposits, both}: a deposit tagged to a DIFFERENT source reduces this source's residual → inline "(less deposit)" label.
L2 — ReportContentRow.isSplit and SourceReportInvoice.isSplit now diverge further under one name. They were already different (isSplit && budgetLines.length > 0); this PR widens the gap — row.isSplit is now false where invoice.isSplit is true for the entire splitKind: 'deposits' population, which is the headline case of the story. buildReportContent.ts reads invoice.isSplit at :174 and writes isSplit at :216, twelve lines apart. Consider renaming the row flag to isPartial — it matches the rendered (partial) label, and it makes the two concepts unmistakable at both sites. Pre-existing, so not a gate on this PR.
On the behaviour change the PO ruled on — I agree, and Scenario 18 hands you a better argument
An invoice with lines entirely in A carrying a deposit tagged to the reported B now shows (partial) alongside (Deposit). I have no objection, and I'd go further than "the server always classified it as split": the row's own numbers already made the claim visible. In Scenario 17 the table prints €1,000.00 and €150.00 side by side; in Scenario 18, €200.00 and €56.25. (partial) does not disclose a new fact — it names a discrepancy the recipient can compute from two adjacent cells. Suppressing it left a visible gap unexplained to an auditing reader, which is worse than explaining it, and matches #1965's stated rationale for the sentence existing at all.
The narrowing argument is also self-defeating, as the PO says: every candidate predicate reduces to budgetLines.length > 0 under a new name, which re-breaks the claim-report zero-contribution case. No narrowing exists that keeps AC 3.1 sound.
What I verified
Query semantics and NULL handling (sourceReportService.ts:269-315) · the dedup-insensitivity claim · the invariant's structural basis (Rail A :130-140 and Rail B :223 are both S-scoped, so S is always in the union) · the AC 1.9 regression fixture actually flips · AC 1.8's pass-through spy is non-trivial (real query still executes; it filters on reconstructed SQL text rather than counting calls against a stub) · AC 5.5's mutation test genuinely flips both row flags and legend membership · AC 1.10's leak check asserts against the serialized payload · residual arithmetic in depositAggregateUtils.ts:545-562 · wiki publish state and predicate wording · no migration or schema surface touched.
Fix H1 and H2 and re-request; everything else is follow-up material.
|
[product-owner] Requirements review of PR #2015 against the acceptance criteria I wrote for #1911 (AC comment) and my Scenario-17 addendum (ACs 3.6/3.7, UAT 4.7). Verdict: CHANGES REQUESTED. Two blocking findings. The derivation itself — the part this story exists for — is correct, well-tested and correctly documented; both blockers are in the evidence layer, not the product logic. What is metI walked all 43 criteria. Satisfied, verified individually against the diff: §1 derivation (1.1–1.10) — all met. The predicate is implemented exactly as specified: §2 type/compat (2.1–2.5) — all met. Required (not optional), §3 row flags (3.1–3.5, 3.7) — met. The §5 levels (5.1, 5.2, 5.5, 5.6) — met. Server tests seed real DB rows. AC 5.5 is satisfied on both halves: the in-suite mutation test ( §7 legend (7.1, 7.2) — met. No key added, removed or altered. AC 7.2 explicitly pinned: §4.1/4.2/4.6 — met. No string changed; no i18n key added; only trigger conditions moved. §6 — met, no new fact class crosses the PDF boundary. AC 3.6 substantively demonstrated on mobile. Scenario 17's mobile branch passes: Blocking findingsB1 — Scenario 17 and Scenario 18 are RED. Both are #1911's own AC evidence.
Both are stale assertions in the new test code, not product defects — I verified the received values are correct behaviour in each case. But AC 3.6 says "asserted on desktop, tablet, and mobile", and an assertion that fails is not an assertion that holds. Fix both, don't waive them. B1a — Scenario 17 ( The row is correct — amount, then the B1b — Scenario 18 ( The labels are right — Update the expectation to B2 — AC 4.5's measurement cannot fail on the content it claims to guard.AC 4.5 was the criterion I labelled "highest-risk — measure it", for the newly-reachable four-run row
So the specific risk AC 4.5 names — What would satisfy it, using techniques already in this file: pdfmake writes one entry into a node's
If per-run positions turn out not to be populated and only the differential half is achievable, say so and take the differential — it is still falsifiable, which is the whole point. If neither is achievable, that is a legitimate outcome too: record it as a documented deviation and AC 4.5 folds into the UAT pass with 4.3/4.4/4.7. What I will not accept is the current state, where a green check stands against an unverified criterion. Non-blocking
On the PR bodyYou asked whether it fairly represents the change. Mostly yes, and in one place notably better than usual — with one overstatement and one understatement. Honest, and worth keeping: the second (over-inclusive) defect is called out as not being in the original issue rather than folded in silently; the Overstates (one line): "AC 4.5's four-run legend row … is measured with a real, unmocked pdfmake render asserting Understates (one line): "Both fixed, plus a new regression guard added" — the guard is genuinely green, but the two scenarios themselves are red on this head. The unchecked Everything else in the body I was able to check checks out: the predicate description, the co-occurrence rationale, "no new i18n keys", and the anti-vacuity figure. UAT disposition — confirmed, with one correction to the routing#1911 still goes to UAT and does not close on merge. Nothing that shipped weakens the reason: The routing needs one correction. Design/wording rejection → reopen #1911: unchanged and still right, the label semantics are this story's deliverable. Width/wrapping rejection → I said route to the already-filed issue; that leg is stale — #1937 is closed, and none of the open follow-ups (#2011–#2014, #1950) covers the allocated column. So the corrected split is:
To clear this review: fix B1a and B1b so both scenarios are green, and make AC 4.5's assertion falsifiable per B2 (or take the documented deviation and fold 4.5 into UAT explicitly). N1 and N2 are follow-ups; N3 is a note for whoever promotes. Re-request me after and I will re-verify only the changed criteria. |
…xtures Review-round fixes on PR #2015 (#1911): - Replace the AC 4.5 geometry assertions, which could not fail. maxHorizontalRatio is vacuous on this table (every column is fixed-width and content-independent since #1929 round 4, and horizontalRatio is recorded at each line's start x), and the "all four labels present verbatim" check read the pdfmake content tree — the test's own input — so it could not observe a drop, clip or wrap. Replaced with per-cell _minWidth <= _calcWidth per the ADR-034 Deviation Log, plus a differential positions.length check against a reduced-label comparator. Both proven to fail by mutation: merging labels into an unbreakable atom breaks the first (113.64 > 75 in de), gating the split-label push breaks the second in all four cases. - Correct two stale E2E assertions that made shards 2 and 9 red. Scenario 17 asserted a substring spanning the deposit badge, which renders between the amount and the note; Scenario 18 kept the untagged-deposit expectation after the fixture was re-tagged. EUR 56.25 is correct under #1891's redirect rule and the derivation is now recorded in a comment, so the change reads as arithmetic rather than as a weakened assertion. - Switch the step-f UNION to UNION ALL and document the load-bearing DISTINCT. Adding the origin column broke the equivalence of COUNT(*) and COUNT(DISTINCT source_id), so a future "simplification" to COUNT(*) would flip isSplit for every invoice with a same-source line and a same-source tagged deposit. - Correct an inverted comment on ReportContentRow.isDepositReduced that restated, on the shared type, the same false "untagged deposits reduce this source" claim this story removes from the bank-facing legend. Refs #1911 Co-Authored-By: Claude backend-developer <noreply@anthropic.com> Co-Authored-By: Claude frontend-developer <noreply@anthropic.com> Co-Authored-By: Claude qa-integration-tester <noreply@anthropic.com> Co-Authored-By: Claude e2e-test-engineer <noreply@anthropic.com> Co-Authored-By: Claude product-architect <noreply@anthropic.com> Co-Authored-By: Claude product-owner <noreply@anthropic.com> Co-Authored-By: Claude security-engineer <noreply@anthropic.com>
…esson - Records that the Bank Report Wizard cluster is parent-less, so /epic-close never runs for it and promotion reaches main through standalone /release, which omits the UAT sections. A UAT disposition therefore cannot rely on the epic lifecycle to hold the gate, and issues in this cluster must use Refs rather than Fixes so they do not auto-close with the appearance of a validation that never ran. - Records the UAT routing for #1911: a labels rejection reopens the issue, a geometry rejection opens a new issue in the #1939 lineage. - Records that a scoped projection is the correct basis for a same-scope predicate, which is why hasOwnTaggedDeposit is not the #1911 defect shape. Refs #1911 Co-Authored-By: Claude product-owner <noreply@anthropic.com>
|
🎉 This PR is included in version 2.14.0-beta.15 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.14.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
SourceReportInvoice.splitKind: 'lines' | 'deposits' | 'both' | null, derived alongsideisSplitin the existing step-f UNION at zero extra query cost. The predicate is "this arm contains a source ≠ S" — not "this arm contains ≥2 distinct sources". The naive ≥2-distinct-sources reading returnsnullfor the headline case (a single non-S source funding the invoice) and reproduces the original bug.isDepositReducedwas also over-inclusive — it fired on a line-split invoice carrying an untagged deposit, whose legend sentence claims the deposit is claimed "separately," while untagged deposits are apportioned back into this source pro-rata. Per Report legend: reinstate the explanatory sentences for partial and deposit-reduced amounts #1965's ruling that "separately" is audit-load-bearing wording, this was a false statement in bank reports shipping today. Fixed alongsidesplitKind.budgetLines.length > 0gate. Visible consequence: an invoice with zero line contribution to the reported source, funded into it solely by a tagged deposit, now correctly shows(partial)alongside its(Deposit)badge. Ruled intended and correct by product-owner (ACs 3.6/3.7, UAT 4.7 added to Source report: add splitKind to distinguish deposit-driven splits from line splits #1911) — the server always classified that invoice as split (COUNT(DISTINCT source_id) = 2), and the pre-Source report: add splitKind to distinguish deposit-driven splits from line splits #1911(Deposit)badge itself already requiredisSplitto render, so the old gate was discarding a correct signal the client already had.Test plan
495 unit tests across 10 files pass locally.
Coverage:
sourceReportService.ts100% stmts/lines/funcs, 93.75% branch (new lines are 100% branch-covered);buildReportContent.ts100% stmts/lines/funcs, 97.54% branch.Anti-vacuity check: restored the pre-Source report: add splitKind to distinguish deposit-driven splits from line splits #1911
buildReportContent.tsfromHEADand re-ran the new/modified test suite — 9 of 70 tests genuinely fail against the old derivation, including the headline test. Confirms the new tests exercise real behavior, not tautologies.AC 4.5's four-run legend row (
(Deposit) (partial) (less deposit) (refund), reachable for the first time under the corrected derivation) is measured with a real, unmocked pdfmake render across budget-overview-7col and claim-1col layouts × en/de locales.Corrected during review. The first version of this test could not fail, and the PR body previously overstated it.
maxHorizontalRatio <= 1is vacuous on this table — since Report PDF layout breaks: usage column overflows the page, rows split across page breaks, running header clipped #1929 round 4 every column is fixed-width and content-independent, andhorizontalRatiois recorded at each line's start x, so it is a cell-origin bound rather than a content-extent check. The ADR-034 Deviation Log already recorded this, from mutation testing during the PR test(reports): ADR-034 horizontal-overflow and legend coverage (#2003, #1980) #2008 review. The companion "all four labels present verbatim" check read the pdfmake content tree — the test's own input — so it could not observe a drop, clip, or wrap either.Replaced with two assertions that were each proven to fail by mutation against the real production path, with the file restored byte-identical afterwards:
_minWidth <= _calcWidthafter a real render, the falsifiable form ADR-034 prescribes (_minWidthis pdfmake's own post-layout measurement of the widest atomTextBreakercould not break). Mutation — merging two labels into one NBSP-joined unbreakable atom — turns it red:Expected <= 75, Received 113.64inde.positions.lengthcomparison against a reduced three-label render.positionsis written by pdfmake during layout, which is exactly why it can observe a drop where.textcannot. Mutation — gating the split-label push off — turns it red in all four cases.E2E correction: the original E2E spec assumed this change was "purely additive" and needed no test updates — that assumption was wrong. A derivation change is never purely additive when the old derivation is what the existing tests encoded. Two scenarios in
reportWizardEditableContent.spec.tsasserted the old (buggy) behavior; Scenario 18 had literally encoded the over-inclusive defect as expected output (seeding an untagged deposit and assertingisDepositReduced: true). Both fixed, plus a new regression guard added for the corrected negative case.The first fix round for those two scenarios was itself wrong and made shards 2 and 9 red: Scenario 17 asserted a substring spanning the deposit badge (which renders between the amount and the note), and Scenario 18 kept the untagged-deposit expectation after the fixture had been re-tagged. Both corrected;
€56.25is the right figure under Bank report wizard follow-up: blob: CSP for preview, status chip sizing, expandable invoice rows & deposit budget source #1891's redirect rule ((200 − 50) / 200 = 0.75,depositFractionsempty because tagged rows are filtered,75 × 0.75), and the derivation is recorded in a comment so the change reads as arithmetic rather than as a weakened assertion.That arithmetic is also the strongest evidence in this PR that the AC 3.2 fix is right, in both directions: foreign-tagged, the allocation genuinely drops to €56.25, so "claimed separately" is true; untagged, the residual fraction (0.75) plus the returned fraction (0.25) sum to 1.0, so nothing is reduced at all and the pre-Source report: add splitKind to distinguish deposit-driven splits from line splits #1911
(less deposit)label was literally false to a bank recipient.Unit tests pass (95%+ coverage)
Integration tests pass
CI Quality Gates pass (typecheck, tests, build, audit)
E2E Gates pass (full matrix, required for this PR — behavioral change to E2E-covered flows)
Notes
#1911 goes to UAT rather than auto-closing on merge — product-owner ruling: mixed-label legibility in the legend (en/de) is a human judgement call no assertion can make.
Fixes #1911
Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude backend-developer noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com
Co-Authored-By: Claude product-owner noreply@anthropic.com