fix(sync): skip pending auto-claim when multiple same-amount candidat…

[dale053]

Summary

• Bug: When two pending transactions with the same amount existed in the reconciliation window, the auto-claim logic would silently overwrite whichever one it matched first, destroying a distinct real transaction (issue Bug: Pending→posted reconciliation can silently destroy a distinct same-amount transaction #2013).
• Fix: Added a .limit(2) guard in find_pending_transaction — if more than one same-amount candidate is found the method returns nil, forcing a new posted entry to be created instead of blindly claiming an ambiguous pending.
• Tests: Updated the existing multi-pending test to assert the correct new behavior (new entry created, both pendings survive), and added two new cases: the ATM collision scenario from Bug: Pending→posted reconciliation can silently destroy a distinct same-amount transaction #2013 and a regression guard confirming single-candidate auto-claim still works.

Test plan

• bin/rails test test/models/account/provider_import_adapter_test.rb — all tests pass
• bin/rubocop -f github -a — no offenses
• bin/brakeman --no-pager — no new warnings
• Manually verify in dev: import two same-amount pending transactions, then post one — both pendings remain, a third posted entry is created
• Manually verify: import a single pending, then post it — still reconciles (no regression)

fixes #2013

Summary by CodeRabbit

• Bug Fixes

  • Transaction reconciliation tightened: the matching logic now considers up to two candidates and will auto-claim only when there is exactly one unique pending match; if multiple same-amount candidates exist, the system creates a new posted entry instead of claiming an ambiguous pending transaction.

• Tests

  • Added tests for same-amount collision scenarios, ensuring ambiguous pending entries are preserved and confirming auto-claim still occurs when a single matching pending transaction exists.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 311c7fb6-acc9-433a-97e7-c5b04211a7ee

📥 Commits

Reviewing files that changed from the base of the PR and between d6e373a and e1911aa.

📒 Files selected for processing (1)

• app/models/account/provider_import_adapter.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• app/models/account/provider_import_adapter.rb

---

📝 Walkthrough

Walkthrough

find_pending_transaction now materializes up to two pending candidates and only auto-claims when exactly one match exists; ambiguous same-amount candidate sets return nil. Tests assert multiple same-amount pendings are not stolen and single-pending claiming still works.

Changes

Prevent silent same-amount pending claims

Layer / File(s)	Summary
Ambiguity guard in find_pending_transaction app/models/account/provider_import_adapter.rb	find_pending_transaction repositions its start, applies limit(2), converts candidates to an array, and returns a match only when exactly one candidate exists; returns nil on multiple candidates to avoid ambiguous auto-claiming.
Test coverage for same-amount collision guard test/models/account/provider_import_adapter_test.rb	Replaces/extends tests to assert that when multiple same-amount pending entries exist, a posted transaction creates a new posted entry and leaves pendings unchanged (including their external_ids); also tests that a single same-amount pending is still claimed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• we-promise/sure#859: Related changes to pending candidate selection affecting pending→posted matching behavior.
• we-promise/sure#1783: Adjusts pending→posted auto-claim behavior and how claims are applied in import flows.

Suggested labels

contributor:verified, pr:verified

Suggested reviewers

• jjmata
• michelroegl-brunner

Poem

🐰
I hopped through code to find what's true,
Two twin withdrawals hid from view.
Now claims wait — only one may pair,
Each penny keeps its rightful share.
Happy hops — no cents vanish here!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check	❓ Inconclusive	The PR partially addresses #2013: it adds a collision guard for same-amount candidates but does not fully implement all acceptance criteria, specifically unified reconciliation paths and reversible auto-claim mechanisms.	Verify that this PR is intended as a partial fix; consider whether unified reconciliation and reversible claims (acceptance criteria 3-4) should be addressed now or deferred to future work.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: skipping pending auto-claim when multiple same-amount candidates exist, which directly addresses the bug referenced in #2013.
Out of Scope Changes check	✅ Passed	All changes are scoped to the pending→posted reconciliation logic in provider_import_adapter.rb and its test file, directly addressing the bug in #2013 with no extraneous modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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 and usage tips.}

[jjmata]

Clean, minimal fix for a tricky edge case. A few observations:

The approach is correct. .limit(2).to_a followed by return nil if candidates.size != 1 is the right shape — it avoids fetching the full result set while still detecting ambiguity. No N+1 risk, no over-fetching.

Tests are thorough. All three scenarios (ambiguous collision → new entry, ATM scenario from the issue, single-candidate → still reconciles) are covered with clear assertions. The renamed test title ("does not auto-claim pending when multiple same-amount candidates exist") is much more descriptive than the old one.

Minor observation: The comment block inside find_pending_transaction is longer than typical for this codebase (per CLAUDE.md conventions — comments should be one short line explaining the non-obvious WHY). The return nil if candidates.size != 1 line is self-documenting enough; the multi-line explanation might be better suited to the PR description (where it already lives). Not a blocker.

Overall this is a solid, focused fix — no unnecessary changes, good regression coverage. 👍

---

Generated by Claude Code