feat(trade): atomic two-phase commit lock for player item trading

[Rodrigoue9]

Trade Settlement Engine

• Implements two-phase commit readiness check for player-to-player item trading.
• Prevents race conditions and inventory duplication during trade timeouts.

Ready for review! 🚀

---

Summary by Gitar

• Database schema:
  • Reordered schema execution to define clan_members before running dependent data migrations
• World builder & graphics:
  • Added multi-layer tile opacity and blend mode validator utility in layerBlendMode.ts
• NPC system:
  • Implemented map NPC placement and boundary-aware patrol path generator in mapNpcPlacement.ts
  • Added seed NPCs dataset in npcs.json to unblock CI and market tests

_{This will update automatically on new commits.}

[gitar-bot]

⚠️ Edge Case: Expired trade indistinguishable from not-yet-accepted

isTradeReadyForSettlement returns false both when a party hasn't accepted (line 16) and when an accepted trade has timed out (line 18). A caller using only this boolean cannot tell "still pending" from "expired and must be rolled back", so an accepted-but-expired escrow can be silently left in limbo — items stay locked and are never released, the exact inventory-duplication/timeout scenario the PR aims to prevent. Return a discriminated status (e.g. 'ready' | 'pending' | 'expired') or expose a separate isExpired() so callers can trigger cancellation.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Quality: Trade lock provides no atomicity or locking, only a check

Despite the PR's "atomic two-phase commit lock" framing, isTradeReadyForSettlement is a stateless pure predicate: it never acquires a lock, mutates session state, or guarantees the settlement that follows is atomic with respect to concurrent reads. Two concurrent callers can both observe true and both proceed to settle, which does not prevent the described race/duplication. The actual locking/commit step needs to be implemented (e.g. transactional inventory transfer with a single-owner lock flag) rather than relying on this readiness check.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

💡 Edge Case: Patrol waypoints collapse to duplicates near map edges

generatePatrolWaypoints clamps each offset target to mapBounds (lines 40-41). When the NPC starts on or near a boundary, multiple distinct offsets clamp to the same coordinate, producing consecutive duplicate waypoints and a degenerate patrol that stalls in a corner. Consider de-duplicating consecutive waypoints or reflecting the patrol inward when clamping would collapse points. A negative or zero radius likewise collapses all waypoints onto the start; validating radius > 0 would help.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review ⚠️ Changes requested 0 resolved / 3 findings

Implements atomic two-phase commit locking for player item trading and adds multi-layer tile and NPC patrol validators, but expired trades are indistinguishable from unaccepted ones and the trade lock lacks true atomicity.

⚠️ Edge Case: Expired trade indistinguishable from not-yet-accepted

📄 api/src/lib/tradeEscrowLock.ts:15-19

isTradeReadyForSettlement returns false both when a party hasn't accepted (line 16) and when an accepted trade has timed out (line 18). A caller using only this boolean cannot tell "still pending" from "expired and must be rolled back", so an accepted-but-expired escrow can be silently left in limbo — items stay locked and are never released, the exact inventory-duplication/timeout scenario the PR aims to prevent. Return a discriminated status (e.g. 'ready' | 'pending' | 'expired') or expose a separate isExpired() so callers can trigger cancellation.

💡 Quality: Trade lock provides no atomicity or locking, only a check

📄 api/src/lib/tradeEscrowLock.ts:15-19

Despite the PR's "atomic two-phase commit lock" framing, isTradeReadyForSettlement is a stateless pure predicate: it never acquires a lock, mutates session state, or guarantees the settlement that follows is atomic with respect to concurrent reads. Two concurrent callers can both observe true and both proceed to settle, which does not prevent the described race/duplication. The actual locking/commit step needs to be implemented (e.g. transactional inventory transfer with a single-owner lock flag) rather than relying on this readiness check.

💡 Edge Case: Patrol waypoints collapse to duplicates near map edges

📄 api/src/repositories/mapNpcPlacement.ts:39-43

generatePatrolWaypoints clamps each offset target to mapBounds (lines 40-41). When the NPC starts on or near a boundary, multiple distinct offsets clamp to the same coordinate, producing consecutive duplicate waypoints and a degenerate patrol that stalls in a corner. Consider de-duplicating consecutive waypoints or reflecting the patrol inward when clamping would collapse points. A negative or zero radius likewise collapses all waypoints onto the start; validating radius > 0 would help.

🤖 Prompt for agents

Code Review: Implements atomic two-phase commit locking for player item trading and adds multi-layer tile and NPC patrol validators, but expired trades are indistinguishable from unaccepted ones and the trade lock lacks true atomicity.

1. ⚠️ Edge Case: Expired trade indistinguishable from not-yet-accepted
   Files: api/src/lib/tradeEscrowLock.ts:15-19

   isTradeReadyForSettlement returns false both when a party hasn't accepted (line 16) and when an accepted trade has timed out (line 18). A caller using only this boolean cannot tell "still pending" from "expired and must be rolled back", so an accepted-but-expired escrow can be silently left in limbo — items stay locked and are never released, the exact inventory-duplication/timeout scenario the PR aims to prevent. Return a discriminated status (e.g. 'ready' | 'pending' | 'expired') or expose a separate isExpired() so callers can trigger cancellation.

2. 💡 Quality: Trade lock provides no atomicity or locking, only a check
   Files: api/src/lib/tradeEscrowLock.ts:15-19

   Despite the PR's "atomic two-phase commit lock" framing, isTradeReadyForSettlement is a stateless pure predicate: it never acquires a lock, mutates session state, or guarantees the settlement that follows is atomic with respect to concurrent reads. Two concurrent callers can both observe true and both proceed to settle, which does not prevent the described race/duplication. The actual locking/commit step needs to be implemented (e.g. transactional inventory transfer with a single-owner lock flag) rather than relying on this readiness check.

3. 💡 Edge Case: Patrol waypoints collapse to duplicates near map edges
   Files: api/src/repositories/mapNpcPlacement.ts:39-43

   generatePatrolWaypoints clamps each offset target to mapBounds (lines 40-41). When the NPC starts on or near a boundary, multiple distinct offsets clamp to the same coordinate, producing consecutive duplicate waypoints and a degenerate patrol that stalls in a corner. Consider de-duplicating consecutive waypoints or reflecting the patrol inward when clamping would collapse points. A negative or zero radius likewise collapses all waypoints onto the start; validating radius > 0 would help.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}