feat: advance data provider and accounting completion - #2283
Conversation
![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 enhances the daily mark-to-market valuation, period-close controls, and trade-fill ledger posting durability. Key changes include a WAL-backed trade-fill posting store for restart-safe replay of accepted fills, atomic hard-close persistence, and a durable remediation SLA queue in the backfill UI. Additionally, connection diagnostics have been refactored into the ProviderSdk. The review feedback suggests optimizing the DailyMarkToMarketService by merging the carrying-value validation loop into the main processing loop to avoid redundant iterations.
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.
| foreach (var key in positionKeys) | ||
| { | ||
| if (!carryingValues.TryGetValue(key, out var carryingValue) || carryingValue is null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Carrying-value source omitted requested security/account key {key.Symbol}/{key.FinancialAccountId ?? "unscoped"}."); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop iterates over all positionKeys just to validate the carryingValues dictionary. To improve efficiency, this check can be merged into the main processing loop below (starting on line 333). This would avoid iterating over the positions twice.
![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: 806ffbe092
ℹ️ 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".
| catch (Exception ex) | ||
| { | ||
| var dropped = Interlocked.Increment(ref _droppedExecutionReports); | ||
| // Preserve the per-side-effect progress object. An identical gateway replay can | ||
| // resume at the failed step without applying portfolio/session/publication twice. | ||
| _logger.LogError( | ||
| "Execution report channel full; dropped fill report for order {OrderId} ({Symbol} {FilledQuantity} @ {FillPrice}); {DroppedCount} dropped in total — ExecutionReports consumers must drain faster", | ||
| fillIncrement.OrderId, fillIncrement.Symbol, fillIncrement.FilledQuantity, fillIncrement.FillPrice, dropped); | ||
| ex, |
There was a problem hiding this comment.
When _tradeEventPublisher.Publish throws before it has accepted the fill durably, this catch runs after the paper portfolio was already mutated and PortfolioApplied was set, but before TradeEventPublished, session recording, and execution-channel publication complete. Because the exception is only logged, PlaceOrderAsync can still return success and there is no guaranteed replay for gateways that do not emit the identical report again, leaving the ledger/session/subscribers missing the fill; rethrow or persist a retry before returning.
Useful? React with 👍 / 👎.
| .Select(position => new MarkToMarketPosition( | ||
| position.Symbol, | ||
| position.Quantity, | ||
| position.CostBasis, | ||
| scope.AccountId))); |
There was a problem hiding this comment.
PositionRecord.Quantity is documented as signed, so durable snapshots can contain negative short positions. This mapping forwards the signed quantity into MarkToMarketPosition, and the valuation run later constructs DailyPortfolioPriceMark, whose constructor rejects Quantity <= 0; any scheduled valuation with an open short therefore fails generically instead of being valued or blocked with a controlled message. Please either handle short valuation semantics here or fail closed before building the mark.
Useful? React with 👍 / 👎.
| request.CompanyId, | ||
| request.EvidenceAssessments), | ||
| request.EvidenceAssessments, | ||
| BatchCorrelationId: null), |
There was a problem hiding this comment.
When an event-projected intake request supplies BatchCorrelationId, this wrapper drops it by always constructing the prepared request with null. IntakeCoreAsync uses that prepared value for both the saved TreasuryContext.BatchCorrelationId and the draft-save correlation id, so event batches cannot retain or recover their shared batch identity and fall back to per-event idempotency keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b998eaafb
ℹ️ 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".
| State = DailyValuationScheduleStateDto.Scheduled, | ||
| LastRunAtUtc = existing?.LastRunAtUtc, | ||
| LastScheduledForUtc = null, | ||
| JournalEntryId = existing?.JournalEntryId, | ||
| JournalEntryId = null, | ||
| JournalEntryIds = [], | ||
| BatchCorrelationId = null, |
There was a problem hiding this comment.
When an existing daily valuation schedule is already Running, this save path still resets it to Scheduled and clears LastScheduledForUtc/batch state. The worker claims a due schedule by persisting Running before it resolves positions and drafts; reconfiguring during that window can erase the durable claim, allow another host/tick to re-run the same NextRunAtUtc, or let the in-flight worker overwrite the operator's new configuration. Mirror the running-state conflict used for monthly schedules before saving the replacement.
Useful? React with 👍 / 👎.
| .Where(item => string.Equals( | ||
| item.TenantId, | ||
| tenantContext.TenantId, | ||
| StringComparison.OrdinalIgnoreCase)) | ||
| .Where(item => tenantContext.CompanyId is null || string.Equals( | ||
| .Where(item => string.Equals( | ||
| item.CompanyId, | ||
| tenantContext.CompanyId, | ||
| StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
For legacy tenantless workstation sessions, tenantContext.TenantId/CompanyId are null and this strict equality now filters out every tenant-owned schedule instead of preserving the prior fail-open read behavior; WorkstationTenantContext still documents that the tenant gate is write-side and that reads stay fail-open. This silently empties the monthly schedule list for those deployments, and the same strict filter was added to the daily valuation schedule list below, so keep the previous tenantContext.* is null || ... behavior unless a read endpoint explicitly requires tenant scope.
Useful? React with 👍 / 👎.
1 participant
Summary
Reason
The provider/accounting WIP needed to close operational gaps in diagnostics, canonical identity, valuation evidence, automated-journal durability, posting idempotency, and accounting-close workflows. Two concrete regressions required follow-up: the approved AAPL valuation failed
LedgerPeriodPostingGuard.ValidateSecurityMasterLineagebecause its test fixture lacked authoritative Security Master evidence, and the UI verification lane could hang indefinitely aroundUiServerstartup.Testing performed
bash scripts/ci.shcompleted successfullyquality-gatepassedFocused evidence collected on Windows/.NET 10:
dotnet build src/Meridian.Ui.Shared/Meridian.Ui.Shared.csproj -c Debug --no-restore --no-dependencies --disable-build-servers -m:1 /p:UseSharedCompilation=false /p:BuildProjectReferences=false— passed with 0 errors and 16 warnings after rebuilding current dependencies.dotnet build src/Meridian.Application/Meridian.Application.csproj -c Debug --no-restore --no-dependencies --disable-build-servers -m:1 /p:UseSharedCompilation=false /p:BuildProjectReferences=false— passed with 0 errors and 30 warnings after correcting theunpriced.Lengthcompile error.DailyValuation_AaplSecurityMasterFixture_PassesPostingGuardxUnit execution — passed, 1/1, against the realLedgerPeriodPostingGuard.UiServer_RegistersLifecycleRoutes_ForManagedShutdownexecution — still aborts under VSTest's two-minute hang detector. The test now has a 30-secondStartAsyncguard, but the current focused run blocks before that guard is reached, indicating remaining constructor/composition work rather than a proven startup fix.DailyValuation_ApprovedPostedAndRestarted_HydratesMarkedStatementsAndNavIdempotentlyexecution andbash scripts/ci.shremain pending; the all-tests assembly was not a reliable validation surface during this run because the worktree required staged dependency rebuilds and its discovery path also stalled.Safety review
mainmainwas performedGovernance changes
Check every governance file modified:
.github/workflows/**.github/CODEOWNERS.github/pull_request_template.mdAGENTS.mdscripts/ci.shGovernance-file changes require explicit human approval.
Remaining before merge
UiServerconstructor/composition hang, then rerun the exact startup test.origin/main; the checkpoint is currently 35 commits behind and 3 commits ahead.bash scripts/ci.shand wait for required GitHub Actions checks.