fix(payments): preserve stored fee parameters on replay (AL-1917)

[cobmojo]

Closes #1917.

A legacy donation can retain empty fee extras after its PaymentIntent was created but Core's completion write failed. HTTP replay previously supplied a new fee quote, changing provider metadata and payment-method parameters under the existing idempotency key. Intake now forwards the stored fee state, including legacy absence, after all existing amount and quote validations.

This preserves the remaining verified repair from closed, unmerged PR #1329. The broader fee policy is already on develop. The nine-path change retains tenant/auth checks, cents arithmetic, first-shot quotes, malformed/full-quote rejection and the existing provider identity.

Validation on published e69602146f1ff4881078912d053ecc4dabfb369e, tree bf473ec342827de9cdec1a99f862063a4dc9aa0d, base bd9acc44313761d3371996c85376373782da02fb:

• The actual HTTP/saga regression reproduces four failures before the repair and 31 passes afterward, with a provider stub that rejects changed parameters under the original key.
• The expanded current-base suite passes 142 tests in 11 files; API types, scoped formatting, data-boundary/mirror checks, strict OpenSpec and 59 delta checks pass. Scoped lint retains one existing import-order warning.
• Two independent source reviews and complete 17,635-entry preservation checks accept the exact nine-path repair and ordinary current-base merge.
• Normal full pre-push ci:preflight passes all three application builds and 4341 tests with 4 existing skips. Fresh GitHub CI/reviews and hosted QA remain outstanding.

The qualified recovery scope is same-actor, same-customer HTTP fee replay. Cross-actor metadata.user_id recovery remains unresolved: the outbox/claim does not record the first actual provider caller. This change does not infer historical attribution, remove it, invent a new payment key or claim actor-independent recovery.

Deploy Checklist (for PRs to production or develop)

• Current-head GitHub CI and final reviews pass
• Base branch confirmed: develop
• Migrations, new environment variables and deployment controls reviewed (N/A)
• Normal full pre-push gate passes
• Rollback: revert this focused repair; no schema or provider mutation
• Applicable hosted preview smoke passes

Draft while shared #1915/#1916 preview/build prerequisites remain unresolved. Refresh against their actual merged base and qualify the resulting candidate before Ready/merge. The full-gates OpenSpec task stays unchecked.

[github-actions]

Shadscan score

Score: 29/100 (grade: F) — floor: 29

Scanned packages/ui with @shadscan/cli@0.1.1. Category breakdown and failing findings are in the job summary.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 44 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f92a467b-c6c2-408a-8b0f-f23b351d222f

📥 Commits

Reviewing files that changed from the base of the PR and between e696021 and 68facf6.

📒 Files selected for processing (9)

• docs/guides/features/guest-giving-cover-fees.md
• docs/guides/operations/donation-saga-outbox.md
• openspec/changes/guest-giving-gift-processing-fee-policy/design.md
• openspec/changes/guest-giving-gift-processing-fee-policy/proposal.md
• openspec/changes/guest-giving-gift-processing-fee-policy/specs/donation-lifecycle/spec.md
• openspec/changes/guest-giving-gift-processing-fee-policy/tasks.md
• packages/api/src/donate/index.ts
• packages/api/tests/unit/donate-post-charge.test.ts
• tests/unit/donation-saga.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a724b48b-b394-4e28-a399-22165f503c69

📥 Commits

Reviewing files that changed from the base of the PR and between bd9acc4 and e696021.

📒 Files selected for processing (9)

• docs/guides/features/guest-giving-cover-fees.md
• docs/guides/operations/donation-saga-outbox.md
• openspec/changes/guest-giving-gift-processing-fee-policy/design.md
• openspec/changes/guest-giving-gift-processing-fee-policy/proposal.md
• openspec/changes/guest-giving-gift-processing-fee-policy/specs/donation-lifecycle/spec.md
• openspec/changes/guest-giving-gift-processing-fee-policy/tasks.md
• packages/api/src/donate/index.ts
• packages/api/tests/unit/donate-post-charge.test.ts
• tests/unit/donation-saga.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (9)

• GitHub Check: Cursor Security Agent: Security Reviewer
• GitHub Check: instant-nav
• GitHub Check: build
• GitHub Check: migrate
• GitHub Check: format
• GitHub Check: integrity
• GitHub Check: lint
• GitHub Check: typecheck
• GitHub Check: test-unit

🧰 Additional context used

📓 Path-based instructions (4)

Focus on correctness, type safety, server/client boundaries, async behavior, error handling, security, performance, and maintainability.

⚙️ CodeRabbit configuration file

Files:

• tests/unit/donation-saga.test.ts
• packages/api/tests/unit/donate-post-charge.test.ts
• packages/api/src/donate/index.ts

Treat package changes as shared contracts.

⚙️ CodeRabbit configuration file

Files:

• packages/api/tests/unit/donate-post-charge.test.ts
• packages/api/src/donate/index.ts

Source excerpt: `packages/api/src/*` is the single canonical layer for business database logic.

📄 CodeRabbit inference engine (packages/api/AGENTS.md)

Files:

• packages/api/src/donate/index.ts

Source excerpt: Editing files under `packages/api/**`

📄 CodeRabbit inference engine (packages/api/AGENTS.md)

Files:

• packages/api/tests/unit/donate-post-charge.test.ts
• packages/api/src/donate/index.ts

🪛 Betterleaks (1.8.1)

packages/api/tests/unit/donate-post-charge.test.ts

[high] 505-505: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.

(stripe-access-token)

🪛 LanguageTool

openspec/changes/guest-giving-gift-processing-fee-policy/design.md

[grammar] ~99-~99: Ensure spelling is correct
Context: ...eir quote atomically at intake; a later replay cannot replace a stored full quote. Ca...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

openspec/changes/guest-giving-gift-processing-fee-policy/tasks.md

[grammar] ~42-~42: Use a hyphen to join words.
Context: ...ss followed by a failed completion write through the actual handler and sag...

(QB_NEW_EN_HYPHEN)

🔇 Additional comments (11)

packages/api/tests/unit/donate-post-charge.test.ts (2)

502-507: Betterleaks flagged Line 505 as a Stripe access token. The value is a fake placeholder.

"rk_test_restricted" is a test-mode placeholder, not a real restricted key. It exposes no credential. No change is required. You can add an allowlist entry so the scanner stops blocking on this literal.

---

1-2: LGTM!

Also applies to: 346-396, 398-526, 602-602

openspec/changes/guest-giving-gift-processing-fee-policy/proposal.md (1)

20-23: LGTM!

openspec/changes/guest-giving-gift-processing-fee-policy/specs/donation-lifecycle/spec.md (1)

78-89: LGTM!

openspec/changes/guest-giving-gift-processing-fee-policy/design.md (1)

73-79: LGTM!

Also applies to: 97-104

packages/api/src/donate/index.ts (2)

196-198: Replay keeps the stored fee state and the saga does not rewrite it.

On replay, storedFeeExtras is either undefined (legacy {}) or equal to the current quote. The equality check at Lines 184-195 guarantees this. processDonationSagaOutboxEvent persists extras only when the value is truthy. For a legacy row, undefined therefore skips the write. resolveDonationSagaFeeExtras then falls back to the stored {}, so the provider parameters stay the same.

A new first-shot request (replayed: false) still passes the current quote. This matches the spec.

LGTM!

---

81-83: LGTM!

docs/guides/operations/donation-saga-outbox.md (1)

28-31: LGTM!

Also applies to: 56-67, 76-102

docs/guides/features/guest-giving-cover-fees.md (1)

50-55: LGTM!

tests/unit/donation-saga.test.ts (1)

37-37: LGTM!

Also applies to: 91-107, 848-921

openspec/changes/guest-giving-gift-processing-fee-policy/tasks.md (1)

31-32: LGTM!

Also applies to: 38-49

---

📝 Summary

• HTTP donation replays with matching charged amounts now preserve stored fee metadata, including when legacy records have no fee extras.
• If a stored full fee quote differs from the current quote, the API still returns 409. New gifts still store their fee quote at intake.
• Regression tests cover legacy replay states and retry after PaymentIntent creation when the completion write fails. They check that retry parameters remain unchanged under the same idempotency key.
• Cross-actor recovery remains out of scope. The outbox does not record the original provider caller.

Author	Lines added	Lines removed
Unavailable from the supplied repository results	Unavailable	Unavailable

Walkthrough

For HTTP donate replays with matching charged cents, intake now passes stored fee metadata to the saga. Legacy empty fee extras remain absent, while conflicting stored full quotes still return 409. Tests cover completed and processing replays, and recovery after provider success followed by a failed completion write.

Changes

Legacy donation replay

Layer / File(s)	Summary
Replay contract and intake behavior openspec/changes/guest-giving-gift-processing-fee-policy/*, packages/api/src/donate/index.ts, packages/api/tests/unit/donate-post-charge.test.ts, docs/guides/features/guest-giving-cover-fees.md, docs/guides/operations/donation-saga-outbox.md	The HTTP handler uses stored fee metadata after checking for a conflicting full quote. For legacy rows without a quote, it passes undefined. Tests cover completed and processing replays.
Provider retry and completion recovery packages/api/tests/unit/donate-post-charge.test.ts, tests/unit/donation-saga.test.ts, openspec/changes/guest-giving-gift-processing-fee-policy/…, docs/guides/operations/donation-saga-outbox.md	Tests cover retrying after provider success and a failed completion write, including unchanged provider parameters and empty fee extras. The runbook and change tasks document verification and the limitation that retry actor identity is not preserved.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: ii-ricky-bobby-ii

Merge Risk: ⚪ Minimal · up to e6960

Legacy donation retries preserve their original fee-related payment parameters. No actionable merge-blocking defect was identified; complete the planned integration checks before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e6960

The change narrows a legacy payment-retry failure without changing the established authentication, tenant, amount, or stored-quote checks. Recovery by a different actor remains a separate limitation, and production rollout has not been verified here.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Replay exposure is bounded by authentication, tenant-scoped reads, and matching charged cents, but a same-tenant caller with an existing key is not checked against the original actor by the begin RPC. A completed outbox can return its stored client secret. This ownership condition is unchanged by the fee repair.

Trust Boundaries and Controls

• observed — Request-derived quote data is validated before replay processing, while stored fee state becomes authoritative for provider parameters. Malformed or unreadable stored extras fail before the saga call.

Resilience and Maintainability Implications

• observed — The provider call precedes database completion. On a completion error, saga processing records failure; the current failure RPC clears the lock and provides a retry or terminal dead-letter transition.

Hardening Proposals

• proposed — In a separate identity-recovery change, persist the first actual provider caller and define an authorization rule for same-key replay before returning a completed intent’s client secret. Preserve the original provider actor on authorized recovery rather than inferring it from the current caller.

🚥 Pre-merge checks | ✅ 6 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (6 skipped: 6 …	Write docstrings for the functions missing them to satisfy the coverage threshold.
Repo Gate Evidence	⚠️ Warning	The PR description names the broad ci:preflight gate, but it does not list an exact focused command for the changed API and saga tests. It also reports strict OpenSpec and delta validation without n…	Add a Validation section with the exact commands and results. Include a focused command such as bunx vitest run packages/api/tests/unit/donate-post-charge.test.ts tests/unit/donation-saga.test.ts, the broader bun run ci:preflight gate, …

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses concise conventional-commit format and accurately describes the replay fix.
Description check	✅ Passed	The description is detailed, follows the required deployment checklist structure, explains the change and validation, and identifies outstanding CI, review, and smoke-test work.
Linked Issues check	✅ Passed	The PR addresses the coding requirements in [#1917]. packages/api/src/donate/index.ts keeps stored fee metadata authoritative after amount and full-quote checks, and passes undefined for legacy em…
Out of Scope Changes check	✅ Passed	The changed source, tests, documentation, OpenSpec design, proposal, specification, and task files all document or verify the legacy replay repair in [#1917]. The changes do not add cross-actor recove…
Generated Mirrors	✅ Passed	No generated skill or agent mirror files changed. The PR changes documentation, OpenSpec files, and donation API/tests only. Therefore, no canonical skill source or sync-tooling change is required for…
Tenant Safety	✅ Passed	Tenant isolation remains intact. The changed POST route runs through withOperation with authenticated donor/admin/staff roles, and it uses ctx.tenantId and ctx.userId from the authenticated cont…

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. (6 skipped: 6 unsupported.)

Full details: Repo Gate Evidence

Explanation

The PR description names the broad ci:preflight gate, but it does not list an exact focused command for the changed API and saga tests. It also reports strict OpenSpec and delta validation without naming their commands. The patch changes packages/api/src/donate/index.ts, packages/api/tests/unit/donate-post-charge.test.ts, and tests/unit/donation-saga.test.ts, so focused validation evidence is relevant.

Resolution

Add a Validation section with the exact commands and results. Include a focused command such as bunx vitest run packages/api/tests/unit/donate-post-charge.test.ts tests/unit/donation-saga.test.ts, the broader bun run ci:preflight gate, and exact OpenSpec commands such as bun run openspec:validate and bun run verify:openspec-deltas if those checks were run.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

✨ Simplify code

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}