fix: Update SDK, ensure idempotency keys are unique and don't retry http 4xx

[laouji]

Relates to TS-581

Load Test 3 Findings — Payment Initiation via Orchestration (Self-Heal + InsertNewStage Fix)

Stack: knonmzexcoal-bblp
Window: 2026-09-07T19:56:59Z → 2026-09-07T20:29:09Z (launch window; workflows continued resolving until ~20:52Z)
Batch size: 500 requests (475 valid / 25 invalid, 95%/5% split), base refs loadtest3-valid-N / loadtest3-invalid-N, actual refs <base>-<8charhexuuid> — every fctl invocation, including retries, got a freshly generated UUID suffix so no request could ever collide with another on Routable's external_id.
Image under test: orchestration:bfe9fb9efb13290e9d20365db3eeed16c8751423 — contains the InsertNewStage ON CONFLICT DO NOTHING idempotency fix and the createTransferInitiationWithSelfHeal CONFLICT-recovery fix (plus a same-behavior refactor: shared self-heal helper, context.go docstring/list cleanup, untracked coverage.txt).

1. Payment-initiation status breakdown (payments API)

All 500 refs found (100%, zero missing). Zero "external_id already exists" failures — the UUID-uniqueness fix worked as intended.

Cohort	Found	PROCESSING	WAITING_FOR_VALIDATION	FAILED
valid (475 expected)	475	323	152	0
invalid (25 expected)	25	3	9	13

All 13 invalid-cohort FAILED records carry the correct, expected reason: create payable: acting_team_member is required. No unexpected failure reasons appeared anywhere in the batch.

2. Signoz error breakdown — V1 create endpoint (v2_transferInitiationsCreate)

status_message	count
object already exists (CONFLICT, self-healed)	3,474
context canceled (client-side StartToCloseTimeout firing)	498

498 context canceled errors across ~500 workflows means almost every workflow's first CreateTransferInitiation attempt hit the 60s timeout — this is not a rare edge case at this load level, it's close to universal. Each of those timed-out-but-actually-successful writes then produces a CONFLICT on the next attempt (3,474 of them, ~7 per workflow), all correctly resolved by createTransferInitiationWithSelfHeal.

3. Temporal workflow completion times (RunSend)

Status	Count
Completed	471
Failed	5 (all invalid-cohort, correctly classified)
Still Running (at last check)	24

Completion-time percentiles (n=473 completed by final check):

	min	p50	p90	p99	max	mean
RunSend duration	34.3s	942.3s	1202.7s	1212.2s	1468.6s	737.9s

Success rate among resolved workflows: 471/476 = 98.9% (vs. loadtest2's 1/500 = 0.2%).

4. CreateTransferInitiation retry count

Directly inspected 15 workflows via Temporal history (exact attempt numbers from ActivityTaskStarted.attempt): attempts of 2, 2, 5, 7, 7, 7, 8, 8, 9, 9, 9, 9, 9, 10 — avg 7.33 in that sample. The scheduled-to-last-start gap for every one of these matched, to within ~0.2s, the formula (attempts−1)×60s + cumulative RetryPolicy backoff (2s, 4s, 8s... doubling, capped at 200s) — confirming every failed attempt is genuinely consuming the full 60s StartToCloseTimeout, not queueing or something else.

Using that verified formula to estimate attempts from duration across all 473 completed workflows (a duration-based estimate, cross-validated against the 15 directly-inspected exact values above): average ≈ 7.8 attempts, median 9, distribution:

attempts	1	2	3	4	5	6	7	8	9	10	11
count	2	7	9	21	30	26	57	82	189	49	1

Self-heal was extensively exercised — 3,474 CONFLICT resolutions across the batch, essentially once per attempt beyond the first for nearly every workflow, all resolving to success (or, in one directly-inspected case, correctly surfacing a genuine terminal failure — see below).

One directly-inspected invalid-cohort failure (658e0895-ebe0-4fea-9b6c-bc10f1e3fda1-0) shows the self-heal path working exactly as designed on a genuine terminal failure: attempt 1 created the transfer initiation, Routable rejected it for the missing acting_team_member, the retry hit CONFLICT, getTransferInitiationByReference fetched the record, found it in a terminal FAILED state, and classifyExistingTransferInitiation correctly returned a non-retryable error carrying the real reason (acting_team_member is required).

5. Three-way comparison

	loadtest (v3 path, pre-fix)	loadtest2 (v1 path, self-heal removed — regression)	loadtest3 (v1 path + self-heal + InsertNewStage fix)
Window	13:54:36Z–15:17:00Z, 1000 launched	15:27:50Z–15:59:29Z, 500 launched	19:56:59Z–20:29:09Z, 500 launched
RunSend / transfer-activity success rate	~100% (old V3 self-heal loop absorbed conflicts via 11–15x retries)	~0.2% (499/500 FAILED — permanent, no recovery)	98.9% of resolved (471/476; remainder still draining, same pattern)
Completion p50 / p99	1.68s / 20.64s	73.1s / 172.3s	942.3s / 1212.2s
Avg CreateTransferInitiation attempts	1.167	2.0 (100% hit exactly 2, then permanent failure)	~7.8 (self-heals repeatedly, then succeeds)
Failure mode	none (self-healed)	timeout → CONFLICT → no recovery → permanent failure	timeout → CONFLICT → self-heal → success, repeated ~7x per workflow before landing

The regression is resolved: the fixes work as intended. InsertNewStage's ON CONFLICT DO NOTHING eliminated the infinite-retry-on-23505 hang, and createTransferInitiationWithSelfHeal turned "timeout-then-CONFLICT" from a permanent failure back into a recoverable, eventually-successful path — restoring (and, via a correctly-surfaced terminal-failure case, slightly improving on) the old V3 path's behavior.

[NumaryBot]

✅ Approve — automated review

No actionable correctness regressions were found in the current diff. Previously reported NumaryBot findings are resolved or were explicitly dismissed.

No findings.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #208 (comment)

[shipfox-ai]

This PR adds a payments-version probe that dispatches transfer initiation between the v3 payment-initiations API and the legacy v1 API, classifies SDK errors as non-retryable Temporal application errors, self-heals v3 CONFLICT via fetch-by-reference, and client-side resolves connector IDs from provider on both paths. The design is sound and the v3 path fully achieves the stated goal ("use payments v3 when possible, don't retry 4xx"). However, on the v1 fallback and the legacy StripeTransfer activity, most 4xx responses are still retried indefinitely, and one comment asserts the opposite of the actual behavior. Recommendation: approve with comments — the v1 findings only affect pre-v3 stacks and are partly documented in-code, but the incorrect comment and the StripeTransfer gap should be addressed.

Standards

No documented coding standards were found to apply to this diff, and no tooling-enforceable violations exist. One confirmed judgement-call finding, low severity:

• Duplicated v1/v3 skeleton with drift risk — internal/workflow/activities/activity_transfer_initiation.go. resolveConnectorID (lines 185–217) and resolveConnectorIDV1 (lines 294–330) share the selector validation, the "either connectorID or provider must be specified" error, and the 0/1/many cardinality cascade verbatim, differing only in the listing call and match predicate. The strings.ToUpper(request.Type) switch is duplicated at lines 89–99 and 233–244 (both currently include the identical default: return fmt.Errorf("invalid transfer type: ...") path — an earlier review claim that v1 lacked it is incorrect), and the default-description block is duplicated at lines 106–113 and 257–264. Extracting the shared validation/cardinality skeleton and parsing request.Type once before dispatch would keep the compatibility-critical fallback from silently drifting. No functional impact today.

Spec

• High — the v1 fallback still retries most 4xx forever, and its comment claims the opposite. Per the code's own known-limitation docstring (activity_transfer_initiation.go:53-66), as of formance-sdk-go v3.8.1 PaymentsErrorsEnum lacks CONFLICT, INVALID_ID, MISSING_OR_INVALID_BODY and CONNECTOR_CAPABILITY_NOT_SUPPORTED, so responses with those codes fail JSON unmarshalling and surface as plain (non-ApplicationError) errors, which createTransferInitiationV1 returns unchanged (lines 288–294). InfiniteRetryContext (internal/workflow/stages/internal/context.go:17-33) only honors its NonRetryableErrorTypes allowlist for ApplicationError types, so a 409 CONFLICT — the most likely error under the timeout-retry scenario the PR targets — retries indefinitely on pre-v3 stacks instead of failing or self-healing. Compounding this, the createTransferInitiationV1 doc comment (lines 226–228) states "a terminal error (CONFLICT included) fails the workflow immediately instead of retrying forever", which the known limitation two paragraphs up directly contradicts. Fix the comment at minimum; consider handling at least CONFLICT (e.g. by status code) without waiting for an SDK release.

• Medium — legacy StripeTransfer still retries every 4xx. internal/workflow/activities/activity_payment_stripe_transfer.go:55-58 returns raw v1 SDK errors unclassified, and the activity runs under InfiniteRetryContext (internal/workflow/stages/send/run.go:323, 663). Even a VALIDATION response — which does decode into *sdkerrors.PaymentsErrorResponse — is returned without classifyV1Error, so validation failures retry indefinitely. This is pre-existing behavior, but the PR modifies this function (connector resolution) and the spec explicitly asks not to retry HTTP 4xx, so the fix is incomplete without it.

• Medium/low — the version probe's own errors are unclassified and retried forever. internal/workflow/activities/payments_version.go:49-52 wraps any PaymentsgetServerInfo error as a plain error, so a persistent 4xx from this prerequisite (auth failure, or an old stack lacking the endpoint) is retried indefinitely under InfiniteRetryContext and blocks both the v3 and v1 initiation paths from ever being selected — the probe that gates the "don't retry 4xx" behavior is itself subject to unbounded retrying.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #208 (comment)

[shipfox-ai]

This PR upgrades formance-sdk-go to v3.8.1, adds a payments-version gate that switches between the v3 and legacy v1 transfer-initiation APIs, classifies SDK errors as retryable/non-retryable, and adds conflict self-healing with tests. The core v3 path is well designed and well tested, but several 4xx error paths still retry indefinitely and the new v1 fallback contradicts the PR's own idempotency-key fix, so as written it does not fully deliver its stated goal ("Return non-retriable errors for http status 4xx", commit 62ae544). Recommendation: request changes on the retry-classification gaps below; the Standards axis has no confirmed material finding.

Spec

1. Several 4xx paths still retry indefinitely, contradicting the PR's own 4xx requirement. All these activities run under InfiniteRetryContext (internal/workflow/stages/internal/context.go:17-29 — no MaximumAttempts), so any returned error that is not a classified/non-retryable error retries forever:

   • internal/workflow/activities/activity_transfer_initiation.go:315-320 and :336-340 (createTransferInitiationV1/resolveConnectorIDV1): per the code's own docstring (:52-62), a v1/v2 server returning CONFLICT, INVALID_ID, MISSING_OR_INVALID_BODY or CONNECTOR_CAPABILITY_NOT_SUPPORTED fails PaymentsErrorsEnum.UnmarshalJSON in SDK v3.8.1, so these requests see a plain retryable error. The limitation is documented but not mitigated — the errors could still be classified as non-retryable (e.g. via the HTTP status from the SDK error/response).
   • internal/workflow/activities/activity_payment_stripe_transfer.go:56-59 (StripeTransfer): create errors are returned raw with no classification at all, so the same four codes (and any other decoded-or-not 4xx shape) retry forever.
   • internal/workflow/activities/payments_version.go:49-51 (getPaymentsVersion): server-info errors (e.g. a 401 from a misconfigured stack) are returned raw and retry indefinitely instead of failing fast.
   • internal/workflow/activities/activity_transfer_initiation.go:144-147 (createTransferInitiationV3): any error that does not decode as *sdkerrors.V3ErrorResponse is returned raw and retryable.
     Impact: requests that can never succeed (4xx) occupy a workflow in an endless retry/backoff loop.

2. Conflict-recovery approve path misclassifies 4xx failures as retryable. internal/workflow/activities/activity_transfer_initiation.go:191-202: when ApprovePaymentInitiation fails, anything other than a decoded V3ErrorResponse with ErrorCode == VALIDATION is wrapped in a retryable temporal.NewApplicationError — including typed 4xx codes such as NOT_FOUND, which then retry forever under InfiniteRetryContext. Conversely, every VALIDATION response is silently assumed to be the benign "already approved" race without checking the message, so a genuine validation failure is swallowed and also results in a retryable error. Only transient/INTERNAL failures should stay retryable; other 4xx approval failures should pass through classifyV3Error, with the already-approved race handled narrowly.

3. The new v1 fallback reintroduces the workflowID-based idempotency key this PR explicitly moved away from. internal/workflow/activities/activity_transfer_initiation.go:305 builds the v1 Reference as WorkflowExecution.ID + ActivityID, while the v3 path (:132-137, commit dada87c) deliberately uses RunID + ActivityID because a Temporal reset reuses the WorkflowID under a new RunID. For pre-v3 stacks this newly introduced fallback therefore collides with the payment initiation created by the pre-reset run — and per finding 1, the resulting CONFLICT is not even classifiable by SDK v3.8.1, so the activity retries forever instead of failing or self-healing. The v1 path should use the same RunID-based key (or at minimum classify the collision as non-retryable).

Standards

No confirmed material finding. The documented standards (pkg/client/CONTRIBUTING.md) govern only generated client code, which this diff does not touch. The duplication between resolveConnectorID and resolveConnectorIDV1 (validation guard, cardinality cascade, repeated "VALIDATION" literals) and the twice-repeated transfer-type switch are real but deliberate-looking trade-offs on freshly written code with no correctness, security, or compatibility impact; they are reasonable non-blocking follow-up refactoring candidates, not defects.

Reviewed independently by GLM (glm-5.3-flash) and Codex (gpt-5.6-sol) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR reworks payment initiation to pick between the payments v3 and v1 APIs based on the target stack's reported payments version, adds error classification (4xx → non-retryable), a bounded retry context, conflict self-healing on v3, and client-side connector resolution to replace the removed Provider request field. The overall structure is sound and unusually well documented, and the new tests cover the classification and conflict paths well. However, I found one likely production-breaking issue in the primary new code path that should be addressed before merge, plus one moderate maintainability concern.

Recommendation: request changes (one blocking concern, one non-blocking).

Standards

1. Provider casing is not normalized in the v3 connector match (compatibility/correctness risk) — internal/workflow/activities/activity_transfer_initiation.go:232-238. resolveConnectorID passes the raw provider value into the v3 $match filter on "provider". Callers supply destination.PSP, which is documented and used as lowercase ("stripe", "wise" — see internal/workflow/stages/send/send.go:60 and run.go:337/677). The code this PR replaces deliberately uppercased the provider before sending (shared.Connector(strings.ToUpper(*request.Provider)) with the comment "Normalize to uppercase to match connector naming convention"), and connectors are stored with uppercase enum values (this PR's own stripeProvider = string(shared.ConnectorStripe); note also that the v1 fallback in this same PR matches case-insensitively via strings.EqualFold at line 357). Unless payments' $match is case-insensitive, a lowercase PSP will match zero connectors and fail non-retryably with no connector installed for provider "stripe" on v3 stacks — breaking the main feature this PR enables. Please normalize (e.g. strings.ToUpper) before the $match, or verify the server-side match semantics and document why no normalization is needed. The v1/v3 asymmetry here (case handled in one path, not the other) suggests an oversight.

2. Duplicated v1/v3 logic in activity_transfer_initiation.go (maintainability, non-blocking) — three blocks are duplicated nearly verbatim between createTransferInitiationV3 and createTransferInitiationV1: the transfer-type switch strings.ToUpper(request.Type) (lines 91 and 277), the default-description fallback (lines 107-113 and 299-305), and the guard clauses plus 0/1/many match-count cascade in resolveConnectorID (line 224) vs resolveConnectorIDV1 (line 338), which differ only in the listing call and match predicate. This duplication is compatibility-critical: the two paths must stay in lockstep, and finding chore(deps): bump the otel group with 2 updates #1 above is exactly the kind of drift (a fix or behavior applied to one path but not the other) this structure invites. Extract shared helpers for the type mapping, description fallback, and the validate-then-switch-on-match-count skeleton.

Other candidate smells reviewed and rejected as non-material: the "VALIDATION" literals (the ErrorCodeValidation constant lives in stages/internal, which activities cannot cleanly import), the write-only raw field on paymentsVersion, cursor message chains, the (connectorID, provider) parameter pair, the stripeProvider variable name, the shared NonRetryableErrorTypes list between the two retry contexts, and the reference-scheme overlap with getIK.

Spec

No spec is available for this change (the PR references an external Linear ticket, TS-581, which is not in the repository), so this axis has no confirmed material finding.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR replaces the ad-hoc v1 transfer-initiation call with a version-aware dual path (v3 payment-initiations with conflict self-healing, v1 fallback for pre-v3 payments stacks), adds client-side connector resolution now that the SDK dropped the Provider hint, introduces bounded retry semantics for PSP-facing activities, and classifies SDK errors into Temporal retryability. The design is careful and well-documented: error classification is centrally handled, the v1 fallback's known SDK gaps are explicitly documented, and the conflict self-heal path is thoroughly tested. No documented coding standard applies to this diff (pkg/client/CONTRIBUTING.md governs only generated code), so all findings below are judgement calls against baseline smells. Recommendation: approve with comments — nothing blocks merge, but the reference inconsistency in StripeTransfer (finding 1) deserves a follow-up.

Standards

1. Reference scheme inconsistency in StripeTransfer (medium) — internal/workflow/activities/activity_payment_stripe_transfer.go:51 still builds the idempotency reference as activityInfo.WorkflowExecution.ID + activityInfo.ActivityID, while this same PR documents at activity_transfer_initiation.go:166-170 and :349-352 that RunID — not WorkflowID — is required so a Temporal reset (same WorkflowID, new RunID) doesn't collide with the payment initiation the pre-reset run already created. StripeTransfer was actively modified in this PR (connector resolution, PaymentInitiationRetryContext) and remains a selectable branch (workflow.GetVersion DefaultVersion in run.go). Concrete impact: after a reset, a retrying Stripe payout collides with the pre-reset initiation; per the PR's own classifyV1Error docstring, CONFLICT isn't declared by PaymentsErrorsEnum, so the error decodes as a plain retryable failure and burns the full ~40-minute retry budget before the workflow fails. Use RunID + ActivityID here too, matching the invariant the PR establishes.

2. Duplicated error-decode cascade (low) — internal/workflow/activities/activity_transfer_initiation.go: the classify-and-return idiom (if v3Err, ok := err.(*sdkerrors.V3ErrorResponse); ok { return classifyV3Error(v3Err) }; return err) recurs at lines 192-196, 242-256, and 295-298, with the PaymentsErrorResponse twin at lines 362-365 and 380-383 — five sites total. Extract a classifySDKError(err) error helper so the two compatibility-critical paths (v3 and v1) cannot drift in how they decode and classify errors.

3. Dead field paymentsVersion.raw (low) — internal/workflow/activities/payments_version.go:18 is written and cached but never read in production code (only payments_version_test.go:30 round-trips it). Speculative generality; remove it until a consumer exists.

4. Duplicated NonRetryableErrorTypes slice (low) — internal/workflow/stages/internal/context.go:21-26 and :49-54 repeat the same four-element slice verbatim in InfiniteRetryContext and PaymentInitiationRetryContext. Hoist to a shared package variable so a future non-retryable code can't be added to one list and missed in the other.

Spec

No spec available — the PR body contains only "Relates to TS-581" (a Linear ticket not reachable via the available integrations), and no issue reference or spec file matching the branch was found. The Spec axis therefore has no confirmed material finding.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #208 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).

Summary: #208 (comment)

[shipfox-ai]

This PR gates CreateTransferInitiation between the v3 payment-initiations API and the legacy v1 API via a cached payments-module version probe, resolves connectorID from provider client-side on both paths, adds CONFLICT self-healing with deliberate error classification, and bounds the PSP-facing activity retries with a new PaymentInitiationRetryContext. The design is well-reasoned, the v3/v1 divergence points are documented inline, and the new tests cover the tricky classification branches. The findings below are maintainability-level (the worst is a reference-scheme inconsistency in StripeTransfer that this PR's own comments establish as wrong); none blocks the merge. Recommendation: approve with comments.

Standards

1. Idempotency reference uses WorkflowID instead of RunID in StripeTransfer (medium) — internal/workflow/activities/activity_payment_stripe_transfer.go:51 still sets Reference: activityInfo.WorkflowExecution.ID + activityInfo.ActivityID, but this same PR documents and applies the RunID scheme everywhere else (activity_transfer_initiation.go:362 for v1, the same rationale in createTransferInitiationV3, and getIK at activity.go:89-91): a Temporal reset restarts the same WorkflowID under a new RunID, so a WorkflowID-based reference collides with the payment initiation the pre-reset run already created. For StripeTransfer that collision surfaces as an unclassified conflict error from V1.CreateTransferInitiation, which — now that this PR moved the activity onto the bounded PaymentInitiationRetryContext (run.go:323, :663) — will exhaust its 15 attempts and fail the workflow instead of recovering. Extract one shared idempotency-reference helper and use it in all three sites.

2. Duplicated error-decode-and-classify cascade (low) — the "type-assert the SDK error, then classify" idiom recurs at five sites in internal/workflow/activities/activity_transfer_initiation.go (:192 create-v3, :252 approve path, :305 resolveConnectorID, :372 and :390 v1 paths). A small classifySDKError(err) error helper would keep the v3 and v1 compatibility paths from drifting apart in how they decode errors. No functional impact today.

3. NonRetryableErrorTypes slice duplicated verbatim (low) — internal/workflow/stages/internal/context.go:24-29 (in InfiniteRetryContext) and :52-57 (in PaymentInitiationRetryContext) repeat the same four-element list. Hoist it to a shared package variable so the two retry policies can't drift.

4. Dead field paymentsVersion.raw (low) — internal/workflow/activities/payments_version.go:22 is populated on every construction path (:73, :78, :81) and cached, but nothing in production reads it; the only consumer is payments_version_test.go:30. Inline it until a real consumer exists.

5. Committed generated artifact (low, hygiene) — coverage.txt (1,372 lines of go test -coverprofile output) is newly committed at the repo root, and .gitignore doesn't cover it. Remove it from the tree and add coverage.txt to .gitignore.

Spec

No spec available for this PR (the referenced Linear ticket TS-581 is not accessible), so this axis has no confirmed material finding.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #208 (comment)

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).

Summary: #208 (comment)

[shipfox-ai]

This PR migrates the codebase to formance-sdk-go v5, drops the silently-failing v3 payments path, makes payments 4xx errors non-retryable (classifyPaymentError), resolves connector IDs client-side where the SDK dropped the Provider hint, and switches payment-initiation activities from an unbounded to a bounded (15-attempt) retry policy with RunID-based idempotency references. The migration is careful and well-documented: the behavior changes are explained against the v2.1.0-era server semantics, the extracted helpers (parseTransferType, defaultDescription, connectorIDOrProviderRequired, resolveProviderMatch) are unit-tested, and the retry-policy swap is applied consistently at all four payment call sites in internal/workflow/stages/send/run.go. I independently inspected the riskiest areas (client-side connector resolution, the Reference collision semantics, the retry-context swap) and found no correctness, security, or compatibility defects beyond the low-severity items below. Recommendation: approve with comments — the findings are all low-severity cleanup, none blocking.

Standards

No documented standards govern this diff (no CODING_STANDARDS.md/AGENTS.md; pkg/client/CONTRIBUTING.md covers only generated code, which is untouched). All findings below are low-severity judgement calls; no hard violations. Spec: see below — no confirmed material finding there.

1. Low — committed stale build artifact coverage.txt (hygiene). A 1,372-line go test -coverprofile output file is newly tracked at the repo root (new file in the diff), is not excluded by .gitignore (which only lists .idea, vendor, orchestration, dist), and is already stale: it references files deleted by this PR's v3-path removal (e.g. internal/workflow/activities/payments_version.go, which no longer exists in the tree). Delete it and add coverage.txt (or coverage*.txt) to .gitignore so it can't be re-committed.

2. Low — duplicated decode-and-classify cascade. The idiom if pErr, ok := err.(*payments.PaymentsErrorResponse); ok { return classifyPaymentError(pErr) }; return err is repeated verbatim at internal/workflow/activities/activity_transfer_initiation.go:162-164, :180-182, and internal/workflow/activities/activity_payment_stripe_transfer.go:61-63. Extract a small classifySDKError(err) error helper so the decode step cannot drift from classification if a new caller or error type appears.

3. Low — idempotency-reference scheme encoded two ways. The RunID+ActivityID key is built in three places with inconsistent formatting: raw concatenation at activity_transfer_initiation.go:157 and activity_payment_stripe_transfer.go:56, but fmt.Sprintf("%s-%s", RunID, ActivityID) in getIK (internal/workflow/activities/activity.go:92-94). Both are fixed-length Temporal UUIDs, so there is no correctness/collision impact today — this is a consistency risk only: the "RunID, not WorkflowID" invariant is enforced in three places and a future edit to one (e.g. adding a salt or changing the format) can silently diverge from the others. Hoist a single shared reference/IK helper.

4. Low — duplicated NonRetryableErrorTypes slice. internal/workflow/stages/internal/context.go repeats the identical four-element list (ErrorCodeValidation, ErrorCodeConflict, ErrorCodeNoScript, ErrorCodeCompilationFailed) in both InfiniteRetryContext (lines ~21-26) and PaymentInitiationRetryContext (lines ~50-55). Note the list is inherited from ledger error codes — NO_SCRIPT and COMPILATION_FAILED can never occur for the PSP activities that PaymentInitiationRetryContext guards — so a shared, per-policy variable would also make that mismatch visible and prevent the two policies from drifting.

5. Low — stale docstring rationale. internal/workflow/stages/internal/context.go:36 still says the classification gap is "see classifyV1Error's docstring: some payments error codes aren't retryable but also aren't recognized as non-retryable by this SDK version". That function no longer exists (renamed classifyPaymentError, activity_transfer_initiation.go:43), and its docstring plus the v5.0.1 bump confirm all seven declared codes now decode and are marked non-retryable — the stated gap is closed. The MaximumAttempts: 15 bound may still be justified for unclassified non-SDK errors (network/5xx), but the comment's stated reason is now wrong and points at a dead symbol; update it.

Spec

No spec is available for this PR (the referenced ticket TS-581 is an external Linear item not reachable here), so no spec-axis findings could be confirmed. Explicitly: Spec has no confirmed material finding — the Spec axis is empty.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR migrates the codebase from formance-sdk-go v3 to v5, reinstates a CONFLICT self-heal path for payment transfer initiations (fetch-by-reference + reclassify instead of blindly retrying), adds client-side connector resolution from provider (the v5 SDK dropped the server-side Provider hint field), and switches PSP-facing activities from infinite retry to a bounded PaymentInitiationRetryContext. I verified both prior reports against the actual diff and code: the core migration mechanics are sound (the InsertNewStage conflict-then-reselect change is correct given Stage's composite PK, and the self-heal/validation semantics are consistent between the two activities), and no correctness, security, or compatibility defect survived verification. What remains are maintainability and hygiene findings. Recommendation: approve with comments.

Standards

1. Duplicated CONFLICT self-heal cascade across the two transfer-initiation activities (moderate — drift risk in payment recovery). internal/workflow/activities/activity_payment_stripe_transfer.go:62-84 and internal/workflow/activities/activity_transfer_initiation.go:160-188 repeat the identical sequence: decode *payments.PaymentsErrorResponse, pass non-CONFLICT errors to classifyPaymentError, on CONFLICT call getTransferInitiationByReference, and on fetch failure return a non-retryable CONFLICT error, else classifyExistingTransferInitiation(...). The new comment in the Stripe path even points at the "identical self-heal comment" in the other file. This is the recovery logic that decides whether a flaky-but-recorded payout becomes a workflow failure; if one copy is ever fixed (e.g. the fetch-failure handling or the re-validation race handling), the other will silently drift. Extract a single shared create-with-self-heal helper both activities call.

2. PaymentInitiationRetryContext docstring is stale and its non-retryable list is a misleading copy-paste (low). internal/workflow/stages/internal/context.go:35-43 still cites classifyV1Error, a symbol that no longer exists (the function is now classifyPaymentError), and claims error codes "aren't recognized as non-retryable by this SDK version" — a gap the v5.0.1 bump this same PR closes (classifyPaymentError now classifies every declared code except INTERNAL). Separately, context.go:52-57 duplicates InfiniteRetryContext's NonRetryableErrorTypes slice verbatim, including NO_SCRIPT and COMPILATION_FAILED — ledger-only codes that can never surface from the PSP activities this context guards. Neither affects runtime behavior today, but both will mislead the next person tuning payment retry policy. Hoist the shared list and rewrite the docstring.

3. Generated coverage artifact committed (low). coverage.txt (1,372 lines of go test -coverprofile output) is newly tracked at the repo root and absent from .gitignore (which only lists .idea, vendor, orchestration, dist). Remove it from the tree and add it to .gitignore.

Candidates rejected during verification: the ledger-defaulting triplication in send/run.go (pre-existing, rename-only, no impact), the (connectorID, provider) data clump and connectorIDOrProviderRequired's two-phase return (unit-tested, no correctness impact), the raw-concat vs fmt.Sprintf("%s-%s") idempotency-reference inconsistency between the transfer activities and getIK (both schemes are deterministic and collision-free per activity; no behavioral difference), and the Message Chains/feature-envy notes (style only). Independent inspection of the riskiest areas — InsertNewStage's ON CONFLICT DO NOTHING + WherePK reselect, the self-heal status classification, the reference/idempotency scheme, and resolveConnectorID's 0/1/many cascade — surfaced no additional material issues.

Spec

No spec is available for this PR (the referenced Linear ticket TS-581 is not fetchable), so the Spec axis has no confirmed material finding.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR migrates to formance-sdk-go v5, makes transfer-initiation idempotency keys unique per Temporal run, classifies payments 4xx errors as non-retryable, adds a CONFLICT self-heal path for transfer initiations, fixes InsertNewStage idempotency with ON CONFLICT DO NOTHING, and bounds payment-initiation retries. The load-test report in the PR body shows the core fixes working under load (98.9% success vs 0.2% before). I verified the retained findings against the diff and the code, including an independent check of the InsertNewStage re-select path (safe: Stage's composite bun PK at internal/workflow/stage.go:14-16 exactly matches the ON CONFLICT target). No repo-documented coding standard applies to these files, and I found no documented-standard violations; all Standards findings below are judgement-call smells. Recommendation: approve with comments — the Spec findings are hardening opportunities in error paths, not blockers, but the first one deserves a deliberate decision before merge.

Standards

No documented standards exist for these files (pkg/client/CONTRIBUTING.md covers only the generated client), so no hard violations; all findings are baseline-smell judgement calls.

1. Duplicated v1/v3 error-classification and connector-listing code — internal/workflow/activities/activity_transfer_initiation.go:54-73 (classifyPaymentError vs classifyV3Error, near-identical "INTERNAL retryable / else non-retryable" logic), :306-312 / :343-349 (the err.(*payments.PaymentsErrorResponse) decode-and-classify idiom repeated in three call sites), and :309-323 vs :347-361 (resolveConnectorIDV3 / resolveConnectorIDV1 share the list→collect-matches→resolveProviderMatch skeleton, differing only in listing call and match predicate). This is payment compatibility code where a fix applied to one path can silently miss its twin; a single parameterized classifier would collapse all three duplications.

2. Idempotency-reference invariant in two formats across three sites — internal/workflow/activities/activity_transfer_initiation.go:153 and internal/workflow/activities/activity_payment_stripe_transfer.go:48 build reference := RunID + ActivityID (raw concat), while getIK at internal/workflow/activities/activity.go:89-91 uses fmt.Sprintf("%s-%s", RunID, ActivityID). Same "RunID, not WorkflowID" invariant, two formats; no collision today (fixed-length UUID RunID), but a future change to one site can drift from the others. Hoist one helper next to getIK.

3. Duplicated ledger/holding-account defaulting block ×3 — internal/workflow/stages/send/run.go:131-137, :158-164, :229-235 repeat the ledgerName/holdingAccount defaulting verbatim in runPaymentToWallet, savePayment, and runPaymentToAccount. A small paymentSourceAccounts(source) helper would remove the drift surface.

Spec

The PR body (a load-test findings/validation report) is the only spec; its implied requirements — SDK v5 migration, RunID-based unique references, no retry on 4xx, CONFLICT self-heal, InsertNewStage ON CONFLICT DO NOTHING, bounded retry context — are all present and verified in the diff.

1. [High] Self-heal fetch failure re-introduces the permanent-failure mode this PR set out to fix — internal/workflow/activities/activity_transfer_initiation.go:202-206: after a CONFLICT, if getTransferInitiationByReference fails for any reason, the activity returns a NewNonRetryableApplicationError and the workflow fails permanently. That fetch fails not only on transient transport errors but also when the $match query returns anything other than exactly one row (:231-233, including 0 matches). The code comment justifies this as avoiding "silently retrying it forever", but the retry policy (PaymentInitiationRetryContext, internal/workflow/stages/internal/context.go:42-52) already bounds retries at 15 attempts — a retryable error here would not retry forever. Under exactly the load conditions this PR targets (the PR body reports 498 context canceled timeouts), a transient failure of the conflict-resolution fetch converts a recoverable CONFLICT into a permanent workflow failure. Suggest returning a retryable error (or the raw fetch error) and letting the bounded retry policy do its job.

2. [Medium] Benign-race detection keys on error-message substring matching — internal/workflow/activities/activity_transfer_initiation.go:277: the approve path treats a VALIDATION error as a benign race only if strings.Contains(strings.ToLower(pErr.ErrorMessage), "already"). This is brittle against upstream wording: if the server phrases the race differently (or rewords it), a benign race becomes a non-retryable failure via classifyPaymentError and the workflow dies; conversely the substring could mask a genuine failure. Suggest keying on a stable discriminator (a dedicated error code, or re-fetching status to confirm) rather than message text.

3. [Low] Scope and intent items worth explicit sign-off — two things go beyond what the PR body describes: (a) commit 7be7759 ("Use asyncronous payments v3 endpoint to initiate transfers when possible") was reversed — the final state always targets the v1 transfer-initiations API, with an in-code rationale that the v3 path silently swallowed failures (internal/workflow/activities/activity_transfer_initiation.go:87-92); (b) the payments-version probing/TTL cache (internal/workflow/activities/payments_version.go) and client-side provider→connectorID resolution (failing fast on 0/>1 matches) are new machinery absent from the PR body — the code comments claim they replicate v2.1.0 server-side semantics, which is plausible, but the behavioral change deserves explicit confirmation, as does the intentional abandonment of the v3 initiation path.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

[shipfox-ai]

This PR migrates orchestration from formance-sdk-go v3 to v5 (type/package reshuffles, GetAccount→GetAccountLedger, script Plain now *string), reworks transfer-initiation creation to resolve the connector client-side (the v5 TransferInitiationRequest dropped the Provider hint), adds a CONFLICT self-heal shared by CreateTransferInitiation and StripeTransfer, classifies payments API errors as non-retryable (fixing the v3.8.1 PaymentsErrorsEnum decode gap), introduces a bounded PaymentInitiationRetryContext for PSP calls, and makes InsertNewStage idempotent on workflow retry. The design is well-reasoned, the new behavior is covered by unit tests, and I verified the new ON CONFLICT clause matches the actual composite PK (instance_id, stage, temporal_run_id, migration 6 in internal/storage/migrations.go). No documented coding standard applies (pkg/client/CONTRIBUTING.md covers only the untouched generated client). Recommendation: approve with comments — one compatibility item below should be confirmed before cutover.

Standards

1. Compatibility: transfer-initiation reference format changes mid-flight, breaking idempotency continuity across a rolling deploy (medium). internal/workflow/activities/activity_transfer_initiation.go:143 and internal/workflow/activities/activity_payment_stripe_transfer.go:48 now build the dedup reference as RunID + ActivityID, whereas the code being replaced used WorkflowExecution.ID + ActivityID (see target-base/internal/workflow/activities/activity_transfer_initiation.go:61). The new RunID-based scheme is the right call going forward (it correctly distinguishes post-reset runs), but the payments service dedupes by Reference and is not itself being upgraded here, so a workflow execution that was mid-retry at cutover — previous attempt reached payments and was recorded, response lost, activity retry pending — will compute a different reference under the new code, see no conflict, and create a second transfer initiation (double payout). Please confirm no in-flight executions carry a pending transfer-initiation retry across this deploy, or accept and document the window.

The remaining candidates from both review passes (duplicated RunID + ActivityID construction vs. getIK's dash-separated format, the repeated "assert *payments.PaymentsErrorResponse then classify" shape, the thrice-repeated ledger/holding-account defaulting in internal/workflow/stages/send/run.go, the stripeProvider package var, message chains like resp.TransferInitiationsCursor.Cursor.Data, the (connectorID, provider) clump, and naming of connectorIDOrProviderRequired) were verified against the code and are accurate observations, but each is a pure maintainability/style preference with no correctness, security, compatibility, or test-risk impact today — including the format mismatch with getIK, which is unambiguous because Temporal RunIDs and ActivityIDs are fixed-length UUIDs. They are noted here only so the authors know they were considered and deliberately not treated as blocking.

Spec

No confirmed material finding — no spec is available for this change, so the spec axis could not be evaluated.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.