Onboarding reliability: brain-unreadable account session, false "Incorrect PIN.", and Repair - #1019
Merged
Merged
Conversation
…ir button, iOS PIN fix, .syncing retirement - credentialStore: race-safe keychain material (no -U clobber), decrypt self-heal, migration abort-on-unreadable + retain/prune of file-backed keys, safeStorage write guard - new osBoundKeyMaterial module + episodeAnalytics helper + ade_account_session_unreadable event - desktop: verified brain Repair (restartBrain + IPC), Tailscale info severity, brain_repair analytics - iOS: PIN sheet isAttached fix, .connected at hello-apply, .syncing retired - ade doctor: brain-restart remedy on session failures; docs + dashboard spec updated Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nosing-issue # Conflicts: # apps/ade-cli/src/services/credentials/credentialStore.test.ts # apps/ade-cli/src/services/credentials/credentialStore.ts # apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts # apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsx # apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx
Windows DPAPI cache invalidation now carries an epoch so a read that started before an invalidation cannot repopulate the cache the self-heal just cleared — the macOS resolver already guarded this, the Windows half did not. Also: require the key-binding directory on the Windows invalidate, name that parameter for what it is (keyBindingDir, the machine key's directory), name the macOS-only resolver accordingly, restore the Windows no-negative-cache and in-flight-dedup rationale lost in the merge, rename no_keychain_material to no_os_key_material (unreachable on win32 under the old meaning; never shipped), drop a dead export, and document the Windows self-heal lock-hold cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe pull request adds credential key-material recovery and failure classification, brain-restart repair flows across the CLI and desktop app, analytics coverage, and iOS connection-state changes that separate transport attachment from hydration. ChangesCredential and brain repair flow
iOS connected-state model
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
arul28
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/ADE/Views/Hub/HubQuickConnect.swift (1)
221-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the reconnect result to report success.
reconnect(toSavedHost:)returns whether the requested host connected. Line [224] checks only the sharedconnectionState. If the attempt fails and the previous machine is restored,connectionStatecan still be.connected, so this branch reports success for the wrong machine. Capture the returned Boolean and branch on it, asapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftdoes at Lines [1191]-[1194].Proposed fix
- await syncService.reconnect(toSavedHost: host) + let reconnected = await syncService.reconnect(toSavedHost: host) connectingId = nil - if syncService.connectionState == .connected { + if reconnected {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Views/Hub/HubQuickConnect.swift` around lines 221 - 229, Update the reconnect flow in the Task around reconnect(toSavedHost:) to capture its returned Boolean and use that result for the success/error branch instead of syncService.connectionState. Preserve the existing haptics, callback, and error-text behavior for each outcome.
🧹 Nitpick comments (2)
apps/ade-cli/src/services/credentials/credentialStore.test.ts (2)
449-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
no_os_key_materialreason.The suite covers
decrypt_failure(line 446) andstore_format(line 459). It does not coverno_os_key_material.That branch cannot be reached from a test process today.
decodeCredentialStoreselects it throughexpectsOsBoundKeyMaterial(), which readsprocess.envdirectly, andosBindingDisabledByEnvreturns true wheneverVITEST === "true". Every test therefore resolves the binding to"disabled"and falls through todecrypt_failure.This reason is the one that gates the desktop brain-repair control for brain-local unreadable sessions, so a silent regression in the classification would not fail any test. Add an injection seam so the branch is testable, for example an optional binding argument on
expectsOsBoundKeyMaterial()or an explicitexpectsOsMaterialinput todecodeCredentialStore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/credentials/credentialStore.test.ts` around lines 449 - 460, Add an injectable binding or explicit OS-material expectation to expectsOsBoundKeyMaterial or decodeCredentialStore, instead of relying exclusively on process.env and the Vitest override, so tests can reach the no_os_key_material classification. Update the credential-store test suite with coverage that exercises this path and asserts getLastReadFailureReason() returns no_os_key_material while preserving existing decrypt_failure and store_format behavior.
962-974: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the epoch guard for an invalidation during an in-flight asynchronous read.
This test covers
inFlightcoalescing. It does not cover therecordepoch check increateMacKeychainMaterialResolver, which is what stops material rejected by an invalidation from being cached again.
invalidate()setsinFlight = nullbut the already-started promise still callsrecord. Only thereadEpoch !== epochcomparison prevents that result from repopulatingcached. The equivalent Windows behavior is tested inapps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.ts; the macOS resolver has no such test.💚 Suggested test
it("does not let a read that started before an invalidation repopulate the cache", async () => { let resolveRead: ((value: { material: Buffer }) => void) | null = null; const readAsync = vi.fn(() => new Promise<{ material: Buffer }>((resolve) => { resolveRead = resolve; })); const read = vi.fn(() => ({ material })); const resolver = createMacKeychainMaterialResolver({ read, readAsync }); const inFlight = resolver.readAsync(); resolver.invalidate(); resolveRead!({ material: Buffer.alloc(32, 9) }); // The read still answers its own caller; only the cache write is dropped. expect(await inFlight).toEqual(Buffer.alloc(32, 9)); expect(resolver.read()).toBe(material); expect(read).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/credentials/credentialStore.test.ts` around lines 962 - 974, Add a test alongside the existing coalescing case that starts a deferred resolver.readAsync() call, invokes invalidate() before resolving it, then verifies the in-flight caller receives the resolved material while a subsequent resolver.read() still returns the pre-existing material and performs no additional OS read. Use createMacKeychainMaterialResolver and assert the invalidation epoch prevents the stale result from repopulating cached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/account/episodeAnalytics.ts`:
- Around line 39-45: In
apps/ade-cli/src/services/account/episodeAnalytics.ts:39-45, update the
analytics capture flow after setting emitted so synchronous exceptions from
args.capture()?. do not escape; swallow the capture failure and preserve
unreadable-session reporting. In
apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts:1421-1496,
add coverage with a throwing capture callback and assert report() does not throw
while the episode remains consumed.
In `@apps/ade-cli/src/services/credentials/credentialStore.ts`:
- Around line 972-976: Update the readAllAsync flow around
bindLegacyCiphertextUnderLock so legacy ciphertext binding does not
synchronously block the brain read path while waiting for
withCredentialFileLock. Move the retry/write work off the asynchronous read path
or apply a bounded wait specifically for this call, while preserving the
returned attempt.values and rewrite behavior.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 3947-3970: Add the IPC.appRestartBackgroundService channel to the
ipcInvokeTimeoutMs table with a timeout covering the 60-second service install
timeout plus the 20-second brain restart timeout, ensuring the renderer does not
time out before captureRepairOutcome records completion or failure.
In `@apps/desktop/src/main/services/runtime/projectRecoveryService.ts`:
- Around line 411-475: Serialize repair and restart operations through a shared
in-flight operation rather than relying only on repairInFlight. Update
restartBrain() and runRepair() so each registers its promise before starting
work, and repair awaits any already-running restart before stopping the service
or accessing the database; preserve rejection of new restarts while repair is
active and ensure the shared state is cleared when the operation settles.
In `@apps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsx`:
- Around line 465-478: Update refreshPublishHealth to maintain a monotonically
increasing request ID for each getInfo request, and capture the ID when starting
the request. In the promise resolution, update setLocalPublishHealth only when
the component is mounted and the response ID matches the latest request ID,
preventing stale responses from overwriting newer state.
In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx`:
- Around line 233-237: The generic token_unreadable state must not independently
enable repair. Add a producer-authoritative repairability field to the shared
account-directory health contract, set it only for session-read failures, and
update SyncDevicesSection to require that typed marker when computing
showRepair; in
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx lines
233-237, gate Repair on the marker, and in
apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx lines
252-262, add a token_unreadable case with the marker false and assert Repair is
absent.
In `@apps/ios/ADETests/SyncRecoveryPolicyTests.swift`:
- Line 1281: Replace the unbounded readiness loop around
performPostHelloRestorationForTesting with a timeout-aware XCTest expectation or
wait that observes restoration.isWaiting; fail the test with a clear message if
the DeferredRecoveryWork continuation is not installed before the timeout, then
continue only after readiness is confirmed.
In `@scripts/posthog/dashboard-spec.mjs`:
- Line 540: Update the reliability dashboard description near the existing brain
wedge, route-publish, and update-flow incident text to also mention
account-session unreadable incidents, matching the
EVENTS.ACCOUNT_SESSION_UNREADABLE series added in the dashboard event nodes.
---
Outside diff comments:
In `@apps/ios/ADE/Views/Hub/HubQuickConnect.swift`:
- Around line 221-229: Update the reconnect flow in the Task around
reconnect(toSavedHost:) to capture its returned Boolean and use that result for
the success/error branch instead of syncService.connectionState. Preserve the
existing haptics, callback, and error-text behavior for each outcome.
---
Nitpick comments:
In `@apps/ade-cli/src/services/credentials/credentialStore.test.ts`:
- Around line 449-460: Add an injectable binding or explicit OS-material
expectation to expectsOsBoundKeyMaterial or decodeCredentialStore, instead of
relying exclusively on process.env and the Vitest override, so tests can reach
the no_os_key_material classification. Update the credential-store test suite
with coverage that exercises this path and asserts getLastReadFailureReason()
returns no_os_key_material while preserving existing decrypt_failure and
store_format behavior.
- Around line 962-974: Add a test alongside the existing coalescing case that
starts a deferred resolver.readAsync() call, invokes invalidate() before
resolving it, then verifies the in-flight caller receives the resolved material
while a subsequent resolver.read() still returns the pre-existing material and
performs no additional OS read. Use createMacKeychainMaterialResolver and assert
the invalidation epoch prevents the stale result from repopulating cached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78467702-b53b-4d2b-826c-83824a8a8077
⛔ Files ignored due to path filters (8)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/remote-runtime/internal-architecture.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (64)
apps/ade-cli/src/commands/doctor.test.tsapps/ade-cli/src/commands/doctor.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/serviceManager/installWindows.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/account/episodeAnalytics.tsapps/ade-cli/src/services/credentials/credentialStore.test.tsapps/ade-cli/src/services/credentials/credentialStore.tsapps/ade-cli/src/services/credentials/osBoundKeyMaterial.tsapps/ade-cli/src/services/credentials/windowsDpapiMaterial.test.tsapps/ade-cli/src/services/credentials/windowsDpapiMaterial.tsapps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/remoteRuntime/runtimeDiscovery.tsapps/desktop/src/main/services/runtime/projectRecoveryService.test.tsapps/desktop/src/main/services/runtime/projectRecoveryService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/settings/BrainRepairButton.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsxapps/desktop/src/renderer/components/settings/SyncDevicesSection.tsxapps/desktop/src/renderer/components/settings/accountDirectorySummary.test.tsapps/desktop/src/renderer/components/settings/accountDirectorySummary.tsapps/desktop/src/renderer/components/settings/useSyncConnections.tsapps/desktop/src/renderer/hooks/useBrainRepair.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/productAnalytics.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Components/ADEDesignSystem.swiftapps/ios/ADE/Views/Cto/CtoRootScreen.swiftapps/ios/ADE/Views/Cto/CtoSessionDestinationView.swiftapps/ios/ADE/Views/Deeplinks/SendToMacCard.swiftapps/ios/ADE/Views/Files/FilesDetailComponents.swiftapps/ios/ADE/Views/Files/FilesRootScreen+Actions.swiftapps/ios/ADE/Views/Hub/HubComposerDrawer.swiftapps/ios/ADE/Views/Hub/HubQuickConnect.swiftapps/ios/ADE/Views/Hub/HubScreen.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/Lanes/LaneRootStateViews.swiftapps/ios/ADE/Views/LanesTabView.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PRs/PrsRootScreen.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsConnectionHeader.swiftapps/ios/ADE/Views/Settings/SettingsPinSheet.swiftapps/ios/ADE/Views/Work/TerminalSessionScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/SyncAccountConnectRecoveryTests.swiftapps/ios/ADETests/SyncRecoveryPolicyTests.swiftscripts/posthog/dashboard-spec.mjs
💤 Files with no reviewable changes (3)
- apps/desktop/src/renderer/components/settings/accountDirectorySummary.test.ts
- apps/ios/ADE/Views/Lanes/LaneRootStateViews.swift
- apps/ios/ADE/Views/Components/ADEDesignSystem.swift
Comment on lines
+972
to
+976
| // The asynchronous path binds legacy ciphertext too. It is the brain's read | ||
| // path, and on a machine whose only reader is the brain the store would | ||
| // otherwise stay machine-key-sealed forever. | ||
| if (attempt.rewriteWithCurrentKey) this.bindLegacyCiphertextUnderLock(attempt.key); | ||
| return attempt.values; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the credential file lock implementation for blocking waits.
set -euo pipefail
fd -t f 'credentialStore.ts|.*[Ll]ock.*\.ts' apps/ade-cli/src | while IFS= read -r f; do
rg -n -C 12 'withCredentialFileLock|withOptionalCredentialFileLock' "$f" || true
done
# Look for synchronous sleep/spin primitives used by the lock helper.
rg -n -C 5 'Atomics\.wait|sleepSync|hrtime|LOCK_RETRY_MS|LOCK_TIMEOUT_MS' apps/ade-cli/srcRepository: arul28/ADE
Length of output: 21145
Avoid blocking the brain read path while binding legacy ciphertext.
readAllAsync synchronously calls bindLegacyCiphertextUnderLock, which enters withCredentialFileLock and performs synchronous file writes during the lock retry. This blocks the event loop until the lock is acquired, so move or delegate the retry wait away from the brain read path, or bound the blocking wait to this call site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/services/credentials/credentialStore.ts` around lines 972 -
976, Update the readAllAsync flow around bindLegacyCiphertextUnderLock so legacy
ciphertext binding does not synchronously block the brain read path while
waiting for withCredentialFileLock. Move the retry/write work off the
asynchronous read path or apply a bounded wait specifically for this call, while
preserving the returned attempt.values and rewrite behavior.
Comment on lines
+233
to
+237
| // A brain-side unreadable account session is the one directory failure a | ||
| // restart clears — same test RemoteTargetList runs on its publish health. | ||
| const showRepair = accountSignedIn | ||
| && isBrainAccountSessionFailure(status.routeHealth?.accountDirectory?.state) | ||
| && repair.available; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat every token_unreadable result as repairable.
The publisher uses token_unreadable when getAccessToken() cannot read or refresh a token. The renderer matches that generic state, so it can offer a background-service restart for a token-refresh or transport fault.
Add a producer-authoritative repairability marker to the shared account-directory health contract. Set it only for session-read failures. Do not infer it from the generic state string.
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx#L233-L237: gate Repair on the new typed marker.apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx#L252-L262: add atoken_unreadableresult with a false repairability marker and assert that Repair is absent.
As per coding guidelines, “Fix underlying services or shared types instead of adding renderer-only workarounds.”
📍 Affects 2 files
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx#L233-L237(this comment)apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx#L252-L262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx` around
lines 233 - 237, The generic token_unreadable state must not independently
enable repair. Add a producer-authoritative repairability field to the shared
account-directory health contract, set it only for session-read failures, and
update SyncDevicesSection to require that typed marker when computing
showRepair; in
apps/desktop/src/renderer/components/settings/SyncDevicesSection.tsx lines
233-237, gate Repair on the marker, and in
apps/desktop/src/renderer/components/settings/SyncDevicesSection.test.tsx lines
252-262, add a token_unreadable case with the marker false and assert Repair is
absent.
Source: Coding guidelines
- serialize repair() behind an in-flight restartBrain() instead of racing it into exclusive database work, with a bounded wait so the recovery screen never parks; repair still runs whatever the restart did - bound the readiness ping (RuntimeRpcClient's default is 10 minutes) and give the restart IPC a budget that covers a forced install queued behind an in-flight one - keep analytics capture best-effort so a throwing sink cannot rewrite a token_unreadable outcome as transport_error - drop stale publish-health responses with a request generation - bound two unbounded waits in SyncRecoveryPolicyTests - name relay suppression in the reliability dashboard description Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
arul28
added a commit
that referenced
this pull request
Aug 4, 2026
Three conflicts, two of them semantic rather than textual. credentialStore.ts — taken entirely from main. #1019 fixed the same destroy-on-migration class from a different angle and did it better: an intrinsic `isFileBackedCredentialKey` predicate instead of the per-caller `sharedLegacyKeys` option I added (so a call site cannot forget it), the sync bootstrap token covered as well as the account session, a throw on writing a file-backed key into safeStorage, and — the part mine lacked — pruning migrated duplicates out of the retained file store. Without that prune both files hold a copy and the brain keeps serving a stale one after the app rotates a token. My option and both of my tests are gone; main's tests cover the same ground and assert more. runtimeDiscovery.ts — genuinely complementary, so both sides kept. Mine rewrote the Tailscale diagnostics to name what is missing from the list and what to do; main added a `severity` field so "not installed" reads as info rather than a warning. The merged form is my copy carrying their severity. installWindows.test.ts — the auto-merge silently produced broken code by combining my 41 renamed lazy-accessor uses with main's 3 remaining old-constant uses. That is the failure mode textual merging has with a rename, and it typechecks as an undefined name rather than a conflict. main's new Repair test asserted `taskkill /PID <supervisor> /T /F`. This branch deliberately drops `/T`: the brain is the supervisor's child, so a tree kill took it down mid-write with no SQLite/CRDT flush and no lock release, and the next start had to recover from a lock whose owner never got to release it. The brain is now talked down separately, after the supervisor can no longer relaunch it. The test's intent — Repair must not degrade into a no-op — still holds and is still asserted; only the args changed. Its positional spawn queue also needed the stale-serve scan this branch added to the install path. ade-cli: 2809 passed. Both typechecks clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 10, 2026
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
`hasPairedHost` answered two different questions with one boolean: "has this phone ever been paired" and "can this phone read the credential for the pairing it has". A Keychain entry that is missing, cleared out from under the app, or unreadable made a paired phone report as never-paired. What that looks like to a user: they pair, the sheet confirms the machine, and the accountless gate then re-presents the same connect sheet they just finished — with nothing said about why. They have already done the step it is asking for, so the app reads as ignoring them. It is the same user-facing class as the token_unreadable work in #1019; the accountless gate simply never got that treatment. `syncPairedHostCredentialState` now separates `notPaired` from `credentialUnreadable`, and `hasPairedHost` is defined as its `isUsable` case so every existing gate keeps its exact behaviour — an unusable credential still blocks entry, because without the secret the phone genuinely cannot use the pairing. Only the words change: the gate names the fault and offers "Pair again", which is the existing repair vocabulary on this platform (`laneOfflineAction`), rather than the unexplained loop. The classifier is pure and takes the credential lookup as a parameter, because the branch that matters is only reachable with a real Keychain fault. The test drives it directly with a persisted paired profile and a lookup that returns nil — standing in for the `errSecMissingEntitlement` (-34018) read failure — and asserts the state is `credentialUnreadable`, is not `notPaired`, and still gates entry. Found while diagnosing why a simulator build could pair over LAN and forget on relaunch. That turned out to be an unentitled build with no Keychain access at all, not a product defect — but it surfaced this gate, which is real and does affect a device whose Keychain entry goes bad. 1306 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
`hasPairedHost` answered two different questions with one boolean: "has this phone ever been paired" and "can this phone read the credential for the pairing it has". A Keychain entry that is missing, cleared out from under the app, or unreadable made a paired phone report as never-paired. What that looks like to a user: they pair, the sheet confirms the machine, and the accountless gate then re-presents the same connect sheet they just finished — with nothing said about why. They have already done the step it is asking for, so the app reads as ignoring them. It is the same user-facing class as the token_unreadable work in #1019; the accountless gate simply never got that treatment. `syncPairedHostCredentialState` now separates `notPaired` from `credentialUnreadable`, and `hasPairedHost` is defined as its `isUsable` case so every existing gate keeps its exact behaviour — an unusable credential still blocks entry, because without the secret the phone genuinely cannot use the pairing. Only the words change: the gate names the fault and offers "Pair again", which is the existing repair vocabulary on this platform (`laneOfflineAction`), rather than the unexplained loop. The classifier is pure and takes the credential lookup as a parameter, because the branch that matters is only reachable with a real Keychain fault. The test drives it directly with a persisted paired profile and a lookup that returns nil — standing in for the `errSecMissingEntitlement` (-34018) read failure — and asserts the state is `credentialUnreadable`, is not `notPaired`, and still gates entry. Found while diagnosing why a simulator build could pair over LAN and forget on relaunch. That turned out to be an unentitled build with no Keychain access at all, not a product defect — but it surfaced this gate, which is real and does affect a device whose Keychain entry goes bad. 1306 tests, same 13 pre-existing failing cases as clean main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arul28
added a commit
that referenced
this pull request
Aug 10, 2026
`hasPairedHost` answered two different questions with one boolean: "has this phone ever been paired" and "can this phone read the credential for the pairing it has". A Keychain entry that is missing, cleared out from under the app, or unreadable made a paired phone report as never-paired. What that looks like to a user: they pair, the sheet confirms the machine, and the accountless gate then re-presents the same connect sheet they just finished — with nothing said about why. They have already done the step it is asking for, so the app reads as ignoring them. It is the same user-facing class as the token_unreadable work in #1019; the accountless gate simply never got that treatment. `syncPairedHostCredentialState` now separates `notPaired` from `credentialUnreadable`, and `hasPairedHost` is defined as its `isUsable` case so every existing gate keeps its exact behaviour — an unusable credential still blocks entry, because without the secret the phone genuinely cannot use the pairing. Only the words change: the gate names the fault and offers "Pair again", which is the existing repair vocabulary on this platform (`laneOfflineAction`), rather than the unexplained loop. The classifier is pure and takes the credential lookup as a parameter, because the branch that matters is only reachable with a real Keychain fault. The test drives it directly with a persisted paired profile and a lookup that returns nil — standing in for the `errSecMissingEntitlement` (-34018) read failure — and asserts the state is `credentialUnreadable`, is not `notPaired`, and still gates entry. Found while diagnosing why a simulator build could pair over LAN and forget on relaunch. That turned out to be an unentitled build with no Keychain access at all, not a product defect — but it surfaced this gate, which is real and does affect a device whose Keychain entry goes bad. 1306 tests, same 13 pre-existing failing cases as clean main. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two real onboarding failures a new user hit on a fresh install, plus the follow-on cleanups the review found. Both bugs were diagnosed from a real report; the cloud side (Clerk sessions, account-directory worker) was verified healthy, so both causes are local.
Bug 1 — the ADE brain could not read the stored account session
The Connections pane showed "Signed in, but this Mac is not published · The ADE brain could not read the stored account session", the account's machine list stayed empty, and "route publish failing" alarmed — all one defect surfaced three times. Only the brain publishes the machine, and it hard-requires the account token, so an undecryptable credential store silently disables cloud publication (and with it, phone quick-connect).
Three flaws could put a machine there; all are fixed:
add-generic-password -Uon any read failure (timeout, locked keychain, item-not-yet-created), so on a first run the app and the brain could each mint a different secret and the loser could never decrypt the file again. Creation now happens only on a genuine not-found, uses a non-clobbering add, and adopts the winner's secret on a lost race; every other outcome fails closed.credentials.json.encand.machine-key.ade_account_session_unreadable(internal-only, edge-triggered per episode, coarsecode) makes field incidence measurable.Bug 2 — "Incorrect PIN." on a pairing that actually succeeded
First-time QR + PIN pairing showed a hard failure and cleared the digits, twice, on a pairing the host had already accepted; dismissing the sheet revealed the phone was connected. #867 moved
.connectedbehind deferred post-hello work, so the sheet'sconnectionState == .connectedcheck was false 100% of the time at the moment it ran, and the successful hello had already clearedlastError— so it fell through to a hardcoded"Incorrect PIN.". Broken for every first-time PIN pairing since ~v1.2.36.The sheet now tests
isAttached(the predicate every other surface already used),.connectedis published at hello-apply, and.syncing— unobservable and dead once that landed — is retired.Also
ade doctorprints the matchingade brain restartremedy.info-severity muted text instead of a warning glyph — every fresh Mac without Tailscale was seeing it.Verification
/quality(3 passes, all findings fixed, gate empty) →/test(prune/consolidate/add, logging + PostHog gate, docs/mobile/CLI/TUI parity) → merge of main → commit-bound quality revalidation of the merge resolution.Desktop 1182 · ade-cli 409 · iOS 82/82 (
SyncRecoveryPolicyTests,SyncAccountConnectRecoveryTests) · both typechecks clean · PostHog spec validators green.Known and deliberate: on Windows the self-heal can hold the credential lock across two PowerShell spawns while the peer lock timeout is 15s, so a peer can see a lock timeout during recovery. Accepted — the alternative is no self-heal at all; raising the timeout or moving key reads outside the lock changes main's concurrency contract and belongs in its own change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements