feat: release candidate of balance overdraft feature (v3.7.0)#2088
Merged
Conversation
chore: sync main to develop [skip ci]
chore: sync main to develop [skip ci]
Adopt the org-wide SECURITY.md template with: - GitHub Security Advisory as preferred reporting channel - Response timeline table (24h ack → 90d resolution) - Clear scope (in/out) with dependency nuance - Supported versions table - Secrets management guidance for dev and production - Standardized contact section Part of the SECURITY.md standardization initiative across all LerianStudio public repositories. Requested-by: @jota-music
…cluding status, legal document, name, and alias ✨
…ion for text search ✨
…tion errors for ledgers and organizations 🔨
chore: sync main to develop [skip ci]
…ses with new b-tree indexes ✨
…tion errors for accounts in PostgreSQL repository 🔨
…lters-1 feat: implement onboarding list endpoint filters
- bump remaining @v1.26.1 refs (s3-upload, api-dog-e2e-tests) to v1.26.3 - add top-level permissions block for pr-security-scan caller
…s-v1.26.3 chore(workflows): bump shared-workflows to v1.26.3
…, entity ID, blocked status, and parent account ID ✨
…tion and implement new status filtering for portfolios 🔨
… services and update validation error messages 🔨
…enhance lifecycle management ✨
…e server readiness management 🔨
feat(ledger): add readyz implementation and related tests
chore: bump lib-commons to v5.1.0 and validate list status filters
chore: update workflow and test tooling dependencies
- Remove PGP key claim (no key actually exists) - Fix .env.example reference (files don't exist in repo) Requested-by: @claraslilith
chore: standardize SECURITY.md
feat: release candidate of balance overdraft feature
WalkthroughThis pull request upgrades the lib-commons dependency from v4 to v5 across CRM and ledger components, updates GitHub Actions workflows to use v1.27.5, transitions the project license from Apache 2.0 to Elastic License 2.0, introduces comprehensive overdraft support with transaction enrichment, implements a new readiness health-check endpoint with dependency validators and TLS enforcement policies, and updates API documentation to reflect new filtering capabilities and schema changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Transaction Handler
participant Enricher as Overdraft Enricher
participant CompLoader as Companion Loader
participant TxCore as Transaction Core
participant DB as Database
Client->>Handler: POST /transactions (with overdraft debit)
Handler->>Handler: Validate input & load balances
Handler->>Enricher: enrichOverdraftOperations()
Enricher->>Enricher: Detect debit overflow on<br/>credit balance
Enricher->>CompLoader: Load overdraft companion<br/>balance
CompLoader-->>Enricher: Companion balance
Enricher->>Enricher: Create synthetic overdraft<br/>operation (debit direction)
Enricher-->>Handler: companionFromTos list
Handler->>TxCore: BuildOperations with<br/>companion operations
TxCore->>TxCore: Calculate snapshots &<br/>overdraft used per-op
TxCore-->>Handler: Operations including<br/>companion operations
Handler->>DB: Persist transaction &<br/>all operations
DB-->>Handler: Success
Handler-->>Client: 201 Created with<br/>source/destination filtered
sequenceDiagram
participant Client
participant Readyz as Readyz Handler
participant Checkers as Dependency Checkers
participant Mongo as MongoDB
participant Metrics as Metrics Registry
Client->>Readyz: GET /readyz
Readyz->>Readyz: Check server lifecycle<br/>(ready/draining)
alt Not ready or draining
Readyz-->>Client: 503 Unavailable
else Ready
Readyz->>Checkers: Run all checks with<br/>timeout
Checkers->>Mongo: Ping()
Mongo-->>Checkers: Latency & status
Checkers-->>Readyz: DependencyCheck[]
Readyz->>Metrics: Record check duration<br/>& status per checker
Readyz->>Readyz: Aggregate status<br/>(all up vs unhealthy)
alt All up
Readyz-->>Client: 200 OK, status=healthy
else Any down
Readyz-->>Client: 503 Unavailable,<br/>status=unhealthy
end
end
|
Contributor
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Pre-release Version Check
✅ No unstable version pins found.
Contributor
🔒 Security Scan Results —
|
| Policy | Status |
|---|---|
| Default non-root user | ✅ Passed |
| No fixable critical/high CVEs | ✅ Passed |
| No high-profile vulnerabilities | ✅ Passed |
| No AGPL v3 licenses | ✅ Passed |
Pre-release Version Check
✅ No unstable version pins found.
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 87.7% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/midaz/v3/components/crm/internal/adapters/http/in |
86.2% |
github.com/LerianStudio/midaz/v3/components/crm/internal/adapters/mongodb/alias |
92.1% |
github.com/LerianStudio/midaz/v3/components/crm/internal/adapters/mongodb/holder |
87.1% |
github.com/LerianStudio/midaz/v3/components/crm/internal/services |
95.2% |
Generated by Go PR Analysis workflow
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 85.3% ✅ PASS |
| Threshold | 85% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/http/in |
85.7% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/mongodb/onboarding |
66.7% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/mongodb/transaction |
66.7% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/account |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/accounttype |
66.7% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/asset |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/assetrate |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/balance |
97.5% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/ledger |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/operation |
89.9% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/operationroute |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/organization |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/portfolio |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/segment |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/transaction |
94.9% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/postgres/transactionroute |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/rabbitmq |
90.6% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/adapters/redis/transaction/balance |
100.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/services/command |
87.0% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/services/query |
93.1% |
github.com/LerianStudio/midaz/v3/components/ledger/internal/services |
0.0% |
Generated by Go PR Analysis workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
components/ledger/internal/adapters/postgres/ledger/ledger.postgresql_integration_test.go (1)
143-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a fixed timestamp in
defaultQueryHeader.This helper now makes every
FindAlltest depend on wall-clock time. Please anchor the range to a fixed instant so the suite stays deterministic and aligned with the repo’s test rules.Suggested fix
+var fixedQueryTime = time.Date(2026, 4, 30, 12, 0, 0, 0, time.UTC) + // defaultQueryHeader returns a QueryHeader with valid date range for tests. func defaultQueryHeader(page, limit int) http.QueryHeader { return http.QueryHeader{ Page: page, Limit: limit, - StartDate: time.Now().AddDate(-1, 0, 0), // 1 year ago - EndDate: time.Now().AddDate(0, 0, 1), // tomorrow + StartDate: fixedQueryTime.AddDate(-1, 0, 0), + EndDate: fixedQueryTime.AddDate(0, 0, 1), } }As per coding guidelines "Do not use
time.Now()in tests; use fixed time utilities instead".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/ledger/ledger.postgresql_integration_test.go` around lines 143 - 150, The helper defaultQueryHeader currently uses time.Now(), making tests time-dependent; change defaultQueryHeader to use a fixed, deterministic timestamp (e.g., a package-level constant or a call to a test time helper) for StartDate and EndDate so FindAll tests are stable—update defaultQueryHeader (and add a fixed time constant or use the repo's fixed time utility) to return StartDate and EndDate based on that fixed instant rather than time.Now().components/crm/internal/services/create-holder.go (1)
35-35: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse structured log fields instead of
fmt.Sprintfin error logs.
Line 35 and Line 59 should logerrvia structured fields to preserve machine-parsable logs.Proposed fix
import ( "context" - "fmt" "time" @@ - logger.Log(ctx, libLog.LevelError, fmt.Sprintf("Failed to generate holder id: %v", err)) + logger.Log(ctx, libLog.LevelError, "Failed to generate holder id", libLog.Err(err)) @@ - logger.Log(ctx, libLog.LevelError, fmt.Sprintf("Failed to create holder: %v", err)) + logger.Log(ctx, libLog.LevelError, "Failed to create holder", libLog.Err(err))As per coding guidelines "Use structured logging with
libLog.Err(),libLog.String(),libLog.Int()instead offmt.Sprintfinside log calls".Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/crm/internal/services/create-holder.go` at line 35, Replace the fmt.Sprintf-style error messages in the logger.Log calls with structured log fields: pass libLog.Err(err) plus a short string field (e.g., libLog.String("msg","Failed to generate holder id")) and any relevant context (e.g., libLog.String("holder", holderID) or libLog.Int(...) as applicable) to logger.Log(ctx, libLog.LevelError, ...). Do this for the call that currently uses fmt.Sprintf("Failed to generate holder id: %v", err) and the other occurrence around the same area (the logger.Log call at the second spot) so both use libLog.Err(err) and appropriate libLog.String()/libLog.Int() fields instead of fmt.Sprintf.components/ledger/internal/adapters/postgres/portfolio/portfolio.postgresql_integration_test.go (1)
358-390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse fixed timestamps in integration test filters.
These query windows are time-dependent and can drift between runs. Replace
time.Now()with a fixed test timestamp fixture to keep pagination/date-range assertions deterministic.Suggested deterministic fixture update
+var fixedQueryTime = time.Date(2026, time.January, 15, 12, 0, 0, 0, time.UTC) + page1Filter := http.QueryHeader{ Limit: 2, Page: 1, SortOrder: "DESC", - StartDate: time.Now().AddDate(-1, 0, 0), - EndDate: time.Now().AddDate(0, 0, 1), + StartDate: fixedQueryTime.AddDate(-1, 0, 0), + EndDate: fixedQueryTime.AddDate(0, 0, 1), } func defaultQueryHeader() http.QueryHeader { return http.QueryHeader{ Limit: 10, Page: 1, SortOrder: "DESC", - StartDate: time.Now().AddDate(-1, 0, 0), - EndDate: time.Now().AddDate(0, 0, 1), + StartDate: fixedQueryTime.AddDate(-1, 0, 0), + EndDate: fixedQueryTime.AddDate(0, 0, 1), } }As per coding guidelines:
**/*test.go: Do NOT usetime.Now()in tests; use fixed time utilities instead.Also applies to: 652-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/portfolio/portfolio.postgresql_integration_test.go` around lines 358 - 390, The test uses time.Now() inside the pagination/date-range filters (page1Filter, page2Filter, page3Filter passed to repo.FindAll) which makes assertions flaky; replace those calls with a fixed test timestamp fixture (e.g., a package-level testTime or a fixture like fixtures.FixedTime) and use deterministic offsets from that fixture (testTime.AddDate(...)) for StartDate/EndDate throughout the file (also update the other occurrences referenced in the comment) so the pagination/date-range assertions are stable across runs.components/ledger/internal/adapters/postgres/accounttype/accounttype.postgresql_integration_test.go (1)
427-445:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace dynamic clock usage in test query windows.
time.Now()in theseQueryHeaderfilters can make boundary-sensitive assertions flaky. Use a fixed test timestamp fixture.Suggested deterministic fixture update
+var fixedQueryTime = time.Date(2026, time.January, 15, 12, 0, 0, 0, time.UTC) + limitFilter := http.QueryHeader{ Limit: 2, SortOrder: "DESC", - StartDate: time.Now().AddDate(-1, 0, 0), - EndDate: time.Now().AddDate(0, 0, 1), + StartDate: fixedQueryTime.AddDate(-1, 0, 0), + EndDate: fixedQueryTime.AddDate(0, 0, 1), } func defaultQueryHeader() http.QueryHeader { return http.QueryHeader{ Limit: 10, SortOrder: "DESC", - StartDate: time.Now().AddDate(-1, 0, 0), - EndDate: time.Now().AddDate(0, 0, 1), + StartDate: fixedQueryTime.AddDate(-1, 0, 0), + EndDate: fixedQueryTime.AddDate(0, 0, 1), } }As per coding guidelines:
**/*test.go: Do NOT usetime.Now()in tests; use fixed time utilities instead.Also applies to: 471-487, 704-710
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/accounttype/accounttype.postgresql_integration_test.go` around lines 427 - 445, Tests use time.Now() in QueryHeader filters (limitFilter, allFilter passed to repo.FindAll), which makes boundary assertions flaky; replace dynamic clock usage with a fixed test timestamp fixture (e.g., a package-level or helper const like testNow or fixedTime from your test fixtures) and compute StartDate/EndDate relative to that fixed timestamp. Update all occurrences noted (including the other blocks around lines referenced) so http.QueryHeader uses the deterministic fixture instead of time.Now(), keeping SortOrder/Limit values unchanged.components/ledger/internal/adapters/postgres/organization/organization.postgresql_integration_test.go (1)
447-454:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a fixed timestamp in the test query helper.
defaultQueryHeadernow bakestime.Now()into everyFindAlltest, which makes the new date-filter coverage time-dependent. Please switch this helper to a fixed instant so pagination/filter failures are reproducible.Suggested change
+var fixedQueryNow = time.Date(2026, 1, 15, 12, 0, 0, 0, time.UTC) + // defaultQueryHeader returns a QueryHeader with valid date range for tests. func defaultQueryHeader(page, limit int) http.QueryHeader { return http.QueryHeader{ Page: page, Limit: limit, - StartDate: time.Now().AddDate(-1, 0, 0), // 1 year ago - EndDate: time.Now().AddDate(0, 0, 1), // tomorrow + StartDate: fixedQueryNow.AddDate(-1, 0, 0), + EndDate: fixedQueryNow.AddDate(0, 0, 1), } }As per coding guidelines
**/*test.go: Do NOT use time.Now() in tests; use fixed time utilities instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/organization/organization.postgresql_integration_test.go` around lines 447 - 454, The test helper defaultQueryHeader embeds time.Now(), making FindAll tests time-dependent; change defaultQueryHeader to return a QueryHeader with fixed StartDate and EndDate (use a deterministic time.Date value or a shared fixedClock constant used across tests) instead of time.Now(), so all pagination/filter tests are reproducible; update any tests that rely on the current helper to use the fixed instant or adjust expectations accordingly (refer to defaultQueryHeader and any FindAll test callers to locate changes).components/ledger/internal/adapters/postgres/account/account.postgresql_integration_test.go (1)
377-383:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse fixed timestamps in integration tests instead of
time.Now().Line 381 and Line 382 (and repeated new filter setups in this file) use wall-clock time. Replace with fixed test timestamps so filter tests remain fully deterministic.
As per coding guidelines
**/*test.go: Do not usetime.Now()in tests; use fixed time utilities instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/account/account.postgresql_integration_test.go` around lines 377 - 383, The test uses wall-clock times when constructing the filter variable (http.QueryHeader named "filter") via time.Now().Add(...); replace these with fixed deterministic timestamps (e.g., time.Date(..., time.UTC) or your test fixed-time helper) for StartDate and EndDate so the filter setup and any repeated filter constructions in this file are deterministic; update every occurrence that uses time.Now().Add(...) in the test to use the chosen fixed timestamp values (and keep them in UTC to avoid timezone flakiness).components/ledger/internal/adapters/postgres/account/account.postgresql.go (1)
233-314: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueCyclomatic complexity is elevated but acceptable.
Static analysis flags this method with complexity 22 (threshold 18). The complexity stems from the many optional filter conditions, each following an identical and readable pattern. While the metric exceeds the threshold, the code remains maintainable because each filter block is self-contained and uniform. If additional filters are added in the future, consider extracting filter application into a helper function or builder pattern to keep complexity manageable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/internal/adapters/postgres/account/account.postgresql.go` around lines 233 - 314, The FindAll method in AccountPostgreSQLRepository has elevated cyclomatic complexity due to many inline optional filter blocks; refactor by extracting the repeated filter-application logic into a helper (e.g., buildAccountFilters or applyAccountFilters) that accepts the squirrel.SelectBuilder, the http.QueryHeader filter and returns the updated builder; move all blocks that add WHERE clauses (EntityIDs, Status, Type, AssetCode, EntityID, Blocked, ParentAccountID, Name, Alias, date range, sort/limit/offset) into that helper and call it from FindAll to keep FindAll focused on setup/teardown and reduce complexity.components/ledger/api/openapi.yaml (1)
7332-7339:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the non-negative constraint from overdraft-capable
availablefields.These schemas still declare
available.minimum: 0, butallowOverdraftnow explicitly allows transactions to driveavailablebelow zero. Any SDK/runtime validator generated from this spec will treat valid overdraft balances as invalid.Also applies to: 7426-7433, 10209-10216, 10323-10329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/openapi.yaml` around lines 7332 - 7339, The OpenAPI schemas declare available.minimum: 0 even for overdraft-capable balances, which conflicts with allowOverdraft allowing negative available; remove the non-negative constraint by deleting the minimum: 0 (and any duplicate minimum/type mismatch entries) from the affected available field definitions so the available property can be negative when allowOverdraft is true; search for the schema property named available and the flag allowOverdraft to locate each occurrence (including the additional occurrences noted in the comment) and remove the minimum: 0 constraint there.components/ledger/api/docs.go (1)
8360-8402:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
refundis documented in prose but missing from the schema.
AccountingEntriesnow says it supportsrefund, but this definition only exposescancel,commit,direct,hold,overdraft, andrevert. That leaves the generated contract internally inconsistent and any generated client will missrefundentirely. Please update the OpenAPI source/annotations so the description and properties match, then regenerate this file.Based on learnings,
components/*/api/docs.gois auto-generated Swagger/OpenAPI output; apply the fix in the source spec/annotations and regenerate rather than editingdocs.gomanually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/docs.go` around lines 8360 - 8402, The OpenAPI schema for AccountingEntries is missing the refund property even though the prose mentions it; update the source OpenAPI/spec or code annotations that define AccountingEntries to add a "refund" property with the same allOf/$ref to AccountingEntry (matching cancel/commit/direct/hold/revert/overdraft), then regenerate the Swagger artifacts so components/ledger/api/docs.go includes refund in the properties; ensure the change is applied to the original spec/annotations (not docs.go) and run the project’s OpenAPI generator to produce the updated docs file.components/ledger/api/swagger.json (2)
8336-8378:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
refundentry toAccountingEntries.The description now says this object covers
refund, but the schema never defines arefundproperty. That leaves the contract inconsistent and prevents generated clients from expressing the new refund accounting entry.Suggested fix
"overdraft": { "description": "The accounting entry for the overdraft lifecycle. Debit rubric classifies\noverdraft usage (deficit grows); credit rubric classifies overdraft\nrepayment (deficit shrinks). Both rubrics are REQUIRED when this entry\nis present.", "allOf": [ { "$ref": "#/definitions/AccountingEntry" } ] }, + "refund": { + "description": "The accounting entry for the refund action.", + "allOf": [ + { + "$ref": "#/definitions/AccountingEntry" + } + ] + }, "revert": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 8336 - 8378, The AccountingEntries schema lists refund in its description but lacks a refund property; add a "refund" property to the AccountingEntries object (alongside cancel/commit/direct/hold/overdraft) with a description like "The accounting entry for the refund action." and an allOf reference to "#/definitions/AccountingEntry" so generated clients can represent the refund accounting entry; mirror the pattern used by the other entries (e.g., "direct" or "commit") when adding the refund property.
8894-8929:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t expose internal-only balance settings in public request schemas.
CreateAdditionalBalanceandUpdateBalanceboth reusemmodel.BalanceSettings, whose contract explicitly allowsbalanceScope: "internal", and the create schema still permits the reserved balance keyoverdraft. That means the OpenAPI spec allows requests the runtime guards already reject.Please split request/response balance-settings schemas, or otherwise constrain the request-side contract so public input cannot send
balanceScope: "internal"and cannot createkey: "overdraft". Based on learnings: Balance API requests must prevent clients from settingbalanceScopeto"internal"and must forbid using the reservedbalancekey"overdraft". This is already enforced by runtime guards; ensure the API contract/docs (e.g., OpenAPI/Swagger schemas such as in swagger.json) also disallow these values so client code can’t legally send them and reviews don’t miss schema/contract mismatches.Also applies to: 10613-10620, 11013-11038
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ledger/api/swagger.json` around lines 8894 - 8929, The CreateAdditionalBalance and UpdateBalance request schemas currently reuse mmodel.BalanceSettings which permits balanceScope: "internal" and also allow the reserved key "overdraft"; update the OpenAPI schemas so client-facing request contracts cannot set those internal-only values: create separate request-side schema(s) (e.g., BalanceSettingsRequest) or add validation constraints/enums to the existing request properties to forbid balanceScope = "internal" and add a pattern/enum or forbidden-value rule for the key property to disallow "overdraft"; ensure these changes are applied to the CreateAdditionalBalance and UpdateBalance definitions (and the other occurrences mentioned) so the public spec no longer permits internal-only settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 16: Replace the mutable tag ref in the uses entry
"LerianStudio/github-actions-shared-workflows/.github/workflows/build.yml@v1.27.5"
with the immutable commit SHA "268d75a43e5339819131d1309d0f39862a8cb7af" (keep
the human-readable tag as a trailing comment if desired), and apply the same
replacement pattern for all other workflow uses refs that point to the same
shared repo (e.g., the occurrences referenced in release.yml,
release-notification.yml, pr-validation.yml, pr-security-scan.yml,
gptchangelog.yml, go-combined-analysis.yml) so CI pins to an immutable commit
SHA rather than a mutable tag.
In `@components/crm/internal/bootstrap/readyz_checkers.go`:
- Around line 66-77: MongoChecker.Check currently calls c.client.Ping(ctx)
without verifying the context; add a fast pre-check for ctx.Err() at the start
of Check to short-circuit and return a DependencyCheck with StatusFailed (or
appropriate status) and the ctx error as Reason when the context is canceled or
deadline exceeded, before performing any I/O; update the logic around
c.client.Ping(ctx), latencyMs calculation, and returned DependencyCheck so the
function returns immediately on ctx.Err() and only measures ping latency when
the context is valid.
- Around line 79-84: The readiness checker currently returns the raw driver
error in DependencyCheck.Error (the err value) which can leak infra details;
change the returned Error to a sanitized message (e.g., "ping failed") while
emitting the full err to internal logs/telemetry instead. Update the code around
the return that builds DependencyCheck (the block using StatusDown, latencyMs,
Error) to set Error to a generic string and call the existing logger (or
standard logger if none) to log fmt.Errorf("mongo ping failed: %v", err) or
similar for internal diagnostics; keep latencyMs and StatusDown as-is. Ensure
you reference the same err variable and DependencyCheck construction so only the
response payload is sanitized.
In `@components/ledger/api/docs.go`:
- Around line 2716-2718: The OpenAPI 404 response for the POST
/accounts/{account_id}/balances endpoint is incorrect in the generated file
(components/.../api/docs.go) — it should reflect a missing parent resource
(account/ledger/organization) instead of "Balance not found"; update the source
OpenAPI annotations/spec for the handler that creates balances (the POST
accounts/{account_id}/balances operation) to change the 404 description to
something like "Account not found" or "Parent resource not found" and ensure the
404 is tied to the account/ledger/org lookup, then regenerate the
Swagger/OpenAPI output so components/*/api/docs.go is updated (do not edit
docs.go directly).
- Around line 11046-11049: The schema for balanceScope is currently a free-form
string; update the OpenAPI source/annotations that define the balance scope enum
(the balanceScope property in the balance/account schema) to declare an enum
with values ["transactional","internal"], set the default to "transactional"
(and document the empty-string backward-compatibility in description if
desired), then regenerate the OpenAPI/Swagger artifacts rather than editing the
generated components/*/api/docs.go; ensure the generated schema shows type:
string, enum: [...], and default: "transactional".
In `@components/ledger/api/openapi.yaml`:
- Around line 3826-3846: The operation-route detail endpoint is missing some
standard error responses; restore the dropped 403 on DELETE
/operation-routes/{operation_route_id} and add the same error response set for
GET /operation-routes/{operation_route_id} so it matches other resource-by-id
routes: include 401, 403, 404, and 500 with application/json content referring
to the existing components/schemas/Error for each response code (use the same
response shape as other id endpoints).
- Around line 5045-5053: The PATCH requestBody for the ledger-settings endpoint
is currently typed as a generic object; change the OpenAPI schema to a typed
object with an "accounting" property that is an object containing boolean
properties "validateAccountType" and "validateRoutes", and set
additionalProperties: false so unknown fields are rejected; because
components/*/api/openapi.yaml is generated by Swaggo, make this change by
updating the source Swaggo annotations or the model/struct that defines ledger
settings (or add a deterministic post-generation transformer) so the generated
schema for the ledger-settings PATCH accurately documents
accounting.validateAccountType and accounting.validateRoutes as booleans and
disallows unknown fields.
- Around line 87-91: The OpenAPI query parameter "status" is currently an
unconstrained string and must be replaced with an enum listing the exact allowed
values for that endpoint's entity; consult the canonical lists in
components/ledger/internal/adapters/http/in/status_validation.go and replace the
generic schema (type: string) with schema.enum containing the per-entity allowed
statuses for the "status" query param used at the shown location and the other
occurrences noted (lines 485-489, 1321-1325, 4024-4028). Ensure each endpoint's
"status" parameter uses the matching enum from status_validation.go so the spec
and adapter validation stay in sync.
In `@components/ledger/api/swagger.json`:
- Around line 6063-6110: The PATCH operation for ledger settings currently uses
a loose "type: object" for the body parameter named "settings" (the "patch"
operation); replace that with a concrete schema that models the allowed, closed
fields: define "settings" as an object with an optional "accounting" object that
has two boolean properties "validateAccountType" and "validateRoutes", set
additionalProperties: false on both the root settings object and the accounting
object to reject unknown fields, and ensure the boolean types are explicit so
generated SDKs and docs match the server's 0147/0148 validation behavior.
In `@components/ledger/internal/adapters/http/in/account.go`:
- Around line 248-255: The handlers GetAccountByID, UpdateAccount, and
DeleteAccountByID are still retrieving the UUID using http.GetUUIDFromLocals(c,
"id") while the route annotations and locals were renamed to "account_id";
update each handler to read http.GetUUIDFromLocals(c, "account_id") (and any
other occurrences noted around the blocks at ~273, ~413-422, ~441, ~484-491,
~510) so the implementation matches the documented route local, and update
corresponding tests/routes to use the "account_id" local key as well.
In
`@components/ledger/internal/adapters/http/in/operation_route_validation_test.go`:
- Line 821: Remove the redundant loop-variable rebinding by deleting the
unnecessary "tt := tt" inside the test's range loop; locate the table-driven
test where the variable named "tt" is used (the inner closure using tt) and
simply use the loop-provided tt directly so each iteration's closure captures
the iteration copy without the extra reassignment.
In `@components/ledger/internal/adapters/http/in/portfolio_test.go`:
- Around line 641-643: The test currently uses gomock.Any() for the 4th argument
of PortfolioRepo.FindAll which lets the call pass without verifying
metadata-derived IDs are included; replace those gomock.Any() matchers (the
expectation around PortfolioRepo.FindAll and the similar one at 718-720) with a
concrete matcher that asserts the filter contains the expected IDs (e.g., build
an expected filter value with the metadata-derived IDs and use
gomock.Eq(expectedFilter) or a custom gomock.Matches function to check the IDs
are present) so the mock verifies the repository receives the filtered IDs from
metadata.
In `@components/ledger/internal/adapters/http/in/segment.go`:
- Around line 196-202: Swagger comments for the three endpoints reference
{segment_id} but the actual routes and handlers use :id and extract via
GetUUIDFromLocals(c, "id"); update the Swagger annotations in the segment.go
comments for GetSegmentByID, UpdateSegment, and DeleteSegmentByID to use {id}
(instead of {segment_id}) so they match the route parameter name and the handler
extraction, keeping the route registrations and handler code unchanged.
In
`@components/ledger/internal/adapters/http/in/transaction_create_snapshot_test.go`:
- Around line 287-306: The test is reimplementing the adapter/writeback bridge
instead of using the production path; replace the inline loop that builds
snapshotAdapters and calls propagateSnapshotToCompanions with a call to the
production bridge (e.g., invoke BuildOperations so it performs the same wiring)
or extract the bridge logic into a shared helper used by both BuildOperations
and the test (the helper should encapsulate the operationForSnapshot assembly
and propagateSnapshotToCompanions invocation) and have both the production code
and tests call that helper so the test no longer duplicates the wiring; apply
the same change to the other occurrence around lines 377-389.
In
`@components/ledger/internal/adapters/http/in/transaction_overdraft_enrichment.go`:
- Around line 823-854: The code currently reuses usageByAlias[alias] for every
cancelled RELEASE/CREDIT leg, allowing sum of overdraft repayments to exceed the
outstanding; change the logic so each time you apply an overdraft to a balance
operation (inside the loop over balanceOps in
transaction_overdraft_enrichment.go), compute applied := min(usage, the
cancelable amount for that balance op), set balanceOps[i].Amount.OverdraftAmount
= applied, then decrement usageByAlias[alias] = usage.Sub(applied) (or remove
the map entry when zero) so subsequent legs consume the remaining outstanding;
reference pendingOverdraftUsageByAlias, balanceOps[i].Amount,
accountAliasFromOperationAlias and collectOverdraftCancelSplits when
implementing the min/apply-and-decrement behavior.
In
`@components/ledger/internal/adapters/postgres/balance/balance_overdraft_integration_test.go`:
- Line 36: Replace uses of time.Now().Truncate(time.Microsecond) in the test
(occurrences in balance_overdraft_integration_test.go around the variable named
now used in the test setup and assertions) with a deterministic fixed time
value: define a package-level variable like testFixedTime (e.g., a specific
time.Date value) and use testFixedTime.Truncate(time.Microsecond) wherever now
is currently set (including the instances used at lines noted in the review).
Update any comparisons or expectations that rely on now to use this
testFixedTime so the tests are deterministic.
In `@components/ledger/internal/adapters/postgres/balance/balance.go`:
- Around line 63-69: The current code swallows json.Marshal errors for
Balance.Settings (setting b.Settings = nil on failure), which incorrectly treats
malformed internal balances as non-internal; change the logic so
json.Marshal(balance.Settings) errors are not ignored: call
json.Marshal(balance.Settings) and if it returns an error, propagate/return a
technical error from the enclosing function instead of setting b.Settings to
nil, and only assign b.Settings when marshal succeeds; apply the same fix to the
duplicate marshal block later that also marshals balance.Settings.
In `@components/ledger/internal/adapters/postgres/balance/balance.postgresql.go`:
- Around line 54-56: The timestamp-based read path in this file still returns a
legacy projection missing the Settings field resulting in historical balances
having Settings == nil; update the timestamp-based read query/projection to
include "settings" alongside "direction" and "overdraft_used" so historical rows
populate mmodel.Balance.Settings, and ensure downstream interpretation follows
the rule "use mmodel.Balance.Settings as the canonical indicator" (i.e., treat a
nil Balance.Settings as NOT internal). Locate the timestamp-based read
function/projection in this file (the code that constructs the
historical/at-timestamp balance object) and add the settings field to the
returned shape and/or mapping logic, and verify any mapping code sets
mmodel.Balance.Settings to nil only when no settings exist so downstream logic
treats nil as non-internal.
- Around line 1531-1541: The current code in balance.postgresql.go parses
balance.OverdraftUsed into overdraftUsed and, on parse error, calls log.Printf
with the sensitive OverdraftUsed value; replace that global logger call with the
request-scoped structured logger (libLog) and do not include the malformed
financial value. Specifically, remove the log.Printf(...) and instead emit a
structured warning via libLog (e.g., libLog.Warn or libLog.With(...).Warn) that
includes the balance ID (ids[i]) and the parseErr message only, omitting
balance.OverdraftUsed, then continue as before so the sync skips the row. Ensure
the change targets the block around overdraftUsed, parseErr and replaces only
the logging call.
In `@components/ledger/internal/adapters/postgres/ledger/ledger.postgresql.go`:
- Around line 283-295: The ORDER BY clause currently interpolates
pagination.SortOrder directly (see the pagination variable and the OrderBy("id "
+ strings.ToUpper(pagination.SortOrder)) call); whitelist and normalize this
value to a safe enum before building the Squirrel query: accept only "ASC" or
"DESC" (e.g., strings.ToUpper and switch or map), default to a safe choice
(e.g., "ASC") or return an error for invalid input, then pass that validated
token into OrderBy (e.g., OrderBy("id " + safeSortOrder)); do not permit
arbitrary strings to be concatenated into the SQL fragment.
In
`@components/ledger/internal/adapters/postgres/operationroute/operationroute_overdraft_test.go`:
- Around line 52-53: Replace the non-deterministic time.Now() usage in the test
setup for CreatedAt and UpdatedAt with a fixed test timestamp (e.g., a shared
test constant or test utility like a FixedTime/TestClock); locate the places
setting CreatedAt and UpdatedAt in the operationroute_overdraft_test.go setup
and assign them the fixed time value so the tests are deterministic and comply
with the test guidelines.
In `@components/ledger/internal/adapters/postgres/transaction/transaction.go`:
- Around line 300-316: The current companion lookup uses fromByAlias/toByAlias
keyed only by account alias which is ambiguous when BalanceKey is preserved;
change the index maps to use a stable composite key (e.g., alias + ":" +
BalanceKey or a small struct key) when populating and when looking up in
addCompanionAmount so each entry is disambiguated by both AccountAlias and
BalanceKey; update the map creation code that fills fromByAlias/toByAlias and
the addCompanionAmount function (and the analogous logic around lines 345-380)
to use that composite key for both storing idx and performing the lookup.
---
Outside diff comments:
In `@components/crm/internal/services/create-holder.go`:
- Line 35: Replace the fmt.Sprintf-style error messages in the logger.Log calls
with structured log fields: pass libLog.Err(err) plus a short string field
(e.g., libLog.String("msg","Failed to generate holder id")) and any relevant
context (e.g., libLog.String("holder", holderID) or libLog.Int(...) as
applicable) to logger.Log(ctx, libLog.LevelError, ...). Do this for the call
that currently uses fmt.Sprintf("Failed to generate holder id: %v", err) and the
other occurrence around the same area (the logger.Log call at the second spot)
so both use libLog.Err(err) and appropriate libLog.String()/libLog.Int() fields
instead of fmt.Sprintf.
In `@components/ledger/api/docs.go`:
- Around line 8360-8402: The OpenAPI schema for AccountingEntries is missing the
refund property even though the prose mentions it; update the source
OpenAPI/spec or code annotations that define AccountingEntries to add a "refund"
property with the same allOf/$ref to AccountingEntry (matching
cancel/commit/direct/hold/revert/overdraft), then regenerate the Swagger
artifacts so components/ledger/api/docs.go includes refund in the properties;
ensure the change is applied to the original spec/annotations (not docs.go) and
run the project’s OpenAPI generator to produce the updated docs file.
In `@components/ledger/api/openapi.yaml`:
- Around line 7332-7339: The OpenAPI schemas declare available.minimum: 0 even
for overdraft-capable balances, which conflicts with allowOverdraft allowing
negative available; remove the non-negative constraint by deleting the minimum:
0 (and any duplicate minimum/type mismatch entries) from the affected available
field definitions so the available property can be negative when allowOverdraft
is true; search for the schema property named available and the flag
allowOverdraft to locate each occurrence (including the additional occurrences
noted in the comment) and remove the minimum: 0 constraint there.
In `@components/ledger/api/swagger.json`:
- Around line 8336-8378: The AccountingEntries schema lists refund in its
description but lacks a refund property; add a "refund" property to the
AccountingEntries object (alongside cancel/commit/direct/hold/overdraft) with a
description like "The accounting entry for the refund action." and an allOf
reference to "#/definitions/AccountingEntry" so generated clients can represent
the refund accounting entry; mirror the pattern used by the other entries (e.g.,
"direct" or "commit") when adding the refund property.
- Around line 8894-8929: The CreateAdditionalBalance and UpdateBalance request
schemas currently reuse mmodel.BalanceSettings which permits balanceScope:
"internal" and also allow the reserved key "overdraft"; update the OpenAPI
schemas so client-facing request contracts cannot set those internal-only
values: create separate request-side schema(s) (e.g., BalanceSettingsRequest) or
add validation constraints/enums to the existing request properties to forbid
balanceScope = "internal" and add a pattern/enum or forbidden-value rule for the
key property to disallow "overdraft"; ensure these changes are applied to the
CreateAdditionalBalance and UpdateBalance definitions (and the other occurrences
mentioned) so the public spec no longer permits internal-only settings.
In
`@components/ledger/internal/adapters/postgres/account/account.postgresql_integration_test.go`:
- Around line 377-383: The test uses wall-clock times when constructing the
filter variable (http.QueryHeader named "filter") via time.Now().Add(...);
replace these with fixed deterministic timestamps (e.g., time.Date(...,
time.UTC) or your test fixed-time helper) for StartDate and EndDate so the
filter setup and any repeated filter constructions in this file are
deterministic; update every occurrence that uses time.Now().Add(...) in the test
to use the chosen fixed timestamp values (and keep them in UTC to avoid timezone
flakiness).
In `@components/ledger/internal/adapters/postgres/account/account.postgresql.go`:
- Around line 233-314: The FindAll method in AccountPostgreSQLRepository has
elevated cyclomatic complexity due to many inline optional filter blocks;
refactor by extracting the repeated filter-application logic into a helper
(e.g., buildAccountFilters or applyAccountFilters) that accepts the
squirrel.SelectBuilder, the http.QueryHeader filter and returns the updated
builder; move all blocks that add WHERE clauses (EntityIDs, Status, Type,
AssetCode, EntityID, Blocked, ParentAccountID, Name, Alias, date range,
sort/limit/offset) into that helper and call it from FindAll to keep FindAll
focused on setup/teardown and reduce complexity.
In
`@components/ledger/internal/adapters/postgres/accounttype/accounttype.postgresql_integration_test.go`:
- Around line 427-445: Tests use time.Now() in QueryHeader filters (limitFilter,
allFilter passed to repo.FindAll), which makes boundary assertions flaky;
replace dynamic clock usage with a fixed test timestamp fixture (e.g., a
package-level or helper const like testNow or fixedTime from your test fixtures)
and compute StartDate/EndDate relative to that fixed timestamp. Update all
occurrences noted (including the other blocks around lines referenced) so
http.QueryHeader uses the deterministic fixture instead of time.Now(), keeping
SortOrder/Limit values unchanged.
In
`@components/ledger/internal/adapters/postgres/ledger/ledger.postgresql_integration_test.go`:
- Around line 143-150: The helper defaultQueryHeader currently uses time.Now(),
making tests time-dependent; change defaultQueryHeader to use a fixed,
deterministic timestamp (e.g., a package-level constant or a call to a test time
helper) for StartDate and EndDate so FindAll tests are stable—update
defaultQueryHeader (and add a fixed time constant or use the repo's fixed time
utility) to return StartDate and EndDate based on that fixed instant rather than
time.Now().
In
`@components/ledger/internal/adapters/postgres/organization/organization.postgresql_integration_test.go`:
- Around line 447-454: The test helper defaultQueryHeader embeds time.Now(),
making FindAll tests time-dependent; change defaultQueryHeader to return a
QueryHeader with fixed StartDate and EndDate (use a deterministic time.Date
value or a shared fixedClock constant used across tests) instead of time.Now(),
so all pagination/filter tests are reproducible; update any tests that rely on
the current helper to use the fixed instant or adjust expectations accordingly
(refer to defaultQueryHeader and any FindAll test callers to locate changes).
In
`@components/ledger/internal/adapters/postgres/portfolio/portfolio.postgresql_integration_test.go`:
- Around line 358-390: The test uses time.Now() inside the pagination/date-range
filters (page1Filter, page2Filter, page3Filter passed to repo.FindAll) which
makes assertions flaky; replace those calls with a fixed test timestamp fixture
(e.g., a package-level testTime or a fixture like fixtures.FixedTime) and use
deterministic offsets from that fixture (testTime.AddDate(...)) for
StartDate/EndDate throughout the file (also update the other occurrences
referenced in the comment) so the pagination/date-range assertions are stable
across runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The comment in the snapshot migration contained a semicolon inside a SQL comment line. Migration runners that split statements on semicolons (e.g. golang-migrate with x-multi-statement) would incorrectly split the comment, producing an invalid statement starting with 'if future queries need filtering by...'. This caused migration failures on AWS RDS environments while working on self-hosted PostgreSQL where the full file is sent as a single execution. Requested-by: @ClaraTersi
…-comment fix: remove semicolon from SQL comment in migration 000033
chore: promote develop to release-candidate
ClaraTersi
temporarily deployed
to
create_release
GitHub Actions
Inactive
Ygohr
approved these changes
Apr 30, 2026
bedatty
approved these changes
Apr 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/readyzreadiness implementations for Ledger and CRM with TLS detection, health checks, metrics, and lifecycle handling.Operational Notes