fix(sync): send known personal record IDs on pull

[9thLevelSoftware]

Summary

Defense-in-depth for the portal pull cursor loop (phoenix-portal#97).

SyncManager sent personalRecordIds = emptyList() on every pull so the portal had to page PRs by cursor only. Combined with JS Date microsecond truncation, that loops after ~4 pages / 407 entities.

The empty list also meant get_personal_record_tombstones never ran.

Changes

• Send local canonical PR UUIDs in knownEntityIds.personalRecordIds.
• Append IDs merged earlier in the same pull so a stuck cursor cannot replay the same page.
• Tombstones can now be fetched for those known IDs.

Test plan

• :shared:testAndroidHostTest --tests PortalPullPaginationTest --tests SyncManagerTest — BUILD SUCCESSFUL
• Pair with phoenix-portal cursor-precision PR, then retry cloud sync

Related to 9thLevelSoftware/phoenix-portal#97.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[chatgpt-codex-connector]

You have reached your Codex usage limits for security reviews. Please try again later.

[Copilot]

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

[kilo-code-bot]

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

The single Ponytail finding from the previous pass — currentKnownEntityIds() interpolated four times in one Logger.i block — has been hoisted into a local val entityIds at SyncManager.kt:1497 and reused across all four interpolations. The fix is exactly as suggested: same line count, same call, just one invocation. Lazy solution. Ship.

Oh wait, this commit is actually clean. I had my flamethrower warmed up for a regression review and the author went and fixed the thing. Rude, but appreciated.

🏆 Best part: Surgical hoist — val entityIds = currentKnownEntityIds() on the line right above the Logger.i. One local, four reuses, zero new abstractions. The senior-engineer-bartending voice in me is almost disappointed there isn't more to roast.

💀 Worst part: There is genuinely nothing wrong with this commit. It's 4 lines changed and the only one I'd quibble about is the comment that justifies the hoist (which is actually fine because future-you will wonder why this variable exists). Strong 🥱 — the best kind of bug.

📊 Overall: Like a decaf espresso — technically wrong by spec, but exactly what the room needed.

Files Reviewed (1 file in incremental diff)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncManager.kt - 0 issues (previous Ponytail finding resolved)

Previous Review Summary (commit 8d67bd6)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 8d67bd6)

Verdict: Approve | Recommendation: Merge after optional cleanup

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	1
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncManager.kt	1497	currentKnownEntityIds() called 4× in a single log message — allocations for breakfast.

---

🏆 Best part: The fix itself is surgical and correct — sending canonical PR UUIDs lets the portal advance the cursor and unlocks get_personal_record_tombstones, which has been silently dead since personalRecordIds = emptyList(). The intra-pull accumulation loop (merge on page N → send on page N+1) is exactly the defense-in-depth posture the linked portal issue called for, and the new pullIncludesPersonalRecordIdsFromEarlierPagesInTheSamePull test pins the behavior down. Tests are tight, scoped to the changed contract, and don't pad coverage.

💀 Worst part: Four currentKnownEntityIds() calls in one Logger.i interpolation. The function name implies it's cheap; the implementation makes you pay for it four times plus a defensive toList() copy on the PR list — every pull sync, regardless of size. Cache to a local.

📊 Overall: Like a pulled-pork sandwich that arrives on a paper plate — the meat is correct, the fix lands, and the only nitpick is that someone plated it four times before handing it to you.

Correctness / Safety Findings

No correctness or safety findings. The pagination fix matches the linked portal issue (#97): known PR UUIDs are sent so get_personal_records_excluding_ids can advance, and tombstones can finally flow through get_personal_record_tombstones. UUID filtering and MAX_PARITY_IDS capping are preserved. Accumulated IDs from earlier pages are appended post-merge so a stuck cursor cannot replay the same page.

Ponytail Review

• shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncManager.kt:L1497: shrink: currentKnownEntityIds() invoked 4× in one log statement. Compute once into a local and reuse across the four interpolations.

Ponytail net: -2 lines.

Suggested Minimal Patch

Compute currentKnownEntityIds() once and reuse in the Logger.i block:

val entityIds = currentKnownEntityIds()
Logger.i("SyncManager") {
    "Parity sync: sending ${entityIds.sessionIds.size} session IDs, " +
        "${entityIds.routineIds.size} routine IDs, ${entityIds.cycleIds.size} cycle IDs, " +
        "${entityIds.personalRecordIds.size} personal record IDs"
}

Final Merge Guidance

Can merge as-is. The single suggestion is an optional cleanup — not blocking. The pagination correctness fix should ship.

---

Fix these issues in Kilo Cloud

---

_{Reviewed by minimax-m3 · Input: 28K · Output: 3.4K · Cached: 201.3K}

_{Review guidance: REVIEW.md from base branch main}