Harden period-close: restrict ClosingEntry to automation, preserve account scope - #2160
Conversation
…count scope Follow-up to the merged period-close work (#2149), addressing two P1 review findings that arrived after merge. Security: the ClosingEntry closed-period exemption keyed off the client-supplied ManualJournalEntryDraftDto.EntryType, letting any ledger-mutation user save a ClosingEntry draft against a closed period and bypass the manual-je.period-closed bar. Only the in-process period-close automation should produce closing entries, so the manual journal save and validate HTTP endpoints now reject client-submitted ClosingEntry drafts. Correctness: scoped closing entries (a temporary account split by symbol or financial account, e.g. broker-specific dividend income) posted to unscoped accounts because the manual-journal posting rebuild dropped the LedgerAccount symbol/financial-account identity, so they never zeroed the scoped trial-balance rows. ManualJournalEntryLineDto now carries the ledger account symbol/financial-account id (with TS mirror), the intake sets them from the projected account, and the posting rebuild restores the scoped LedgerAccount. The store already persists both columns, so this round-trips; client-entered lines leave them null (unchanged). Adds an endpoint test for the client rejection and an intake test for scoped-account preservation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PdMuw9yNjY451tw5VEordn
There was a problem hiding this comment.
Code Review
This pull request enhances the ledger system by preserving structured ledger-account identities (symbol and financial account scope) on automated journal drafts, ensuring scoped postings correctly zero out scoped balances. It also introduces validation in LedgerEndpoints.cs to reject client-submitted closing entries, restricting their creation to the governed period-close workflow. Feedback on these changes highlights potential NullReferenceException risks in the new endpoint validation logic where request.Draft.EntryType is accessed directly, which could lead to 500 Internal Server Errors if the draft is omitted in the request.
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.
| // Closing entries are the sanctioned exception to the closed-period posting bar; only the | ||
| // in-process period-close automation may produce them. Reject client-submitted ClosingEntry | ||
| // drafts so this HTTP boundary cannot be used to post to a closed period. | ||
| if (request.Draft.EntryType == ManualJournalEntryTypeDto.ClosingEntry) |
There was a problem hiding this comment.
Accessing request.Draft.EntryType directly can throw a NullReferenceException (resulting in a 500 Internal Server Error) if the client sends a request body where the Draft property is null or omitted. Use null-conditional operators to safely evaluate the entry type.
if (request?.Draft?.EntryType == ManualJournalEntryTypeDto.ClosingEntry)| return ServiceUnavailable(); | ||
| } | ||
|
|
||
| if (request.Draft.EntryType == ManualJournalEntryTypeDto.ClosingEntry) |
There was a problem hiding this comment.
Accessing request.Draft.EntryType directly can throw a NullReferenceException (resulting in a 500 Internal Server Error) if the client sends a request body where the Draft property is null or omitted. Use null-conditional operators to safely evaluate the entry type.
if (request?.Draft?.EntryType == ManualJournalEntryTypeDto.ClosingEntry)…accounting-brainstorm-o73lsw
- Null-conditional on the manual-journal endpoint ClosingEntry checks (request.Draft?.EntryType) so a missing draft body returns the normal validation path instead of a 500 (review nit). - Update the file-size baseline for the three already-oversized files the ClosingEntry contract/logic additions grew (AccountingConfiguration Dtos, AccountingConfigurationService, dashboard types.ts). - Merge latest main and refresh generated doc inventories. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PdMuw9yNjY451tw5VEordn
Summary
Follow-up to the merged period-close work (#2149), addressing two P1 review findings that landed after that PR merged.
ClosingEntryto the in-process automation. The ClosingEntry closed-period exemption keyed offManualJournalEntryDraftDto.EntryType, which is client-supplied. Any ledger-mutation user could save a manual draft withEntryType = ClosingEntryagainst a closed period, skip themanual-je.period-closedcritical validation, and post into a closed period (posting maps that type to theClosingEntryposting kind the guard now permits). Only the in-process period-close automation should produce closing entries — it calls the workbench service directly — so the client-facing manual-journal save and validate HTTP endpoints now reject client-submittedClosingEntrydrafts with a 400.SymbolorFinancialAccountId(e.g. broker-specific dividend income), the close projection carried that scope online.account, but the manual-journal posting rebuild reconstructednew LedgerAccount(name, type)— dropping symbol/financial-account. The approved closing entry then posted to unscoped accounts and did not zero the scoped trial-balance rows.ManualJournalEntryLineDtonow carriesLedgerAccountSymbol/LedgerAccountFinancialAccountId(with matching TS mirror), the intake sets them from the projected account, and the posting rebuild restores the scopedLedgerAccount. The journal-leg store already persists both columns and reconstructs them, so this round-trips; client-entered lines leave them null (unchanged behavior).Reason
Post-merge P1 review findings on #2149's period-close intake: a client-exploitable bypass of the closed-period posting bar, and loss of symbol/financial-account scope on closing entries. Restarted from the merged
mainper the follow-up-work workflow (same branch name, new PR).Testing performed
bash scripts/ci.shcompleted successfullyquality-gatepassedLocal validation:
dotnet build Meridian.sln -c Release /p:EnableWindowsTargeting=true→ 0 errors. New/affected suites green: endpoint rejection (ManualJournalEntrySaveEndpoint_RejectsClientSubmittedClosingEntry) + scoped-account preservation (Runner_PeriodCloseIntake_PreservesAccountSymbolAndFinancialAccountScope) among 170 intake/guard/projector tests; 105 contract-snapshot + accounting-endpoint tests. Docs inventories regenerated. Fullquality-gateruns on this PR.Safety review
mainmainwas performedGovernance changes
Check every governance file modified:
.github/workflows/**.github/CODEOWNERS.github/pull_request_template.mdAGENTS.mdscripts/ci.sh🤖 Generated with Claude Code
https://claude.ai/code/session_01PdMuw9yNjY451tw5VEordn
Generated by Claude Code