fix(deals): complete deal/archive offer only when ALL milestones settled (#108)

[adamkrawczyk]

What

releaseMilestonePayment() completed the entire deal and archived the backing offer on the release of any single milestone — even with sibling milestones still outstanding (pending / in_progress / delivered-but-unreleased / disputed). Multi-milestone deals were misrepresented as fully settled to both parties, to dashboards, and to fee-revenue reporting, while 2/3 of the value was still in flight — and the seller's offer was pulled off the market mid-deal.

Implements the approved Option 1 from the decision on #108: guard the deals.status='completed' + offers.status='archived' writes with the coverage-not-existence idiom already proven in this same file (completeDealMilestones()'s unbacked-milestone check, deal-helpers.ts ~486).

Change

apps/api/src/shared/deal-helpers.ts — releaseMilestonePayment(), both completion paths:

1. Funded path (CAS transaction): both UPDATEs gain AND NOT EXISTS (SELECT 1 FROM milestones sibling WHERE sibling.deal_id = $1 AND sibling.id != $2 AND sibling.status NOT IN ('accepted','cancelled')). Uses payment.deal_id from the existing join — no extra reads.
2. Zero-payment early-return path: same guard added.
3. New: deal-row FOR UPDATE lock at transaction start (both paths). Breaker attack fix(WIS-249): make browse audit writes nonblocking #2 found that under READ COMMITTED, two parallel releases of sibling milestones each observe the other still outstanding → both skip completion → fully-settled deal stuck at 'delivered' forever. The lock makes sibling-coverage check + completion atomic per deal. Single lock row per transaction, no lock-ordering cycle (cross-deal parallel releases covered by test).

Single-milestone deals (today's common case): the NOT EXISTS is trivially true — behavior preserved byte-for-byte, covered by an explicit legacy regression test.

Impact on callers

All 4 call sites (payments.ts buyer-accept, disputes.ts ×2, completeDealMilestones tail) get the corrected semantics from the single shared implementation — no per-caller changes needed. completeDealMilestones()'s per-milestone loop now converges correctly: each release marks its milestone accepted; the LAST one flips the deal.

audit_log 'payment.release' rows, notifications, fee math, settlement-integrity gate, and the buyer-signed release flow are untouched.

Breaker report

RED proof (pre-fix, same branch minus the fix): 4/7 tests failed —

AssertionError: expected 'completed' not to be 'completed'
 ❯ milestone-release-deal-guard.test.ts:148 — deal completed off 1 of 3 milestones
 ❯ :161 — deal completed while pending sibling outstanding
 ❯ :222 — deal completed while DISPUTED sibling outstanding
 ❯ zero-payment path — deal completed while in_progress sibling outstanding
Tests  4 failed | 3 passed (7)

Post-fix: 9/9 green, including 2 concurrency attacks.

Attacks run (≥3):

1. Boundary/combinatorial: single-milestone (completes ✓), cancelled-sibling (completes ✓ — cancelled = settled), disputed-sibling (blocks ✓), last-milestone-release (completes ✓), zero-payment variants of both (✓). Milestone statuses are NOT NULL + CHECK-constrained (migrations/001_init.sql:88) — no NULL trap in NOT IN.
2. Concurrency (the real hole): Promise.all release of 2 sibling milestones — pre-lock this leaves the deal stuck at 'delivered' with all milestones accepted (both txns skip completion). Fixed by the FOR UPDATE deal-row lock; test asserts completed+archived. Cross-deal parallel releases: no deadlock (different rows), test asserts both settle.
3. Error path: zero-payment path on a milestone whose deal row doesn't exist (dangling ref) — FOR UPDATE subselect returns no rows, no throw, milestone still transitions. Full existing suite (370/370) confirms no regression on dispute paths, idempotency, silent-zero guard, onchain-release-integrity, lifecycle smoke.

Verification (H5 full suite)

• npm run build — 0 errors (all workspaces)
• npm run test — 370/370 pass (50 files; 9 new tests)
• bash scripts/lint-routes.sh — clean

Notes

• SKILL.md checked: no public claims about deal-status completion semantics — no doc drift (fee split / escrow address / release flow untouched).
• 5-step stage: FIX (delete/simplify rejected — the completion transition is load-bearing, consumed behavior; this is the minimal correct repair).
• 5-step stage (musk): FIX. Approved via decision on releaseMilestonePayment() completes the whole deal + archives the offer on release of ONE milestone (multi-milestone deals) #108 (settlement-semantics scope-boundary cleared by owner decision on the issue).

Closes #108

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[adamkrawczyk]

Merge-order coordination with #107 (per the decision on #108): both PRs edit releaseMilestonePayment()'s CAS block — #107 adds the WHERE status='funded' RETURNING compare-and-swap, this PR adds the deal-row FOR UPDATE + guarded completion UPDATEs. Semantically they compose (the lock runs before the CAS; the guard doesn't touch the intent UPDATE). Whichever merges second needs a mechanical rebase of apps/api/src/shared/deal-helpers.ts — expect a small textual conflict, trivially resolvable by keeping both changes. Not self-merging either.