fix: surface incomplete local data deletion

[agent-p1p]

Fixes #465

Summary

• Replace enumeration-dependent account cleanup with an authoritative macOS Keychain purge for every generic-password credential under Marmot's com.marmot.whitenoise service.
• Run the Keychain purge before runtime shutdown and storage deletion, so any Keychain error is surfaced while the runtime and storage remain intact for retry.
• Remove the incomplete listAccounts / per-account removeAccount cleanup path, closing both tombstone-before-delete and partial-enumeration gaps.
• Add regression coverage for purge failure, operation ordering, post-tombstone orphaned credentials, and credentials hidden by partial enumeration.

Verification

• git diff --check — passed.
• conflict-marker scan of changed Swift files — passed.
• scripts/ci/macos-sanity-checks.sh — unavailable on this Linux worker because xcodebuild is not installed.
• Xcode/Swift tests unavailable locally; GitHub macOS CI must run format, unit-test, release-build, and static-analysis gates.

Sensitive paths

• whitenoise-mac/Core/MarmotClient.swift: destructive local reset and account private-key/Keychain lifecycle.

---

[coderabbitai]

Warning

Review limit reached

@mubarakcoded, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d60bb882-40b0-46c2-9fd2-e98d2ccca513

📥 Commits

Reviewing files that changed from the base of the PR and between 490ca5b and 382cbec.

📒 Files selected for processing (2)

• whitenoise-mac/Core/MarmotClient.swift
• whitenoise-macTests/whitenoise_macTests.swift

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pip/whitenoise-mac-465

---

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.}

[stage-review]

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

	Title
1	Define Keychain purge logic and error types
2	Update local data deletion to use Keychain purge
3	Verify Keychain purge and operation ordering

_{Chapters generated by Stage for commit 382cbec on Jul 13, 2026 12:41pm UTC.}

[agent-p1p]

Pip adversarial review for issue #465 at bcb687b1dda615d5b1caa0b824ed0b7c75a5432b.

BLOCKING:

1. A failed Keychain deletion becomes unretryable, so the next reset can still report success with the private key present. MarmotClient.deleteAllLocalData now stops on the first removeAccount error (whitenoise-mac/Core/MarmotClient.swift:240-243), but the vendored MDK at f9ef165a has already atomically moved that account out of the live accounts/ namespace before it attempts the fallible Keychain deletion (marmot-account/src/home.rs:365,374). If Keychain deletion fails, the first reset surfaces an error, but listAccounts() no longer returns that account. A second press enumerates only the surviving records, wipes the storage root, and returns success without ever retrying the orphaned Keychain credential. This is the exact residual-key outcome Security/Privacy: "Delete All Local Data" reports success and wipes storage even when zero accounts were removed, leaving account private keys (Keychain) on the device #465 is meant to close. The MDK post-commit behavior is independently tracked in mdk#819; swallowing that post-commit error would also be insufficient here because the nsec would still remain in Keychain.

2. A successful listAccounts() result is not proof that every Keychain credential was enumerated. At the vendored MDK SHA, AccountHome::accounts() silently skips account records it cannot deserialize (marmot-account/src/home.rs:278-284) and returns the partial list after logging a warning. Therefore this PR's try listAccountRefs() can succeed with zero or fewer accounts, after which the helper shuts down, wipes storage, and reports success while the omitted account's Keychain nsec survives. The new listing-failure test covers only a thrown error, not the actual partial-enumeration behavior.

The destructive reset needs an authoritative credential wipe that does not depend on live account enumeration—for example, a service-scoped Keychain purge (com.marmot.whitenoise) or an MDK bulk-wipe API that reports credential deletion separately and cannot declare success until all credentials are gone. Please add regressions for (a) a removal that throws after the account disappears from the next enumeration and (b) a partial-but-successful enumeration. A durable pending-key list would also work, but it must survive the storage-root wipe it gates.

SUGGESTION:

• None beyond the required security-contract tests above.

NITPICK:

• None.

CI:

• Static Analysis passed.
• Swift format and project sanity checks passed.
• Both new regression tests passed on the macOS runner.
• Overall PR Checks is red on the known master-baseline failures tracked by #457: three messageMediaDiskCacheReadDeletionMaintainsTrackedFootprint assertions plus the intermittent workspaceCoalescesAndCachesSourceEpochZeroMediaReferenceResolution race. Release smoke was skipped after unit-test failure.
• The CodeRabbit status is green, but its comment says the review itself was not run because the rate limit was reached.

VERDICT: CHANGES REQUIRED (2 blocking security findings).

SENSITIVE_PATHS: whitenoise-mac/Core/MarmotClient.swift — destructive reset and account private-key/Keychain lifecycle.

[agent-p1p]

Addressed both blocking findings in signed commit 8d0cd9f. Delete All Local Data no longer depends on MDK account enumeration or per-account removal: it authoritatively deletes all generic-password credentials for Keychain service com.marmot.whitenoise, surfaces any non-success/non-not-found OSStatus before shutdown or disk deletion, then shuts down and wipes storage. Added regressions for post-tombstone orphaned credentials, partial-success enumeration, purge failure preservation, and operation ordering. macOS CI is running.

[agent-p1p]

CI update: commit 8d0cd9f compiled successfully; Swift format, project sanity, Static Analysis, and all four new Delete All Local Data regressions passed. PR Checks is red only on the three deterministic messageMediaDiskCacheReadDeletionMaintainsTrackedFootprint assertions already red on master and tracked by #457. The clean fix is PR #464, which is green but not merged. Under the ever-red policy, do not merge PR #478; after #464 restores a green master this change needs a clean replacement PR.