[product-owner] Follow-up from the PR #2015 review (product-architect §2b). Non-blocking. The coordinator asked me to rate this one myself rather than inherit the architect's rating — I have, and I rate it lower than the architect implied, on a framing I disagree with. Reasoning below, because the framing changes what the fix should be.
As a developer changing the deposit filter in sourceReportService.ts, I want a test to fail if my change silently removes the (Deposit) label from bank-facing reports, so that the invariant the label depends on is pinned rather than assumed.
Parent Epic: none — parent-less Bank Report Wizard cluster.
Priority: Could Have
Where I disagree with the review framing
The finding was flagged as "the same shape as the bug we just fixed" and "one filter change from being the next bug". The first half is not right, and it matters:
const hasOwnTaggedDeposit = invoice.deposits.some((d) => d.budgetSourceId === report.source.id);
The #1911 bug derived an unscoped predicate — "is this invoice split?" — from a source-scoped projection. That is impossible to do correctly, because the projection has thrown away exactly the rows the predicate needs. hasOwnTaggedDeposit derives a scoped predicate — "does this source have a tagged deposit on this invoice?" — from a projection scoped to that same source. The scopes match. That is not an accident of the filter's direction; it is the correct and intended use of a scoped projection, and it is sound by construction, not by luck.
This distinction is load-bearing for the ADR filed alongside this issue: the principle as stated ("a source-scoped projection can never be the basis for an unscoped predicate") does not condemn this line, and if the ADR is applied without its converse it will generate false positives exactly like this one. I have asked for the converse to be written into the ADR explicitly.
What is genuinely worth fixing
The real exposure is narrower but still real: nothing pins the invariant. The step-i filter keeps budgetSourceId === null || === sourceId. If a future change narrowed it to untagged-only — a plausible change, e.g. "stop echoing this source's own deposits back into the response" — then hasOwnTaggedDeposit silently returns false, the (Deposit) badge silently disappears from every constituted row, and no test fails. The failure is silent, and it lands in a document handed to a bank.
That deserves a guard test. It does not deserve a refactor: moving the predicate server-side would duplicate a fact the server already exposes correctly, for no gain.
Acceptance Criteria
Notes
Rated Could Have, not Should: the defect is latent, not live; the predicate is correct today for a principled reason rather than an accidental one; and the fix is one test plus one comment. I would not hold a release for it, but it is cheap and the failure mode is silent.
This is the "comment keeps the rationale, issue owns the guard" split: the invariant is bounded and quantified, so it earns a tracked test rather than documentation alone.
Related: #1911, and the ADR issue filed alongside this one.
[product-owner] Follow-up from the PR #2015 review (product-architect §2b). Non-blocking. The coordinator asked me to rate this one myself rather than inherit the architect's rating — I have, and I rate it lower than the architect implied, on a framing I disagree with. Reasoning below, because the framing changes what the fix should be.
As a developer changing the deposit filter in
sourceReportService.ts, I want a test to fail if my change silently removes the(Deposit)label from bank-facing reports, so that the invariant the label depends on is pinned rather than assumed.Parent Epic: none — parent-less Bank Report Wizard cluster.
Priority: Could Have
Where I disagree with the review framing
The finding was flagged as "the same shape as the bug we just fixed" and "one filter change from being the next bug". The first half is not right, and it matters:
The #1911 bug derived an unscoped predicate — "is this invoice split?" — from a source-scoped projection. That is impossible to do correctly, because the projection has thrown away exactly the rows the predicate needs.
hasOwnTaggedDepositderives a scoped predicate — "does this source have a tagged deposit on this invoice?" — from a projection scoped to that same source. The scopes match. That is not an accident of the filter's direction; it is the correct and intended use of a scoped projection, and it is sound by construction, not by luck.This distinction is load-bearing for the ADR filed alongside this issue: the principle as stated ("a source-scoped projection can never be the basis for an unscoped predicate") does not condemn this line, and if the ADR is applied without its converse it will generate false positives exactly like this one. I have asked for the converse to be written into the ADR explicitly.
What is genuinely worth fixing
The real exposure is narrower but still real: nothing pins the invariant. The step-i filter keeps
budgetSourceId === null || === sourceId. If a future change narrowed it to untagged-only — a plausible change, e.g. "stop echoing this source's own deposits back into the response" — thenhasOwnTaggedDepositsilently returnsfalse, the(Deposit)badge silently disappears from every constituted row, and no test fails. The failure is silent, and it lands in a document handed to a bank.That deserves a guard test. It does not deserve a refactor: moving the predicate server-side would duplicate a fact the server already exposes correctly, for no gain.
Acceptance Criteria
hasOwnTaggedDeposit: given a report whose invoice carries a deposit tagged to the reported source, the response's deposit array contains that deposit and the built row hasisDeposit === true. Named and commented so the coupling is the stated subject, not a side effect.buildReportContent.ts:173states the invariant in one line — this predicate is sound because the step-i filter's scope and this predicate's scope are the same source; narrowing that filter breaks this label — with a pointer to the guard test.Notes
Rated Could Have, not Should: the defect is latent, not live; the predicate is correct today for a principled reason rather than an accidental one; and the fix is one test plus one comment. I would not hold a release for it, but it is cheap and the failure mode is silent.
This is the "comment keeps the rationale, issue owns the guard" split: the invariant is bounded and quantified, so it earns a tracked test rather than documentation alone.
Related: #1911, and the ADR issue filed alongside this one.