feat: icloud device removal - #3350
Conversation
Adds CONTEXT.md with the iCloud Sync vocabulary (Device, Device Record, Fleet, Usage Snapshot, Stale Device) and an ADR recording why device identity moves to the machine while existing installs keep the identifier they already have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reinstalling minted a fresh device identifier, stranding the previous device record and its usage snapshots in CloudKit with nothing alive to refresh them, and the menu kept projecting those snapshots as a second usage card. Fixes steipete#3234. - Each row in Settings -> iCloud Sync -> Macs gets a trash button that, after confirmation, deletes the device record and every usage snapshot that device published. Removal reuses the existing persistent delete retry path, and a terminal CloudKit failure puts the row back. - Device identity now derives from a hashed kern.uuid, which survives a reinstall. Installs that already have an identifier keep it. - Re-registering a device cancels its own pending delete, so the current Mac cannot retire the record it just saved. - The trash button is disabled while sync cannot reach CloudKit, so it never looks like it worked when nothing was sent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Queueing a delete no longer hides the row. The fleet entry goes when the delete is confirmed, or when CloudKit reports the record was already gone, which is the same path a delete made on another Mac already took. A Device Record delete is attempted once: retrying would race the Mac that owns it re-registering, and delete the record it just published. Failures that get no retry are surfaced instead of being dropped, and the pane shows them until a push completes with nothing failing in it. The pending-delete set now holds Device Records as well as Usage Snapshots, so it is named for what it holds; the persisted key is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxuRSmcgehfT9gxRzczXGQ
The removal strings sat in the general block; move them beside the other iCloud Sync strings, and add the message shown when removal is attempted before the engine is running. ADR 0002 records why removal is offered for the current Mac even though triage scoped this to non-current devices, and the glossary gains Device Identifier, the term the identity decision rests on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxuRSmcgehfT9gxRzczXGQ
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2331691d13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await self.forgetFleetRecords( | ||
| CloudSyncSnapshotMigration.confirmedDeletedNames(deletedIDs: deletedIDs, failures: failures)) |
There was a problem hiding this comment.
When CloudKit confirms the Device Record deletion but returns a terminal error for one of its Usage Snapshot deletions, this immediately removes the device row while finishedFailedDeleteNames also abandons the failed snapshot. The snapshot remains in fleetSnapshots and continues to be rendered by StatusItemController+FleetAccounts.swift, but the missing device row leaves the user no way to retry its removal. Keep the row or another retryable removal record until every associated snapshot is confirmed absent.
Useful? React with 👍 / 👎.
| if changes.failedRecordSaves.isEmpty, changes.failedRecordDeletes.isEmpty, | ||
| !changes.savedRecords.isEmpty || !changes.deletedRecordIDs.isEmpty | ||
| { | ||
| await MainActor.run { self.state.status.lastError = nil } |
There was a problem hiding this comment.
When a removal retry reports unknownItem—for example, after the first response was lost or another Mac completed the deletion—confirmedDeletedNames correctly treats the record as deleted and removes the row, while reportableFailedDeletes suppresses the error. This condition nevertheless refuses to clear the previous lastError because failedRecordDeletes is nonempty, so the pane can retain a red failure banner even though the requested removal succeeded and no later push is guaranteed. Treat an all-unknownItem delete result as successful for banner clearing.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 8:30 AM ET / 12:30 UTC. ClawSweeper reviewWhat this changesThe PR adds iCloud Sync controls to remove a selected device record and its usage snapshots, and derives device IDs for new installations from a hashed hardware UUID. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 7 items remain Keep this PR open: it resolves the earlier cache-refresh finding, but current-device removal can leave newly published snapshots without a removable device row, and the central CloudKit workflow still lacks real after-fix proof. Priority: P2 Review scores
Verification
How this fits togetherCodexBar’s iCloud Sync subsystem stores per-Mac usage snapshots in CloudKit and projects the fleet into Settings and the menu. The new settings action refreshes the fleet, deletes the selected device’s records, and updates the local cached fleet after CloudKit confirms deletion. flowchart LR
A[Settings device list] --> B[Removal confirmation]
B --> C[Sync engine]
C --> D[Refresh CloudKit fleet]
D --> E[Delete device and snapshots]
E --> F[Local fleet cache]
F --> G[Settings and menu]
Decision needed
Why: The PR intentionally goes beyond the reported cleanup, and its current-device behavior needs a defined lifecycle before the implementation can be judged safe. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Limit removal to non-current stale devices unless maintainers explicitly want reset semantics, then make that reset preserve a removable/re-registered device before snapshot writes resume and verify it on an entitled build. Do we have a high-confidence way to reproduce the issue? Yes—source establishes the path: remove the current device, then allow that Mac’s normal snapshot queue to send again without a corresponding device re-registration. This review did not run the entitled CloudKit path because the checkout is read-only and the contributor reports no suitable signed build. Is this the best way to solve the issue? No. The current implementation makes an unapproved current-device reset available without preserving the device-to-snapshot lifecycle; a non-current-only action or an explicitly ordered reset is safer. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c3a25fd85ff0. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles)
|
Addresses two findings from the Codex/ClawSweeper review of 2331691. [P2] Retain a recovery handle for partial snapshot deletion (CloudSyncEngine.swift:1353-1358). When CloudKit confirms the Device Record delete but terminally rejects one of its Usage Snapshot deletes, the snapshot kept projecting into the fleet menu with no Macs row left to remove it from. Rather than hold the row open for a delete that will not be retried, the confirmed device delete now takes its orphaned snapshots with it, so nothing unreachable is displayed. [P2] Clear resolved unknown-item deletion errors (CloudSyncEngine.swift:849-852). An all-unknownItem delete result is a removal that landed, but a non-empty failedRecordDeletes kept the red banner up. The banner now clears on reportable failures being absent, and counts confirmed deletes as push progress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxuRSmcgehfT9gxRzczXGQ
|
Pushed 39bc822 addressing two of the review findings. Clear resolved unknown-item deletion errors ( Retain a recovery handle for partial snapshot deletion ( Still open:
I'm trying to close those 3 findings. |
Addresses [P1] Reconcile all remote snapshots before completing removal from the review of 2331691. Removal built its delete set from the fleet cache alone, so a usage snapshot published since the last fetch outlived the device record it belonged to, with no Macs row left to remove it from. Removal now fetches first and builds the set from what comes back. A fetch that fails leaves no grounds to delete anything, so it stops the removal; fetchChanges reports its outcome for that, and already surfaces the error itself. This is the same rule the confirmed-delete path follows: nothing is retired on a result CloudKit did not give us. A CKQuery for the device's snapshots was the reviewer's suggestion and would guarantee no more: both are authoritative only up to the moment they run. Records published after that are out of reach either way, which is the intended outcome for a live Mac that re-registers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UxuRSmcgehfT9gxRzczXGQ
clawsweeper
Bot
added
the
merge-risk: 🚨 session-state
|
Pushed 39bc822 and bb885b5, addressing all three findings. [P1] Reconcile all remote snapshots ( [P2] Retain a recovery handle for partial snapshot deletion ( [P2] Clear resolved unknown-item deletion errors ( Still open, both outside the diff:
|
1 participant
Closes #3234.
This is not completely verified. see below: Verification
Generated by Claude, Reviewed by kwnms04 and Claude.
What this does
Settings → iCloud Sync → Macs now lists every synced Mac and lets you remove one, with confirmation. Removing a Mac deletes its Device Record and every Usage Snapshot it published, on every Mac on the account.
The list renders from local persistence, so it is visible even with syncing off — otherwise there is nothing to remove a stale device from, which is the state the issue reports.
Why the row does not disappear immediately
An earlier revision removed the row optimistically and put it back on failure. That machinery was the source of most review findings, so it is gone. The row now goes on one signal: CloudKit confirmed the delete, or reported the record was already gone. That is the same path a delete made on another Mac already took, so both cases share one code path.
A Device Record delete is attempted once. Retrying would race the Mac that owns it re-registering itself and delete the record it just published. Failures that get no retry are surfaced in the pane instead of being dropped, and clear when a push completes with nothing failing in it.
Scope
Two deliberate departures, both recorded as ADRs:
queueDeviceRecordcancels its own pending delete, so the removal is self-healing rather than destructive.Also out of scope, as the issue notes: "reset icloud data".
Duplicates are the point
The confirmation dialog shows the device's absolute last-seen time, because the case this issue is about is two rows with the same host name. On the machine this was developed on,
kwnmbp.localcurrently has four Device Records (0.49.4 → 0.55.1 → 0.55.1 → 0.56.1). Host name alone cannot tell them apart.Verification
make check— swiftformat clean, swiftlint--strict0 violations across 2075 files, locales 22 catalogs / 1494 keys, packaging and signing checks pass.make test— all 82 groups run green exceptClaudeOAuthCredentialsStoreNeverPromptCacheTests, which shells out to/usr/bin/defaults; that tool cannot write any domain on this machine, so those two tests fail for reasons unrelated to this change. The other 11 suites in that group were run separately and pass.Not verified locally: the removal itself, the confirmation dialog, and the failure banner. iCloud entitlements are only embedded in identity-signed release builds of
com.steipete.codexbar, and this machine has no signing identity. What was verified against a running build is that the Macs list renders with syncing off and the remove controls are correctly disabled without the entitlement.Follow-ups, not addressed here
CloudSyncState.removeDeviceis a closure on an@Observablestate object — behaviour where state belongs.CloudSyncSnapshotMigrationnow owns Device Record delete policy as well as snapshot migration; it has two reasons to change."device-"name prefix rather than a type.macsFootnoteconcatenates two localized sentences, which is not translatable as a unit.