Finance: the bank line drives SEPA booking, no stored payment refs - #1775
peterdrier wants to merge 11 commits into
Conversation
Adds IHoldedClient.ListBankMovementsAsync / ReconcileBankMovementAsync plus HoldedBankMovementDto / HoldedReconcileDocumentRef / HoldedReconcileDocumentType, following ListLedgerEntriesAsync's conventions (GetPagedAsync, JsonNode parsing, HoldedTransientException/HoldedPermanentException mapping). Client tests only — nothing calls these yet (T2/T3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
T2) Drops sepa_payout_transfers.HoldedPaymentRefs, adds HoldedBankMovementId and ReconciledAt. Re-signs SaveSepaTransferBookingAsync, adds MarkSepaTransferReconciledAsync, reshapes the /Finance/Sepa page models (SepaPayoutTransferRow, new SepaBankMovementVm/SepaPayoutsPageVm), and updates the three Finance docs. Migration SepaTransferBankMovement generated via dotnet ef, verified with has-pending-model-changes. Minimal compile-fix edits in Service.cs/FinanceController.cs/Sepa.cshtml keep the solution building; the real bank-line-driven booking flow, job and view redesign are later tasks (T3/T4) of nobodies-collective#1185. The HoldedPaymentRefs column drop is NOT yet approved per memory/architecture/no-drops-until-prod-verified.md: no baseline entry was added, so the Web.Tests architecture suite's NoDestructiveMigrationOpsRule will fail until Peter approves the drop and that entry is added. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
…ligent-feynman-giwz70
BookSepaTransferAsync now takes the Sabadell bank movement it settles, re-validates the pairing server-side, reads the live creditor balance and what this transfer's tag already posted, pays the open purchase documents FIFO dated the bank line, journals the remainder, persists before reconciling, and audits every Holded id. A refused run writes nothing and resumes off the tag sum, so the terminal partial state is gone. Adds the sepa-bank-booking sweep (ISepaBankBooking + SepaBankBookingJob, "17 */2 * * *") that books matched lines unattended and re-checks pending reconciles, and 21 tests mapping 1:1 to the issue's acceptance criteria. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
/Finance/Sepa's Book button now posts the candidate bank movement id paired with each transfer, renders it under the button, and shows a "reconcile pending" badge and a "bank lines needing a human" panel plus a bank-feed error banner (§D3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
…#1185) Adversarial review of the bank-line booking rework found three ways a booking could still post more than the transfer, or claim a booking that did not fully happen. - A process-wide gate serialises BookSepaTransferAsync, so a Book click racing the sweep no longer reads "not booked" twice and posts twice (H1). - The tagged-lines window now runs from the file's generation day to today, so a pre-#1185 posting dated the click is counted instead of re-posted (H2), and the creditor account is filtered client-side as well as in the query (M1). - A run that cannot close the gap audits SHORT and leaves the row unbooked (H3); a row save that fails after Holded accepted the postings audits PARTIAL (M2). - Page fixes: a not-bookable row no longer swallows its bank line (L2), a stale transfer no longer blocks a newer one and says why itself (M4), and the Book button shows the paired line's date, amount and text (design D3). - Amounts are rounded to cents before they are sent (L3). A resumed booking still ends reconcile-pending — Holded's purchase list does not expose which payment settled a document — now documented as a known limit (M3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
|
The preview deployment for humans-qa is ready. 🟢 Open Preview | Open Build Logs | Open Application Logs Last updated at: 2026-09-21 03:06:55 CET |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR Surface ReportCompared Summary: 30 changed file(s) | EF migrations: 1 added file(s), max 1/1 per context Reforge Surface Score
Section Deltas
Section Size & Complexity Deltas
Rule Deltas
Corpus Size & Complexity
At head: largest class Published Write Surface14 of 48 sections publish write capability, 23 interfaces (0). Interface SurfaceNew interfaces
Added interface methods
Diff Size
New Files
EF Migrations (OK)
|
|
CI status on 013b3b5:
Generated by Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed commit 013b3b5. 1 inline finding(s) posted. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013b3b5af2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Mechanical dotnet format whitespace fix for the code-quality check; not a review round. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
…atching (#1185) F1 (claude[bot], column drop without approval) — fixed. The migration is regenerated with the EF tooling so Up() only adds HoldedBankMovementId and ReconciledAt; HoldedPaymentRefs stays on sepa_payout_transfers, mapped and documented as retained unused. Dropping it is its own PR and needs Peter's approval (no-drops-until-prod-verified, no-column-drops-for-decoupling). has-pending-model-changes reports none. Finance.md and features/sepa-payout.md say retained, not dropped. F2 (Codex, bank lines that predate their payout) — fixed. UnbookedMatches now takes the line's date and refuses a line dated before its file's generation day less two days' slack — the same slack the sweep's feed window already used. An old unreconciled line of the same amount on the same account can no longer claim a new transfer. F3 (Codex, partially reconciled bank lines) — fixed. Only a pending line can start a booking; a partial one has documents settled against it by hand already, and posting the transfer on top would pay some of them twice. PairingRefusal refuses it by name. F4 (Codex, two lines matching one transfer) — fixed. A line becomes a candidate only when it is the only line claiming that transfer; rivals go to the "needs a human" panel, and PairingRefusal refuses an ambiguous pairing so neither the button nor the sweep can post against an arbitrary line. Seven tests in SepaBankBookingTests cover the three, each failing before. Two ServiceTests fixtures seeded a file generated after the bank line that paid it; they now generate it five days earlier, which is what a real payout looks like. Review-round: 1 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f666ea6e05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…fail closed (#1185) G1 (P1, real): ListBankMovementsAsync defaulted an absent `status` to "pending" — the one value the SEPA sweep reads as "nothing tied to this line yet, bookable". A response-shape change could have turned an already-settled line into a bookable one. Absent now throws HoldedPermanentException like the other required fields, so Finance renders its bank-feed error instead of booking. An unknown *present* value needs no guard: anything but "pending" already fails closed. G2 (P2, real): GetPagedAsync parsed the page envelope outside every caller's try block, so a malformed 200 escaped the client as a raw JsonException. Finance catches only the typed Holded exceptions, so /Finance/Sepa failed the request instead of warning, and the sweep aborted. Fixed at the shared source — the paged walk itself — rather than per endpoint; five list methods had the same gap. G3 (P2, real): the sweep's pending-reconcile lookback keyed off BookedAt. A row booked days after its movement date started the next feed read *after* the line it was waiting on, so a later manual reconcile was never observed and ReconciledAt plus its audit entry stayed missing forever. Both lists now key off the existing FirstPayableDate(GeneratedAt) helper — no schema change, no new column. Tests (all four fail before, pass after): three in HoldedClientTreasuryTests (missing status, malformed body, wrong-typed envelope field) and Sweep_ReconcilePendingRow_BookedDaysAfterItsLine_StillReadsBackFarEnough in SepaBankBookingTests, whose feed stub honours the requested window. Review-round: 2 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fecd1a66d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…fields non-blank (#1185) J1 (fixed): Article 15 export (GetSepaPayoutsForUserAsync/ContributeForUserAsync/ SepaPayoutExportRow) omitted HoldedBankMovementId and ReconciledAt. Extended the export row, the repository projection, and the service mapping the same way BookedAt is already exported; extended the export test. J2 (fixed): the bank-movement parse only null-checked id/account/status, so a blank string passed, could be booked against, and would send reconcile to an invalid URL. Added ReadRequiredString (fails closed on null, empty or whitespace) alongside the existing ReadRequiredInt/ReadRequiredDecimalV2 helpers, used it for id/account/status. No other required-string reads exist in this PR's HoldedClient additions. Added a blank-id test. Review-round: 3 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 225143009d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pending rows never age out (#1185) K1 (P1, valid): the purchases-only reconcile fallback drops the journal-entry remainder of the same bank line, so Holded can leave the line `partial` — yet the code returned an Instant, stamped ReconciledAt and audited "reconciled", taking the row out of the sweep's pending re-check with the remainder unmatched. The fallback now re-reads that line on the feed and stamps only when Holded reports `reconciled`; anything else (including a re-read that throws) stays reconcile pending. K2 (P2, valid — second finding on this window): the 90-day floor applied to the pending-reconcile read too, so a reconcile-pending row older than the window was never looked at again and ReconciledAt plus its audit entry stayed missing forever. Rather than a third guard, the invariant is now structural: every reconcile-pending row's line lies inside the window read for it — the pending bound is the oldest pending row's first payable day with no floor, and the 90-day floor bounds only the matching of new transfers. Widening the read cannot widen matching: UnbookedMatches already refuses stale rows and lines dated before their file. No new column, no migration. Tests: the docs-only fallback test now pins "not stamped, audited RECONCILE PENDING", with a sibling covering the stamp when Holded does read `reconciled`; a new sweep test pins a 200-day-old pending row still being read back. Both fail on 2251430. Feature doc updated for both behaviours. Review-round budget is now spent (4 of 5); further findings go to Peter. Review-round: 4 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06727dd995
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (entryRef is { Length: > 0 } | ||
| && !entryRef.StartsWith("unconfirmed:", StringComparison.Ordinal)) | ||
| docs.Add(new HoldedReconcileDocumentRef(entryRef, HoldedReconcileDocumentType.LedgerEntry)); |
There was a problem hiding this comment.
When a mixed booking has purchase payments plus a journal remainder and PostLedgerEntryAsync returns unconfirmed:entry—the documented success path for an accepted response with no readable ID—these lines omit the journal from the reconciliation payload. If the purchase-only reconciliation succeeds, TryReconcileAsync immediately stamps ReconciledAt without checking the feed, although the line can remain partial; the sweep then stops checking it and the audit incorrectly reports it reconciled. Fresh evidence beyond the previously fixed exception fallback is this separate sentinel path: verify the movement status after this purchase-only request or leave it reconcile-pending.
AGENTS.md reference: AGENTS.md:L29-L31
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified, valid. When PostLedgerEntryAsync returns an unconfirmed: ref the journal entry is left out of the first reconcile call, so a purchases-only success stamps ReconciledAt without the feed re-read that round 4 added to the exception fallback. Same defect, second path.
Not fixed here: the unattended review-round budget is spent (4 of 5). Proposed fix for Peter: route the first attempt through the same "stamp only when the feed reads reconciled" check whenever the payload omitted a journal remainder, plus a test on the unconfirmed: path. Left open for Peter.
Generated by Claude Code
| IReadOnlyList<HoldedBankMovementDto> movements; | ||
| try | ||
| { | ||
| movements = await ReadBankFeedAsync(Today().PlusDays(-FeedWindowDays), ct); |
There was a problem hiding this comment.
For an unbooked transfer generated 89–90 days ago, a movement dated up to two days before GeneratedAt is explicitly accepted by UnbookedMatches, but this query starts exactly 90 days ago and the Holded client filters older movements out. The prior pending-row window fix does not cover these unbooked rows, so a valid 91–92-day-old line is never seen and the transfer eventually ages into manual settlement. Include GenerationSlackDays in the feed lookback, or align the staleness cutoff with the actual readable window.
AGENTS.md reference: AGENTS.md:L91-L91
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified, valid at the boundary: a transfer generated 89 or 90 days ago may have a line dated 91 or 92 days ago (the 2-day slack), which the 90-day read never sees, so the row ages into manual settlement. Two-day edge, no data harm, the documented manual path applies.
Not fixed here: review-round budget spent. Proposed fix for Peter: read the feed from Today - FeedWindowDays - GenerationSlackDays at the two matching call sites so the readable window equals the accepted window. Left open for Peter.
Generated by Claude Code
| logger.LogInformation( | ||
| "SEPA sweep: transfer {TransferId} was not booked against line {MovementId}: {Reason}", | ||
| matches[0].TransferId, m.Id, result.Message); |
There was a problem hiding this comment.
When the sweep finds a unique match but BookSepaTransferAsync refuses it—for example because the live balance or binding changed—this branch logs only at Information, which is absent from the production log viewer. The recurring automation can therefore fail every two hours without an operational signal unless an admin happens to inspect the page; log this expected problem at Warning as required by memory/code/always-log-problems.md.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified, valid per memory/code/always-log-problems.md: a sweep refusal is an expected problem and belongs at Warning, no exception object. One-line change.
Not fixed here: review-round budget spent. Left open for Peter.
Generated by Claude Code
Review-round ceiling reachedFour review-round commits spent (f666ea6, fecd1a6, 2251430, 06727dd). Codex's round on 06727dd raised three more; all verified real, none fixed, per the steward ceiling. Unsubscribing from PR activity now. Open findings (each has a disposition reply in its thread)
Decisions needed from Peter
CI is green on 06727dd's predecessor and pending on the head. Preview: http://1775.n.burn.camp Generated by Claude Code |
2 participants
What
Closes nobodies-collective#1185. SEPA payout booking is now driven by the outgoing Sabadell bank line in Holded instead of a click: a sweep job (and the re-aimed Book button) pairs each unbooked transfer with its bank movement by the remittance text
<account> - NCA - <name>, amount and date, checks the live creditor balance, registers FIFO payments on the member's open purchase docs dated the bank line, posts any remainder as one journal entry, reconciles the bank line, and stores the movement id andReconciledAt.HoldedPaymentRefsis no longer read or written; the column itself stays and its drop is a separate PR needing Peter's approval (no-column-drops-for-decoupling,no-drops-until-prod-verified).Why
The stored CSV of Holded ids overflowed at ~20 docs after Holded had already accepted the payments, leaving the row unbooked and a retry journaling the full amount twice. Retry safety now comes from tagging every posting
SEPA payout E<id>and posting only the difference between the transfer amount and what already carries the tag on the creditor account.Existing surface checked
PayPurchaseDocumentAsync,PostLedgerEntryAsync,ListLedgerEntriesAsync,ListAccountingAccountsAsync,GetPagedAsync/SendAsync. AddedListBankMovementsAsync+ReconcileBankMovementAsyncand three DTOs toHumans.Holded.Contracts(no existing wrapper covers treasury endpoints; endpoints per the Holded v2 migration design doc's live probe).AuditAction.SepaPayoutTransferBooked,SaveSepaTransferBookingAsync. AddedISepaBankBooking(one method) inHumans.Finance.Contractsfor the job. Both additions are new public surface and want Peter's nod (interface-method-additions-are-debt).UI changes / screenshots
/Finance/Sepa(admin): Book now shows the paired bank line (date, amount, text) and posts its id; unmatched or ambiguous bank movements are listed with a reason; a bank-feed error shows as a banner. No preview screenshot: the container has no Holded credentials.Checklist
section:finance+section:holded(no label rights from this session).mainonpeterdrier/Humans.origin/main.SepaTransferBankMovement: 2 AddColumn), Finance context only.NuGet packages updated?none.New project rule?none.dotnet test Humans.slnxgreen.Reviewer notes
Review round 1 (f666ea6): kept the column per the Claude bot's block; Codex's three P1 matcher findings verified and fixed (a line dated before its file's generation is refused; only
pendinglines start a booking,partialones no longer do, a deliberate tightening of the design's literal rule; two candidate lines for one transfer is ambiguous, nothing posted).Known limits, documented in
sepa-payout.md: a booking resumed after a crash cannot reconcile the earlier run's doc payments (needs a payments field on the purchase-doc DTO, new Holded surface); the matcher looks back 90 days, so an older unbooked transfer is settled by hand in Holded. Two Holded conventions are unverified without a live key: the sign of outgoing movements and whether a journal entry is a valid reconciledocument_type(fallback ladder in code).🤖 Generated with Claude Code
https://claude.ai/code/session_013SEgxi2u5mKP8RJnbuUf46