feat(coordinator): extract rider protocol logic into :common coordinators

[variablefate]

Summary

Extracts rider-side protocol logic from the 5,731-line rider-app/RiderViewModel god-object into three focused domain coordinators in :common/coordinator/, as specified in Issue #65.

New files in :common/coordinator/:

• AvailabilityMonitorPolicy.kt — pure policy object for pre-confirmation driver availability monitoring; adapted from rider-app with RideStage replaced by isWaitingForAcceptance: Boolean for :common compatibility
• OfferCoordinator.kt — offer sending (direct / broadcast / RoadFlare), batch RoadFlare with proximity ordering, pre-confirmation driver monitoring (Issue Update rider UI when selected driver goes offline #22), SharedFlow<OfferEvent> output
• PaymentCoordinator.kt — HTLC lock, Kind 3175 confirmation, escrow-bypass retry dialog, PIN verification, SAME_MINT preimage share, CROSS_MINT Lightning bridge + pending poll, ride-completion HTLC marking, post-confirm ack timeout (60 s); all AtomicBoolean CAS race guards from the original ViewModel preserved exactly
• RoadflareRiderCoordinator.kt — Kind 3186 key-share listener, Kind 3188 key-ack, Kind 3189 driver-ping stub (pending NostrService.publishDriverPing())

Binding constraints respected:

• No @Hilt / @Singleton / @Inject annotations (Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52 owns the DI migration); each coordinator has // TODO(#52): convert to @Singleton @Inject
• RideSession, DriverInfo, ChatMessage, RouteResult left in current packages (no moves, no conflicts with parallel Decompose high-churn Compose screens into focused components #67 session)
• No Android Context or ViewModel in constructor — every coordinator is unit-testable without a ViewModel
• Full KDoc on every public symbol; internal on implementation details

Remaining steps (this PR)

• A-4: Wire coordinators into rider-app/RiderViewModel.kt — ViewModel becomes thin state-composition layer
• A-5: Migrate roadflare-rider/RiderViewModel.kt to consume common coordinators (adapter pattern — keep existing types stable)

Test plan

• Confirm :common module compiles with ./gradlew :common:compileDebugKotlin
• Confirm rider-app still builds after A-4 wiring with ./gradlew :rider-app:assembleDebug
• Confirm RoadFlare rider flow (key share → ack → ping) still works end-to-end
• Confirm direct offer → acceptance → HTLC confirmation → PIN → preimage share flow
• Confirm cross-mint bridge payment flow (pending + poll resolution)
• Regression: escrow failure dialog with retry and auto-cancel at deadline

🤖 Generated with Claude Code
Closes #65

[variablefate]

Code review

Found 7 issues (all fixed in 17674a7):

1. RoadflareRiderCoordinator.destroy() cancelled viewModelScope — scope.cancel() was called on the ViewModel's own scope, which would have killed every coroutine in the ViewModel mid-session.

ridestr/common/src/main/java/com/ridestr/common/coordinator/RoadflareRiderCoordinator.kt

Lines 221 to 228 in 17674a7

	* Stop all subscriptions. Call from the owning ViewModel's `onCleared()`.
	* Does NOT cancel [scope] — the coordinator does not own the scope lifecycle.
	*/
	fun destroy() {
	stopKeyShareListener()
	Log.d(TAG, "Destroyed")
	}
	}

2. Double destination reveal on correct PIN — handlePaymentEvent(PinVerified) called revealPreciseDestination() after PaymentCoordinator.handlePinSubmission() already published the DESTINATION action, producing a duplicate Kind 30181 entry.

ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

Lines 5369 to 5383 in 17674a7

	at = now
	)
	) else emptyList(),
	finalFare = event.finalFareSats,
	invoice = null,
	createdAt = now
	)
	handleRideCompletion(synthetic)
	}
	is PaymentEvent.DriverCancelled -> {
	handleDriverCancellation(event.reason, "coordinator")
	}
	is PaymentEvent.PinVerified -> {

3. HTLC unlocked for legacy driver + SAME_MINT — The claimSuccess=null branch in handleRideCompletion() called clearHtlcRideProtected() despite the comment saying "keep LOCKED for safety". PaymentCoordinator.handleCompletion() correctly does nothing in this case; the ViewModel's copy undid that.

ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

Lines 4850 to 4866 in 17674a7

	val state = _uiState.value
	val session = state.rideSession
	val finalFareSats = statusData.finalFare?.toLong() ?: state.fareEstimate?.toLong() ?: 0L
	val driver = session.selectedDriver
	val driverProfile = driver?.let { state.driverProfiles[it.driverPubKey] }
	val ridePaymentMethod = session.activePaymentMethod ?: settingsRepository.getDefaultPaymentMethod()
	// Conditionally mark HTLC based on driver's claim result
	val paymentHash = session.activePaymentHash
	if (paymentHash != null) {
	val completedAction = statusData.history.filterIsInstance<DriverRideAction.Status>()
	.lastOrNull { it.status == DriverStatusType.COMPLETED }
	val claimSuccess = completedAction?.claimSuccess
	if (claimSuccess == true) {
	// Driver confirmed claim succeeded — mark HTLC as claimed
	val marked = walletService?.markHtlcClaimedByPaymentHash(paymentHash) ?: false

4. Availability monitoring not stopped after acceptance — handleOfferEvent(Accepted) never called offerCoordinator.onAcceptanceHandled(), leaving the driver-availability and Kind 5 deletion subscriptions open. This could emit spurious DriverUnavailable events after the ride was already confirmed.

ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

Lines 5502 to 5515 in 17674a7

	}
	is OfferEvent.SendFailed -> {
	_uiState.update { current ->
	current.copy(
	error = event.message,
	rideSession = current.rideSession.copy(isSendingOffer = false)
	)
	}
	}
	is OfferEvent.Accepted -> {
	val acceptance = event.acceptance
	Log.d(TAG, "OfferEvent.Accepted from ${acceptance.driverPubKey.take(8)} (batch=${event.isBatch})")

5. Insufficient-funds dialog never shown — handleOfferEvent(InsufficientFunds) only cleared isSendingOffer and did not set showInsufficientFundsDialog, insufficientFundsAmount, or the pending-driver fields, making the deposit dialog invisible.

ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

Lines 5532 to 5546 in 17674a7

	OfferEvent.BroadcastTimedOut -> {
	updateRideSession { copy(broadcastTimedOut = true) }
	}
	OfferEvent.DriverUnavailable -> {
	updateRideSession { copy(showDriverUnavailableDialog = true) }
	}
	is OfferEvent.BatchProgress -> {
	Log.d(TAG, "Batch progress: ${event.contacted}/${event.total} drivers contacted")
	}
	is OfferEvent.InsufficientFunds -> {
	Log.w(TAG, "Insufficient funds: shortfall=${event.shortfall} sats (roadflare=${event.isRoadflare}, batch=${event.isBatch})")
	_uiState.update { current ->

6. Coordinator state not restored after process death — restorePostAcceptanceState() never called paymentCoordinator.restoreRideState(), so after an app restart the coordinator held empty dedup sets and no active confirmation context, risking re-processing of already-handled driver events and a reset PIN counter.

ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

Lines 1042 to 1060 in 17674a7

	if (confirmationEventId != null) {
	paymentCoordinator.restoreRideState(
	confirmationEventId = confirmationEventId,
	paymentPath = paymentPath,
	paymentHash = activePaymentHash,
	preimage = activePreimage,
	escrowToken = escrowToken,
	pickupPin = pickupPin,
	pinVerified = pinVerified,
	destination = destination,
	postConfirmDeadlineMs = postConfirmAckDeadlineMs
	)
	}
	// B10: Post-confirm ack timeout — restart-stable via absolute deadline
	if (confirmationEventId != null) {
	if (postConfirmAckDeadlineMs > 0) {
	val remaining = postConfirmAckDeadlineMs - System.currentTimeMillis()
	if (remaining <= 0) {

7. Dead confirmationInFlight gate + autoConfirmRideLegacy — The ViewModel still held its own AtomicBoolean confirmationInFlight, reset it in resetRideUiState(), and kept 260-line autoConfirmRideLegacy with @Suppress("unused"). The coordinator owns the active CAS gate; the ViewModel's reset was a no-op against the wrong lock. Removed the legacy function and its private helpers (escrowFailureMessage, startEscrowRetryDeadline) along with 3 now-unused imports.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[variablefate]

Code review — Second pass

First pass fixed 7 bugs. Five fresh review agents ran independently against the current HEAD and surfaced more findings. Summary: 4 more real bugs fixed in ec5f01f; 5 findings verified-false or out of scope (details below).

Fixed in ec5f01f

1. Double post-confirm ack timeout after process-death restore. paymentCoordinator.restoreRideState() (added in the first pass) armed the coordinator's timer, and restorePostAcceptanceState() then armed the ViewModel's own timer for the same deadline. Two timers would both fire handleDriverCancellation() ~60 s later. Consolidated into the coordinator; removed the ViewModel's startPostConfirmAckTimeout + postConfirmAckTimeoutJob.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 1040 to 1068 in ec5f01f

   	// Coordinator owns the post-confirm ack timer. Compute the effective deadline here so
   	// legacy saves (no deadline persisted) still arm a fresh timer, and an already-expired
   	// deadline triggers an immediate cancel without touching the coordinator's timer.
   	if (confirmationEventId != null) {
   	val effectiveDeadline = when {
   	postConfirmAckDeadlineMs > 0 -> postConfirmAckDeadlineMs
   	stage == RideStage.DRIVER_ACCEPTED ->
   	System.currentTimeMillis() + POST_CONFIRM_ACK_TIMEOUT_MS
   	else -> 0L
   	}
   	if (effectiveDeadline in 1..System.currentTimeMillis()) {
   	Log.w(TAG, "Post-confirm ack timeout expired during process death — cancelling")
   	handleDriverCancellation("No response from driver", source = "postConfirmAck")
   	return // CRITICAL: do not continue into re-subscriptions/service updates
   	}
   	paymentCoordinator.restoreRideState(
   	confirmationEventId = confirmationEventId,
   	paymentPath = paymentPath,
   	paymentHash = activePaymentHash,
   	preimage = activePreimage,
   	escrowToken = escrowToken,
   	pickupPin = pickupPin,
   	pinVerified = pinVerified,
   	destination = destination,
   	postConfirmDeadlineMs = effectiveDeadline
   	)
   	}
   	// Re-subscribe to relevant events

2. Missing "Case 2" same-ride cancel guard. The original autoConfirmRide() handled two post-suspension cases after confirmRide() returned: Case 1 (cross-ride, acceptance changed → targeted cancel only) and Case 2 (same-ride, rider cancelled while we were suspended → targeted cancel + author-wide cleanup). The coordinator extraction only kept Case 1. Added PaymentEvent.ConfirmationCancelledBySelf and distinguished the two cases by currentAcceptanceEventId == null.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 684 to 707 in ec5f01f

   	rideEventIds.add(eventId)
   	// Post-suspension stale-ride guard. Two cases that share the same symptom
   	// (currentAcceptanceEventId != acceptance.eventId) but need different cleanup:
   	// - Case 1 (cross-ride): a NEW ride's acceptance is active → targeted cancel
   	// only; never run author-wide NIP-09 cleanup because
   	// it would delete the new ride's live events.
   	// - Case 2 (same-ride cancel): the rider cancelled and coordinator state was
   	// reset (currentAcceptanceEventId == null) → safe to
   	// run author-wide cleanup; no other ride is active.
   	if (currentAcceptanceEventId != acceptance.eventId) {
   	if (currentAcceptanceEventId == null) {
   	Log.w(TAG, "[$rideCorrelationId] Ride cancelled during confirmRide() suspension")
   	_events.emit(PaymentEvent.ConfirmationCancelledBySelf(eventId, acceptance.driverPubKey))
   	} else {
   	Log.w(TAG, "[$rideCorrelationId] Stale confirmation — acceptance changed post-suspend")
   	_events.emit(PaymentEvent.ConfirmationStale(eventId, acceptance.driverPubKey))
   	}
   	// confirmationInFlight stays true — new ride owns the lock (or resetInternalState
   	// already cleared it for the self-cancel case).
   	return@launch
   	}
   	// Protect HTLC from auto-refund now that the ride is confirmed.

3. retryEscrowLock() bypassed the CAS gate. It called runConfirmation() directly, skipping the confirmationInFlight.compareAndSet(false, true) guard that lives only in onAcceptanceReceived(). A rapid double-tap on "Retry" would launch two concurrent confirmation coroutines, violating the one-confirmation-per-ride invariant. Added the same CAS guard.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 371 to 384 in ec5f01f

   	/**
   	* Retry the HTLC escrow lock after the user taps "Retry" in the escrow failure dialog.
   	*
   	* Cancels the auto-cancel deadline timer, clears the dialog state, and re-runs the
   	* confirmation flow. The CAS gate was reset when the lock failed, so this call succeeds.
   	* A rapid double-tap on "Retry" is guarded by the same CAS: the second tap sees
   	* [confirmationInFlight] already true and becomes a no-op.
   	*
   	* No-op if there is no pending retry acceptance (guards against stale UI interactions).
   	*/
   	fun retryEscrowLock() {
   	val acceptance = pendingRetryAcceptance ?: return
   	val inputs = pendingRetryInputs ?: return
   	if (!confirmationInFlight.compareAndSet(false, true)) {

4. pingDriver() was a lying stub. Emitted RoadflareRiderEvent.PingSent optimistically without publishing any Kind 3189 event (the underlying NostrService.publishDriverPing() doesn't exist yet). Callers observing PingSent would believe a driver ping was sent when it was not. No callers existed, so removed the stub method, the PingSent event variant, and the ViewModel's dead handler branch. Added a TODO for Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/RoadflareRiderCoordinator.kt

   Lines 188 to 197 in ec5f01f

   	status = status
   	)
   	}
   	// TODO(#52): add `pingDriver()` once NostrService exposes a `publishDriverPing()` method.
   	// The coordinator class-level KDoc lists this capability. The method was deliberately
   	// omitted until the underlying publisher exists, to avoid a stub that lies to callers by
   	// emitting PingSent for a no-op.
   	/**

Findings that advanced to verification but were not fixed

• OfferCoordinator wired but never driven. The ViewModel collects offerCoordinator.events and calls onAcceptanceHandled()/destroy(), but never calls any offer-sending method (sendRideOffer, broadcastRideRequest, sendRoadflareToAll). All offer sending still flows through the legacy ViewModel methods; the coordinator sits idle. This isn't a regression — offers work fine — but the class's central purpose is undelivered. Scope: actually driving the coordinator is a major ViewModel rewrite that belongs in its own PR (tracked by Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52's Hilt migration). Leaving the wiring as pre-plumbing.

• HTLC completion logic duplicated between ViewModel and coordinator. handleRideCompletion() re-runs the same markHtlcClaimedByPaymentHash / clearHtlcRideProtected / refreshBalance branches the coordinator already ran. Idempotent — not a functional bug — but entropy. Deferring to the coordinator-only ownership migration, same rationale as above.

Verified-false / out of scope

• AvailabilityMonitorPolicy dead staleness check (Agents Replace expanded area search button with hashtag-based ride discovery popup menu #2, Bitcoin price service fetches price twice every 5 minutes #5): the timestamp update happens before the policy call so its internal eventCreatedAt < lastSeenTimestamp guard is unreachable. Pre-existing pattern faithfully copied from main — not introduced by this PR.

• offerCoordinator not closed in closeAllRideSubscriptionsAndJobs() (Agent Driver app needs wallet setup screen in onboarding flow #1): moot because the coordinator never opens subscriptions (see "wired but never driven" above).

• RoadflareRiderCoordinator Kind 3186 subscription has no re-arm (Agent Transaction ledger missing mint fees, causing balance discrepancy #4): the subscription lifecycle matches existing patterns elsewhere in the codebase; no re-arm was present before the extraction either.

• roadflare-rider still calls nostrService.disconnect() (Agent Transaction ledger missing mint fees, causing balance discrepancy #4): pre-existing issue from PR Phase 2: Wire roadflare-rider to ridestr :common #54, not introduced here.

• OfferCoordinator scope fragility (Agent Transaction ledger missing mint fees, causing balance discrepancy #4): structural concern about future misuse, not a current bug.

Net change vs first pass: −80 / +77 lines (PaymentCoordinator, RoadflareRiderCoordinator, RiderViewModel). Build will run in CI.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[variablefate]

Code review — Third pass

First two passes fixed 11 bugs across commits 17674a7 and ec5f01f. The third-pass review ran five fresh agents independently. Findings converged on one large entropy problem that was real and worth fixing regardless of scope, plus a handful of smaller issues. Result: 7 findings acted on in 145605a; several verified-false.

Fixed in 145605a

1. ~800 lines of shadow/dead code in RiderViewModel. The coordinator extraction replaced the ViewModel's Kind 30180 handling by routing the subscription callback straight to paymentCoordinator.onDriverRideStateReceived(). But the old ViewModel-side implementation was never deleted: handleDriverRideState, handlePinSubmission, handleDepositInvoiceShare, executeBridgePayment, startBridgePendingPoll, handleBridgeSuccessFromPoll, checkAndRevealPrecisePickup, revealPrecisePickup, revealPreciseDestination, revealLocation, publishPinVerification, and sharePreimageWithDriver were all unreachable but still present — a full shadow copy of the coordinator's payment/PIN/bridge path. Removed them all, along with their backing fields: riderStateHistory, historyMutex, lastReceivedDriverStateId, processedDriverStateEventIds, currentRiderPhase, bridgePendingPollJob, escrowRetryDeadlineJob.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 325 to 333 in 145605a

   	private val processedCancellationEventIds = mutableSetOf<String>()
   	// === STATE MACHINE (Phase 1: Validation Only) ===
   	// The state machine validates transitions but doesn't control flow yet.
   	// It logs warnings when existing code attempts invalid transitions.
   	private val stateMachine = RideStateMachine()
   	private var rideState: RideState = RideState.CANCELLED // No active ride
   	private var rideContext: RideContext? = null

2. Bridge-pending poll after process-death restore split between ViewModel and coordinator. restorePostAcceptanceState() was calling the ViewModel's own startBridgePendingPoll, which appended BridgeComplete to the ViewModel's riderStateHistory — a separate accumulator from the coordinator's. On an in-flight bridge that survived an app kill, the resulting Kind 30181 could carry a different history array than the coordinator's, corrupting the AtoB chain for the driver. Added paymentCoordinator.resumeBridgePoll() as the SDK-surface restore hook and rerouted the ViewModel to use it. The coordinator is now the single publisher of BridgeComplete.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 572 to 583 in 145605a

   	*/
   	fun getLastProcessedDriverActionCount(): Int = lastProcessedDriverActionCount
   	/**
   	* Return all event IDs published by this coordinator during the current ride and clear the
   	* internal list. The ViewModel should combine these with OfferCoordinator event IDs for
   	* NIP-09 deletion on ride end.
   	*/
   	fun getAndClearRideEventIds(): List<String> {
   	val ids = rideEventIds.toList()
   	rideEventIds.clear()
   	return ids

3. lastProcessedDriverActionCount preservation across process death. Once the ViewModel-side field went away, the dedup counter that prevents Kind 30180 history replay (PIN, ARRIVED, IN_PROGRESS) after restart had to survive somehow. Added lastProcessedDriverActionCount: Int = 0 parameter to PaymentCoordinator.restoreRideState() and a getLastProcessedDriverActionCount() getter; ViewModel threads the persisted value through on save/restore.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 528 to 569 in 145605a

   	* @param preimage Persisted preimage.
   	* @param escrowToken Persisted escrow token (null for non-SAME_MINT rides).
   	* @param pickupPin Persisted pickup PIN (null if already verified).
   	* @param pinVerified Whether PIN was already verified before process death.
   	* @param destination Destination location for post-PIN reveal.
   	* @param postConfirmDeadlineMs Persisted post-confirm ack deadline, or 0 to skip timeout.
   	* @param lastProcessedDriverActionCount
   	* Persisted count of driver actions processed pre-death. Kind 30180 is parametric
   	* replaceable, so on re-delivery after restart the event will often carry a new eventId
   	* (not blocked by the dedup set). Seeding this counter prevents replay of history actions
   	* (EN_ROUTE_PICKUP → ARRIVED etc.) that produces UI flicker.
   	*/
   	fun restoreRideState(
   	confirmationEventId: String,
   	paymentPath: PaymentPath,
   	paymentHash: String?,
   	preimage: String?,
   	escrowToken: String?,
   	pickupPin: String?,
   	pinVerified: Boolean,
   	destination: Location?,
   	postConfirmDeadlineMs: Long = 0L,
   	lastProcessedDriverActionCount: Int = 0
   	) {
   	activeConfirmationEventId = confirmationEventId
   	currentAcceptanceEventId = confirmationEventId // best-effort (acceptance not persisted)
   	activePaymentPath = paymentPath
   	activePaymentHash = paymentHash
   	activePreimage = preimage
   	activeEscrowToken = escrowToken
   	activePickupPin = pickupPin
   	activePinVerified = pinVerified
   	activeDestination = destination
   	this.lastProcessedDriverActionCount = lastProcessedDriverActionCount
   	if (postConfirmDeadlineMs > System.currentTimeMillis()) {
   	startPostConfirmAckTimeout(postConfirmDeadlineMs, confirmationEventId)
   	}
   	Log.d(TAG, "Restored ride state for confirmation ${confirmationEventId.take(8)}")
   	}
   	/**
   	* The count of driver actions processed so far for the active ride. The ViewModel persists

4. setHtlcRideProtected called twice per confirmation. PaymentCoordinator.runConfirmation() calls it before emitting Confirmed, and the ViewModel's handlePaymentEvent(Confirmed) handler was calling it again on the same payment hash — a redundant NIP-60 wallet-metadata write on every SAME_MINT ride. Removed the ViewModel call; coordinator is authoritative.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 4395 to 4400 in 145605a

   	/**
   	* Get the fare boost amount based on currency setting.
   	* USD mode: $1 converted to sats
   	* SATS mode: 1000 sats
   	*/

5. Misleading ConfirmationCancelledBySelf comment. The second-pass guard said confirmationInFlight stays true — new ride owns the lock (or resetInternalState already cleared it for the self-cancel case), conflating two distinct outcomes. For Case 1 (cross-ride) it stays true; for Case 2 (self-cancel) resetInternalState() has already cleared it. Comment rewritten to describe each case explicitly.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 701 to 707 in 145605a

   	}
   	val eventId = nostrService.confirmRide(
   	acceptance = acceptance,
   	precisePickup = pickupToSend,
   	paymentHash = inputs.paymentHash,
   	escrowToken = escrowToken

6. Vestigial escrowRetryDeadlineJob field. Cancelled in two places but never set (the function that started it, startEscrowRetryDeadline, was removed in the first pass — coordinator owns the timer now). Removed.

7. Unused imports after dead-code removal. Dropped RiderRideAction, RiderRideStateEvent, MeltQuoteState, kotlinx.coroutines.sync.withLock.

Verified-false / out of scope

• effectiveDeadline in 1..System.currentTimeMillis() is fragile. Agent Replace expanded area search button with hashtag-based ride discovery popup menu #2 flagged this as a possible logic defect. Traced each case (deadline=0 skips, deadline>now skips, deadline≤now triggers immediate cancel); behavior is correct. Kept as-is.
• OfferCoordinator wired but never driven. Known from the second pass; actually driving the coordinator requires rewriting every offer-sending code path in the ViewModel — out of scope. Tracked for Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52 (Hilt migration).
• Coordinator test coverage. Agent Bitcoin price service fetches price twice every 5 minutes #5 correctly flagged that PaymentCoordinator, OfferCoordinator, RoadflareRiderCoordinator, and the common-module AvailabilityMonitorPolicy have no tests. Real gap — but adding coordinator tests is a separate workstream; the existing AvailabilityMonitorPolicyTest still covers the rider-app copy (which is still used by the rider-app ViewModel's direct subscription path). Leaving for a follow-up PR.
• Coordinator scope-ownership asymmetry (OfferCoordinator has its own scope, PaymentCoordinator / RoadflareRiderCoordinator borrow viewModelScope): noted, structural, not a bug.

Convergence check

Three passes have now fixed 18 issues total (7 + 4 + 7). The remaining open items are all either:

• Out of scope for this PR (offer-send coordinator wiring, coordinator test suite),
• Structural/future-proofing concerns that don't change behavior,
• Or pre-existing patterns faithfully copied from main.

I think another full pass would likely find at most one or two more small issues. Calling this the convergence point; further cleanup belongs in follow-up PRs tracked by Issue #52.

Net change this pass: −850 / +52 lines. RiderViewModel.kt is now ~4977 lines (was ~5800 before this pass, and ~5731 on main before the whole PR).

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[variablefate]

Code review — 4th Pass

Prior passes fixed 18 bugs across 3 commits (17674a7, ec5f01f, 145605a). This pass targeted remaining structural entropy — dead surface, shadow types, duplicated policy logic, and stale naming. Five fresh agents ran independently.

Fixed in 4fd7577

1. Dead handleDriverArrived() method. Orphaned by the coordinator migration — Kind 30180 ARRIVED is now routed via handleDriverStatusAction through PaymentEvent.DriverStatusUpdated. No callers.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 4193 to 4207 in 4fd7577

   	* Send a chat message to the driver.
   	*/
   	fun sendChatMessage(message: String) {
   	val state = _uiState.value
   	val confirmationEventId = state.rideSession.confirmationEventId ?: return
   	val driverPubKey = state.rideSession.acceptance?.driverPubKey ?: return
   	if (message.isBlank()) return
   	viewModelScope.launch {
   	updateRideSession { copy(isSendingMessage = true) }
   	val eventId = nostrService.sendChatMessage(
   	confirmationEventId = confirmationEventId,
   	recipientPubKey = driverPubKey,

2. Dead RoadflareRiderCoordinator.sendKeyAck(). Public suspend fun with zero callers anywhere in the repo. The live Kind 3188 ack path (stale-key refresh) still calls NostrService.publishRoadflareKeyAck() directly from MainActivity / RoadflareTab. Left a TODO(Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52) for re-introducing when those call sites migrate through the coordinator.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/RoadflareRiderCoordinator.kt

   Lines 163 to 167 in 4fd7577

   	// TODO(#52): `sendKeyAck()` and `pingDriver()` will move here once the live callers
   	// (MainActivity, RoadflareTab) are migrated through the coordinator. Today they publish
   	// Kind 3188 / 3189 directly via `NostrService.publishRoadflareKeyAck()` so wrapping them in
   	// the coordinator would just be dead surface.
   	// TODO(#52): add `pingDriver()` once NostrService exposes a `publishDriverPing()` method.

3. Dead PaymentCoordinator.getAndClearRideEventIds() + rideEventIds accumulator. Public SDK surface with zero callers, backed by a mutableListOf<String> fed by six .add() calls scattered through the coordinator. Author-wide NIP-09 cleanup (nostrService.backgroundCleanupRideshareEvents) already catches all rideshare events by this user's pubkey, so the parallel list was dead tracking — 24 lines removed.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/PaymentCoordinator.kt

   Lines 320 to 340 in 4fd7577

   	private var activePreimage: String? = null
   	private var activePaymentHash: String? = null
   	private var activeEscrowToken: String? = null
   	private var activePickupPin: String? = null
   	private var activePinAttempts = 0
   	private var activePinVerified = false
   	private var activeDestination: Location? = null
   	private var driverDepositInvoice: String? = null
   	// ── Escrow retry state ────────────────────────────────────────────────────
   	private var pendingRetryAcceptance: RideAcceptanceData? = null
   	private var pendingRetryInputs: ConfirmationInputs? = null
   	// ── Jobs ──────────────────────────────────────────────────────────────────
   	private var escrowRetryDeadlineJob: Job? = null
   	private var postConfirmAckTimeoutJob: Job? = null
   	private var bridgePendingPollJob: Job? = null
   	// ── Public API ────────────────────────────────────────────────────────────

4. Private data class FareCalc shadow in RiderViewModel. The file already imported com.ridestr.common.coordinator.FareCalc (line 62) but then redeclared a private data class FareCalc(val sats: Double, val usdAmount: String?) with the same shape. The local shadow won for unqualified references, so 20+ internal fare values couldn't be passed to coordinator methods without a conversion — the imported type was dead, and the wall between ViewModel and coordinator offer logic was structurally enforced. Deleted the shadow; the imported type now binds.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 60 to 66 in 4fd7577

   	import com.ridestr.common.roadflare.RoadflareDriverPresenceCoordinator
   	import com.ridestr.common.coordinator.AvailabilityMonitorPolicy
   	import com.ridestr.common.coordinator.ConfirmationInputs
   	import com.ridestr.common.coordinator.FareCalc
   	import com.ridestr.common.coordinator.OfferCoordinator
   	import com.ridestr.common.coordinator.OfferEvent
   	import com.ridestr.common.coordinator.PaymentCoordinator

5. No-op offerCoordinator.onAcceptanceHandled() call. Added in the first pass, but OfferCoordinator has no live offer-sending callers today, so OfferEvent.Accepted never fires from the coordinator — the hook was closing subscriptions the coordinator never opened. Removed the call; coordinator method remains. Comment explains when it will come back (Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52).

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 4630 to 4655 in 4fd7577

   	_uiState.update { current ->
   	current.copy(
   	error = event.message,
   	rideSession = current.rideSession.copy(isSendingOffer = false)
   	)
   	}
   	}
   	is OfferEvent.Accepted -> {
   	val acceptance = event.acceptance
   	Log.d(TAG, "OfferEvent.Accepted from ${acceptance.driverPubKey.take(8)} (batch=${event.isBatch})")
   	_uiState.update { current ->
   	current.copy(
   	rideSession = current.rideSession.copy(
   	rideStage = RideStage.DRIVER_ACCEPTED,
   	acceptance = acceptance
   	)
   	)
   	}
   	autoConfirmRide(acceptance)
   	// Note: `offerCoordinator.onAcceptanceHandled()` is intentionally NOT called here —
   	// this path fires when the ViewModel's legacy offer-send code receives an acceptance,
   	// not the coordinator's (OfferCoordinator currently has no live offer-send callers).
   	// When Issue #52 migrates offer-send to the coordinator, the acceptance subscription
   	// will live inside OfferCoordinator and it will close its own subs automatically.
   	}

6. Renamed clearRiderStateHistory() → clearRideCoordinatorState(). Pass 3 removed the ViewModel's rider-state history fields, leaving the helper with only processedCancellationEventIds.clear() + paymentCoordinator.reset(). The old name lied to every reader at its 12 call sites. New name + KDoc reflect actual behavior.

   ridestr/rider-app/src/main/java/com/ridestr/rider/viewmodels/RiderViewModel.kt

   Lines 390 to 402 in 4fd7577

   	/**
   	* Reset the ride-scoped coordinator state and ViewModel-side dedup set. Call at every ride
   	* boundary (start, completion, cancellation) to guarantee no stale events leak into the next
   	* ride. The coordinator owns rider-state history, driver-state dedup, phase, and transition
   	* chain; the ViewModel owns the Kind 3179 cancellation dedup set.
   	*/
   	private fun clearRideCoordinatorState() {
   	processedCancellationEventIds.clear()
   	paymentCoordinator.reset()
   	Log.d(TAG, "Reset ride coordinator state and cancellation dedup set")
   	}

7. Unified AvailabilityMonitorPolicy — one truth instead of two diverged copies. The rider-app copy (stage: RideStage parameter) and the common copy (isWaitingForAcceptance: Boolean) both lived on trunk. RiderViewModel used the rider-app copy; OfferCoordinator used the common copy. Two policies, one logic, different APIs — and the test only covered the rider-app one. Fixed: deleted the rider-app copy, made the common copy public (was internal), migrated the ViewModel to the boolean API, moved the test to common/src/test/java/com/ridestr/common/coordinator/ and rewrote it with isWaitingForAcceptance assertions (including a new test for the equal-timestamp edge case that pass 3 flagged as previously untested).

   Also dropped the SHOW_UNAVAILABLE enum variant — never returned by either decision function; its when arms in OfferCoordinator and RiderViewModel existed only as exhaustive-match placeholders.

   ridestr/common/src/main/java/com/ridestr/common/coordinator/AvailabilityMonitorPolicy.kt

   Lines 10 to 23 in 4fd7577

   	*/
   	object AvailabilityMonitorPolicy {
   	enum class Action {
   	/** Out-of-order, driver-available, or not currently waiting for acceptance. */
   	IGNORE,
   	/**
   	* Availability went offline (or was deleted) while waiting for an acceptance.
   	* Caller should re-check after a grace period and only then surface "driver unavailable".
   	* A same-window Kind 3174 acceptance will cancel the deferred check.
   	*/
   	DEFER_CHECK
   	}

Verified-false / deferred

• Double resetInternalState() via closeAllRideSubscriptionsAndJobs() + clearRideCoordinatorState() (Agent Replace expanded area search button with hashtag-based ride discovery popup menu #2, Agent Transaction ledger missing mint fees, causing balance discrepancy #4 pass 3): verified idempotent; no race window that matters.
• processedDriverStateEventIds not seeded on restore (Agent Replace expanded area search button with hashtag-based ride discovery popup menu #2): verified — the lastProcessedDriverActionCount counter covers the practical case; pure-stale-cached-event edge is extremely narrow.
• OfferCoordinator.subs not closed on logout (Agent Replace expanded area search button with hashtag-based ride discovery popup menu #2): verified false — LogoutManager.performFullCleanup runs AFTER riderViewModel.performLogoutCleanup() (which calls offerCoordinator.destroy() → closes all subs).
• ViewModel's myRideEventIds is also dead tracking — confirmed, but this is pre-existing on main (not introduced by the PR). Out of scope.
• Duplicate offer-send infrastructure (Agent Driver app needs wallet setup screen in onboarding flow #1, repeat finding): still out of scope — tracked for Issue Migrate SettingsManager to Hilt DI + DataStore + Repository pattern #52. Huge refactor.
• android.util.Log leaking into common SDK (Agent Lightning address withdrawals don't prompt for amount #3): real SDK-grade concern but fixing it needs a logger abstraction across all coordinators. Defer.
• onDriverRideStateReceived naming is misleading (Agent Lightning address withdrawals don't prompt for amount #3): stylistic; skipped.
• DriverViewModel is the same god object (Agent Bitcoin price service fetches price twice every 5 minutes #5): confirmed — explicit follow-up for the driver-side decomposition.
• Coordinator test coverage gap (Agent Bitcoin price service fetches price twice every 5 minutes #5): PaymentCoordinator, OfferCoordinator, RoadflareRiderCoordinator still have no direct tests. This pass added one (common-module AvailabilityMonitorPolicyTest). Broader test coverage tracked separately.
• CLAUDE.md stale refs to removed fields (Agent Bitcoin price service fetches price twice every 5 minutes #5): re-verified — CLAUDE.md does NOT reference riderStateHistory, historyMutex, or currentRiderPhase. Not a real finding.

Convergence

Four passes total: 7 + 4 + 7 + 7 = 25 items addressed. Net this pass: +66 / −222 lines across 7 files. RiderViewModel.kt is now 4942 lines (down from ~5800 at pass-2 start; −789 vs main, which was 5731).

The remaining items (offer-send coordinator wiring, logger abstraction, DriverViewModel decomposition, broader coordinator test coverage) are genuinely separate work. I'd call this the convergence point: a fifth pass might find one more nit but nothing substantive given the current scope fence.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[variablefate]

Follow-up — applying the filter to deferred items

Re-examining the four deferred items from pass 4 against the actual criteria (lowers entropy / improves codebase / fixes real bug):

1. Logger abstraction — VERIFIED FALSE blocker

Agent #3 flagged android.util.Log in common/ as an SDK-grade concern preventing pure-JVM unit tests. Verified: common/build.gradle.kts:45-49 uses Robolectric (isIncludeAndroidResources = true) and RobolectricTestRunner is the standard for common-module tests, which stubs android.util.Log. Tests pass today. Logger abstraction would be aesthetic — no real blocker. Skipping.

2. DriverViewModel god-object decomposition — out-of-scope by design

PR #70 scope is "rider protocol logic into :common coordinators" (Issue #65). The parallel driver-side decomposition is a separate workstream. Genuinely deferred to a follow-up issue; not attempted here.

3. Coordinator test coverage — QUALIFIES, acted on

Added PaymentCoordinatorTest.kt in aa0478a. 11 tests covering the public-API contracts the four review passes introduced or clarified:

• Cancellation dedup roundtrip (markCancellationProcessed / isCancellationProcessed / reset)
• Process-death roundtrip (restoreRideState(lastProcessedDriverActionCount) → getLastProcessedDriverActionCount), including default-zero + last-write-wins sequential restores
• reset / onRideCancelled idempotency
• retryEscrowLock stale-UI guard

The confirmation coroutine is exercised via the ViewModel integration tests; this file pins only the state contracts that live entirely inside the coordinator.

4. Offer-send migration through OfferCoordinator — QUALIFIES, attempted and reverted

This was the biggest entropy source (~1700 lines of coordinator code unreachable, parallel impls in ViewModel + coordinator). I attempted it and reverted. Gaming out the behavior revealed that the migration is larger than "swap the send call":

The blocker: the ViewModel's subscribeToAcceptance callback at RiderViewModel.kt:3442 does more than route to autoConfirmRide. It also: cancels the acceptance timeout, clears acceptanceTimeoutStartMs and showDriverUnavailableDialog, runs a shouldConfirm CAS against StateFlow to atomically transition stage, cancels the RoadFlare batch job, closes batch acceptance subs, and empties contactedDrivers. The handleOfferEvent(Accepted) handler sets only rideStage + acceptance.

For a clean migration, handleOfferEvent(Accepted) needs to absorb all of that cross-cutting state cleanup (timeout, dialog, batch). That's doable but it's its own substantive refactor — the kind that warrants a focused PR with:

• Integration tests for each offer path (direct, roadflare-single, roadflare-with-alt-payment, roadflare-batch, broadcast)
• A local build to catch the many state-mutation subtleties
• Review against the current acceptance-callback edge cases (shouldConfirm CAS, batch winner cleanup, pending deletion job)

Without a local build in this environment, landing this here is too risky after four clean passes. Deferring with a concrete plan: tracking for Issue #52 (which already owns the Hilt migration — a natural bundling since Hilt'd coordinators become the obvious place to put offer-send).

Net for this follow-up

Item	Filter result	Action
Logger abstraction	False-positive	Skipped
DriverViewModel decomp	Out of scope	Deferred
Coordinator test coverage	Real	Fixed (aa0478a)
Offer-send migration	Real	Attempted, reverted, concrete plan filed

Four passes + this follow-up: 25 items addressed, 1 test file added, 1 item deferred with a clear exit plan. The PR is ready for merge on coordinator-extraction grounds; offer-send wiring is the next issue.

🤖 Generated with Claude Code

[variablefate]

Code review — 5th Pass (entropy audit)

The question this pass: is the refactored code lower-entropy than what it replaced? I gamed out the completion flow end-to-end and found five real issues — all tied to "belt and suspenders" duplication between VM and coordinator that prior passes missed because each duplicate was individually harmless (idempotent / unreachable / aesthetic).

Fixed in bc2269a:

1. Duplicate HTLC mark/clear. PaymentCoordinator.handleCompletion() already runs the full claimSuccess == true → markHtlcClaimedByPaymentHash / false → clearHtlcRideProtected / null + SAME_MINT → keep locked chain before emitting DriverCompleted. The VM's handleRideCompletion() re-ran the identical chain on the same payment hash. Idempotent but duplicated; dropped.

2. Duplicate walletService.refreshBalance(). Coordinator calls it, VM called it again in a follow-up coroutine. One extra NIP-60 round-trip per completion for zero benefit. Dropped.

3. Synthetic DriverRideStateData in handlePaymentEvent(DriverCompleted). The handler built a fake DriverRideStateData just so handleRideCompletion(statusData) could pull statusData.finalFare + statusData.history for the HTLC logic. Once (1) moves entirely into the coordinator, only finalFare is needed — so handleRideCompletion(finalFareSats: Long?) replaces the wrapper, and the handler becomes a one-liner.

4. Dead COMPLETED / CANCELLED branches in handleDriverStatusAction(). Coordinator routes COMPLETED via DriverCompleted and CANCELLED via DriverCancelled — neither ever reaches DriverStatusUpdated. Gaming it out: driver publishes Kind 30180 with status=COMPLETED → coordinator.onDriverRideStateReceived → handleDriverStatus → DriverStatusType.COMPLETED arm → handleCompletion → emits DriverCompleted. The VM's handleDriverStatusAction.COMPLETED arm is unreachable. Removed both dead arms.

5. Dead fields in the event contract. With (1)–(4) applied, PaymentEvent.DriverCompleted.claimSuccess and PaymentEvent.DriverStatusUpdated.driverState had no remaining consumer. Dropped from the data classes; coordinator emit sites and VM handler updated. Smaller public contract = lower entropy.

Cascading simplifications that fell out:

• handleRideCompletion(statusData: DriverRideStateData) → handleRideCompletion(finalFareSats: Long?).
• handleDriverStatusAction(action: DriverRideAction.Status, driverState: DriverRideStateData, confirmationEventId: String) → handleDriverStatusAction(status: String, confirmationEventId: String).
• DriverRideAction and DriverRideStateData imports dropped from the ViewModel entirely.

Net for this pass: +31 / −104 lines across two files. RiderViewModel.kt now 4871 lines (−71).

Entropy snapshot across all 5 passes:

• Pre-PR: RiderViewModel.kt = 5731 lines on main.
• Post-PR (pre-review): ~5800 lines + 3 new coordinators.
• After 5 review passes: 4871 lines + lean coordinator APIs + one test file.
• RiderViewModel.kt is now −860 lines vs main, and the payment/roadflare/coordination logic lives in focused, separately-owned files.

The remaining entropy source is OfferCoordinator's unused 1700 lines. Migrating offer-send through it requires handleOfferEvent(Accepted) to absorb the VM's current subscribeToAcceptance callback logic (acceptance-timeout cancel, shouldConfirm CAS, batch cleanup, dialog clear) — a contained refactor of its own. Tracked for Issue #52.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[variablefate]

Code review — 6th Pass (final convergence)

Five prior passes closed 25+ findings. This pass targeted the failure/cancellation paths specifically — prior passes had de-duplicated the happy path (completion), but the parallel duplication on the cancel side was missed.

Fixed in a6117b4

clearHtlcRideProtected was called 2–3× on cancellation paths. Gaming it out for each trigger:

Path	Calls before	Calls after
Driver publishes Kind 30180 status=CANCELLED	3 (coord pre-emit + VM direct + onRideCancelled)	1 (onRideCancelled)
MaxPinAttemptsReached (3 wrong PINs)	2 (coord pre-emit + VM direct)	1 (onRideCancelled)
Rider taps cancel (clearRide)	1 (VM direct), but inconsistent with event-driven paths	1 (onRideCancelled)
Kind 3179 driver cancellation	2 (VM direct + onRideCancelled)	1 (onRideCancelled)
BridgeFailed → handleDriverCancellation	2 (VM direct + onRideCancelled)	1 (onRideCancelled)
PostConfirmAckTimeout → handleDriverCancellation	2 (VM direct + onRideCancelled)	1 (onRideCancelled)

All idempotent but structurally redundant. Consolidated to a single authoritative HTLC-unlock path: paymentCoordinator.onRideCancelled() (coordinator line 395). Removed:

• PaymentCoordinator.handleDriverStatus CANCELLED branch pre-emit clear
• PaymentCoordinator.handlePinSubmission MaxPinAttemptsReached pre-emit clear
• RiderViewModel.handleDriverCancellation direct clear (line 4056)
• RiderViewModel.handlePaymentEvent(MaxPinAttemptsReached) direct clear (lines 4463-4465)
• RiderViewModel.clearRide direct clear (line 3028) — replaced with onRideCancelled

Left alone: PaymentCoordinator.handleCompletion's claimSuccess == false branch (line 827). That's the completion path's unlock for rider refund when the driver's claim fails — a different semantic than cancellation, single-caller, correct.

Verified-false / out-of-scope

• Agent Driver app needs wallet setup screen in onboarding flow #1 — Test divergence (RideCompletionClaimTest asserts CLEAR_PROTECTION for null+SAME_MINT): pre-existing test bug, not introduced by this PR. Test's decideHtlcAction() helper is wrong relative to the coordinator's correct behavior. Flagged for follow-up but not fixing here (test cleanup is its own concern).
• Agent Lightning address withdrawals don't prompt for amount #3 — DepositInvoiceReceived.amount dropped: the coordinator logs the amount and uses it internally for executeBridgePayment; no UI component renders "X sats to driver's mint" (never did on main either). Not a real regression.
• Agent Lightning address withdrawals don't prompt for amount #3 — Removing claimSuccess from DriverCompleted masks a failed-claim UX banner: verified false — the pre-refactor code didn't show a banner either. It would be a new feature, not a regression.
• Agent Bitcoin price service fetches price twice every 5 minutes #5 — MaxPinAttemptsReached doesn't publish Kind 3179 to driver: pre-existing on main; driver relies on its own confirmation timeout. Legitimate UX gap but not introduced by this PR.
• Agent Bitcoin price service fetches price twice every 5 minutes #5 — ConfirmationStale missing markCancellationProcessed: verified false. markCancellationProcessed tracks INCOMING cancellations; ConfirmationStale handler publishes an OUTGOING Kind 3179. Wrong direction.
• Agent Transaction ledger missing mint fees, causing balance discrepancy #4 — All 10 CLAUDE.md invariants: verified intact.

Final snapshot

Pass	Fixes landed	Commit
1	7 bugs	17674a7
2	4 bugs	ec5f01f
3	~820 lines shadow code	145605a
4	7 entropy items	4fd7577
5	5 completion-flow duplications	bc2269a
6	HTLC unlock consolidation (this)	a6117b4

Plus aa0478a — PaymentCoordinatorTest.kt (11 tests pinning the coordinator's public contracts).

Net across all passes: ~30 items fixed. RiderViewModel.kt = 4871 lines (−860 vs main), with payment/roadflare/coordination logic in focused coordinators with minimal public contracts. One single HTLC-unlock path. One test file pinning the hardest invariants.

This feels like convergence. Further passes would find pre-existing quirks on main that aren't this PR's to fix.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}