Conversation
WalkthroughThis pull request introduces major architectural changes across multiple systems: a new idempotency-hit detection mechanism for transaction/metadata operations that returns an additional boolean flag and sets HTTP response headers; a complete image configuration refactoring replacing simple tag strings with structured Registry/Repository/Tag objects; new exporter update capability with full gRPC/storage/controller/API support; extensive typed mock code generation enhancements; storage optimizations including per-bucket scoped queries and multi-value filter support; and numerous database migrations for schema enhancements. The changes span deployment, command-line interfaces, HTTP API handlers, controller methods, storage layers, and test infrastructure. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
40-62:⚠️ Potential issue | 🔴 CriticalPremature loop exit when
transactions_seqvalues have gaps larger than_batch_size.If
moves_viewcontainstransactions_seqvalues with gaps exceeding 1000 (e.g., due to rolled-back transactions), the CTE returns zero rows for that batch,NOT FOUNDbecomes true, and the loop exits — silently skipping all remaining rows. TheNOT VALIDconstraint on line 68 won't catch this, leavingpost_commit_volumesas NULL for those transactions.A safer approach is to select the next batch based on actual rows in
moves_viewrather than assuming contiguous seq ranges:Proposed fix: use row-based batching instead of range-based
loop with data as ( select transactions_seq, volumes from moves_view - -- play better than offset/limit - where transactions_seq > _offset and transactions_seq <= _offset + _batch_size + order by transactions_seq + limit _batch_size ) update transactions set post_commit_volumes = data.volumes from data where transactions.seq = data.transactions_seq; if not found then drop table moves_view; exit; end if; - _offset = _offset + _batch_size; + -- Remove processed rows so the next iteration picks up the next batch + delete from moves_view + where transactions_seq in ( + select transactions_seq from moves_view order by transactions_seq limit _batch_size + ); perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size); commit; end loop;Alternatively, if you want to keep range-based scanning for performance, replace the
NOT FOUNDexit condition with:if (select count(*) from moves_view where transactions_seq > _offset) = 0 thenOr use
SELECT max(transactions_seq) FROM moves_viewto determine the upper bound and loop until_offsetexceeds it.internal/storage/common/cursor.go (1)
79-94:⚠️ Potential issue | 🔴 CriticalLine 79 should use
PaginatedQuery[Options]instead ofPaginatedQuery[OF].The
Iteratefunction has two type parameters with different purposes:OF(the cursor element type) andOptions(the query options type). Line 79 incorrectly declaresqueryasPaginatedQuery[OF], causing a semantic type mismatch:
initialQueryisInitialPaginatedQuery[Options](line 74)iteratorexpectsPaginatedQuery[Options](line 75)unmarshalCursor[Options]returnsPaginatedQuery[Options](line 94)The code compiles only because
PaginatedQueryis a marker interface where the type parameter doesn't appear in any method signatures, making all instantiations structurally identical. However, this is semantically incorrect. Compare with theExtractfunction (lines 14–28), which consistently uses the same type parameter throughout.Change line 79 to:
- var query PaginatedQuery[OF] = initialQuery + var query PaginatedQuery[Options] = initialQuerydeployments/pulumi/pkg/common/common.go (1)
22-41:⚠️ Potential issue | 🟠 MajorRemove duplicate
Tagdefaulting logic.Lines 22–31 in
CommonArgs.SetDefaults()explicitly defaultargs.Tagto"latest", but this is redundant. Line 41 callsargs.ImageConfiguration.SetDefaults(), which contains identical Tag defaulting logic at lines 85–94 ofdeployments/pulumi/pkg/utils/convert.go. SinceCommonArgsembedsutils.ImageConfiguration, the Tag field is promoted and gets defaulted twice with the same logic.Remove the manual Tag defaulting (lines 22–31) and rely solely on
ImageConfiguration.SetDefaults()to handle it.deployments/pulumi/pkg/config/config.go (1)
812-860:⚠️ Potential issue | 🟠 MajorAvoid overriding default image settings when
imageis absent/empty.
When the config key is missing,TryObjectleaves an empty struct, so registry/repository are set to empty strings and can clobber defaults. Also, the error message references “generator”. Treat missing/empty config as nil (or ignore empty fields) and fix the message.Suggested fix
- image := &ImageConfiguration{} - if err := cfg.TryObject("image", image); err != nil { - if !errors.Is(err, config.ErrMissingVar) { - return nil, fmt.Errorf("error reading generator config: %w", err) - } - } + image := &ImageConfiguration{} + if err := cfg.TryObject("image", image); err != nil { + if !errors.Is(err, config.ErrMissingVar) { + return nil, fmt.Errorf("error reading image config: %w", err) + } + image = nil + } + if image != nil && image.Registry == "" && image.Repository == "" && image.Tag == "" { + image = nil + } @@ -func imageConfigurationOrTag(configuration *ImageConfiguration, tag string) utils.ImageConfiguration { - if configuration == nil { +func imageConfigurationOrTag(configuration *ImageConfiguration, tag string) utils.ImageConfiguration { + if configuration == nil || (configuration.Registry == "" && configuration.Repository == "" && configuration.Tag == "") { return utils.ImageConfiguration{ Tag: pulumix.Val(tag), } }
🤖 Fix all issues with AI agents
In `@deployments/pulumi/pkg/storage/component_postgres.go`:
- Around line 64-70: The SetDefaults method for PostgresInstallArgs currently
sets ChartVersion to an empty string which causes Helm to resolve to the latest
OCI chart; change SetDefaults (function PostgresInstallArgs.SetDefaults) to
restore the prior hardcoded default by setting args.ChartVersion to "16.4.7"
when nil instead of pulumix.Val(""), ensuring deterministic chart resolution for
the Helm release (also check other default branches in SetDefaults to follow the
same pattern if present).
In `@deployments/pulumi/pkg/utils/convert.go`:
- Around line 8-34: The GetImage function must call
ImageConfiguration.SetDefaults() before using its fields to ensure pulumix.Apply
never receives nil Inputs; modify GetImage (and ensure GetMainImage still
delegates to it) to invoke imageConfiguration.SetDefaults() (or call via a local
cfg := imageConfiguration; cfg.SetDefaults() if needed) at the top, then use
that cfg in the pulumix.Apply calls so registry/repository/tag are non-nil and
runtime crashes are avoided.
In `@docs/api/README.md`:
- Around line 2412-2466: Update the Update exporter operation documentation to
match the other exporter CRUD docs: change the authentication aside from an
authorization-required warning to the same no-auth success aside used by Create
exporter and Delete exporter (ensure the text and CSS class mirror those
endpoints) and update the request body schema reference from
V2ExporterConfiguration to the dedicated V2UpdateExporterRequest so the body
parameter row and the example reference the correct schema; locate the Update
exporter section by its operation title ("Update exporter" /
opIdv2UpdateExporter) and update the auth aside and the body schema name
accordingly.
In `@internal/api/v2/controllers_exporters_update.go`:
- Line 25: Replace the call to api.InternalServerError(w, r, err) with
common.InternalServerError(w, r, err) in controllers_exporters_update.go so the
handler uses the repository's observability wrapper (consistent with
controllers_logs_import.go and controllers_ledgers_create.go); locate the call
to api.InternalServerError in the Update exporter handler and swap it to
common.InternalServerError, ensuring the existing error variable and parameters
remain unchanged.
In `@internal/controller/ledger/controller_with_too_many_client_handling.go`:
- Line 15: The go:generate directive in
controller_with_too_many_client_handling.go contains a duplicated -typed flag;
edit the directive that references DelayCalculator (the //go:generate line) and
remove the redundant -typed so only a single -typed flag remains, keeping all
other flags (-write_source_comment, -write_package_comment, -source,
-destination, -package, and the interface name DelayCalculator) unchanged.
In `@internal/replication/controller_grpc_server.go`:
- Around line 72-89: Check for a nil request.Config before accessing its fields
in UpdateExporter (and mirror the same guard in CreateExporter) to avoid
nil-pointer panics: if request.Config is nil, return a gRPC InvalidArgument
(status.Errorf(codes.InvalidArgument, ...)) indicating missing exporter
configuration; only construct ledger.ExporterConfiguration{Driver:
request.Config.Driver, Config: json.RawMessage(request.Config.Config)} after the
nil check so json.RawMessage conversion is not performed on a nil pointer.
In `@internal/replication/pipeline.go`:
- Line 165: Update the log message(s) that currently read "Shutdowning pipeline"
to the correct phrasing "Shutting down pipeline" in the p.logger.Infof calls in
internal/replication/pipeline.go (the call referenced as p.logger.Infof). There
are two occurrences (one at the shown diff and the second near the other log on
line 171); replace both messages to use "Shutting down pipeline" to fix the
typo.
In `@internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql`:
- Around line 8-9: The temp table creation uses "CREATE TEMPORARY TABLE
tmp_volumes ON COMMIT DROP", which causes tmp_volumes to be automatically
removed at the first COMMIT inside the batch loop and breaks subsequent
iterations; modify the CREATE statement for tmp_volumes to remove "ON COMMIT
DROP" so the table persists across commits, while keeping the initial "DROP
TABLE IF EXISTS tmp_volumes" cleanup and the explicit "DROP TABLE tmp_volumes"
at the end of the script.
In `@internal/storage/bucket/migrations/39-clean-useless-features/up.sql`:
- Around line 13-26: The DROP TRIGGER statements that build vsql for
"transaction_set_addresses_<ledger.id>",
"accounts_set_address_array_<ledger.id>" and
"transaction_set_addresses_segments_<ledger.id>" should use "DROP TRIGGER IF
EXISTS" to avoid failing the migration when a trigger is missing; update the
three vsql string constructions (the ones assigning vsql before execute for
transaction_set_addresses_, accounts_set_address_array_, and
transaction_set_addresses_segments_) to include IF EXISTS in the DROP TRIGGER
clause while keeping the existing ON <table> and identifier concatenation logic.
In `@pkg/client/docs/models/operations/addmetadatatoaccountresponse.md`:
- Around line 8-9: The markdown linter flags the type notation
map[string][]*string* in
pkg/client/docs/models/operations/addmetadatatoaccountresponse.md as a broken
reference; update the table so the type is rendered as inline code or escaped
characters (e.g., replace map[string][]*string* with `map[string][]*string*` or
escape the square brackets) for the Headers column, and if this file is
generated by Speakeasy, note the change and consider filing the fix upstream so
future generated docs include properly formatted types.
In `@pkg/client/docs/models/operations/createtransactionresponse.md`:
- Line 10: The generated docs show the Headers field type incorrectly as
`map[string][]*string*`; update the docs generation so the Headers field is
rendered as `map[string][]string` to match the source type. Locate the template
or formatter that emits operation model field types (the logic that renders the
Headers field in createtransactionresponse.md) and remove the stray
asterisks/markdown emphasis from the element type rendering, ensuring the
generator outputs the exact Go type string "map[string][]string" for the Headers
field.
In `@pkg/client/docs/models/operations/reverttransactionresponse.md`:
- Line 10: Fix the markdown typo for the `Headers` field type: replace the
incorrect `map[string][]*string*` with `map[string][]*string` so the trailing
`*` is removed and the Go type renders correctly; if this markdown is generated,
update the generator template that emits the `Headers` type to drop the stray
trailing asterisk.
In `@pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md`:
- Line 9: The docs show an incorrect type `map[string][]*string*` for `Headers`
but the Go model in addmetadatatoaccount.go uses `map[string][]string`; update
the generator/template producing
pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md (or the
generator config) to emit `map[string][]string` (no pointer and remove the stray
trailing `*`) so the markdown matches the Go type and avoids the Markdown lint
MD052 rendering issue; ensure the template variable for `Headers` uses the exact
Go type string and re-generate the docs.
In `@pkg/client/docs/models/operations/v2createtransactionresponse.md`:
- Line 10: The table cell for the `Headers` type currently contains a trailing
asterisk (`map[string][]*string*`) that renders as italic in Markdown and
triggers MD052; fix it by ensuring the Go pointer type is rendered literally —
e.g., replace the malformed `map[string][]*string*` with a properly delimited
inline code representation `map[string][]*string` or escape the asterisks so
they are not interpreted as emphasis; if this file is generated, update the
generator that emits the `Headers` type to avoid appending a stray `*`.
In `@pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md`:
- Line 9: The Markdown table cell for the `Headers` type contains an extra
trailing asterisk causing `map[string][]*string*` to render incorrectly; update
the type entry to use the correct fenced code representation
`map[string][]*string` (i.e., remove the stray trailing `*`) so the Go type
displays properly and the MD052 lint warning is resolved; locate the row
containing `Headers` in the v2DeleteTransactionMetadataResponse documentation
and replace the malformed type text with the corrected backticked type.
In `@pkg/client/docs/models/operations/v2reverttransactionresponse.md`:
- Line 10: The Markdown table for the Headers field uses a malformed type
`map[string][]*string*` with stray asterisks; update the type to
`map[string][]string` in the v2RevertTransactionResponse docs (and the other
response docs that added the Headers field) so the table shows the correct type
and no stray markdown emphasis or MD052 link errors; locate the line that
defines the Headers type in the docs file (the row mentioning `Headers`) and
remove the extra `*` characters around `string`.
In `@tools/provisioner/Dockerfile`:
- Line 2: Remove the duplicate Dockerfile syntax directive so only the single `#
syntax=docker/dockerfile:1` remains at the very top, and in the runtime stage
that starts with `FROM alpine:3.21` create a non-root user (e.g. `appuser`), set
ownership where needed, and add `USER appuser` before the `ENTRYPOINT` to ensure
the container does not run as root; keep the compiler stage name `compiler`
unchanged.
🧹 Nitpick comments (33)
internal/storage/bucket/migrations/45-fix-missing-primary-keys/up.sql (1)
1-9: Consider adding idempotency guards for existing primary keys.If any of these tables already has a primary key (e.g., from a partial or retried migration),
ALTER TABLE ... ADD PRIMARY KEYwill fail with"multiple primary keys are not allowed". Wrapping each statement in a conditional check would make this migration more robust:Proposed idempotent version
do $$ begin set search_path = '{{ .Schema }}'; - ALTER TABLE transactions ADD PRIMARY KEY USING INDEX transactions_ledger; - ALTER TABLE accounts ADD PRIMARY KEY USING INDEX accounts_ledger; - ALTER TABLE logs ADD PRIMARY KEY USING INDEX logs_ledger; + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = 'transactions'::regclass AND contype = 'p' + ) THEN + ALTER TABLE transactions ADD PRIMARY KEY USING INDEX transactions_ledger; + END IF; + + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = 'accounts'::regclass AND contype = 'p' + ) THEN + ALTER TABLE accounts ADD PRIMARY KEY USING INDEX accounts_ledger; + END IF; + + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conrelid = 'logs'::regclass AND contype = 'p' + ) THEN + ALTER TABLE logs ADD PRIMARY KEY USING INDEX logs_ledger; + END IF; end $$;pkg/client/docs/models/operations/v2reverttransactionrequest.md (1)
13-13: Consider enhancing the IdempotencyKey documentation.The description could be more informative about the purpose and usage of idempotency keys. Additionally, providing an example value would help developers understand the expected format.
📝 Suggested documentation enhancement
-| `IdempotencyKey` | **string* | :heavy_minus_sign: | Use an idempotency key | | +| `IdempotencyKey` | **string* | :heavy_minus_sign: | Use an idempotency key to safely retry requests. Requests with the same key will return the cached result if already processed. | a1b2c3d4-e5f6-7890-abcd-ef1234567890 |pkg/assets/asset_test.go (1)
22-36: Consider adding a lowercase-after-underscore negative case.The invalid tests are thorough, but there's no case for a lowercase letter in the underscore segment (e.g.,
EUR_col). While the existing"a"test proves lowercase is rejected for the base, the underscore sub-pattern(_[A-Z]{1,16})is a separate group worth explicitly covering.Suggested addition
require.False(t, assets.IsValid("A_/2")) + require.False(t, assets.IsValid("EUR_col")) }internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql (1)
53-53: Consider usingDROP TABLE IF EXISTSfor resilience.If
ON COMMIT DROPis removed per the above fix, this line will work as intended. However, switching toIF EXISTShere makes the tail-end cleanup defensive against partial-run scenarios as well.Proposed change
- drop table tmp_volumes; + drop table if exists tmp_volumes;internal/storage/bucket/migrations/39-clean-useless-features/up.sql (1)
32-49: Commented-out code with TODO is noted.The future cleanup plan for 2.4 is clear from the TODO. Consider tracking this in an issue so it doesn't get lost.
internal/storage/bucket/migrations/34-fix-memento-format/up.sql (1)
84-86: Termination check is one batch late — always runs one extra empty iteration.The exit condition tests
_offset >= max(seq), but at this point the batch that was just processed covered rows up to_offset + _batch_size. So after processing the final real batch, the check still fails,_offsetis incremented, and the loop runs once more with zero matching rows before finally exiting.This isn't a correctness bug (the UPDATE simply matches nothing), but it's easy to fix:
♻️ Tighten the exit condition
- if _offset >= (select max(seq) from logs) then + if _offset + _batch_size >= (select max(seq) from logs) then exit; end if;internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql (1)
20-26:_maxdeclared asintegerbutcount(*)returnsbigint.If the number of rows with
transactions_id IS NULLever exceeds ~2.1 billion, the assignment would overflow at runtime. Practically unlikely for this migration, but declaring_max bigintwould be a trivial defensive fix and consistent with PostgreSQL'scount(*)return type.Proposed fix
declare _batch_size integer := 1000; - _max integer; + _max bigint;internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (1)
34-37: Doublecount(*)scan onmoves_view— minor optimization opportunity.Lines 34 and 39 both perform
select count(*) from moves_view, resulting in two full scans of the table. Consider storing the count in a variable to avoid the redundant scan.♻️ Proposed refactor to cache the count
+ declare + _count integer; ... - if (select count(*) from moves_view) = 0 then + _count := (select count(*) from moves_view); + + if _count = 0 then drop table moves_view; return; end if; - perform pg_notify('migrations-{{ .Schema }}', 'init: ' || (select count(*) from moves_view)); + perform pg_notify('migrations-{{ .Schema }}', 'init: ' || _count);internal/storage/bucket/migrations/31-fix-transaction-updated-at/up.sql (1)
10-13: Potential column name collision withSELECT *If the
transactionstable ever gains a column namedrow_number, thisCREATE TABLE AS SELECT row_number() OVER (...) AS row_number, *will fail with a duplicate column error. Consider using explicit column names or aliasing the window function to something more specific (e.g.,_row_numberorbatch_row_num).pkg/client/docs/models/operations/createtransactionresponse.md (1)
10-10: Improve the Headers field description.The description "N/A" is not informative for users. Consider adding a brief description explaining what HTTP response headers are exposed in this field, particularly related to the idempotency-hit detection mentioned in the PR objectives.
📝 Suggested documentation improvement
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | +| `Headers` | map[string][]*string | :heavy_check_mark: | HTTP response headers, including idempotency-related headers |internal/machine/vm/machine_test.go (1)
2337-2364: Regression test doesn't fully prove save-then-send balance interaction.The stated goal is to verify no panic when
saveis followed bysend ... allowing unbounded overdraft. However, sincewithdrawAlwaysalways returns the full requested amount regardless of the tracked balance, the expected posting (240000) would be identical whether or notsaveactually reduced the internal balance from 2,000,000 to 500,000.To strengthen this, consider adding an assertion on
m.Balances["source"]["EUR/4"]after execution to confirm the save deduction was applied (expected final balance: 500,000 − 240,000 = 260,000). Alternatively, add a subsequent boundedsendthat would fail if the save hadn't taken effect.That said, the primary purpose (no panic) is achieved.
internal/storage/bucket/migrations/42-fix-missing-inserted-at-in-log-data/up.sql (1)
8-12: Add a defensiveDROP TABLE IF EXISTSbefore creating the temp table.Migration 18 (
logs_transactions) includes adrop table if existsbeforecreate temporary tableto handle retries within the same session. This migration should follow the same pattern for consistency and resilience.Proposed fix
+ drop table if exists logs_view; create temp table logs_view as select row_number() over (order by id) as row_number, id, ledger from logs where type = 'NEW_TRANSACTION' or type = 'REVERTED_TRANSACTION';test/e2e/api_balances_aggregated_test.go (1)
208-211: Consider asserting the actual aggregated balance value.Other test cases in this file (lines 142, 160, 179) verify both
HaveLen(1)and the specificbig.Intvalue forUSD/2. This test only checks the length, which could mask incorrect aggregation results. Adding a value assertion would strengthen the regression test.Proposed enhancement
Expect(err).ToNot(HaveOccurred()) // bank1 + bank2 + world all have USD/2, total should be aggregated Expect(response.V2AggregateBalancesResponse.Data).To(HaveLen(1)) +Expect(response.V2AggregateBalancesResponse.Data["USD/2"]).NotTo(BeNil())internal/api/v2/controllers_accounts_delete_metadata.go (1)
30-31: Nit: stray blank line inside error-handling block.There's a blank line between
if err != nil {andcommon.HandleCommonWriteErrors(...). Minor formatting inconsistency.Proposed fix
if err != nil { - common.HandleCommonWriteErrors(w, r, err) return }internal/replication/grpc/replication_service.proto (1)
71-76: Consider returning the updatedExporterinUpdateExporterResponse.
CreateExporterResponsereturns the createdExporter, butUpdateExporterResponseis empty. Returning the updated exporter would let clients see the final state (e.g., if server-side fields change) without a follow-upGetExportercall. That said,DeleteExporterResponseis also empty, so this may be an intentional convention here.If desired:
-message UpdateExporterResponse {} +message UpdateExporterResponse { + Exporter exporter = 1; +}internal/api/v2/controllers_accounts_delete_metadata_test.go (1)
71-71: Consider adding a test case foridempotencyHit = true.All test cases mock
idempotencyHitasfalse. There's no coverage verifying that whenidempotencyHitistrue, the handler sets theIdempotency-Hitresponse header. Given that the production code incontrollers_accounts_delete_metadata.gobranches on this value, a test case exercising that path would increase confidence.tools/provisioner/Dockerfile (2)
18-18: Remove duplicate# syntax=docker/dockerfile:1directive.The syntax directive is only meaningful as the very first line of the Dockerfile. This second occurrence on line 18 (before the runtime stage) is a no-op and should be removed to avoid confusion.
Proposed fix
-# syntax=docker/dockerfile:1 FROM alpine:3.21
19-23: Add a non-rootUSERto the runtime stage.The runtime stage runs as root. Trivy DS002 flags this as a security concern. Adding a dedicated non-root user reduces the blast radius of container escapes.
Proposed fix
FROM alpine:3.21 LABEL org.opencontainers.image.source=https://github.com/formancehq/ledger +RUN addgroup -S appgroup && adduser -S appuser -G appgroup COPY --from=compiler /src/tools/provisioner/provisioner /bin/provisioner +USER appuser ENTRYPOINT ["/bin/provisioner"] CMD ["--help"]internal/replication/pipeline.go (1)
69-72:stophelper closes the error channel but never sends an error on it.The
Shutdownmethod (line 173) reads fromerrorChannel. Sincestoponly closes the channel, the read will receive a zero-valuenilerror, which happens to signal success. This works but relies on the implicit zero-value behavior of a closed channel. If you ever need to propagate a real error on shutdown, this pattern will need adjustment. Current behavior is correct for the "always succeed" case.test/e2e/api_exporters_update_test.go (1)
235-235: Inconsistent error assertion style:Succeed()vsBeNil().Lines 235 and 303 use
Expect(err).To(Succeed())while the rest of the file usesExpect(err).To(BeNil()). While both work forerrorvalues,Succeed()is idiomatically intended for function return values, and mixing styles reduces readability. Consider usingBeNil()consistently.Proposed fix
- Expect(err).To(Succeed()) + Expect(err).To(BeNil())Same on line 303.
internal/api/v1/controllers_transactions_delete_metadata_test.go (1)
60-69: Consider adding a test case foridempotencyHit = true.The mock always returns
falsefor the idempotency hit flag. There's no test coverage for verifying theIdempotency-Hit: trueresponse header is set when the controller signals an idempotency hit. Consider adding a test case where the mock returnstrueand asserts the header is present.#!/bin/bash # Check if other v1 delete metadata tests cover the idempotency hit case rg -n "idempotencyHit\|Idempotency-Hit" --type=go -C3 -g '*delete_metadata*'internal/storage/ledger/store.go (1)
180-186: Small correctness window when a second ledger is created in the same bucket.Between the moment the new ledger row is INSERTed and
SetAloneInBucket(false)is called (both inside the same transaction inCreateLedger), concurrent readers on other connections may still seealoneInBucket == trueand skip theWHERE ledger = ?filter. In practice the window is tiny and the new ledger won't have data yet, but it's worth noting in the comment for future maintainers.internal/api/v1/controllers_accounts_delete_metadata_test.go (1)
75-75: Mock return updated to match new signature — consider adding an idempotency-hit test case.The
falseforidempotencyHitis correct for the existing non-idempotent scenarios. A test case withidempotencyHit = truewould verify that theIdempotency-Hitresponse header is actually set by the handler.internal/storage/driver/driver.go (1)
93-112: Consider caching thealoneInBucketflag per bucket to avoid repeatedCOUNTqueries on everyOpenLedgercall.
OpenLedgeris called per-request (confirmed ininternal/replication/manager.goandinternal/controller/system/controller.go). TheCountLedgersInBucketcall executes a database COUNT query on each invocation. Since theDefaultFactoryalready maintains a sharedbucketFlagsmap to cache*atomic.Boolper bucket, leverage this to initialize the flag once per bucket on first access, then skip the count on subsequent opens. This eliminates per-request database overhead for the common case.internal/controller/system/controller.go (1)
88-93: Minor inconsistency:UpdateExporterdoesn't wrap errors likeCreateExporterdoes.
CreateExporter(line 83) wraps the backend error withfmt.Errorf("failed to create exporter: %w", err), whileUpdateExporterreturns the backend error directly. Consider wrapping for consistency and easier tracing.♻️ Optional: wrap the error for consistency
func (ctrl *DefaultController) UpdateExporter(ctx context.Context, id string, configuration ledger.ExporterConfiguration) error { - return ctrl.replicationBackend.UpdateExporter(ctx, id, configuration) + if err := ctrl.replicationBackend.UpdateExporter(ctx, id, configuration); err != nil { + return fmt.Errorf("failed to update exporter: %w", err) + } + return nil }internal/api/v2/controllers_exporters_update_test.go (1)
89-96: Consider settingContent-Type: application/jsonon the test request.The request body is JSON-encoded but no
Content-Typeheader is set. If the handler or any middleware inspects the content type to decide how to parse the body, this could cause unexpected failures. This is a common pattern in Go tests, so it may already work fine depending on the handler implementation.♻️ Optional: set Content-Type header
req := httptest.NewRequest(http.MethodPut, "/_/exporters/"+testCase.exporterID, bytes.NewBuffer(data)) + req.Header.Set("Content-Type", "application/json") req = req.WithContext(ctx)test/e2e/api_idempotency_hit_header_test.go (1)
74-82: Nit: inconsistent error assertion style.Some assertions use
Expect(err).ToNot(HaveOccurred())(e.g., Line 76) while others useExpect(err).To(Succeed())(e.g., Line 139). Both are functionally equivalent in Gomega, but picking one convention improves readability.Also applies to: 132-145
internal/api/common/mocks_system_controller_test.go (1)
477-489: Auto-generated mock code looks correct.The
UpdateExportermock methods forSystemControllerfollow the established pattern and correctly match the interface contract.Note: The
mockgendirective on line 5 does not include the-typedflag, unlikeinternal/controller/ledger/controller.go(line 18). If typed mock wrappers are desired here as well, the directive should be updated and mocks regenerated.internal/storage/system/store.go (4)
258-276: Inconsistent error wrapping inDeletePipelinecompared toDeleteExporter.
DeleteExporter(line 189) wraps the DB error withpostgres.ResolveError(err), butDeletePipeline(line 264) returns the rawerr. This meansDeletePipelinewon't translate DB-specific errors (e.g.,sql.ErrNoRows) into the application-level error types that callers may rely on.Also, line 275
return erris alwaysnilat that point (sincepanichandles the error case fromRowsAffected). Considerreturn nilfor clarity.Proposed fix
func (d *DefaultStore) DeletePipeline(ctx context.Context, id string) error { ret, err := d.db.NewDelete(). Model(&ledger.Pipeline{}). Where("id = ?", id). Exec(ctx) if err != nil { - return err + return postgres.ResolveError(err) } rowsAffected, err := ret.RowsAffected() if err != nil { panic(err) } if rowsAffected == 0 { return postgres.ErrNotFound } - return err + return nil }
278-289:GetPipelinedoes not wrap errors withpostgres.ResolveError, unlikeGetExporter.
GetExporter(line 210) wraps DB errors, butGetPipeline(line 285) returns raw errors. This means callers won't seepostgres.ErrNotFoundfor missing pipelines — they'll getsql.ErrNoRowsinstead.Proposed fix
func (d *DefaultStore) GetPipeline(ctx context.Context, id string) (*ledger.Pipeline, error) { ret := &ledger.Pipeline{} err := d.db.NewSelect(). Model(ret). Where("id = ?", id). Scan(ctx) if err != nil { - return nil, err + return nil, postgres.ResolveError(err) } return ret, nil }
322-341:UpdateExporterhandlesRowsAffectederror differently from sibling methods.
DeleteExporter(line 194) andDeletePipeline(line 269)paniconRowsAffected()errors, whileUpdateExporter(line 333) returns the error. Thereturn errapproach is safer, but the inconsistency may be confusing. Consider aligning all methods to the same pattern — ideally the non-panicking one.
196-200:DeleteExporterreturnserr(always nil) on the success path.After the
rowsAffected == 0check, if execution reaches line 200,erris thenilresult fromret.RowsAffected(). While functionally correct,return nilwould be clearer.Proposed fix
if rowsAffected == 0 { return postgres.ErrNotFound } - return err + return nil }internal/controller/ledger/controller_with_events.go (1)
46-50: Consider suppressing event emission on idempotency hits.Events (e.g.,
CommittedTransactions,RevertedTransaction,SavedMetadata,DeletedMetadata) are emitted even whenidempotencyHitistrue, meaning the operation was already performed in a previous call. If downstream consumers are not idempotent, this could cause duplicate processing.If this is intentional (e.g., consumers handle dedup, or events serve an audit purpose), feel free to disregard.
- if !parameters.DryRun { + if !parameters.DryRun && !idempotencyHit { c.handleEvent(ctx, func() { c.listener.CommittedTransactions(ctx, c.ledger.Name, ret.Transaction, ret.AccountMetadata) }) }The same pattern would apply to all six methods.
| func (args *PostgresInstallArgs) SetDefaults() { | ||
| if args.Username == nil { | ||
| args.Username = pulumix.Val("") | ||
| } | ||
| if args.ChartVersion == nil { | ||
| args.ChartVersion = pulumix.Val("") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for ChartVersion usage and context
rg -n "ChartVersion" --type=go -B2 -A2Repository: formancehq/ledger
Length of output: 2451
🏁 Script executed:
# Also check the specific file to understand the full context
fd component_postgres.go -x cat -n {}Repository: formancehq/ledger
Length of output: 5662
🏁 Script executed:
# Search for any version-related defaults or configuration
rg -n "16\.4\.7|postgresql.*version|chart.*version" --type=go -iRepository: formancehq/ledger
Length of output: 782
🏁 Script executed:
# Check git history for this file to see what changed
git log --oneline -n 20 deployments/pulumi/pkg/storage/component_postgres.goRepository: formancehq/ledger
Length of output: 109
🏁 Script executed:
# Look at the recent changes to understand what was changed
git diff HEAD~5 HEAD -- deployments/pulumi/pkg/storage/component_postgres.go 2>/dev/null | head -100Repository: formancehq/ledger
Length of output: 43
🏁 Script executed:
# Also check the config file for any defaults
cat -n deployments/pulumi/pkg/config/config.go | sed -n '150,200p'Repository: formancehq/ledger
Length of output: 1823
🏁 Script executed:
# Check git log with more context to see the actual changes
git log --all --oneline -- deployments/pulumi/pkg/storage/component_postgres.go | head -20Repository: formancehq/ledger
Length of output: 353
🏁 Script executed:
# Get the full git diff to see what was changed
git show HEAD:deployments/pulumi/pkg/storage/component_postgres.go 2>/dev/null > /tmp/head.go
git show HEAD~1:deployments/pulumi/pkg/storage/component_postgres.go 2>/dev/null > /tmp/prev.go
diff -u /tmp/prev.go /tmp/head.go 2>/dev/null | head -150Repository: formancehq/ledger
Length of output: 4305
🏁 Script executed:
# Search for any historical hardcoded postgres chart version
rg -n "postgresql.*16\.4\.|16\.4\.7" deployments/Repository: formancehq/ledger
Length of output: 43
🏁 Script executed:
# Check if there are any environment-based or configuration-based defaults for chart version
rg -n "ChartVersion|chart.*version" deployments/pulumi/pkg/config -B3 -A3Repository: formancehq/ledger
Length of output: 1187
🏁 Script executed:
# Check the specific commit that fixed pulumi postgres chart version
git show 0870634 --statRepository: formancehq/ledger
Length of output: 506
🏁 Script executed:
# Get the details of that commit
git show 0870634Repository: formancehq/ledger
Length of output: 2572
Restore default chart version to prevent unpredictable Helm behavior.
The refactor made ChartVersion configurable, but defaults to an empty string when not provided. This removes the previous hardcoded default of "16.4.7". Empty version strings cause Helm to fetch the latest available version for the OCI chart, which is unpredictable. Deployments relying on implicit defaults will suddenly pull an unvetted version. Restore the previous default:
Proposed fix
if args.ChartVersion == nil {
- args.ChartVersion = pulumix.Val("")
+ args.ChartVersion = pulumix.Val("16.4.7")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (args *PostgresInstallArgs) SetDefaults() { | |
| if args.Username == nil { | |
| args.Username = pulumix.Val("") | |
| } | |
| if args.ChartVersion == nil { | |
| args.ChartVersion = pulumix.Val("") | |
| } | |
| func (args *PostgresInstallArgs) SetDefaults() { | |
| if args.Username == nil { | |
| args.Username = pulumix.Val("") | |
| } | |
| if args.ChartVersion == nil { | |
| args.ChartVersion = pulumix.Val("16.4.7") | |
| } |
🤖 Prompt for AI Agents
In `@deployments/pulumi/pkg/storage/component_postgres.go` around lines 64 - 70,
The SetDefaults method for PostgresInstallArgs currently sets ChartVersion to an
empty string which causes Helm to resolve to the latest OCI chart; change
SetDefaults (function PostgresInstallArgs.SetDefaults) to restore the prior
hardcoded default by setting args.ChartVersion to "16.4.7" when nil instead of
pulumix.Val(""), ensuring deterministic chart resolution for the Helm release
(also check other default branches in SetDefaults to follow the same pattern if
present).
| func GetMainImage(imageConfiguration ImageConfiguration) pulumi.StringOutput { | ||
| return GetImage(imageConfiguration, pulumi.String("ledger")) | ||
| } | ||
|
|
||
| func GetImage(component, tag pulumix.Input[string]) pulumi.StringOutput { | ||
| return pulumi.Sprintf("ghcr.io/formancehq/%s:%s", component, pulumix.Apply(tag, func(version string) string { | ||
| if version == "" { | ||
| return "latest" | ||
| } | ||
| return version | ||
| })) | ||
| func GetImage(imageConfiguration ImageConfiguration, component pulumix.Input[string]) pulumi.StringOutput { | ||
| return pulumi.Sprintf( | ||
| "%s/%s/%s:%s", | ||
| pulumix.Apply(imageConfiguration.Registry, func(registry string) string { | ||
| if registry == "" { | ||
| return "ghcr.io" | ||
| } | ||
| return registry | ||
| }), | ||
| pulumix.Apply(imageConfiguration.Repository, func(repository string) string { | ||
| if repository == "" { | ||
| return "formancehq" | ||
| } | ||
| return repository | ||
| }), | ||
| component, | ||
| pulumix.Apply(imageConfiguration.Tag, func(version string) string { | ||
| if version == "" { | ||
| return "latest" | ||
| } | ||
| return version | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the ImageConfiguration struct definition
find . -type f -name "*.go" | head -20
rg "type ImageConfiguration" --type goRepository: formancehq/ledger
Length of output: 761
🏁 Script executed:
# Check for SetDefaults method in the file or nearby files
rg "SetDefaults" deployments/pulumi/pkg/utils/ -A 3 -B 1Repository: formancehq/ledger
Length of output: 389
🏁 Script executed:
# Get the full context of the convert.go file
cat -n deployments/pulumi/pkg/utils/convert.goRepository: formancehq/ledger
Length of output: 2959
🌐 Web query:
Pulumi pulumix.Apply nil input behavior documentation
💡 Result:
pulumix.Apply expects a non-nil pulumix.Input[T] value. The public docs for Apply describe its signature/semantics (“applies a function to an input, returning an Output”), but do not define any special behavior for a nil Input—so passing nil is not a supported/meaningful “empty input” and will typically just fail at runtime when Pulumi tries to convert/await the input (because there’s no actual Input implementation to work with). [1]
If what you actually have is “an optional value”, use one of these patterns instead:
- Use a real Input that holds a known value (including a typed nil when appropriate) via
pulumix.Val(...)(seeValin the same package docs). [1] - Model optionality as a pointer and handle nil at the pointer layer, e.g.
pulumix.PtrOf(...)/PtrOutput.Elem(). Notably,Elem()is documented to handle a nil pointer by producing the zero value ofT(this is nil-pointer handling, not nil-Input handling). [1] - More generally, Pulumi’s “apply” concept is for transforming resolved values of outputs/inputs; it’s not meant as a nil-coalescing mechanism. [2]
Sources: pulumix package reference (including Apply, Val, PtrOutput.Elem) [1]; Pulumi “Apply” concept docs [2].
Call SetDefaults() in GetImage to prevent runtime crashes with nil inputs.
pulumix.Apply() requires non-nil Input values and will fail at runtime when passed nil. The current code is vulnerable when a zero-value ImageConfiguration is passed. The SetDefaults() method already exists to handle this; calling it ensures the configuration is safe regardless of how the function is invoked.
♻️ Suggested fix
func GetImage(imageConfiguration ImageConfiguration, component pulumix.Input[string]) pulumi.StringOutput {
+ cfg := imageConfiguration
+ cfg.SetDefaults()
return pulumi.Sprintf(
"%s/%s/%s:%s",
- pulumix.Apply(imageConfiguration.Registry, func(registry string) string {
+ pulumix.Apply(cfg.Registry, func(registry string) string {
if registry == "" {
return "ghcr.io"
}
return registry
}),
- pulumix.Apply(imageConfiguration.Repository, func(repository string) string {
+ pulumix.Apply(cfg.Repository, func(repository string) string {
if repository == "" {
return "formancehq"
}
return repository
}),
component,
- pulumix.Apply(imageConfiguration.Tag, func(version string) string {
+ pulumix.Apply(cfg.Tag, func(version string) string {
if version == "" {
return "latest"
}
return version
}),
)
}🤖 Prompt for AI Agents
In `@deployments/pulumi/pkg/utils/convert.go` around lines 8 - 34, The GetImage
function must call ImageConfiguration.SetDefaults() before using its fields to
ensure pulumix.Apply never receives nil Inputs; modify GetImage (and ensure
GetMainImage still delegates to it) to invoke imageConfiguration.SetDefaults()
(or call via a local cfg := imageConfiguration; cfg.SetDefaults() if needed) at
the top, then use that cfg in the pulumix.Apply calls so registry/repository/tag
are non-nil and runtime crashes are avoided.
| ## Update exporter | ||
|
|
||
| <a id="opIdv2UpdateExporter"></a> | ||
|
|
||
| > Code samples | ||
|
|
||
| ```http | ||
| PUT http://localhost:8080/v2/_/exporters/{exporterID} HTTP/1.1 | ||
| Host: localhost:8080 | ||
| Content-Type: application/json | ||
| Accept: application/json | ||
|
|
||
| ``` | ||
|
|
||
| `PUT /v2/_/exporters/{exporterID}` | ||
|
|
||
| > Body parameter | ||
|
|
||
| ```json | ||
| { | ||
| "driver": "string", | ||
| "config": {} | ||
| } | ||
| ``` | ||
|
|
||
| <h3 id="update-exporter-parameters">Parameters</h3> | ||
|
|
||
| |Name|In|Type|Required|Description| | ||
| |---|---|---|---|---| | ||
| |body|body|[V2ExporterConfiguration](#schemav2exporterconfiguration)|true|none| | ||
| |exporterID|path|string|true|The exporter id| | ||
|
|
||
| > Example responses | ||
|
|
||
| > default Response | ||
|
|
||
| ```json | ||
| { | ||
| "errorCode": "VALIDATION", | ||
| "errorMessage": "[VALIDATION] invalid 'cursor' query param", | ||
| "details": "https://play.numscript.org/?payload=eyJlcnJvciI6ImFjY291bnQgaGFkIGluc3VmZmljaWVudCBmdW5kcyJ9" | ||
| } | ||
| ``` | ||
|
|
||
| <h3 id="update-exporter-responses">Responses</h3> | ||
|
|
||
| |Status|Meaning|Description|Schema| | ||
| |---|---|---|---| | ||
| |204|[No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5)|Exporter updated|None| | ||
| |default|Default|Error|[V2ErrorResponse](#schemav2errorresponse)| | ||
|
|
||
| <aside class="warning"> | ||
| To perform this operation, you must be authenticated by means of one of the following methods: | ||
| Authorization ( Scopes: ledger:write ) | ||
| </aside> |
There was a problem hiding this comment.
Inconsistent authentication requirement for Update exporter.
The Update exporter endpoint requires Authorization (Scopes: ledger:write), but the sibling endpoints — Create exporter (Line 2337) and Delete exporter (Line 2508) — are marked with <aside class="success"> indicating no authentication is required. This inconsistency across CRUD operations on the same resource seems unintentional.
Also, the request body references V2ExporterConfiguration (Line 2441) rather than the newly defined V2UpdateExporterRequest schema (Line 5570). Consider aligning the body reference with the dedicated schema.
🤖 Prompt for AI Agents
In `@docs/api/README.md` around lines 2412 - 2466, Update the Update exporter
operation documentation to match the other exporter CRUD docs: change the
authentication aside from an authorization-required warning to the same no-auth
success aside used by Create exporter and Delete exporter (ensure the text and
CSS class mirror those endpoints) and update the request body schema reference
from V2ExporterConfiguration to the dedicated V2UpdateExporterRequest so the
body parameter row and the example reference the correct schema; locate the
Update exporter section by its operation title ("Update exporter" /
opIdv2UpdateExporter) and update the auth aside and the body schema name
accordingly.
| case errors.Is(err, systemcontroller.ErrExporterNotFound("")): | ||
| api.NotFound(w, err) | ||
| default: | ||
| api.InternalServerError(w, r, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for InternalServerError definitions and usages
echo "=== Searching for InternalServerError definitions and usages ==="
rg -n "InternalServerError" --type=go -B2 -A2
echo -e "\n=== Checking imports in the file ==="
head -30 internal/api/v2/controllers_exporters_update.go
echo -e "\n=== Checking common/errors.go for InternalServerError ==="
rg -n "func.*InternalServerError" internal/api/common/errors.go -A5Repository: formancehq/ledger
Length of output: 23283
🏁 Script executed:
#!/bin/bash
# Search for any local definition of InternalServerError in api package
echo "=== Searching for api.InternalServerError definition or any local alias ==="
rg -n "func.*InternalServerError|type.*InternalServerError" --type=go internal/
echo -e "\n=== Check go.mod for go-libs version ==="
grep "go-libs" go.mod
echo -e "\n=== Search for any go-libs source or vendor directory ==="
find . -type d -name "*go-libs*" 2>/dev/null | head -20
echo -e "\n=== Check if there's a mock or stub of api.InternalServerError ==="
rg -n "InternalServerError.*=" --type=go internal/api/ | grep -v "expectError" | head -20Repository: formancehq/ledger
Length of output: 457
Use common.InternalServerError for consistent observability instrumentation
The code currently calls api.InternalServerError(w, r, err), which is imported from github.com/formancehq/go-libs/v3/api. However, common.InternalServerError (available in this file's imports) includes OTLP error recording and structured logging that the go-libs version lacks. Other v2 controllers like controllers_logs_import.go and controllers_ledgers_create.go use common.InternalServerError for consistency. Change line 25 to use common.InternalServerError(w, r, err).
🤖 Prompt for AI Agents
In `@internal/api/v2/controllers_exporters_update.go` at line 25, Replace the call
to api.InternalServerError(w, r, err) with common.InternalServerError(w, r, err)
in controllers_exporters_update.go so the handler uses the repository's
observability wrapper (consistent with controllers_logs_import.go and
controllers_ledgers_create.go); locate the call to api.InternalServerError in
the Update exporter handler and swap it to common.InternalServerError, ensuring
the existing error variable and parameters remain unchanged.
| ) | ||
|
|
||
| //go:generate mockgen -write_source_comment=false -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed | ||
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed |
There was a problem hiding this comment.
Duplicate -typed flag in mockgen directive.
The -typed flag appears twice in the //go:generate directive. While mockgen likely ignores the duplicate, this looks like a copy-paste artifact.
-//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed
+//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed | |
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator |
🤖 Prompt for AI Agents
In `@internal/controller/ledger/controller_with_too_many_client_handling.go` at
line 15, The go:generate directive in
controller_with_too_many_client_handling.go contains a duplicated -typed flag;
edit the directive that references DelayCalculator (the //go:generate line) and
remove the redundant -typed so only a single -typed flag remains, keeping all
other flags (-write_source_comment, -write_package_comment, -source,
-destination, -package, and the interface name DelayCalculator) unchanged.
| | ------------------------------------------------------------------ | ------------------------------------------------------------------ | ------------------------------------------------------------------ | ------------------------------------------------------------------ | | ||
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Type mismatch between docs and Go code; trailing stray * in type.
The doc shows map[string][]*string* but the actual Go type in addmetadatatoaccount.go:45 is map[string][]string (no pointer, no trailing asterisk). The trailing * also causes a markdown rendering issue (flagged by markdownlint MD052).
Since this appears to be Speakeasy-generated documentation, this may need to be fixed in the generator config rather than manually.
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In `@pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md` at line
9, The docs show an incorrect type `map[string][]*string*` for `Headers` but the
Go model in addmetadatatoaccount.go uses `map[string][]string`; update the
generator/template producing
pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md (or the
generator config) to emit `map[string][]string` (no pointer and remove the stray
trailing `*`) so the markdown matches the Go type and avoids the Markdown lint
MD052 rendering issue; ensure the template variable for `Headers` uses the exact
Go type string and re-generate the docs.
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Malformed type rendering — trailing * creates italic markdown instead of showing the pointer type.
The type map[string][]*string* has a trailing * that causes Markdown to render *string* as italic rather than displaying the intended Go type map[string][]*string. The static analysis tool also flagged [string] as an undefined reference link (MD052), which further confirms the rendering issue.
If this documentation is auto-generated, this may need to be fixed in the generator. Otherwise:
Proposed fix
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A |
+| `Headers` | map[string][]`*string` | :heavy_check_mark: | N/A |🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In `@pkg/client/docs/models/operations/v2createtransactionresponse.md` at line 10,
The table cell for the `Headers` type currently contains a trailing asterisk
(`map[string][]*string*`) that renders as italic in Markdown and triggers MD052;
fix it by ensuring the Go pointer type is rendered literally — e.g., replace the
malformed `map[string][]*string*` with a properly delimited inline code
representation `map[string][]*string` or escape the asterisks so they are not
interpreted as emphasis; if this file is generated, update the generator that
emits the `Headers` type to avoid appending a stray `*`.
| | ------------------------------------------------------------------ | ------------------------------------------------------------------ | ------------------------------------------------------------------ | ------------------------------------------------------------------ | | ||
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Markdown formatting issue in the type column.
The type map[string][]*string* has a stray trailing * that causes "string" to render as italic text instead of displaying the intended Go type map[string][]*string. This also triggers the markdownlint MD052 warning about an undefined reference link for [string].
📝 Proposed fix
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A |
+| `Headers` | map[string][]`*string` | :heavy_check_mark: | N/A |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | | |
| | `Headers` | map[string][]`*string` | :heavy_check_mark: | N/A | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In `@pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md` at
line 9, The Markdown table cell for the `Headers` type contains an extra
trailing asterisk causing `map[string][]*string*` to render incorrectly; update
the type entry to use the correct fenced code representation
`map[string][]*string` (i.e., remove the stray trailing `*`) so the Go type
displays properly and the MD052 lint warning is resolved; locate the row
containing `Headers` in the v2DeleteTransactionMetadataResponse documentation
and replace the malformed type text with the corrected backticked type.
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Malformed type notation: extra asterisk around string.
The type map[string][]*string* has a stray trailing * that renders "string" as italic and triggers MD052 (undefined reference link [string]). This same issue appears in the other response docs that added the Headers field.
Should be:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A |
+| `Headers` | map[string][]*string | :heavy_check_mark: | N/A |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | | |
| | `Headers` | map[string][]*string | :heavy_check_mark: | N/A | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In `@pkg/client/docs/models/operations/v2reverttransactionresponse.md` at line 10,
The Markdown table for the Headers field uses a malformed type
`map[string][]*string*` with stray asterisks; update the type to
`map[string][]string` in the v2RevertTransactionResponse docs (and the other
response docs that added the Headers field) so the table shows the correct type
and no stray markdown emphasis or MD052 link errors; locate the line that
defines the Headers type in the docs file (the row mentioning `Headers`) and
remove the extra `*` characters around `string`.
| @@ -1,4 +1,5 @@ | |||
| FROM golang:1.24-alpine AS compiler | |||
| # syntax=docker/dockerfile:1 | |||
| FROM golang:1.25-alpine AS compiler | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Dockerfile" -path "*/provisioner/*" | head -20Repository: formancehq/ledger
Length of output: 91
🌐 Web query:
golang 1.25 release date Docker Hub
💡 Result:
- Go 1.25.0 release date: 2025-08-12 (August 12, 2025). [1]
- Docker Hub (official image): Go is published as the Docker Official Image
golang, with tags including1.25and patch tags such as1.25.6(plus variants like-bookworm,-alpine, Windows tags, etc.). [2]
Example pulls:
docker pull golang:1.25
docker pull golang:1.25.6Sources: [1] Go release history (go1.25.0) [2] Docker Hub “golang” official image tags.
🏁 Script executed:
cat -n ./tools/provisioner/DockerfileRepository: formancehq/ledger
Length of output: 921
Remove duplicate syntax directive on line 18 and add a non-root USER instruction in the runtime stage.
Line 18 contains an unnecessary duplicate # syntax=docker/dockerfile:1 directive. The syntax directive must appear only once as the very first line of the Dockerfile; remove the redundant occurrence.
The runtime stage (from alpine:3.21 onward) lacks a USER instruction and will run as root, a security best practice violation. Add a non-root user with USER appuser (or similar) before the ENTRYPOINT.
🤖 Prompt for AI Agents
In `@tools/provisioner/Dockerfile` at line 2, Remove the duplicate Dockerfile
syntax directive so only the single `# syntax=docker/dockerfile:1` remains at
the very top, and in the runtime stage that starts with `FROM alpine:3.21`
create a non-root user (e.g. `appuser`), set ownership where needed, and add
`USER appuser` before the `ENTRYPOINT` to ensure the container does not run as
root; keep the compiler stage name `compiler` unchanged.
1 participant
No description provided.