Repository navigation
fix(native): recover unattended sync helper setup - #681
Farhan (farhankhalaf) wants to merge 2 commits into
Conversation
🎨 Storybook previewUpdated for 4e5f802
|
📋 PR Overview
🔬 Coverage
|
Farhan (farhankhalaf)
changed the title
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
|
| Option | What users see | Cost | Effort to change later |
|---|---|---|---|
| Current (10 s fixed) | Spinner, then failure | May be too long on fast machines; too short on slow ones | Change one backend constant |
| P90-derived threshold | Spinner stops near the true tail | Requires timing instrumentation on real hardware | Needs data collection before the constant can change |
| Immediate failure | No spinner; failure from first attempt | Alarming on healthy installs that just take time | Low effort but wrong UX |
4. Retry is a separate action from Enable (Grant). Retry re-executes the stored decision without changing it or opening System Settings. Enable changes the user's opt-in.
A user who opted in and whose helper failed does not need to re-confirm the opt-in to try again. Conflating Retry with Enable would either force a redundant confirmation or silently turn a failed opt-out into a new opt-in. The separation is also correct for the failed-disable case: the permissions panel shows Retry for a failed removal without showing Enable, so the user cannot accidentally re-enable by clicking the recovery button.
5. Dismissing a helper banner clears when the phase changes, not permanently. A later failure re-shows the banner even if the user dismissed an earlier progress notice.
Permanent dismissal was the wrong model: a user who dismissed a reconciling banner should still see failed if recovery does not happen. Keying the dismiss state to the phase makes the dismiss mean "I see this progress state; do not repeat it" rather than "I never want to hear about the helper again." The phase is the identity that matters, not the banner kind.
6. After a Disable action completes, the permissions panel reads the live row, not the RPC response, to decide what to show.
The RPC response described the state at the moment the action ran. The row reflects the state after reconciliation and publication. Showing the RPC response as a notice could outlive the state it described: if activation was running when Disable was called, the response might say "waiting for activation" long after activation finished and the helper was removed. The live row is always current; the RPC result is a snapshot from one moment.
7. A `notFound` response from ServiceManagement no longer automatically means the installation is broken. The app checks the bundled plist and binary first, and registers if they are valid.
macOS's notFound is ambiguous: it means either "this service was never registered" or "the plist this service would use is gone." A valid app on a first install is in the first state, not the second. Treating both as a broken installation blocked first-time users from enabling the helper at all -- an Enable button was never shown. Checking the bundle independently resolves the ambiguity without relying on an unstable framework error code.
8. When a concurrent background check cannot get the reconciliation slot, the last meaningful row is preserved rather than replaced by a generic `reconciling` state.
A Busy result says only that this observation raced and lost. It carries no information about actual helper state. Replacing an actionable instruction such as "approve in Login Items" with a generic spinner would suppress information the user needs. Preserving the last row means the UI shows the most recent real observation until a real observation replaces it.
9. WaitingForActivation shows no user action -- no Retry, no Open Settings. The banner displays a spinner and the backend loops without a bounded wait.
Showing no action while an activation is legitimately in progress is correct: a registration attempt mid-activation would conflict with the running activation. The concern -- unchanged from the prior review -- is that the loop's WaitingOnActivation path does not increment the mutation counter and has no wall-clock timeout. If a darwin-rebuild activation hangs -- for example, an osascript call that blocks waiting for a UI prompt -- the user sees a spinner indefinitely. Dismissing the banner hides it; because dismissal clears on phase change and the phase does not change during a hang, the banner does not return after dismissal. The user is left with a stuck helper and no in-app signal or recovery path. This requires either a bounded wait in the helper loop or a confirmed statement that the activation layer has its own timeout that bounds this phase.
10. The Login Items instruction changes from "Approve nixmac" to "Allow nixmac".
The macOS Login Items & Extensions UI uses the word "Allow", not "Approve". Matching the system vocabulary reduces the chance that a user searches for a button that does not exist.
11. Helper failure diagnostics are included in the opt-in feedback snapshot, redacted of credential-shaped strings.
The failure detail (for example, SMAppServiceErrorDomain 1) is exactly the information the team needs to reproduce and fix real helper failures. The redaction test confirms the pattern removes token-shaped strings before the snapshot is sent. This is the correct tradeoff: diagnostic value with credential safety.
Since the last review
- Addressed in code: Ten-second neutral window before failure escalation is a constant chosen without production timing data. (The frontend
HELPER_GRACE_MS = 10_000timer is removed. The backendFAILURE_PRESENTATION_AFTER: Duration = Duration::from_secs(10)replaces it. The user now sees areconcilingspinner during this window rather than nothing, which lowers the UX cost of an overly long threshold. The constant is still hardcoded but the stakes are lower.) - Still open: WaitingForActivation loop does not increment the mutation counter, so a hung activation leaves the user with an indefinite spinner and no in-app exit. (The
driveloop still doeswait(WAITING_INTERVAL); continue;onReconciled::WaitingOnActivation(_)without incrementingworkingor checking elapsed time. The new testoffers no repair action while waiting for an activationconfirms no action is offered, and no timeout is added to this path.) - Addressed in code: Retry button copy does not distinguish a failed enable from a failed disable, so the user cannot confirm what Retry will do. (The repair banner condition gates on
helperPreference === "granted", so "Couldn't finish enabling unattended sync" is contextually correct there. In the permissions panel, a failed disable shows Retry without an Enable button (confirmed by the testretries a failed removal without offering to enable the helper again), preventing accidental re-opt-in even though the copy is generic.) - Still open: No p90 reconciliation timing data to validate the ten-second neutral window on real hardware. (No timing data or instrumentation is added in this PR. The
FAILURE_PRESENTATION_AFTERconstant remainsDuration::from_secs(10)with no comment citing measurement.) - Still open: Unknown whether the darwin-rebuild activation layer has its own timeout bounding the WaitingForActivation phase. (No timeout is added to the
WaitingOnActivationloop path, and the PR description and diff contain no statement about whether the activation layer provides its own bound.)
Open questions
-
What is the p90 reconciliation time on a healthy first install on real hardware? This determines whether the ten-second failure presentation window correctly separates transient failures from persistent ones, or whether it is too short for slow machines and too long for fast ones.
-
Does the darwin-rebuild activation layer have its own wall-clock timeout that bounds the WaitingForActivation phase? If it does not, a hung activation leaves the user with an indefinite spinner and no in-app recovery after dismissal.
Recommendation
Ship with changes
The PR fixes a real first-install defect, replaces fragile UI-side text parsing with backend-owned typed phases, and separates Retry from Enable correctly. One prior concern remains fully open: the WaitingForActivation loop has no bounded timeout, and after a user dismisses the banner during a hung activation, no in-app signal or recovery returns. This is pre-existing but the PR explicitly tests that no action is offered during this phase without adding a bound, making it a confirmed design choice that needs a clear answer -- either a timeout in the loop or evidence that the activation layer already bounds this phase -- before this behavior is treated as finished.
![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: 9a9974b350
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Follow-up on the activation-wait and timing questions:
Source: helper wait behavior, client reply timeout. |
|
Follow-up is blocked only on #682's current Danger run, which is queued with no ARC runner assigned. The previous metadata run waited 20 minutes before the status edit superseded it. CI maintainers need to inspect ARC capacity/dispatch, get the current run executed, and check its coverage and Danger results. Validation remains incomplete until then. Heads are unchanged: #681
Both CI checklists are updated, with #682 explicitly incomplete and native evidence limits preserved. No source edits, pushes or merges. Human review remains requested from czxtm, scottmcmaster and arximboldi. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if nothing_more_to_do(&report) { | ||
| presentation.note_settled_report(&report, attempt_generation); | ||
| publish(&report); | ||
| return; |
There was a problem hiding this comment.
The generation check protects the saved failure, but not the row that drive_with_clock publishes. If the loop gets AtThisBuild, then a newer Disable attempt fails before publication, the loop still publishes the old Ready row and exits. The newer action cannot start another loop while CONVERGING is set. This hides the failed removal and can stop automatic recovery.
Skip stale reports before publishing or exiting, and ensure the newer attempt gets another pass. Update the race test to check the actual published row rather than a separate synthetic failure.
There was a problem hiding this comment.
Fixed in 7ae7c85. A report whose generation changed during the pass is skipped, and the loop observes again instead of exiting. After a settled publish it exits only if the generation is unchanged, and if an action starts while a row is being published the loop re-observes right away. a_stale_ready_row_never_hides_a_newer_failed_action asserts on the published rows (Stopped, then Removed).
| setNotice({ | ||
| tone: "error", | ||
| message: `Retrying the unattended sync helper failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| }); |
There was a problem hiding this comment.
The new Retry error is defined directly in PermissionsPanel. The repository requires user-facing TypeScript errors to be defined and reported through nostics in errors.ts, with a diagnostic code, title, and suggestion. Add the Retry diagnostic there and use it here instead of the ad hoc message. This repository requirement must be satisfied before merging.
Context Used: TypeScript error handling with nostics in the native app (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 4e5f802. Retry failures from this panel and from the repair banner now report through nostics as HELPER_E001 in errors.ts, with its explanation and fix.
cooper (czxtm)
left a comment
cooper (czxtm)
left a comment
There was a problem hiding this comment.
Requesting changes: this needs a rebase onto main and a decision on registration policy, because #691 (f28a2e2, "Register valid helpers on explicit Enable") already landed an incompatible fix for the same notFound problem and the branch conflicts with it in 5 files (protocol.rs, reconcile.rs, service.rs, permissions-panel.tsx and its test).
Blocking
- Registration policy.
convergeregisters on(NotRegistered | NotFound, Goal::Install)with no distinction between a background status check and a user click (reconcile.rs#L531-L534). With a saved Enable preference, any observation or permissions refresh can call register without a click. Main splits this withenum Invocation, andInvocation::ObservereturnsReconciled::RegistrationUnavailableand never registers. Please rebase, adopt main's explicit-Enable policy unless you want to make the case for background registration, and route the newhelperRetrythrough the Enable invocation when the saved decision is Enable. The tests that expect background re-registration should go. - Bundle validation.
check_service_definition(service.rs#L68-L115) reads withfs::readandfs::metadata, which follow symlinks, and it rejectsProgrambut notProgramArguments. The helper runs as root, so a bundle whose plist or binary resolves outside the bundle, or which carriesProgramArguments, passes this check. Drop it and keep main'svalidate_bundled_definition, which canonicalizes, requires regular files inside the bundle, and rejects both keys. If you want an Unavailable versus Invalid split, layer it on top of that.
Two Greptile threads are still open (the stale-Ready publish race in drive_with_clock, and the Retry diagnostics one). The race is a merge blocker for me too, and the fix needs a regression test that asserts on the published row.
Smaller things
- After the rebase,
phase_with_failure_visibilityand thedebug_assert!indrive_with_clockneed to handleReconciled::RegistrationUnavailableexplicitly, with a phase that offers Enable or Retry. Otherwise it can fail to compile or fall into a wildcard arm. - Danger flags no Linear ID, and the PR bundles the registration fix, typed lifecycle, Retry UI, diagnostics and a Clippy cleanup. Once the validation piece is dropped, splitting the backend phase and retry from the UI and diagnostics would help, since #682 is stacked on this.
- #682 will need restacking after this lands, and the test results were produced before the rebase onto #691, so please refresh them on the new head.
…current Port the helper recovery backend onto the explicit-Enable registration policy. Observation still never registers a missing helper. - Add a Retry action that carries out the stored decision. A stored Enable retries as an explicit Enable, so a missing registration gets one new register. A stored Disable retries the removal. An undecided user is handled exactly like an observation: Retry never registers a missing helper, though it adopts an existing registration the same way an observation does. Retry never opens System Settings. - After a register error, judge the result from one fresh status read (approval, enabled and verified, or still missing) and keep the register error in the report instead of issuing a second register. - Report a typed HelperPermissionPhase on the permission row and on helper action results. RegistrationUnavailable maps to Failed, which offers Retry. While a stored Disable is in progress, waiting on an activation or failed, the row describes the removal instead of setup. - Classify every report into an exhaustive loop step instead of relying on a debug assertion. - Skip a convergence report that predates a newer user action, so a stale Ready row can no longer hide a failed Disable. A start request that arrives while the loop is ending is recorded and served, and each pass acknowledges the requests it serves, so no request is dropped and none buys a second attempt budget. - Busy observations preserve the last meaningful row, and the last helper failure is included in the opt-in feedback snapshot. - Regenerate the oRPC and specta TypeScript bindings, and give the existing panel test's HelperReport the new required phase so this commit typechecks on its own. - Remove a redundant `.nixmacignore` binding that strict Clippy rejects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…backend - The permissions row and the repair banner read the typed helper phase: approval, setup progress, an active build or restore, a needed user action, removal and failure each get their own copy and action. - A failed phase, including a missing registration, offers Retry, which calls helperRetry and keeps the saved Enable or Disable decision. Disable stays available for a wanted helper. No Enable is offered while a stored Disable has not finished; a failed removal offers Retry. - Helper action notices follow the live row instead of copying an earlier request result. While an Enable replaces the previous row, the banner shows setup progress rather than a failure. - A failed Retry from the panel or the repair banner is reported through the HELPER_E001 nostics diagnostic. The banner clears that error when its failure episode ends, and ignores a Retry that settles after its episode ended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9383d71 to
4e5f802
Compare
|
Rebuilt on main (1492663) as two commits: the backend (7ae7c85) and the UI (4e5f802).
Local test results on the new head are in the description. The signed-app helper smoke on a real Mac has not been rerun on this head. #682 will be restacked once this lands. |
|
Summary
Builds on main's explicit-Enable registration policy. #682 is stacked on this branch and will be restacked after this lands. Linear: ENG-1447.
Helper registration can look like a persistent failure while macOS is still settling, or lose a newer failure during concurrent checks. Approval, setup progress, active-sync waits, manual remediation and failure now have distinct phases and matching actions, and removal has its own copy. Retry repeats the stored decision: a stored Enable retries as an explicit Enable, a stored Disable retries the removal, and an unset decision is handled like an observation. Observation never registers a missing helper. Busy observations preserve the last meaningful status, and a report from before a newer action is skipped rather than published.
Registration errors keep their diagnostic detail, including when macOS reports Enabled but helper verification then fails. Helper action messages follow the live permission row, and Retry failures from the panel and the repair banner report through the
HELPER_E001diagnostic. Bundle validation is main'svalidate_bundled_definition, unchanged. A small cleanup removes a redundant.nixmacignorebinding so strict Clippy passes.The change is two commits: the backend (7ae7c85) and the UI (4e5f802). They ship together because the backend alone would show "Try again" copy with no Retry button.
Test Plan
cargo test -p nixmac1,130 passed, 10 ignored; nixmac-helper 77 and nixmac-sync-agent 49 passed. Strict workspace Clippy with all targets and codegen and the rustfmt check passed.system-defaults-ctatests that fail identically on main under local Node 25 (localStorage.clear is not a function).Docs
🤖 Generated with Claude Code
https://claude.ai/code/session_01GziRseGZMsJ5Yf7EboZLMc