diff --git a/apps/native/src-tauri/src/evolve/nixmac_ignore.rs b/apps/native/src-tauri/src/evolve/nixmac_ignore.rs index 4363f8b98..d10352427 100644 --- a/apps/native/src-tauri/src/evolve/nixmac_ignore.rs +++ b/apps/native/src-tauri/src/evolve/nixmac_ignore.rs @@ -96,19 +96,9 @@ impl NixmacIgnoreChecker { return false; } - let ignored = matcher + matcher .matched_path_or_any_parents(relative_path, is_dir) - .is_ignore(); - - // This diagnostic is very noisy so it's commented out by default, but it's useful - // for validating the ignore rules. - // log::debug!( - // "nixmac ignore check: {} (dir={}) => {}", - // relative_path.display(), - // is_dir, - // ignored - // ); - ignored + .is_ignore() } } diff --git a/apps/native/src-tauri/src/feedback.rs b/apps/native/src-tauri/src/feedback.rs index 1af1db6f4..dc63a2e05 100644 --- a/apps/native/src-tauri/src/feedback.rs +++ b/apps/native/src-tauri/src/feedback.rs @@ -390,6 +390,7 @@ pub fn gather_app_state(app: &AppHandle, feedback_type: &str) -> Value { "configDir": config_dir, "hostAttr": host_attr, "feedbackType": feedback_type, + "helperFailure": crate::system::helper_permission::last_failure_detail(), "evolution": evolution_state, "timestamp": Utc::now().to_rfc3339(), }) @@ -849,6 +850,7 @@ regex = "token=([A-Za-z0-9]+)" let mut metadata = empty_metadata(); metadata.current_app_state_snapshot = Some(json!({ "token": "token=abc123", + "helperFailure": "SMAppServiceErrorDomain 1: token=abc123", "nested": { "value": "token=xyz" } })); metadata.ai_provider_model_info = Some(types::FeedbackAiProviderModelInfo { @@ -868,6 +870,9 @@ regex = "token=([A-Za-z0-9]+)" assert!(redacted_fields.contains(&"ai_provider_model_info")); let state = metadata.current_app_state_snapshot.unwrap(); + let helper_failure = state["helperFailure"].as_str().unwrap(); + assert!(helper_failure.contains("SMAppServiceErrorDomain 1")); + assert!(!helper_failure.contains("abc123")); assert!( state .get("token") diff --git a/apps/native/src-tauri/src/orpc/darwin.rs b/apps/native/src-tauri/src/orpc/darwin.rs index 6e02e110c..453ed3f0a 100644 --- a/apps/native/src-tauri/src/orpc/darwin.rs +++ b/apps/native/src-tauri/src/orpc/darwin.rs @@ -8,7 +8,7 @@ use crate::privileged_helper::{ }; use crate::shared_types::{ AppManagementCheckResult, BuildCheckResult, EtcClobberCheckResult, EvolveCancelResult, - OkResult, RebuildStatus, RollbackResult, + HelperPermissionPhase, OkResult, RebuildStatus, RollbackResult, }; use crate::system::helper_permission; use orpc::*; @@ -75,6 +75,7 @@ struct InstallSyncAgentInput { #[serde(rename_all = "camelCase")] struct HelperReport { at_this_build: bool, + phase: HelperPermissionPhase, detail: String, } @@ -85,7 +86,8 @@ impl HelperReport { report, crate::privileged_helper::reconcile::Reconciled::AtThisBuild ), - detail: helper_permission::describe(report), + phase: helper_permission::phase(report), + detail: helper_permission::action_detail(report), } } } @@ -233,6 +235,14 @@ async fn helper_grant(ctx: OrpcCtx, _input: ()) -> Result Result { + Ok(HelperReport::of(&helper_permission::retry(&ctx.app))) +} + /// The explicit Disable action: unregister the helper (deferring while an /// activation runs) and /// register nothing. No later automatic run overrides it. @@ -318,6 +328,9 @@ pub fn routes() -> Router { "helperGrant" => os::() .output(orpc_specta::specta::()) .handler(helper_grant), + "helperRetry" => os::() + .output(orpc_specta::specta::()) + .handler(helper_retry), "helperDisable" => os::() .output(orpc_specta::specta::()) .handler(helper_disable), diff --git a/apps/native/src-tauri/src/privileged_helper/reconcile.rs b/apps/native/src-tauri/src/privileged_helper/reconcile.rs index 524cfa70f..73c374e50 100644 --- a/apps/native/src-tauri/src/privileged_helper/reconcile.rs +++ b/apps/native/src-tauri/src/privileged_helper/reconcile.rs @@ -3,8 +3,9 @@ // One idempotent function. Every run observes the world, drives it one step // closer to the stored decision, and forgets everything: no phase, no // progress, no recovery mode. A run starts over from fresh observation; -// an absent registration needs an explicit Enable, including after a crash -// between unregister and register, so an uncertain call is never replayed. +// an absent registration needs an explicit Enable (a Grant, or a Retry under a +// stored Grant), including after a crash between unregister and register, so +// an uncertain call is never replayed by observation. // That is the whole // design — there is deliberately nothing here that coordinates, queues, or // remembers. @@ -123,8 +124,9 @@ pub enum Reconciled { /// The actual bundled plist or executable failed validation. ServiceDefinitionBroken, /// The bundled definition is present, but ServiceManagement cannot find - /// the registration. An explicit Enable may attempt registration once; - /// observation must not turn this uncertainty into repeated mutations. + /// the registration. An explicit Enable (a Grant, or a Retry under a + /// stored Grant) may attempt registration once; observation must not turn + /// this uncertainty into repeated mutations. RegistrationUnavailable, /// Retryable: the run stopped short. Nothing further was mutated; a later /// run converges from what it observes then. @@ -458,6 +460,20 @@ pub fn grant(env: &E) -> Reconciled { decide(&RUNNING, env, HelperDecision::Granted) } +/// The explicit Retry action: carry out the decision already stored. A stored +/// Enable retries as an explicit Enable, so a missing registration gets one new +/// attempt; a stored Disable retries the removal. An undecided user is handled +/// exactly like an observation, including [`goal`]'s adoption of a registration +/// that already exists as the earlier opt-in; with nothing to adopt it registers +/// nothing. Unlike an observation this waits for the single-flight slot, and +/// unlike [`grant`] it never records an opt-in of its own or registers a +/// missing helper for anyone who has not chosen Enable. +/// +/// Opening System Settings is never this function's action, nor its caller's. +pub fn retry(env: &E) -> Reconciled { + retry_with(&RUNNING, env) +} + /// The explicit Disable action: record the decision, then reconcile under it. /// The decision is written first, so a crash halfway resumes the removal /// rather than repairing the helper. @@ -491,6 +507,29 @@ fn decide(running: &Mutex<()>, env: &E, decision: HelperDecision reported(converge(env, invocation).unwrap_or_else(|report| report)) } +/// Retries under the stored decision. The slot is a parameter for the same +/// reason as [`decide`]'s. +/// +/// The decision is read after the slot is held, so a Grant or Disable that +/// finished while this waited is the one retried. Only a stored Enable may +/// register a missing helper, and [`commit_to_register`] re-reads the decision +/// immediately before that register, so a Disable recorded in between still +/// declines it. Any other decision runs the observation table unchanged. +fn retry_with(running: &Mutex<()>, env: &E) -> Reconciled { + // Nothing is written before the pass, so the pass's own first gate is the + // only one a retry needs. Poison handled as in [`decide`]. + let _slot = running.lock().unwrap_or_else(PoisonError::into_inner); + let invocation = match env.preference() { + Ok(HelperPreference::Granted) => Invocation::Enable, + // Removal and an undecided user need no explicit invocation: the + // observation table already removes a disabled helper and never + // registers for an undecided one. + Ok(HelperPreference::Disabled | HelperPreference::Unset) => Invocation::Observe, + Err(detail) => return reported(preference_unreadable(detail)), + }; + reported(converge(env, invocation).unwrap_or_else(|report| report)) +} + fn run(running: &Mutex<()>, env: &E) -> Reconciled { let _slot = match running.try_lock() { Ok(slot) => slot, @@ -738,20 +777,24 @@ fn remove(env: &E) -> Step { fn replace(env: &E) -> Step { match env.replace_helper(&|| commit_to_register(env)) { Ok(()) => verify(env), - Err(failure) => Err(match failure { + Err(failure) => match failure { // The register step's own report, whatever it was: this pass // unregistered the old helper and deliberately registered // nothing in its place. - ReplaceFailure::RegisterDeclined(report) => report, - ReplaceFailure::UnregisterFailed(error) => unregister_failed(error), - ReplaceFailure::UnregisterSilent => unregister_failed("the unregister never reported"), - ReplaceFailure::RegisterFailed(error) => register_failed(error), - ReplaceFailure::RegisterSilent => register_failed("the register never reported"), - // Refused before anything was dispatched, so nothing changed. - ReplaceFailure::CalledOnMainThread => { - register_failed("a helper replacement cannot run on the main thread") + ReplaceFailure::RegisterDeclined(report) => Err(report), + ReplaceFailure::UnregisterFailed(error) => Err(unregister_failed(error)), + ReplaceFailure::UnregisterSilent => { + Err(unregister_failed("the unregister never reported")) } - }), + ReplaceFailure::RegisterFailed(error) => after_register_failure(env, error.to_string()), + ReplaceFailure::RegisterSilent => { + after_register_failure(env, "the register never reported".to_string()) + } + // Refused before anything was dispatched, so nothing changed. + ReplaceFailure::CalledOnMainThread => Err(register_failed( + "a helper replacement cannot run on the main thread", + )), + }, } } @@ -759,14 +802,45 @@ fn register_fresh(env: &E) -> Step { // The gates are inside the commitment, which is where every register's // immediately-before checks live. let committed = commit_to_register(env)?; - env.register(committed).map_err(|failure| match failure { - RegisterFailure::Failed(error) => register_failed(error), - RegisterFailure::Silent => register_failed("the register never reported"), - RegisterFailure::CalledOnMainThread => { - register_failed("a registration cannot run on the main thread") + match env.register(committed) { + Ok(()) => verify(env), + Err(RegisterFailure::Failed(error)) => after_register_failure(env, error.to_string()), + Err(RegisterFailure::Silent) => { + after_register_failure(env, "the register never reported".to_string()) } - })?; - verify(env) + Err(RegisterFailure::CalledOnMainThread) => Err(register_failed( + "a registration cannot run on the main thread", + )), + } +} + +/// A ServiceManagement register can report an error after macOS has already +/// moved the service into a meaningful state. Judge that state from one fresh +/// status read, without keying behavior to an unstable numeric error code and +/// without a second register in this pass. +/// +/// A registration that is still missing keeps the register's own error as a +/// stop rather than becoming [`Reconciled::RegistrationUnavailable`]: the error +/// is the only diagnostic this pass has. The next observation of the same +/// missing registration reports `RegistrationUnavailable` and never replays it. +fn after_register_failure(env: &E, register_error: String) -> Step { + log::warn!("helper register reported an error: {register_error}"); + match env.registration_status() { + Ok(RegistrationStatus::RequiresApproval) => Ok(Reconciled::PendingApproval), + Ok(RegistrationStatus::Enabled) => verify_listening(env).map_err(|report| match report { + Reconciled::Stopped(detail) => Reconciled::Stopped(format!( + "{register_error}; helper verification failed: {detail}" + )), + Reconciled::RegistrationUnavailable => register_failed(®ister_error), + other => other, + }), + Ok(RegistrationStatus::NotRegistered | RegistrationStatus::NotFound) => { + Err(register_failed(register_error)) + } + Err(status_error) => Err(register_failed(format!( + "{register_error}; the resulting registration status could not be read: {status_error}" + ))), + } } /// The register step's immediately-before checks, and the only mint of the @@ -1162,6 +1236,21 @@ mod tests { } } + /// The pass an explicit Retry runs under the default stored Enable: what a + /// first registration, and every register-error judgment below, run under + /// since observation never registers a missing helper. + fn retried(world: &World) -> Reconciled { + retry_with(&Mutex::new(()), world) + } + + fn register_refusal() -> service::ServiceCallError { + service::ServiceCallError { + domain: "SMAppServiceErrorDomain".to_string(), + code: 1, + localized: "operation not permitted".to_string(), + } + } + fn displaced_text(report: &Reconciled) -> String { match report { Reconciled::Displaced(text) => text.clone(), @@ -1586,6 +1675,259 @@ mod tests { // ── refusals and failures that end a pass ────────────────────────────── + #[test] + fn a_fresh_register_error_is_judged_from_one_authoritative_status_read() { + for (resulting_status, expected) in [ + ( + Ok(RegistrationStatus::RequiresApproval), + Reconciled::PendingApproval, + ), + ( + Ok(RegistrationStatus::NotFound), + register_failed(register_refusal()), + ), + ] { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + resulting_status, + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + ..World::default() + }; + + assert_eq!(retried(&world), expected); + assert_eq!(world.acts(), vec![Act::Registered]); + } + + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::Enabled), + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + assert_eq!(retried(&world), Reconciled::AtThisBuild); + assert_eq!(world.acts(), vec![Act::Registered]); + } + + #[test] + fn a_fresh_register_error_keeps_its_diagnostics_and_is_not_replayed() { + for resulting_status in [ + Ok(RegistrationStatus::NotRegistered), + Err("status unavailable".to_string()), + ] { + let status_was_unavailable = resulting_status.is_err(); + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + resulting_status, + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + ..World::default() + }; + + let detail = stopped_text(&retried(&world)); + assert!(detail.contains("SMAppServiceErrorDomain 1")); + if status_was_unavailable { + assert!(detail.contains("status unavailable")); + assert!(detail.contains("resulting registration status could not be read")); + } else { + // The next observation sees the same missing registration + // and reports it without a second register. + assert_eq!(fresh_run(&world), Reconciled::RegistrationUnavailable); + } + assert_eq!(world.acts(), vec![Act::Registered]); + } + } + + #[test] + fn register_errors_survive_failed_verification_for_fresh_and_replacement_helpers() { + for (peers, verification_detail) in [ + ( + vec![Peer::RootUnverifiable], + "the helper socket is held by something unidentified: ad-hoc signature".to_string(), + ), + ( + vec![Peer::Answered(status_reply( + OTHER_BUILD, + HelperStateName::Idle, + None, + ))], + format!("the registered helper reports build {OTHER_BUILD}, not this build"), + ), + ( + vec![Peer::Unreachable; VERIFY_LISTEN_ATTEMPTS as usize], + "the registered helper never answered on its socket".to_string(), + ), + ] { + for replacing in [false, true] { + let mut exchanges = Vec::new(); + if replacing { + exchanges.push(Peer::Answered(status_reply( + OTHER_BUILD, + HelperStateName::Idle, + None, + ))); + } + exchanges.extend(peers.clone()); + let world = World { + status: RefCell::new(vec![ + Ok(if replacing { + RegistrationStatus::Enabled + } else { + RegistrationStatus::NotRegistered + }), + Ok(RegistrationStatus::Enabled), + ]), + register: RefCell::new( + (!replacing).then(|| RegisterFailure::Failed(register_refusal())), + ), + replace: RefCell::new( + replacing.then(|| ReplaceFailure::RegisterFailed(register_refusal())), + ), + exchanges: RefCell::new(exchanges), + ..World::default() + }; + + // A replacement is the observation table's own repair; a + // missing helper is registered only by an explicit Enable. + let report = if replacing { + fresh_run(&world) + } else { + retried(&world) + }; + assert_eq!( + stopped_text(&report), + format!( + "{}; helper verification failed: {verification_detail}", + register_refusal() + ) + ); + assert_eq!( + world.acts(), + vec![if replacing { + Act::Replaced + } else { + Act::Registered + }] + ); + assert_eq!(*world.preference.borrow(), Ok(HelperPreference::Granted)); + } + } + } + + #[test] + fn register_error_does_not_change_approval_discovered_during_verification() { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::Enabled), + Ok(RegistrationStatus::RequiresApproval), + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + exchanges: RefCell::new(vec![Peer::Unreachable; VERIFY_LISTEN_ATTEMPTS as usize]), + ..World::default() + }; + + assert_eq!(retried(&world), Reconciled::PendingApproval); + assert_eq!(world.acts(), vec![Act::Registered]); + } + + #[test] + fn register_error_keeps_its_detail_when_the_registration_vanishes_during_verification() { + for vanished in [ + RegistrationStatus::NotFound, + RegistrationStatus::NotRegistered, + ] { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::Enabled), + Ok(vanished), + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + exchanges: RefCell::new(vec![Peer::Unreachable; VERIFY_LISTEN_ATTEMPTS as usize]), + ..World::default() + }; + + assert_eq!(retried(&world), register_failed(register_refusal())); + assert_eq!(fresh_run(&world), Reconciled::RegistrationUnavailable); + assert_eq!(world.acts(), vec![Act::Registered]); + } + } + + #[test] + fn a_silent_fresh_register_uses_the_same_status_judgment() { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::RequiresApproval), + ]), + register: RefCell::new(Some(RegisterFailure::Silent)), + ..World::default() + }; + + assert_eq!(retried(&world), Reconciled::PendingApproval); + assert_eq!(world.acts(), vec![Act::Registered]); + } + + #[test] + fn first_time_grant_surfaces_approval_even_when_register_reports_an_error() { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::RequiresApproval), + ]), + preference: RefCell::new(Ok(HelperPreference::Unset)), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + ..World::default() + }; + + assert_eq!( + decide(&Mutex::new(()), &world, HelperDecision::Granted), + Reconciled::PendingApproval + ); + assert_eq!(world.acts(), vec![Act::StoredGranted, Act::Registered]); + } + + #[test] + fn a_replacement_register_error_uses_the_same_status_judgment() { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::Enabled), + Ok(RegistrationStatus::RequiresApproval), + ]), + exchanges: RefCell::new(vec![Peer::Answered(status_reply( + OTHER_BUILD, + HelperStateName::Idle, + None, + ))]), + replace: RefCell::new(Some(ReplaceFailure::RegisterFailed(register_refusal()))), + ..World::default() + }; + + assert_eq!(fresh_run(&world), Reconciled::PendingApproval); + assert_eq!(world.acts(), vec![Act::Replaced]); + + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::Enabled), + Ok(RegistrationStatus::Enabled), + ]), + exchanges: RefCell::new(vec![ + Peer::Answered(status_reply(OTHER_BUILD, HelperStateName::Idle, None)), + Peer::Answered(this_build_idle()), + ]), + replace: RefCell::new(Some(ReplaceFailure::RegisterSilent)), + ..World::default() + }; + assert_eq!(fresh_run(&world), Reconciled::AtThisBuild); + assert_eq!(world.acts(), vec![Act::Replaced]); + } + #[test] fn a_reply_that_is_not_a_status_is_a_refusal() { let world = World::answering(HelperReply::RequestNotUnderstood); @@ -1812,4 +2154,171 @@ mod tests { assert_eq!(acts, vec![Act::StoredGranted, Act::Registered]); }); } + + // ── explicit retry ───────────────────────────────────────────────────── + + #[test] + fn retry_registers_a_missing_helper_only_under_a_stored_enable() { + for missing in [ + RegistrationStatus::NotRegistered, + RegistrationStatus::NotFound, + ] { + let world = World { + status: RefCell::new(vec![Ok(missing), Ok(RegistrationStatus::Enabled)]), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + assert_eq!(retried(&world), Reconciled::AtThisBuild); + assert_eq!( + world.acts(), + vec![Act::Registered], + "a retry records no decision" + ); + assert_eq!(world.preference(), Ok(HelperPreference::Granted)); + } + } + + #[test] + fn retry_never_installs_for_an_undecided_or_disabled_user() { + for preference in [HelperPreference::Unset, HelperPreference::Disabled] { + for missing in [ + RegistrationStatus::NotRegistered, + RegistrationStatus::NotFound, + ] { + let world = World { + preference: RefCell::new(Ok(preference)), + status: RefCell::new(vec![Ok(missing)]), + ..World::default() + }; + assert_eq!(retried(&world), Reconciled::NoHelper); + assert!(world.acts().is_empty()); + assert_eq!(world.preference(), Ok(preference)); + } + } + } + + #[test] + fn an_undecided_retry_behaves_exactly_like_an_observation_including_adoption() { + // An existing registration is the user's earlier opt-in on every + // path that resolves an undecided user, Retry included. + for existing in [ + RegistrationStatus::RequiresApproval, + RegistrationStatus::Enabled, + ] { + let observed = World { + preference: RefCell::new(Ok(HelperPreference::Unset)), + status: RefCell::new(vec![Ok(existing)]), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + let retried_world = World { + preference: RefCell::new(Ok(HelperPreference::Unset)), + status: RefCell::new(vec![Ok(existing)]), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + assert_eq!(retried(&retried_world), fresh_run(&observed)); + assert_eq!(retried_world.acts(), observed.acts()); + assert_eq!(retried_world.acts().first(), Some(&Act::StoredGranted)); + } + } + + #[test] + fn retry_carries_out_a_stored_disable_without_rewriting_it() { + let world = World { + preference: RefCell::new(Ok(HelperPreference::Disabled)), + status: RefCell::new(vec![Ok(RegistrationStatus::RequiresApproval)]), + ..World::default() + }; + assert_eq!(retried(&world), Reconciled::Removed); + assert_eq!(world.acts(), vec![Act::Unregistered]); + assert_eq!(world.preference(), Ok(HelperPreference::Disabled)); + + let world = World { + preference: RefCell::new(Ok(HelperPreference::Disabled)), + ..World::answering(this_build_idle()) + }; + assert_eq!(retried(&world), Reconciled::Removed); + assert_eq!(world.acts(), vec![Act::Unregistered]); + assert_eq!(world.preference(), Ok(HelperPreference::Disabled)); + } + + #[test] + fn a_failed_first_registration_waits_for_retry_and_is_never_replayed_by_observation() { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotFound), + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::Enabled), + ]), + register: RefCell::new(Some(RegisterFailure::Failed(register_refusal()))), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + + let enabled = decide(&Mutex::new(()), &world, HelperDecision::Granted); + assert!(stopped_text(&enabled).contains("SMAppServiceErrorDomain 1")); + assert_eq!(world.acts(), vec![Act::StoredGranted, Act::Registered]); + + assert_eq!(fresh_run(&world), Reconciled::RegistrationUnavailable); + assert_eq!( + world.acts(), + vec![Act::StoredGranted, Act::Registered], + "observation must not register" + ); + + assert_eq!(retried(&world), Reconciled::AtThisBuild); + assert_eq!( + world.acts(), + vec![Act::StoredGranted, Act::Registered, Act::Registered] + ); + } + + #[test] + fn retry_is_gated_and_reports_an_unreadable_decision_without_acting() { + let world = World::default() + .with_gates(vec![Err(Reconciled::Displaced("app replaced".to_string()))]); + assert_eq!( + retried(&world), + Reconciled::Displaced("app replaced".to_string()) + ); + assert!(world.acts().is_empty()); + + let world = World { + preference: RefCell::new(Err("preferences locked".to_string())), + status: RefCell::new(vec![Ok(RegistrationStatus::NotRegistered)]), + ..World::default() + }; + assert!(stopped_text(&retried(&world)).contains("could not be read")); + assert!(world.acts().is_empty()); + } + + #[test] + fn retry_waits_for_a_real_pass_without_rewriting_the_decision() { + let running = Mutex::new(()); + let held = running.lock().expect("a fresh lock"); + std::thread::scope(|scope| { + let retried = scope.spawn(|| { + let world = World { + status: RefCell::new(vec![ + Ok(RegistrationStatus::NotRegistered), + Ok(RegistrationStatus::Enabled), + ]), + exchanges: RefCell::new(vec![Peer::Answered(this_build_idle())]), + ..World::default() + }; + let report = retry_with(&running, &world); + (report, world.acts()) + }); + std::thread::sleep(Duration::from_millis(100)); + assert!(!retried.is_finished(), "retry did not wait for the slot"); + drop(held); + + let (report, acts) = retried.join().expect("retry"); + assert_eq!(report, Reconciled::AtThisBuild); + assert_eq!(acts, vec![Act::Registered]); + }); + } } diff --git a/apps/native/src-tauri/src/privileged_helper/service.rs b/apps/native/src-tauri/src/privileged_helper/service.rs index ee5f4be11..b824b8cfa 100644 --- a/apps/native/src-tauri/src/privileged_helper/service.rs +++ b/apps/native/src-tauri/src/privileged_helper/service.rs @@ -151,8 +151,9 @@ pub enum ReplaceFailure { /// helper is registered now. The reason is the caller's own: this module /// supplies the point where such a decision is possible and takes none. RegisterDeclined(E), - /// The old process is gone and the replacement was refused: no helper is - /// registered now. + /// The old process is gone and the replacement reported a refusal. macOS + /// can still move the service to approval-required or enabled, so callers + /// must re-read status before describing the resulting state. RegisterFailed(ServiceCallError), /// The old process is gone and the replacement never reported. Whether it /// took is unknown — only a fresh observation can say. @@ -181,7 +182,8 @@ pub enum RegisterFailure { /// Refused before anything was dispatched: the main thread is the one that /// has to make the call, so awaiting it there would starve the queue. CalledOnMainThread, - /// The platform refused the registration; nothing is registered. + /// The platform reported a registration refusal. macOS can still move the + /// service to approval-required or enabled, so callers re-read status. Failed(ServiceCallError), /// The call never reported inside the window. Whether it took is unknown — /// only a fresh observation can say. diff --git a/apps/native/src-tauri/src/shared_types/system.rs b/apps/native/src-tauri/src/shared_types/system.rs index 051a2747e..2419c4274 100644 --- a/apps/native/src-tauri/src/shared_types/system.rs +++ b/apps/native/src-tauri/src/shared_types/system.rs @@ -57,6 +57,24 @@ pub enum PermissionStatus { Unknown, } +/// Product-facing state of the unattended sync helper. +/// +/// The helper is represented in the permissions list, but its lifecycle has +/// more states than a normal permission grant. Keeping those states typed lets +/// every UI surface use the same copy and actions without parsing diagnostic +/// text or overloading [`PermissionStatus`]. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Type, Copy)] +#[serde(rename_all = "camelCase")] +pub enum HelperPermissionPhase { + Ready, + ApprovalRequired, + Reconciling, + WaitingForActivation, + NeedsUserAction, + Failed, + Disabled, +} + /// Individual permission state. #[derive(Debug, Clone, Serialize, Deserialize, Type)] #[serde(rename_all = "camelCase")] @@ -83,6 +101,9 @@ pub struct Permission { /// Manual instructions for permissions that cannot be requested programmatically. #[serde(skip_serializing_if = "Option::is_none")] pub instructions: Option, + /// Helper-specific lifecycle state. Absent for normal permission rows. + #[serde(skip_serializing_if = "Option::is_none")] + pub helper_phase: Option, } /// All permissions state. diff --git a/apps/native/src-tauri/src/system/helper_permission.rs b/apps/native/src-tauri/src/system/helper_permission.rs index 057c3c7d7..66d50fc44 100644 --- a/apps/native/src-tauri/src/system/helper_permission.rs +++ b/apps/native/src-tauri/src/system/helper_permission.rs @@ -2,10 +2,10 @@ //! //! One reconciliation function decides everything about the installed helper //! (`privileged_helper::reconcile`). This module is the only place the GUI calls -//! it from, and it holds nothing but the wiring: +//! it from, and it owns the wiring plus the one shared presentation window: //! -//! * [`observe`] for startup and status refreshes, [`grant`] and [`disable`] -//! for the two explicit user actions; +//! * [`observe`] for startup and status refreshes, plus [`grant`], [`retry`], +//! and [`disable`] for explicit user actions; //! * [`describe`] and [`row`], which turn one run's report into the sentence //! and the permission state the UI shows. //! @@ -15,17 +15,27 @@ //! approval a report describes, a run no click is waiting on opens nothing. use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; +use std::sync::{Mutex, PoisonError}; +use std::time::{Duration, Instant}; use tauri::{AppHandle, Runtime}; use crate::privileged_helper::reconcile::{self, LiveEnvironment, Reconciled}; use crate::privileged_helper::service::{self, RegistrationStatus}; -use crate::shared_types::PermissionStatus; -use crate::state::permissions_state; +use crate::shared_types::{HelperPermissionPhase, HelperPreference, PermissionStatus}; +use crate::state::{permissions_state, preferences}; use crate::system::permissions; -const APPROVE_IN_LOGIN_ITEMS: &str = "Approve nixmac in System Settings → General → Login Items & Extensions to finish enabling the unattended sync helper."; +const APPROVE_IN_LOGIN_ITEMS: &str = "Allow nixmac in System Settings → General → Login Items & Extensions to finish enabling unattended sync."; +const FINISHING_SETUP: &str = "nixmac is finishing unattended sync setup."; +const HELPER_UPDATE_FAILED: &str = "nixmac couldn’t update the unattended sync helper. Try again. If this keeps happening, report the problem."; +const ACTION_FAILED: &str = "nixmac couldn’t finish changing the unattended sync helper. Try again. If this keeps happening, report the problem."; +const SERVICE_DEFINITION_BROKEN: &str = + "This copy of nixmac can’t install its unattended sync helper. Reinstall or update nixmac."; +const REMOVING: &str = "nixmac is removing the unattended sync helper."; +const REMOVAL_FAILED: &str = "nixmac couldn’t remove the unattended sync helper. Try again. If this keeps happening, report the problem."; +const REMOVAL_WAITS_ON_ACTIVATION: &str = + "nixmac will remove the unattended sync helper when the running activation finishes."; /// Reconciles the installed helper with this build, and reports. /// @@ -41,7 +51,19 @@ const APPROVE_IN_LOGIN_ITEMS: &str = "Approve nixmac in System Settings → Gene /// so a main-thread run that only has to read a status or remove a helper is not /// caught by that refusal. pub fn observe(app: &AppHandle) -> Reconciled { - reconcile::reconcile(&LiveEnvironment::new(app)) + observe_with(&PRESENTATION, || { + reconcile::reconcile(&LiveEnvironment::new(app)) + }) +} + +fn observe_with( + presentation: &FailurePresentation, + observe: impl FnOnce() -> Reconciled, +) -> Reconciled { + let generation = presentation.generation(); + let report = observe(); + presentation.note_settled_report(&report, generation); + report } /// The explicit Grant action: record the decision, reconcile under it, and open @@ -75,6 +97,7 @@ pub fn observe(app: &AppHandle) -> Reconciled { /// pass may be awaiting a main-queue dispatch — a deadlock, where an /// observation merely misreports. Every current caller is an async handler. pub fn grant(app: &AppHandle) -> Reconciled { + let generation = PRESENTATION.begin_user_attempt(); // Read before the run, because the run is what changes it. let awaiting_approval_already = matches!( service::registration_status(), @@ -84,6 +107,7 @@ pub fn grant(app: &AppHandle) -> Reconciled { service::open_login_items_settings(); } let report = reconcile::grant(&LiveEnvironment::new(app)); + PRESENTATION.note_settled_report(&report, generation); if !awaiting_approval_already && matches!(report, Reconciled::PendingApproval) { service::open_login_items_settings(); } @@ -91,24 +115,144 @@ pub fn grant(app: &AppHandle) -> Reconciled { report } +/// Retry the stored decision without recording a new one or opening System +/// Settings. +/// +/// This is deliberately separate from [`grant`]: a failed, already-enabled +/// helper needs another reconciliation attempt, not another opt-in action. A +/// stored Enable retries as an explicit Enable, so a missing registration gets +/// exactly one new register; a stored Disable retries the removal; an undecided +/// user is handled exactly like an observation, including its adoption of a +/// registration that already exists (see [`reconcile::retry`]). When an Enable +/// retry ends at approval, the row's deep link is the way to Login Items. +pub fn retry(app: &AppHandle) -> Reconciled { + let generation = PRESENTATION.begin_user_attempt(); + let report = reconcile::retry(&LiveEnvironment::new(app)); + PRESENTATION.note_settled_report(&report, generation); + if matches!(report, Reconciled::Stopped(_)) { + // This was an explicit, bounded retry whose progress the button showed. + // If that pass still stopped, keep recovery available rather than + // leaving a spinner up until the slow background cadence wakes again. + PRESENTATION.show_failure(generation); + } + start_converging(app); + report +} + /// The explicit Disable action: record the decision, then reconcile under it — /// unregister the helper, deferring while an activation runs, done. Never on /// the main thread, for [`grant`]'s reason. pub fn disable(app: &AppHandle) -> Reconciled { + let generation = PRESENTATION.begin_user_attempt(); let report = reconcile::disable(&LiveEnvironment::new(app)); + PRESENTATION.note_settled_report(&report, generation); start_converging(app); report } +/// Whether the stored decision is Disable, so a pass still in progress, waiting, +/// or failed is about removing the helper rather than setting it up. An +/// unreadable decision reads as not removing; the report says why it stopped. +pub(crate) fn removing(app: &AppHandle) -> bool { + matches!( + preferences::read_helper_preference(app), + Ok(HelperPreference::Disabled) + ) +} + /// The permission row for one report: only a helper of this build, ready, is -/// granted, and every report says what it found. -pub fn row(report: &Reconciled) -> (PermissionStatus, String) { +/// granted, and every report says what it found. `removing` is [`removing`]'s +/// answer for the decision the report was produced under: progress, waiting +/// and failure copy then describe the removal. +pub fn row( + report: &Reconciled, + removing: bool, +) -> (PermissionStatus, String, HelperPermissionPhase) { + row_with_failure_visibility(report, removing, PRESENTATION.failure_visible()) +} + +fn row_with_failure_visibility( + report: &Reconciled, + removing: bool, + stopped_failure_visible: bool, +) -> (PermissionStatus, String, HelperPermissionPhase) { let status = match report { Reconciled::AtThisBuild => PermissionStatus::Granted, Reconciled::RegistrationUnavailable => PermissionStatus::Unknown, _ => PermissionStatus::Pending, }; - (status, describe(report)) + let phase = phase_with_failure_visibility(report, stopped_failure_visible); + let detail = match phase { + // A missing registration is a failure with a specific remedy: one + // explicit retry, which may still end at macOS approval. + HelperPermissionPhase::Failed if matches!(report, Reconciled::RegistrationUnavailable) => { + describe(report) + } + HelperPermissionPhase::Reconciling if removing => REMOVING.to_string(), + HelperPermissionPhase::Reconciling => FINISHING_SETUP.to_string(), + HelperPermissionPhase::Failed if removing => REMOVAL_FAILED.to_string(), + HelperPermissionPhase::Failed => HELPER_UPDATE_FAILED.to_string(), + HelperPermissionPhase::WaitingForActivation if removing => { + REMOVAL_WAITS_ON_ACTIVATION.to_string() + } + HelperPermissionPhase::NeedsUserAction + if matches!(report, Reconciled::ServiceDefinitionBroken) => + { + SERVICE_DEFINITION_BROKEN.to_string() + } + _ => describe(report), + }; + (status, detail, phase) +} + +/// Friendly detail for helper actions returned outside the permission row. +/// Internal callers that need exact diagnostics continue to use [`describe`]. +pub fn action_detail(report: &Reconciled) -> String { + match report { + Reconciled::Stopped(_) => ACTION_FAILED.to_string(), + Reconciled::Busy => FINISHING_SETUP.to_string(), + Reconciled::ServiceDefinitionBroken => SERVICE_DEFINITION_BROKEN.to_string(), + _ => describe(report), + } +} + +/// Typed product state for every reconciliation report. +pub fn phase(report: &Reconciled) -> HelperPermissionPhase { + phase_with_failure_visibility(report, PRESENTATION.failure_visible()) +} + +fn phase_with_failure_visibility( + report: &Reconciled, + stopped_failure_visible: bool, +) -> HelperPermissionPhase { + match report { + Reconciled::AtThisBuild => HelperPermissionPhase::Ready, + Reconciled::PendingApproval => HelperPermissionPhase::ApprovalRequired, + Reconciled::NoHelper | Reconciled::Removed => HelperPermissionPhase::Disabled, + Reconciled::Busy => { + if stopped_failure_visible { + HelperPermissionPhase::Failed + } else { + HelperPermissionPhase::Reconciling + } + } + Reconciled::WaitingOnActivation(_) => HelperPermissionPhase::WaitingForActivation, + Reconciled::Displaced(_) | Reconciled::ServiceDefinitionBroken => { + HelperPermissionPhase::NeedsUserAction + } + // Final for the loop, and never retried by observation: the row + // offers Retry, which runs an explicit Enable under the stored + // decision. Failed regardless of the display window, because no + // later pass will change it. + Reconciled::RegistrationUnavailable => HelperPermissionPhase::Failed, + Reconciled::Stopped(_) => { + if stopped_failure_visible { + HelperPermissionPhase::Failed + } else { + HelperPermissionPhase::Reconciling + } + } + } } /// Whether a pane of System Settings is where the user finishes this report. @@ -123,7 +267,7 @@ pub fn row(report: &Reconciled) -> (PermissionStatus, String) { /// /// Not "can a later run change this": several reports are equally terminal /// without a run — a displaced copy has to be moved or restarted (see -/// [`nothing_more_to_do`], which classifies those as final for that reason). What +/// [`loop_step`], which classifies those as settled for that reason). What /// separates this one is *where* the remedy is. The others are answered by a run, /// or by the user doing something to the app itself, not in System Settings. pub fn can_request_programmatically(report: &Reconciled) -> bool { @@ -205,6 +349,11 @@ const FAST_ATTEMPTS: u32 = 30; /// glance. const WAITING_INTERVAL: Duration = Duration::from_secs(5); +/// How long retryable reports stay a neutral setup state before the UI offers +/// recovery. This is presentation timing only; the independent safety bound +/// below still governs how many mutations the loop may attempt. +const FAILURE_PRESENTATION_AFTER: Duration = Duration::from_secs(10); + /// Working attempts one loop makes before giving up and leaving repair to the /// next launch or the next click. /// @@ -213,10 +362,120 @@ const WAITING_INTERVAL: Duration = Duration::from_secs(5); /// that could churn a registration, and nothing else. const MAX_ATTEMPTS: u32 = 100; -/// Whether re-running could still change anything. -fn nothing_more_to_do(report: &Reconciled) -> bool { - matches!( - report, +/// Shared presentation state for explicit attempts and every row producer. +/// A Retry starts a new display window without resetting the loop's mutation +/// bound. Once its own pass fails, only another attempt or a settled observation +/// clears that failure; the background loop must not reset it again. +/// The generation and visibility change under one lock, so an older observation +/// cannot clear a newer failure between a generation check and its update. +/// No reconciliation, publication, or other external call runs under this lock. +#[derive(Default)] +struct FailurePresentation { + state: Mutex, +} + +#[derive(Default)] +struct FailurePresentationState { + generation: u64, + failure_visible: bool, + last_failure_detail: Option, +} + +static PRESENTATION: FailurePresentation = FailurePresentation { + state: Mutex::new(FailurePresentationState { + generation: 0, + failure_visible: false, + last_failure_detail: None, + }), +}; + +/// Last observed helper failure for the existing opt-in feedback snapshot. +/// Reading it never probes permissions or changes helper registration. +pub(crate) fn last_failure_detail() -> Option { + PRESENTATION.last_failure_detail() +} + +impl FailurePresentation { + fn generation(&self) -> u64 { + self.state + .lock() + .unwrap_or_else(PoisonError::into_inner) + .generation + } + + fn failure_visible(&self) -> bool { + self.state + .lock() + .unwrap_or_else(PoisonError::into_inner) + .failure_visible + } + + fn last_failure_detail(&self) -> Option { + self.state + .lock() + .unwrap_or_else(PoisonError::into_inner) + .last_failure_detail + .clone() + } + + fn begin_user_attempt(&self) -> u64 { + let mut state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + state.generation = state.generation.wrapping_add(1); + state.failure_visible = false; + state.generation + } + + fn show_failure(&self, generation: u64) { + let mut state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + if state.generation == generation { + state.failure_visible = true; + } + } + + fn note_settled_report(&self, report: &Reconciled, generation: u64) { + let mut state = self.state.lock().unwrap_or_else(PoisonError::into_inner); + if state.generation != generation || matches!(report, Reconciled::Busy) { + return; + } + match report { + Reconciled::Stopped(_) => state.last_failure_detail = Some(describe(report)), + // The pass that found the registration missing carries no error of + // its own. Keep the register error that usually precedes it. + Reconciled::RegistrationUnavailable => { + if state.last_failure_detail.is_none() { + state.last_failure_detail = Some(describe(report)); + } + } + _ => { + state.last_failure_detail = None; + state.failure_visible = false; + } + } + } +} + +fn should_show_failure(elapsed: Duration, working: u32) -> bool { + elapsed >= FAILURE_PRESENTATION_AFTER || working >= MAX_ATTEMPTS +} + +/// What the loop does with one report. Exhaustive on purpose: a new report +/// variant has to be classified here before it compiles, rather than falling +/// into the working counter. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LoopStep { + /// The observation lost the single-flight race and learned nothing. + Lost, + /// Nothing more for this loop to do. + Settled, + /// A wait that attempted nothing: approval, or a running activation. + Waiting, + /// A pass that stopped short and counts toward the mutation bound. + Working, +} + +fn loop_step(report: &Reconciled) -> LoopStep { + match report { + Reconciled::Busy => LoopStep::Lost, // The stored decision is carried out — one success state per goal. Reconciled::AtThisBuild | Reconciled::Removed | Reconciled::NoHelper // Or nothing this loop can do will change the answer. A displaced copy @@ -225,30 +484,89 @@ fn nothing_more_to_do(report: &Reconciled) -> bool { // and the realistic route here is a quarantined copy, which macOS runs // from a read-only mount that is never under /Applications. A broken // service definition is a property of the bundle, read identically every - // time. - | Reconciled::Displaced(_) - | Reconciled::ServiceDefinitionBroken - | Reconciled::RegistrationUnavailable - ) + // time. A missing registration needs an explicit Enable or Retry, which + // observation never replays. + | Reconciled::Displaced(_) + | Reconciled::ServiceDefinitionBroken + | Reconciled::RegistrationUnavailable => LoopStep::Settled, + Reconciled::PendingApproval | Reconciled::WaitingOnActivation(_) => LoopStep::Waiting, + Reconciled::Stopped(_) => LoopStep::Working, + } } /// The one loop, as a flag rather than a handle: nothing waits on it, cancels /// it, or asks it anything. static CONVERGING: AtomicBool = AtomicBool::new(false); +/// A start request that no loop has served yet. Set before every claim +/// attempt, so a request refused because a loop is still running is not lost: +/// that loop, after releasing [`CONVERGING`], sees it and runs again. +static START_REQUESTED: AtomicBool = AtomicBool::new(false); + /// Held for a loop's lifetime, and released by `Drop` on every way out /// including a panic — a leaked flag is permanent, since nothing else clears it. /// -/// A unit struct on purpose. An earlier version passed a value carrying the flag -/// into `bool::then_some`, which evaluates its argument eagerly: the guard was -/// built and dropped on the *refused* path too, releasing the running loop's -/// flag, and loops doubled every tick until the process ran out of threads. -/// There is nothing here for a claim to construct speculatively. -struct Converging; - -impl Drop for Converging { +/// Built only by [`claim`], and only after its compare-exchange succeeded. An +/// earlier version passed a guard into `bool::then_some`, which evaluates its +/// argument eagerly: the guard was built and dropped on the *refused* path too, +/// releasing the running loop's flag, and loops doubled every tick until the +/// process ran out of threads. `Option::map` runs its closure only on success. +struct Converging<'a>(&'a AtomicBool); + +impl Drop for Converging<'_> { fn drop(&mut self) { - CONVERGING.store(false, Ordering::SeqCst); + self.0.store(false, Ordering::SeqCst); + } +} + +fn claim(running: &AtomicBool) -> Option> { + running + .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) + .ok() + .map(|_| Converging(running)) +} + +/// Records a start request, then tries to claim the loop. `None` means a loop +/// is running and will serve the request when it ends. +fn request_loop<'a>(running: &'a AtomicBool, requested: &AtomicBool) -> Option> { + requested.store(true, Ordering::SeqCst); + claim(running) +} + +/// Runs the loop until no start request is left unserved. +/// +/// `converge` runs one loop and calls the acknowledgement it is given at the +/// start of every pass: a pass that begins after a request serves it, so a +/// request only causes another loop when it arrived after the last pass began. +/// A refresh early in a long loop therefore cannot buy a second attempt budget. +/// +/// The exit releases the claim before it reads `requested`, and a request sets +/// `requested` before it tries to claim. With both sequentially consistent, a +/// request either claims the released flag itself or is seen here, so none is +/// dropped between a loop's last check and its exit. `released` is a test seam +/// for that window and does nothing in production. +fn serve_requests( + mut held: Converging<'_>, + requested: &AtomicBool, + mut converge: impl FnMut(&dyn Fn()), + mut released: impl FnMut(), +) { + let acknowledge = || requested.store(false, Ordering::SeqCst); + loop { + acknowledge(); + converge(&acknowledge); + let running = held.0; + drop(held); + released(); + if !requested.load(Ordering::SeqCst) { + return; + } + // A request arrived while this loop ran or as it ended. Run again, + // unless that request's caller already claimed a loop of its own. + match claim(running) { + Some(reclaimed) => held = reclaimed, + None => return, + } } } @@ -258,8 +576,9 @@ impl Drop for Converging { /// observing approval and registered-helper replacement. A missing registration /// ends the loop and needs another explicit Enable, including after a crash /// between unregister and register; an uncertain registration is never replayed. -/// A loop already running needs no help: every pass re-reads the stored decision, -/// so it picks up a Grant or a Disable by itself. +/// A loop already running needs no new thread: every pass re-reads the stored +/// decision, so it picks up a Grant or a Disable by itself, and a request it +/// refused makes it run again once it ends ([`serve_requests`]). pub fn start_converging(app: &AppHandle) { // A build that reports every permission granted without probing must not go // and register a real helper behind that fiction. `replace_row` refuses too, @@ -267,15 +586,11 @@ pub fn start_converging(app: &AppHandle) { if permissions::skip_enabled() { return; } - if CONVERGING - .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) - .is_err() - { + // Moved into the closure, so a `spawn` that fails drops it and gives the + // flag back. + let Some(guard) = request_loop(&CONVERGING, &START_REQUESTED) else { return; - } - // Built only after the claim succeeded, and moved into the closure — so a - // `spawn` that fails drops it and gives the flag back. - let guard = Converging; + }; let app = app.clone(); // Off the calling thread for two reasons. It must not be the main thread: // the ServiceManagement calls are issued *on* the main queue and awaited, so @@ -284,26 +599,103 @@ pub fn start_converging(app: &AppHandle) { // pool, because this one sleeps for as long as an approval takes — a pool // slot it would never give back. std::thread::spawn(move || { - let _guard = guard; - drive( - || { - let report = observe(&app); - permissions_state::replace_row(&app, permissions::helper_row(&report)); - report + serve_requests( + guard, + &START_REQUESTED, + |acknowledge| { + drive( + &PRESENTATION, + || { + // This pass serves every start request made so far. + acknowledge(); + observe(&app) + }, + |report| { + permissions_state::replace_row( + &app, + permissions::helper_row(report, removing(&app)), + ); + }, + std::thread::sleep, + ); }, - std::thread::sleep, + || {}, ); }); } -/// Run, publish, stop when there is nothing left to do, otherwise wait and go -/// again. -fn drive(mut attempt: impl FnMut() -> Reconciled, mut wait: impl FnMut(Duration)) { +/// Run, publish one shared product state, and stop when there is nothing left +/// to do. Retry timing lives here so full refreshes and both UI surfaces read +/// the same phase rather than independently interpreting transient reports. +fn drive( + presentation: &FailurePresentation, + attempt: impl FnMut() -> Reconciled, + publish: impl FnMut(&Reconciled), + wait: impl FnMut(Duration), +) { + drive_with_clock(presentation, attempt, publish, wait, Instant::now); +} + +fn drive_with_clock( + presentation: &FailurePresentation, + mut attempt: impl FnMut() -> Reconciled, + mut publish: impl FnMut(&Reconciled), + mut wait: impl FnMut(Duration), + mut now: impl FnMut() -> Instant, +) { + // `working` remains solely the mutation-safety bound and cadence selector. + // Presentation timing is deliberately independent so a human approval or + // a long activation wait cannot reset the churn limit. let mut working = 0; + let mut user_generation = presentation.generation(); + let mut presentation_started: Option = None; loop { + // The generation this pass runs under. A newer user action changes it, + // and starts a fresh display window here on the next pass. + let attempt_generation = presentation.generation(); + if attempt_generation != user_generation { + user_generation = attempt_generation; + presentation_started = Some(now()); + } else if presentation_started.is_none() && !presentation.failure_visible() { + presentation_started = Some(now()); + } + let report = attempt(); - if nothing_more_to_do(&report) { - return; + let step = loop_step(&report); + + // Busy says only that this observation lost the single-flight race. It + // must not replace an approval, failure, or actionable instruction. + if step == LoopStep::Lost { + wait(WAITING_INTERVAL); + continue; + } + + // A user action began during this pass, so this report may describe + // the world before that action, which has since run (or is waiting to + // run) a pass of its own under the slot. Publishing it could put a + // Ready row over a Disable that has since failed, and ending here would + // strand that action: `start_converging` refuses while this loop runs, + // so the action relies on this loop for its retries. Skip the report and + // observe again; the top of the loop starts the newer attempt's display + // window. A stale pass that stopped short still counts toward the + // mutation bound, since it may have mutated. + if presentation.generation() != attempt_generation { + if step == LoopStep::Working { + working += 1; + } + wait(FAST_INTERVAL); + continue; + } + + if step == LoopStep::Settled { + presentation.note_settled_report(&report, attempt_generation); + publish(&report); + // An action that began while this published gets a pass too, for + // the reason above, instead of an exit that strands it. + if presentation.generation() == attempt_generation { + return; + } + continue; } // None of these attempted anything. `PendingApproval` is macOS // waiting on the user — which may be weeks. `Busy` is another run @@ -325,14 +717,28 @@ fn drive(mut attempt: impl FnMut() -> Reconciled, mut wait: impl FnMut(Duration) // status × goal table's row and touches nothing. Getting back to a // mutating pass takes an external event: an approval, or a removal in // Login Items. Every such cycle is therefore paced by a human. - if matches!( - report, - Reconciled::PendingApproval | Reconciled::Busy | Reconciled::WaitingOnActivation(_) - ) { + if step == LoopStep::Waiting { + // This is a genuine wait boundary, not another failed mutation. + // Give the work after it a fresh *display* window while leaving the + // lifetime `working` safety counter untouched. + presentation_started = None; + presentation.note_settled_report(&report, attempt_generation); + publish(&report); wait(WAITING_INTERVAL); continue; } + + // Every other step continued or returned above. + debug_assert_eq!(step, LoopStep::Working, "{report:?}"); working += 1; + let elapsed = presentation_started + .map(|started| now().saturating_duration_since(started)) + .unwrap_or_default(); + if should_show_failure(elapsed, working) { + presentation.show_failure(attempt_generation); + } + publish(&report); + if working >= MAX_ATTEMPTS { log::warn!( "helper convergence: giving up after {working} attempts, last report {report:?}; \ @@ -370,6 +776,14 @@ mod tests { Reconciled::Stopped(sentence.to_string()) } + fn activation_info() -> crate::privileged_helper::protocol::ActivationInfo { + crate::privileged_helper::protocol::ActivationInfo { + request_id: "request".to_string(), + script_path: "/nix/store/abc-darwin-system/activate".to_string(), + client_kind: crate::privileged_helper::peer_auth::ClientKind::SyncAgent, + } + } + fn displaced() -> Reconciled { Reconciled::Displaced( "nixmac is not running from an app bundle in /Applications".to_string(), @@ -378,18 +792,238 @@ mod tests { #[test] fn unavailable_registration_offers_explicit_retry_without_claiming_permission() { - let (status, detail) = row(&Reconciled::RegistrationUnavailable); + for failure_visible in [false, true] { + assert_eq!( + phase_with_failure_visibility( + &Reconciled::RegistrationUnavailable, + failure_visible + ), + HelperPermissionPhase::Failed, + "the row must offer Retry whatever the display window says" + ); + } + let (status, detail, phase) = row(&Reconciled::RegistrationUnavailable, false); assert_eq!(status, PermissionStatus::Unknown); + assert_eq!(phase, HelperPermissionPhase::Failed); assert!(detail.contains("Retry enabling")); + assert!(action_detail(&Reconciled::RegistrationUnavailable).contains("Retry enabling")); + let mut observations = 0; + let mut published = Vec::new(); drive( + &FailurePresentation::default(), || { observations += 1; Reconciled::RegistrationUnavailable }, + |report| published.push(report.clone()), |_| panic!("an unavailable registration must not schedule another attempt"), ); assert_eq!(observations, 1); + assert_eq!(published, vec![Reconciled::RegistrationUnavailable]); + } + + #[test] + fn a_stored_disable_describes_the_removal_in_progress_waiting_and_failed() { + let refused = stopped("the helper could not be unregistered: refused"); + let removal = + |report: &Reconciled, visible| row_with_failure_visibility(report, true, visible); + + let (_, detail, phase) = removal(&refused, false); + assert_eq!(phase, HelperPermissionPhase::Reconciling); + assert_eq!(detail, REMOVING); + + let (_, detail, phase) = removal(&refused, true); + assert_eq!(phase, HelperPermissionPhase::Failed); + assert_eq!(detail, REMOVAL_FAILED); + + for waiting in [ + Reconciled::WaitingOnActivation(None), + Reconciled::WaitingOnActivation(Some(activation_info())), + ] { + let (_, detail, phase) = removal(&waiting, false); + assert_eq!(phase, HelperPermissionPhase::WaitingForActivation); + assert_eq!(detail, REMOVAL_WAITS_ON_ACTIVATION); + } + + // The setup copy stays for an Enable. + let (_, detail, _) = row_with_failure_visibility(&refused, false, false); + assert_eq!(detail, FINISHING_SETUP); + let (_, detail, _) = row_with_failure_visibility(&refused, false, true); + assert_eq!(detail, HELPER_UPDATE_FAILED); + for copy in [REMOVING, REMOVAL_FAILED, REMOVAL_WAITS_ON_ACTIVATION] { + assert!( + !copy.contains("setup") && !copy.contains("update"), + "{copy}" + ); + } + } + + #[test] + fn a_start_request_refused_while_a_loop_ends_makes_it_run_again() { + // The lost wakeup: the loop has made its final check, a Disable then + // fails and asks for a loop, and is refused because this one still + // holds the flag. The request must not die with the old loop. + let running = AtomicBool::new(false); + let requested = AtomicBool::new(false); + let held = request_loop(&running, &requested).expect("a free flag"); + let mut passes = 0; + serve_requests( + held, + &requested, + |acknowledge| { + acknowledge(); + passes += 1; + if passes == 1 { + assert!( + request_loop(&running, &requested).is_none(), + "a second loop claimed a taken flag" + ); + } + }, + || {}, + ); + assert_eq!(passes, 2, "the refused request was never served"); + assert!(!running.load(Ordering::SeqCst)); + assert!(!requested.load(Ordering::SeqCst)); + } + + #[test] + fn a_start_request_refused_mid_loop_is_served_without_a_second_attempt_budget() { + // A refresh asks for a loop early in a long run of failures. A later + // pass of the same loop serves it, so the loop still gives up after + // one MAX_ATTEMPTS budget rather than starting another. + let running = AtomicBool::new(false); + let requested = AtomicBool::new(false); + let held = request_loop(&running, &requested).expect("a free flag"); + let attempts = std::cell::Cell::new(0); + serve_requests( + held, + &requested, + |acknowledge| { + drive( + &FailurePresentation::default(), + || { + acknowledge(); + attempts.set(attempts.get() + 1); + if attempts.get() == 1 { + assert!(request_loop(&running, &requested).is_none()); + } + stopped("the helper could not be registered: not permitted") + }, + |_| {}, + |_| {}, + ); + }, + || {}, + ); + assert_eq!(attempts.get(), MAX_ATTEMPTS); + assert!(!running.load(Ordering::SeqCst)); + } + + #[test] + fn a_start_request_in_the_release_window_starts_its_own_loop() { + // The request lands after the old loop released the flag but before + // it read the request: the request's own claim succeeds, and the old + // loop leaves the work to it rather than running a second loop. + let running = AtomicBool::new(false); + let requested = AtomicBool::new(false); + let held = request_loop(&running, &requested).expect("a free flag"); + let mut passes = 0; + let mut theirs = None; + serve_requests( + held, + &requested, + |acknowledge| { + acknowledge(); + passes += 1; + }, + || { + if theirs.is_none() { + theirs = Some(request_loop(&running, &requested)); + } + }, + ); + assert_eq!(passes, 1); + let theirs = theirs + .expect("the window ran") + .expect("the request claimed the free flag"); + assert!( + running.load(Ordering::SeqCst), + "only the new loop holds the flag" + ); + drop(theirs); + assert!(!running.load(Ordering::SeqCst)); + } + + #[test] + fn a_loop_with_no_request_left_exits_and_releases_the_flag() { + let running = AtomicBool::new(false); + let requested = AtomicBool::new(false); + let held = request_loop(&running, &requested).expect("a free flag"); + let mut passes = 0; + serve_requests( + held, + &requested, + |acknowledge| { + acknowledge(); + passes += 1; + }, + || {}, + ); + assert_eq!(passes, 1); + assert!(!running.load(Ordering::SeqCst)); + assert!(claim(&running).is_some()); + } + + #[test] + fn every_report_has_exactly_one_loop_step() { + for (report, expected) in [ + (Reconciled::Busy, LoopStep::Lost), + (Reconciled::AtThisBuild, LoopStep::Settled), + (Reconciled::Removed, LoopStep::Settled), + (Reconciled::NoHelper, LoopStep::Settled), + (displaced(), LoopStep::Settled), + (Reconciled::ServiceDefinitionBroken, LoopStep::Settled), + (Reconciled::RegistrationUnavailable, LoopStep::Settled), + (Reconciled::PendingApproval, LoopStep::Waiting), + (Reconciled::WaitingOnActivation(None), LoopStep::Waiting), + ( + stopped("the helper could not be registered"), + LoopStep::Working, + ), + ] { + assert_eq!(loop_step(&report), expected, "{report:?}"); + } + } + + #[test] + fn a_missing_registration_keeps_the_register_error_for_feedback() { + let presentation = FailurePresentation::default(); + let attempt = presentation.begin_user_attempt(); + presentation.note_settled_report( + &stopped("the helper could not be registered: SMAppServiceErrorDomain 1"), + attempt, + ); + presentation.note_settled_report(&Reconciled::RegistrationUnavailable, attempt); + assert!( + presentation + .last_failure_detail() + .unwrap() + .contains("SMAppServiceErrorDomain 1") + ); + + let fresh = FailurePresentation::default(); + let attempt = fresh.begin_user_attempt(); + fresh.note_settled_report(&Reconciled::RegistrationUnavailable, attempt); + assert!( + fresh + .last_failure_detail() + .unwrap() + .contains("cannot find the helper registration") + ); + fresh.note_settled_report(&Reconciled::AtThisBuild, attempt); + assert!(fresh.last_failure_detail().is_none()); } #[test] @@ -402,7 +1036,7 @@ mod tests { Reconciled::ServiceDefinitionBroken, Reconciled::RegistrationUnavailable, ] { - assert!(nothing_more_to_do(&done), "{done:?}"); + assert_eq!(loop_step(&done), LoopStep::Settled, "{done:?}"); } // Everything else is worth another go — the refusal window, the // approval wait, a caller turned away, a helper that has not answered @@ -416,7 +1050,7 @@ mod tests { stopped("the helper could not be unregistered: refused"), stopped("the helper is no longer granted; it was being replaced"), ] { - assert!(!nothing_more_to_do(&keep_going), "{keep_going:?}"); + assert_ne!(loop_step(&keep_going), LoopStep::Settled, "{keep_going:?}"); } } @@ -445,11 +1079,314 @@ mod tests { } } + #[test] + fn every_report_has_a_typed_product_phase() { + for (report, expected) in [ + (Reconciled::AtThisBuild, HelperPermissionPhase::Ready), + ( + Reconciled::PendingApproval, + HelperPermissionPhase::ApprovalRequired, + ), + (Reconciled::NoHelper, HelperPermissionPhase::Disabled), + (Reconciled::Removed, HelperPermissionPhase::Disabled), + ( + Reconciled::WaitingOnActivation(None), + HelperPermissionPhase::WaitingForActivation, + ), + (displaced(), HelperPermissionPhase::NeedsUserAction), + ( + Reconciled::ServiceDefinitionBroken, + HelperPermissionPhase::NeedsUserAction, + ), + ( + Reconciled::RegistrationUnavailable, + HelperPermissionPhase::Failed, + ), + ] { + assert_eq!(phase_with_failure_visibility(&report, false), expected); + } + + let stopped = stopped("SMAppServiceErrorDomain 1: operation not permitted"); + assert_eq!( + phase_with_failure_visibility(&stopped, false), + HelperPermissionPhase::Reconciling + ); + assert_eq!( + phase_with_failure_visibility(&stopped, true), + HelperPermissionPhase::Failed + ); + } + + #[test] + fn technical_stops_are_friendly_only_at_the_row_boundary() { + let report = stopped("SMAppServiceErrorDomain 1: operation not permitted"); + assert!(describe(&report).contains("SMAppServiceErrorDomain 1")); + assert!(!action_detail(&report).contains("SMAppServiceErrorDomain")); + } + + #[test] + fn presentation_uses_time_without_changing_the_mutation_bound() { + assert!(!should_show_failure(Duration::from_secs(9), 99)); + assert!(should_show_failure(FAILURE_PRESENTATION_AFTER, 1)); + assert!(should_show_failure(Duration::ZERO, MAX_ATTEMPTS)); + } + + #[test] + fn feedback_keeps_failure_details_without_changing_friendly_copy() { + let presentation = FailurePresentation::default(); + let first = presentation.begin_user_attempt(); + let failure = stopped("SMAppServiceErrorDomain 1: registration refused"); + presentation.note_settled_report(&failure, first); + assert!( + presentation + .last_failure_detail() + .unwrap() + .contains("SMAppServiceErrorDomain 1") + ); + assert!(!action_detail(&failure).contains("SMAppServiceErrorDomain")); + presentation.note_settled_report(&Reconciled::Busy, first); + assert!(presentation.last_failure_detail().is_some()); + + let next = presentation.begin_user_attempt(); + presentation.note_settled_report(&Reconciled::AtThisBuild, first); + assert!(presentation.last_failure_detail().is_some()); + presentation.note_settled_report(&Reconciled::AtThisBuild, next); + assert!(presentation.last_failure_detail().is_none()); + } + + #[test] + fn a_stale_ready_row_never_hides_a_newer_failed_action() { + // Greptile's race, end to end: this loop's pass reached Ready and + // released reconciliation's slot; a Disable then ran and failed before + // the loop published. The old Ready row must never be published, and + // the loop must not exit, because the Disable's own + // `start_converging` was refused while this loop ran. + let presentation = FailurePresentation::default(); + let attempts = std::cell::Cell::new(0); + let mut published = Vec::new(); + let mut phases = Vec::new(); + let mut waited = Vec::new(); + let refused = stopped("the helper could not be unregistered: refused"); + drive( + &presentation, + || { + attempts.set(attempts.get() + 1); + match attempts.get() { + 1 => observe_with(&presentation, || { + let disable = presentation.begin_user_attempt(); + presentation.note_settled_report(&refused, disable); + presentation.show_failure(disable); + Reconciled::AtThisBuild + }), + // The next pass observes the world the Disable left. + 2 => refused.clone(), + _ => Reconciled::Removed, + } + }, + |report| { + published.push(report.clone()); + phases.push(phase_with_failure_visibility( + report, + presentation.failure_visible(), + )); + }, + |interval| waited.push(interval), + ); + assert_eq!(attempts.get(), 3, "the loop exited on the stale report"); + assert_eq!(published, vec![refused.clone(), Reconciled::Removed]); + assert_eq!( + phases, + vec![ + HelperPermissionPhase::Failed, + HelperPermissionPhase::Disabled + ] + ); + assert_eq!(waited[0], FAST_INTERVAL, "a stale report is not hurried"); + } + + #[test] + fn a_stale_failure_from_before_a_retry_is_neither_published_nor_clears_it() { + let presentation = FailurePresentation::default(); + let attempts = std::cell::Cell::new(0); + let mut published = Vec::new(); + let mut phases = Vec::new(); + drive( + &presentation, + || { + attempts.set(attempts.get() + 1); + match attempts.get() { + 1 => { + // The background pass releases reconciliation's slot, + // then Retry runs and fails before this loop publishes. + let retry = presentation.begin_user_attempt(); + presentation.show_failure(retry); + stopped("the background pass could not register") + } + 2 => { + assert!( + presentation.failure_visible(), + "the stale pass cleared Retry's failure" + ); + stopped("the helper could not be registered") + } + _ => Reconciled::AtThisBuild, + } + }, + |report| { + published.push(report.clone()); + phases.push(phase_with_failure_visibility( + report, + presentation.failure_visible(), + )); + }, + |_| {}, + ); + assert_eq!( + published, + vec![ + stopped("the helper could not be registered"), + Reconciled::AtThisBuild + ] + ); + assert_eq!( + phases, + vec![HelperPermissionPhase::Failed, HelperPermissionPhase::Ready] + ); + } + + #[test] + fn an_action_that_begins_while_a_settled_row_publishes_gets_another_pass() { + let presentation = FailurePresentation::default(); + let attempts = std::cell::Cell::new(0); + let mut published = Vec::new(); + drive( + &presentation, + || { + attempts.set(attempts.get() + 1); + if attempts.get() == 1 { + Reconciled::AtThisBuild + } else { + Reconciled::Removed + } + }, + |report| { + if published.is_empty() { + // A Disable starts after the generation check but before + // this loop would have exited. + presentation.begin_user_attempt(); + } + published.push(report.clone()); + }, + |_| {}, + ); + assert_eq!(attempts.get(), 2); + assert_eq!( + published, + vec![Reconciled::AtThisBuild, Reconciled::Removed] + ); + } + + #[test] + fn a_stale_settled_observation_cannot_clear_a_newer_failed_retry() { + let presentation = FailurePresentation::default(); + let attempts = std::cell::Cell::new(0); + drive( + &presentation, + || { + attempts.set(attempts.get() + 1); + if attempts.get() == 1 { + observe_with(&presentation, || { + // This observation already obtained Ready and released + // the reconciliation slot. Retry acquires it and fails + // before the older observation returns. + let retry_generation = presentation.begin_user_attempt(); + presentation.show_failure(retry_generation); + Reconciled::AtThisBuild + }) + } else { + assert!( + presentation.failure_visible(), + "observe() or drive() cleared Retry's failure with a stale report" + ); + Reconciled::RegistrationUnavailable + } + }, + |report| assert_eq!(report, &Reconciled::RegistrationUnavailable), + |_| {}, + ); + assert_eq!(attempts.get(), 2); + assert!(presentation.failure_visible()); + } + + #[test] + fn the_loop_shows_recovery_after_the_display_window() { + let presentation = FailurePresentation::default(); + let started = Instant::now(); + let elapsed = std::cell::Cell::new(Duration::ZERO); + let mut attempts = 0; + let mut phases = Vec::new(); + drive_with_clock( + &presentation, + || { + attempts += 1; + if attempts <= 3 { + stopped("the helper could not be registered") + } else { + Reconciled::AtThisBuild + } + }, + |report| { + phases.push(phase_with_failure_visibility( + report, + presentation.failure_visible(), + )); + }, + |_| { + elapsed.set(if elapsed.get().is_zero() { + Duration::from_secs(9) + } else { + elapsed.get() + Duration::from_secs(1) + }); + }, + || started + elapsed.get(), + ); + assert_eq!( + phases, + vec![ + HelperPermissionPhase::Reconciling, + HelperPermissionPhase::Reconciling, + HelperPermissionPhase::Failed, + HelperPermissionPhase::Ready, + ] + ); + } + + #[test] + fn busy_never_replaces_the_last_truthful_row() { + let mut attempts = 0; + let mut published = Vec::new(); + drive( + &FailurePresentation::default(), + || { + attempts += 1; + if attempts == 1 { + Reconciled::Busy + } else { + Reconciled::AtThisBuild + } + }, + |report| published.push(report.clone()), + |_| {}, + ); + assert_eq!(published, vec![Reconciled::AtThisBuild]); + } + #[test] fn it_stops_as_soon_as_the_decision_is_carried_out() { let mut attempts = 0; let mut waited = Vec::new(); drive( + &FailurePresentation::default(), || { attempts += 1; if attempts < 3 { @@ -458,6 +1395,7 @@ mod tests { Reconciled::AtThisBuild } }, + |_| {}, |interval| waited.push(interval), ); assert_eq!(attempts, 3); @@ -473,6 +1411,7 @@ mod tests { let mut attempts = 0; let mut waited = Vec::new(); drive( + &FailurePresentation::default(), || { attempts += 1; if attempts <= MAX_ATTEMPTS * 10 { @@ -481,6 +1420,7 @@ mod tests { Reconciled::AtThisBuild } }, + |_| {}, |interval| waited.push(interval), ); assert_eq!(attempts, MAX_ATTEMPTS * 10 + 1, "the loop stopped watching"); @@ -496,6 +1436,7 @@ mod tests { let mut attempts = 0; let mut working = 0; drive( + &FailurePresentation::default(), || { attempts += 1; if attempts <= 500 { @@ -506,6 +1447,7 @@ mod tests { } }, |_| {}, + |_| {}, ); assert_eq!(working, MAX_ATTEMPTS); } @@ -515,10 +1457,12 @@ mod tests { let mut attempts = 0; let mut waited = Vec::new(); drive( + &FailurePresentation::default(), || { attempts += 1; stopped("the helper could not be registered: not permitted") }, + |_| {}, |interval| waited.push(interval), ); assert_eq!(attempts, MAX_ATTEMPTS); @@ -538,27 +1482,22 @@ mod tests { #[test] fn the_claim_primitives_hold_one_loop_and_survive_a_panic() { - // Note this drives `CONVERGING` and `Converging` directly, not + // Note this drives `claim` and `Converging` directly, not // `start_converging` — the claim/spawn/guard *sequencing* in that // function is what once had the doubling bug, and it is not covered // here. + let held = claim(&CONVERGING).expect("a free flag"); assert!( - CONVERGING - .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) - .is_ok() - ); - assert!( - CONVERGING - .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) - .is_err(), + claim(&CONVERGING).is_none(), "a second loop claimed a taken flag" ); // A refused claim must not release what the holder has — the bug that // spawned loops until the process ran out of threads. assert!(CONVERGING.load(Ordering::SeqCst)); + drop(held); let outcome = std::panic::catch_unwind(|| { - let _guard = Converging; + let _guard = claim(&CONVERGING).expect("a free flag"); panic!("the run panicked"); }); assert!(outcome.is_err()); diff --git a/apps/native/src-tauri/src/system/permissions.rs b/apps/native/src-tauri/src/system/permissions.rs index 896ba0ded..634d72d71 100644 --- a/apps/native/src-tauri/src/system/permissions.rs +++ b/apps/native/src-tauri/src/system/permissions.rs @@ -11,7 +11,9 @@ //! - Administrator privileges (sudo access) use crate::privileged_helper::reconcile::Reconciled; +use crate::shared_types::HelperPermissionPhase; pub(crate) use crate::shared_types::{Permission, PermissionStatus, PermissionsState}; +use crate::state::permissions_state; use crate::system::helper_permission; use anyhow::Result; use log::{debug, info, warn}; @@ -48,6 +50,7 @@ fn get_default_permissions() -> Vec { can_request_programmatically: true, status: default_status, instructions: None, + helper_phase: None, }, Permission { id: "documents".to_string(), @@ -58,6 +61,7 @@ fn get_default_permissions() -> Vec { can_request_programmatically: true, status: default_status, instructions: None, + helper_phase: None, }, Permission { id: "admin".to_string(), @@ -68,6 +72,7 @@ fn get_default_permissions() -> Vec { can_request_programmatically: false, status: default_status, instructions: Some("You will be prompted for your password when needed".to_string()), + helper_phase: None, }, Permission { id: "full-disk".to_string(), @@ -80,6 +85,7 @@ fn get_default_permissions() -> Vec { "First make sure nixmac is in your Applications folder (not running from the install disk image). Then go to System Settings → Privacy & Security → Full Disk Access and add nixmac to the list." .to_string(), ), + helper_phase: None, }, app_management_permission(PermissionStatus::Pending), privileged_helper_permission( @@ -88,6 +94,7 @@ fn get_default_permissions() -> Vec { // No report yet, so nothing says macOS is waiting on the user. Every // path that has one replaces this row whole (`helper_row`). true, + Some(HelperPermissionPhase::Disabled), ), ] } @@ -125,6 +132,9 @@ fn granted_permissions_state() -> PermissionsState { let mut permissions = get_default_permissions(); for perm in &mut permissions { perm.status = PermissionStatus::Granted; + if perm.id == "privileged-helper" { + perm.helper_phase = Some(HelperPermissionPhase::Ready); + } // Granted here is a debug-build fiction, not a probe result; say so // instead of letting the row imply the real thing was verified. perm.instructions = Some( @@ -148,6 +158,7 @@ fn granted_folder_permission(id: &str, name: &str, description: &str) -> Permiss can_request_programmatically: true, status: PermissionStatus::Granted, instructions: None, + helper_phase: None, } } @@ -155,6 +166,7 @@ fn privileged_helper_permission( status: PermissionStatus, instructions: &str, can_request_programmatically: bool, + helper_phase: Option, ) -> Permission { Permission { id: "privileged-helper".to_string(), @@ -166,6 +178,7 @@ fn privileged_helper_permission( can_request_programmatically, status, instructions: Some(instructions.to_string()), + helper_phase, } } @@ -202,6 +215,7 @@ fn app_management_permission(status: PermissionStatus) -> Permission { "Open System Settings → Privacy & Security → App Management and enable nixmac. macOS does not expose a reliable way for nixmac to verify this grant." .to_string(), ), + helper_phase: None, } } @@ -412,7 +426,7 @@ pub fn check_all_permissions(app: &AppHandle) -> PermissionsState // get anywhere on its own — so it is replaced rather than patched field by // field, and `helper_row` stays the only place a report becomes a row. if perm.id == "privileged-helper" { - *perm = helper_row(&check_privileged_helper(app)); + *perm = check_privileged_helper(app); } else { perm.status = match perm.id.as_str() { "desktop" => check_desktop_access(), @@ -509,6 +523,7 @@ pub fn request_permission( can_request_programmatically: true, status: PermissionStatus::Granted, instructions: None, + helper_phase: None, }) } Err(_) => Ok(Permission { @@ -520,6 +535,7 @@ pub fn request_permission( can_request_programmatically: true, status: PermissionStatus::Denied, instructions: None, + helper_phase: None, }), } } @@ -551,6 +567,7 @@ pub fn request_permission( can_request_programmatically: true, status: PermissionStatus::Granted, instructions: None, + helper_phase: None, }) } Err(_) => Ok(Permission { @@ -563,6 +580,7 @@ pub fn request_permission( can_request_programmatically: true, status: PermissionStatus::Denied, instructions: None, + helper_phase: None, }), } } @@ -579,6 +597,7 @@ pub fn request_permission( instructions: Some( "You will be prompted for your password when needed".to_string(), ), + helper_phase: None, }) } "full-disk" => { @@ -602,6 +621,7 @@ pub fn request_permission( "First make sure nixmac is in your Applications folder (not running from the install disk image). Then go to System Settings → Privacy & Security → Full Disk Access and add nixmac to the list." .to_string() })), + helper_phase: None, }) } "app-management" => { @@ -621,7 +641,7 @@ pub fn request_permission( // registration and repairing a half-finished one are the same step, and // the write is idempotent. Grant is the only action that may open Login // Items; `darwin.helperGrant` is this same action. - "privileged-helper" => Ok(helper_row(&helper_permission::grant(app))), + "privileged-helper" => Ok(helper_row(&helper_permission::grant(app), false)), _ => Err(anyhow::anyhow!("Unknown permission: {}", permission_id)), } } @@ -635,7 +655,7 @@ pub fn request_permission( /// a report, and the report is what tells the user what to do: approve in Login /// Items, move the app to /Applications, restart the app, or wait out a running /// activation. -fn check_privileged_helper(app: &AppHandle) -> Reconciled { +fn check_privileged_helper(app: &AppHandle) -> Permission { let report = helper_permission::observe(app); // A refresh is how a user comes back to this — opening the panel, or // pressing Check again — and one run is usually not enough: the platform @@ -644,16 +664,40 @@ fn check_privileged_helper(app: &AppHandle) -> Reconciled { // Starting the loop here means the visit converges instead of handing back a // scary report. A loop already running ignores this. helper_permission::start_converging(app); - report + let previous = permissions_state::get(app).and_then(|state| { + state + .permissions + .into_iter() + .find(|permission| permission.id == "privileged-helper") + }); + helper_row_from_observation(&report, helper_permission::removing(app), previous) } -/// One report as the permission row it produces. -pub(crate) fn helper_row(report: &Reconciled) -> Permission { - let (status, detail) = helper_permission::row(report); +fn helper_row_from_observation( + report: &Reconciled, + removing: bool, + previous: Option, +) -> Permission { + // Busy means this observation learned nothing. Preserve the last truthful + // helper row rather than downgrading approval, failure, or an actionable + // instruction to a generic progress state. + if matches!(report, Reconciled::Busy) + && let Some(row) = previous + { + return row; + } + helper_row(report, removing) +} + +/// One report as the permission row it produces, under a stored Disable when +/// `removing` (see [`helper_permission::removing`]). +pub(crate) fn helper_row(report: &Reconciled, removing: bool) -> Permission { + let (status, detail, phase) = helper_permission::row(report, removing); privileged_helper_permission( status, &detail, helper_permission::can_request_programmatically(report), + Some(phase), ) } @@ -681,9 +725,35 @@ mod tests { /// them is what the UI actually reads and nothing else pins it. #[test] fn the_row_takes_its_deep_link_from_the_report() { - assert!(!helper_row(&Reconciled::PendingApproval).can_request_programmatically); - assert!(helper_row(&Reconciled::AtThisBuild).can_request_programmatically); - assert!(helper_row(&Reconciled::NoHelper).can_request_programmatically); + let approval = helper_row(&Reconciled::PendingApproval, false); + assert!(!approval.can_request_programmatically); + assert_eq!( + approval.helper_phase, + Some(HelperPermissionPhase::ApprovalRequired) + ); + + let ready = helper_row(&Reconciled::AtThisBuild, false); + assert!(ready.can_request_programmatically); + assert_eq!(ready.helper_phase, Some(HelperPermissionPhase::Ready)); + + let disabled = helper_row(&Reconciled::NoHelper, false); + assert!(disabled.can_request_programmatically); + assert_eq!(disabled.helper_phase, Some(HelperPermissionPhase::Disabled)); + } + + #[test] + fn a_busy_full_refresh_preserves_the_previous_helper_row() { + let previous = helper_row(&Reconciled::PendingApproval, false); + assert_eq!( + helper_row_from_observation(&Reconciled::Busy, false, Some(previous.clone())) + .helper_phase, + previous.helper_phase + ); + assert_eq!( + helper_row_from_observation(&Reconciled::Busy, false, Some(previous.clone())) + .instructions, + previous.instructions + ); } /// A bare app handle. The skip flags below short-circuit before anything diff --git a/apps/native/src/components/widget/permissions/permissions-panel.test.tsx b/apps/native/src/components/widget/permissions/permissions-panel.test.tsx index b852f9250..47bdb9b73 100644 --- a/apps/native/src/components/widget/permissions/permissions-panel.test.tsx +++ b/apps/native/src/components/widget/permissions/permissions-panel.test.tsx @@ -7,15 +7,15 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; /** * The helper row is the one permission nixmac installs rather than asks macOS - * for, so it is the one row that can hand it back — as one button, a toggle of - * the standing decision. Everything else the row shows comes from the backend's - * reconciliation report: the status decides whether "Granted" is shown, and - * `instructions` is the sentence. + * for, so it is the one row that can hand it back. A persistent failure keeps + * that Disable escape hatch and adds a non-destructive Retry. Everything else + * comes from the backend's typed phase and customer-facing sentence. */ const mockRefresh = vi.fn<() => Promise>(); const mockRequest = vi.fn<(permissionId: string) => Promise>>(); const mockDisableHelper = vi.fn<() => Promise>(); +const mockRetryHelper = vi.fn<() => Promise>(); vi.mock("@/ipc/api", () => ({ tauriAPI: { @@ -31,6 +31,7 @@ vi.mock("@/lib/orpc", () => ({ client: { darwin: { helperDisable: () => mockDisableHelper(), + helperRetry: () => mockRetryHelper(), }, permissions: { refresh: () => mockRefresh(), @@ -71,6 +72,7 @@ function helperPermission(overrides: Partial): Permission { canRequestProgrammatically: true, status: "pending", instructions: "a report", + helperPhase: "reconciling", ...overrides, }; } @@ -94,6 +96,7 @@ function awaitingApprovalRow() { helperPermission({ instructions: APPROVE_IN_LOGIN_ITEMS, canRequestProgrammatically: false, + helperPhase: "approvalRequired", }), ], allRequiredGranted: false, @@ -139,9 +142,16 @@ describe("PermissionsPanel — the unattended sync helper row", () => { it("offers Enable, and only Enable, while no helper is wanted", async () => { for (const preference of ["unset", "disabled"] as const) { helperPreference.mockReturnValue(preference); - permissionsState.mockReturnValue( - helperRow("pending", "The unattended sync helper is not installed."), - ); + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ + helperPhase: "disabled", + instructions: "The unattended sync helper is not installed.", + }), + ], + allRequiredGranted: false, + checkedAt: null, + }); const { unmount } = await panel(); @@ -170,31 +180,215 @@ describe("PermissionsPanel — the unattended sync helper row", () => { } }); - it("allows one explicit retry when registration is unavailable, while preserving Disable", async () => { + it("retries a missing registration through Retry, never through a new Enable", async () => { + // RegistrationUnavailable: the row's status is unknown and its phase is + // failed. Observing the row never registers; Retry asks the backend to + // carry out the stored Enable once, and Disable stays available. helperPreference.mockReturnValue("granted"); - permissionsState.mockReturnValue(helperRow("unknown", "macOS cannot find the helper registration.")); - let finishGrant!: () => void; - mockRequest.mockReturnValue(new Promise((resolve) => { - finishGrant = () => resolve({ id: HELPER_PERMISSION_ID, status: "unknown" }); - })); + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ + status: "unknown", + helperPhase: "failed", + instructions: "macOS cannot find the helper registration. Retry enabling it; macOS may still require your approval.", + }), + ], + allRequiredGranted: false, + checkedAt: null, + }); + let finishRetry!: () => void; + mockRetryHelper.mockReturnValue( + new Promise((resolve) => { + finishRetry = () => + resolve({ atThisBuild: false, phase: "approvalRequired", detail: APPROVE_IN_LOGIN_ITEMS }); + }), + ); await panel(); - expect(mockRequest).not.toHaveBeenCalled(); + expect(mockRetryHelper).not.toHaveBeenCalled(); expect(screen.getByRole("button", { name: "Disable" })).toBeTruthy(); - fireEvent.click(screen.getByRole("button", { name: "Retry Enable" })); + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); - const pending = await screen.findByRole("button", { name: "Enabling…" }); + const pending = await screen.findByRole("button", { name: "Retrying…" }); expect(pending).toBeDisabled(); fireEvent.click(pending); - expect(mockRequest).toHaveBeenCalledTimes(1); - expect(mockRequest).toHaveBeenCalledWith(HELPER_PERMISSION_ID); + expect(mockRetryHelper).toHaveBeenCalledTimes(1); + finishRetry(); + await waitFor(() => expect(mockRefresh).toHaveBeenCalled()); + expect(mockRequest).not.toHaveBeenCalled(); expect(mockDisableHelper).not.toHaveBeenCalled(); - finishGrant(); + }); + + it("reports a failed Retry request through its diagnostic", async () => { + helperPreference.mockReturnValue("granted"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "failed" })], + allRequiredGranted: false, + checkedAt: null, + }); + mockRetryHelper.mockRejectedValue(new Error("IPC channel closed")); + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); + + await panel(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + + expect( + await screen.findByText( + "Retrying the unattended sync helper failed: IPC channel closed Try again. If this keeps happening, report the problem.", + ), + ).toBeTruthy(); + expect(screen.getByRole("button", { name: "Retry" })).toBeEnabled(); + consoleError.mockRestore(); + }); + + it("offers Retry without removing Disable after automatic recovery fails", async () => { + helperPreference.mockReturnValue("granted"); + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ + helperPhase: "failed", + instructions: "nixmac couldn’t finish enabling unattended sync. Try again.", + }), + ], + allRequiredGranted: false, + checkedAt: null, + }); + mockRetryHelper.mockResolvedValue({ + atThisBuild: false, + phase: "reconciling", + detail: "nixmac is finishing unattended sync setup.", + }); + + const { getByRole } = await panel(); + expect(getByRole("button", { name: "Retry" })).toBeTruthy(); + expect(getByRole("button", { name: "Disable" })).toBeTruthy(); + + fireEvent.click(getByRole("button", { name: "Retry" })); await waitFor(() => { - expect(screen.getByRole("button", { name: "Retry Enable" })).toBeTruthy(); - expect(screen.getByRole("button", { name: "Disable" })).toBeTruthy(); + expect(mockRetryHelper).toHaveBeenCalledTimes(1); + expect(mockRefresh).toHaveBeenCalled(); }); - expect(mockRequest).toHaveBeenCalledTimes(1); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it.each(["Retry", "Disable"] as const)( + "keeps %s as the only helper action until it finishes", + async (action) => { + helperPreference.mockReturnValue("granted"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "failed" })], + allRequiredGranted: false, + checkedAt: null, + }); + let finish: () => void = () => {}; + const pending = new Promise((resolve) => { + finish = () => resolve({ atThisBuild: false, phase: "failed", detail: "Finished." }); + }); + (action === "Retry" ? mockRetryHelper : mockDisableHelper).mockReturnValue(pending); + + const { repaint } = await panel(); + fireEvent.click(screen.getByRole("button", { name: action })); + + const busyLabel = action === "Retry" ? "Retrying…" : "Disabling…"; + expect(screen.getByRole("button", { name: busyLabel })).toBeDisabled(); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Disable" })).toBeNull(); + + // Reconciliation publishes progress before the request settles. + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "reconciling" })], + allRequiredGranted: false, + checkedAt: null, + }); + repaint(); + expect(screen.getByRole("button", { name: busyLabel })).toBeDisabled(); + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + + finish(); + await waitFor(() => { + expect(screen.queryByRole("button", { name: busyLabel })).toBeNull(); + expect(screen.getByRole("button", { name: "Disable" })).toBeEnabled(); + }); + }, + ); + + it.each([ + ["reconciling", "nixmac is removing the unattended sync helper."], + [ + "waitingForActivation", + "nixmac will remove the unattended sync helper when the running activation finishes.", + ], + ] as const)("offers no opt-in while a stored Disable is %s", async (phase, instructions) => { + helperPreference.mockReturnValue("disabled"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: phase, instructions })], + allRequiredGranted: false, + checkedAt: null, + }); + + await panel(); + expect(screen.getByText(instructions)).toBeTruthy(); + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Disable" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("shows removal-failure copy and only Retry after a failed Disable", async () => { + const removalFailed = + "nixmac couldn’t remove the unattended sync helper. Try again. If this keeps happening, report the problem."; + helperPreference.mockReturnValue("disabled"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "failed", instructions: removalFailed })], + allRequiredGranted: false, + checkedAt: null, + }); + + await panel(); + expect(screen.getByText(removalFailed)).toBeTruthy(); + expect(screen.getByRole("button", { name: "Retry" })).toBeTruthy(); + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Disable" })).toBeNull(); + }); + + it("offers Enable again once the removal settles", async () => { + helperPreference.mockReturnValue("disabled"); + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ + helperPhase: "disabled", + instructions: "The unattended sync helper is disabled and has been removed.", + }), + ], + allRequiredGranted: false, + checkedAt: null, + }); + + await panel(); + expect(screen.getByRole("button", { name: "Enable" })).toBeTruthy(); + }); + + it("retries a failed removal without offering to enable the helper again", async () => { + helperPreference.mockReturnValue("disabled"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "failed" })], + allRequiredGranted: false, + checkedAt: null, + }); + mockRetryHelper.mockResolvedValue({ atThisBuild: false, phase: "disabled", detail: "Removed." }); + + await panel(); + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Disable" })).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + + await waitFor(() => { + expect(mockRetryHelper).toHaveBeenCalledOnce(); + expect(screen.getByRole("button", { name: "Retry" })).toBeEnabled(); + }); + expect(mockRequest).not.toHaveBeenCalled(); + expect(mockDisableHelper).not.toHaveBeenCalled(); }); it("offers the Login Items deep link, not Disable, while approval is pending", async () => { @@ -218,6 +412,25 @@ describe("PermissionsPanel — the unattended sync helper row", () => { } }); + it("does not offer an ineffective Enable for a helper that needs an app move or restart", async () => { + helperPreference.mockReturnValue("unset"); + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ + helperPhase: "needsUserAction", + instructions: "Move nixmac to /Applications and restart it.", + }), + ], + allRequiredGranted: false, + checkedAt: null, + }); + + await panel(); + + expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); + expect(screen.getByText("Move nixmac to /Applications and restart it.")).toBeTruthy(); + }); + it("opens Login Items through the same grant action the row always used", async () => { // The deep link is not a new backend call: it is `permissions.request` for // this row, which records the decision and reconciles, and is the only action @@ -238,6 +451,55 @@ describe("PermissionsPanel — the unattended sync helper row", () => { expect(mockDisableHelper).not.toHaveBeenCalled(); }); + it.each(["before", "after"] as const)( + "drops approval instructions when the helper becomes ready %s the grant response", + async (readyWhen) => { + permissionsState.mockReturnValue( + helperRow("pending", "The unattended sync helper is not installed."), + ); + let finishGrant: () => void = () => {}; + mockRequest.mockReturnValue( + new Promise((resolve) => { + finishGrant = () => resolve(awaitingApprovalRow().permissions[0]); + }), + ); + + const { repaint } = await panel(); + fireEvent.click(screen.getByRole("button", { name: "Enable" })); + helperPreference.mockReturnValue("granted"); + permissionsState.mockReturnValue(awaitingApprovalRow()); + repaint(); + + if (readyWhen === "after") { + finishGrant(); + await waitFor(() => expect(mockRefresh).toHaveBeenCalledTimes(2)); + expect(screen.getAllByText(APPROVE_IN_LOGIN_ITEMS)).toHaveLength(1); + } + + const ready = "The unattended sync helper is installed and answering."; + permissionsState.mockReturnValue({ + permissions: [ + helperPermission({ status: "granted", helperPhase: "ready", instructions: ready }), + ], + allRequiredGranted: true, + checkedAt: null, + }); + repaint(); + + if (readyWhen === "before") { + finishGrant(); + await waitFor(() => expect(mockRefresh).toHaveBeenCalledTimes(2)); + } + + await waitFor(() => + expect(screen.getByRole("button", { name: "Disable" })).toBeEnabled(), + ); + expect(screen.getByText(ready)).toBeTruthy(); + expect(screen.queryByText(APPROVE_IN_LOGIN_ITEMS)).toBeNull(); + expect(screen.queryByRole("button", { name: /Open Settings/ })).toBeNull(); + }, + ); + it("offers Disable on a granted row whose decision was never recorded", async () => { // An adopted registration: the reconciliation run records `granted` before // it reports, so this is the window before the mirrored preference catches @@ -251,7 +513,7 @@ describe("PermissionsPanel — the unattended sync helper row", () => { expect(screen.queryByRole("button", { name: "Enable" })).toBeNull(); }); - it("renders every report the backend can send for a row that is not granted", async () => { + it("renders actionable and friendly helper copy", async () => { // The vocabulary: move the app, restart it, wait out a running activation // (with and without the activation's details), and plain failures. Each // arrives as the row's instructions and is shown verbatim — the UI never @@ -265,7 +527,7 @@ describe("PermissionsPanel — the unattended sync helper row", () => { "this app was replaced while running (build a is running, b is installed) — restart nixmac.", "nixmac is waiting for a running activation to finish before updating the unattended sync helper (/nix/store/abc/activate submitted by the sync agent).", "nixmac is waiting for a running activation to finish before updating the unattended sync helper.", - "the helper could not be unregistered: SMAppService 1 refused.", + "nixmac couldn’t update the unattended sync helper. Try again. If this keeps happening, report the problem.", ]) { permissionsState.mockReturnValue(helperRow("pending", report)); @@ -357,20 +619,75 @@ describe("PermissionsPanel — the unattended sync helper row", () => { expect(running).toBeDisabled(); }); - it("disabling reports what the run did and re-probes", async () => { + it("uses the live row after disabling, without retaining a stale RPC report", async () => { permissionsState.mockReturnValue(helperRow("granted", "installed and answering")); mockDisableHelper.mockResolvedValue({ atThisBuild: false, - detail: "The unattended sync helper is disabled and has been removed.", + phase: "waitingForActivation", + detail: "nixmac is waiting for a running activation to finish before updating the unattended sync helper.", }); - const { getByRole } = await panel(); + const { getByRole, repaint } = await panel(); fireEvent.click(getByRole("button", { name: "Disable" })); await waitFor(() => { expect(mockDisableHelper).toHaveBeenCalledTimes(1); - expect(mockRefresh).toHaveBeenCalled(); + expect(mockRefresh).toHaveBeenCalledTimes(2); + }); + const removed = "The unattended sync helper is disabled and has been removed."; + helperPreference.mockReturnValue("disabled"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "disabled", instructions: removed })], + allRequiredGranted: false, + checkedAt: null, }); - expect(screen.getByText("The unattended sync helper is disabled and has been removed.")).toBeTruthy(); + repaint(); + + expect(screen.getAllByText(removed)).toHaveLength(1); + expect(screen.queryByText(/waiting for a running activation/)).toBeNull(); + expect(screen.getByRole("button", { name: "Enable" })).toBeEnabled(); + }); + + it.each(["Enable", "Retry", "Disable"] as const)( + "preserves a thrown %s error as a notice", + async (action) => { + helperPreference.mockReturnValue(action === "Enable" ? "unset" : "granted"); + permissionsState.mockReturnValue({ + permissions: [helperPermission({ helperPhase: "failed" })], + allRequiredGranted: false, + checkedAt: null, + }); + const request = { + Enable: mockRequest, + Retry: mockRetryHelper, + Disable: mockDisableHelper, + }[action]; + request.mockRejectedValue(new Error("helper action transport failed")); + + await panel(); + fireEvent.click(screen.getByRole("button", { name: action })); + + await waitFor(() => expect(screen.getByText(/helper action transport failed/)).toBeTruthy()); + }, + ); + + it("preserves App Management guidance when the helper row changes", async () => { + const appManagement = { + ...adminRow, + id: "app-management", + name: "App Management", + instructions: "Allow nixmac to update managed apps.", + }; + permissionsState.mockReturnValue(helperRow("pending", "a report", appManagement)); + mockRequest.mockResolvedValue({ id: "app-management", status: "pending", instructions: null }); + + const { repaint } = await panel(); + fireEvent.click(screen.getByRole("button", { name: /Open Settings/ })); + await waitFor(() => expect(screen.getByText(/nixmac opened System Settings/)).toBeTruthy()); + + permissionsState.mockReturnValue(helperRow("granted", "installed and answering", appManagement)); + repaint(); + expect(screen.getByText(/nixmac opened System Settings/)).toBeTruthy(); + await waitFor(() => expect(mockRefresh).toHaveBeenCalledTimes(2), { timeout: 2000 }); }); }); diff --git a/apps/native/src/components/widget/permissions/permissions-panel.tsx b/apps/native/src/components/widget/permissions/permissions-panel.tsx index 5895e615e..294ae55d1 100644 --- a/apps/native/src/components/widget/permissions/permissions-panel.tsx +++ b/apps/native/src/components/widget/permissions/permissions-panel.tsx @@ -3,6 +3,7 @@ import { Button } from "@/components/ui/button"; import { tauriAPI } from "@/ipc/api"; import type { Permission } from "@/ipc/types"; +import { DIAGNOSTIC_CODES, diagnostics } from "@/lib/errors"; import { client, orpc } from "@/lib/orpc"; import { HELPER_PERMISSION_ID } from "@/lib/permissions"; import { cn } from "@/lib/utils"; @@ -69,7 +70,7 @@ function grantLabel(perm: Permission): Pick>( + const [requesting, setRequesting] = useState< + ReadonlyMap + >( () => new Map(), ); const [notice, setNotice] = useState<{ tone: "info" | "error"; message: string } | null>(null); // Updater form: two rows can start or settle in the same tick. - function startAction(id: string, action: "grant" | "disable") { + function startAction(id: string, action: "grant" | "retry" | "disable") { setRequesting((inFlight) => new Map(inFlight).set(id, action)); } @@ -143,21 +146,9 @@ export function PermissionsPanel() { "nixmac opened System Settings → Privacy & Security → App Management. Enable nixmac there, then return here. macOS does not let nixmac verify this permission, so this recommended row may remain pending.", }); await new Promise((resolve) => setTimeout(resolve, 1000)); - } else if (permission.id === HELPER_PERMISSION_ID) { - // The backend records the decision and reconciles; it may open Login - // Items when approval is pending. Show this run's report only when it - // differs from the row's sentence — otherwise every click while - // approval is pending prints the same sentence twice. - const result = await tauriAPI.permissions.request(permission.id); - if (result.status !== "granted" && result.instructions !== permission.instructions) { - setNotice({ - tone: "info", - message: - result.instructions ?? - "nixmac could not finish enabling the unattended sync helper.", - }); - } } else { + // Helper instructions come from the live permission row. Copying a + // request's result into a notice can outlive approval or recovery. // deprecated(orpc): replace with client/orpc from @/lib/orpc await tauriAPI.permissions.request(permission.id); } @@ -183,8 +174,7 @@ export function PermissionsPanel() { startAction(HELPER_PERMISSION_ID, "disable"); setNotice(null); try { - const report = await client.darwin.helperDisable(); - setNotice({ tone: "info", message: report.detail }); + await client.darwin.helperDisable(); await client.permissions.refresh(); } catch (error) { console.error("Failed to disable the unattended sync helper:", error); @@ -197,6 +187,27 @@ export function PermissionsPanel() { } } + /** + * Retry the stored Enable or Disable without recording a new decision. The + * row offers it only for those two decisions. + */ + async function handleRetryHelper() { + startAction(HELPER_PERMISSION_ID, "retry"); + setNotice(null); + try { + await client.darwin.helperRetry(); + await client.permissions.refresh(); + } catch (error) { + // Reporting the diagnostic also logs it, with its code and fix. + const diagnostic = diagnostics[DIAGNOSTIC_CODES.HELPER_RETRY_FAILED]({ + detail: error instanceof Error ? error.message : String(error), + }); + setNotice({ tone: "error", message: `${diagnostic.message} ${diagnostic.fix ?? ""}`.trim() }); + } finally { + finishAction(HELPER_PERMISSION_ID); + } + } + const permissions = permissionsState?.permissions ?? []; return ( @@ -233,10 +244,12 @@ export function PermissionsPanel() { const isGranted = perm.status === "granted"; const pendingAction = requesting.get(perm.id) ?? null; const isGranting = pendingAction === "grant"; + const isRetrying = pendingAction === "retry"; const isDisabling = pendingAction === "disable"; - // The one action this row offers. The helper is the one permission - // nixmac installs rather than asks macOS for, so its row can hand it - // back — a toggle of the standing decision, not of the row's status. + // The helper is the one permission nixmac installs rather than asks + // macOS for, so its normal action toggles the standing decision, not + // the row's status. Retry repeats the stored decision, including a + // failed removal. An enabled helper also keeps Disable available. // While macOS holds the registration pending approval the backend // clears `canRequestProgrammatically` and the row deep-links to Login // Items instead. An action in flight keeps its own button: it has @@ -245,12 +258,30 @@ export function PermissionsPanel() { perm.id === HELPER_PERMISSION_ID && perm.canRequestProgrammatically && (helperPreference === "granted" || isGranted); - // The helper's unknown status means registration could not be - // found. Retain the standing decision and its Disable action, but - // allow one explicit retry; observing this row never registers it. - const offersRetry = offersDisable && perm.status === "unknown"; + const offersRetry = + perm.id === HELPER_PERMISSION_ID && + (isRetrying || + (pendingAction === null && + perm.helperPhase === "failed" && + (helperPreference === "granted" || helperPreference === "disabled"))); + // A stored Disable that is still being carried out (in progress, + // waiting on an activation, or failed) offers no opt-in: Enable + // would record the opposite decision mid-removal. Enable returns + // once the removal settles at the disabled phase. + const removalUnfinished = + perm.id === HELPER_PERMISSION_ID && + helperPreference === "disabled" && + perm.helperPhase != null && + perm.helperPhase !== "disabled"; const action: "grant" | "disable" | null = - pendingAction ?? (offersDisable ? "disable" : isGranted ? null : "grant"); + pendingAction === "retry" + ? null + : (pendingAction ?? + (offersDisable + ? "disable" + : isGranted || perm.helperPhase === "needsUserAction" || removalUnfinished + ? null + : "grant")); // A fresh DOM node whenever the button changes shape: the // design-system Button animates a reused one, cross-fading fill and // width between shapes. @@ -335,11 +366,12 @@ export function PermissionsPanel() { Granted ) : null} - {offersRetry && pendingAction === null ? ( + {offersRetry ? ( handleGrant(perm)} + idle="Retry" + busy="Retrying…" + isBusy={isRetrying} + onClick={() => void handleRetryHelper()} /> ) : null} {action === "disable" ? ( diff --git a/apps/native/src/components/widget/repair/lib.test.ts b/apps/native/src/components/widget/repair/lib.test.ts index 110008860..637bd6b1a 100644 --- a/apps/native/src/components/widget/repair/lib.test.ts +++ b/apps/native/src/components/widget/repair/lib.test.ts @@ -15,7 +15,6 @@ function makeInputs(overrides: Partial = {}): RepairInputs { permissions: makeGrantedPermissions(), helperRow: null, helperPreference: "unset", - helperGraceElapsed: false, skipPermissions: false, nixInstalledOverride: false, ...overrides, @@ -95,7 +94,6 @@ describe("computeRepairPlan", () => { makeInputs({ helperPreference, helperRow: makeHelperRow(), - helperGraceElapsed: true, }), ); expect(plan.banners).toEqual([]); @@ -106,37 +104,63 @@ describe("computeRepairPlan", () => { const plan = computeRepairPlan( makeInputs({ helperPreference: "granted", - helperRow: makeHelperRow({ status: "granted" }), - helperGraceElapsed: true, + helperRow: makeHelperRow({ status: "granted", helperPhase: "ready" }), }), ); expect(plan.banners).toEqual([]); }); - it("waits for the grace period before naming the helper", () => { + it("banners the wanted helper with the backend's typed phase and copy", () => { const plan = computeRepairPlan( makeInputs({ helperPreference: "granted", helperRow: makeHelperRow(), - helperGraceElapsed: false, - }), - ); - expect(plan.banners).toEqual([]); - }); - - it("banners the wanted-but-silent helper with the row's own sentence", () => { - const plan = computeRepairPlan( - makeInputs({ - helperPreference: "granted", - helperRow: makeHelperRow(), - helperGraceElapsed: true, }), ); expect(plan.banners).toEqual([ - { kind: "helper-inactive", instructions: APPROVE_IN_LOGIN_ITEMS }, + { + kind: "helper-inactive", + phase: "approvalRequired", + instructions: APPROVE_IN_LOGIN_ITEMS, + }, ]); }); + it("preserves every actionable helper phase", () => { + for (const phase of [ + "reconciling", + "waitingForActivation", + "needsUserAction", + "failed", + ] as const) { + const plan = computeRepairPlan( + makeInputs({ + helperPreference: "granted", + helperRow: makeHelperRow({ + helperPhase: phase, + canRequestProgrammatically: true, + instructions: `copy for ${phase}`, + }), + }), + ); + expect(plan.banners).toEqual([ + { kind: "helper-inactive", phase, instructions: `copy for ${phase}` }, + ]); + } + }); + + it("shows setup progress, not a failure, while an Enable replaces the old row", () => { + for (const helperPhase of ["disabled", "ready"] as const) { + const plan = computeRepairPlan( + makeInputs({ + helperPreference: "granted", + helperRow: makeHelperRow({ helperPhase }), + }), + ); + expect(plan.banners[0]).toMatchObject({ kind: "helper-inactive", phase: "reconciling" }); + } + }); + it("leaves the helper out of the revoked-permissions list", () => { const helperRow = makeHelperRow(); const plan = computeRepairPlan( @@ -158,12 +182,15 @@ describe("computeRepairPlan", () => { }, helperPreference: "granted", helperRow, - helperGraceElapsed: true, }), ); expect(plan.banners).toEqual([ { kind: "permissions-revoked", missing: [{ id: "full-disk", name: "Full Disk Access" }] }, - { kind: "helper-inactive", instructions: APPROVE_IN_LOGIN_ITEMS }, + { + kind: "helper-inactive", + phase: "approvalRequired", + instructions: APPROVE_IN_LOGIN_ITEMS, + }, ]); }); @@ -178,11 +205,14 @@ describe("computeRepairPlan", () => { }, helperPreference: "granted", helperRow, - helperGraceElapsed: true, }), ); expect(plan.banners).toEqual([ - { kind: "helper-inactive", instructions: APPROVE_IN_LOGIN_ITEMS }, + { + kind: "helper-inactive", + phase: "approvalRequired", + instructions: APPROVE_IN_LOGIN_ITEMS, + }, ]); }); @@ -199,7 +229,6 @@ describe("computeRepairPlan", () => { }, helperPreference: "granted", helperRow: makeHelperRow(), - helperGraceElapsed: true, }), ); expect(plan).toEqual({ blocking: null, banners: [] }); diff --git a/apps/native/src/components/widget/repair/lib.ts b/apps/native/src/components/widget/repair/lib.ts index 19d89ba33..954005480 100644 --- a/apps/native/src/components/widget/repair/lib.ts +++ b/apps/native/src/components/widget/repair/lib.ts @@ -6,8 +6,12 @@ export type RepairIssue = | { kind: "config-missing"; configDir: string } | { kind: "nix-missing" } | { kind: "permissions-revoked"; missing: { id: string; name: string }[] } - /** The user asked for the unattended sync helper and it is not answering. */ - | { kind: "helper-inactive"; instructions: string | null }; + /** The user asked for the unattended sync helper and it is not ready. */ + | { + kind: "helper-inactive"; + phase: NonNullable; + instructions: string | null; + }; export interface RepairInputs { /** Onboarding completion latch; repair is a post-completion concept. */ @@ -25,8 +29,6 @@ export interface RepairInputs { helperRow: Permission | null; /** The user's standing decision about the helper; null before hydration. */ helperPreference: HelperPreference | null; - /** Whether the helper condition has held long enough to be worth saying. */ - helperGraceElapsed: boolean; /** Dev-profile overrides; a skipped gate must not resurface as a repair. */ skipPermissions: boolean; nixInstalledOverride: boolean; @@ -46,7 +48,7 @@ export interface RepairPlan { * Only a missing configuration blocks: every main surface operates on it, * while the other regressions degrade specific actions and banner instead. * - * Every input except the three helper ones is snapshotted when the widget + * Every input except the two helper ones is snapshotted when the widget * mounts and re-read only through "Check again", keeping the blocking card * decided once per launch, as D7 requires. The helper's inputs are live: its * row is expected to be wrong for the first seconds of a launch and to come @@ -81,23 +83,28 @@ export function computeRepairPlan(inputs: RepairInputs): RepairPlan { } } - // The user asked for the helper and the helper is not answering. The row - // carries no marker for *why* (approval pending, a misplaced copy, a failed - // register), so one headline states the condition and the row's own - // `instructions` say what is true right now. For the same reason the button - // is the row's Grant action in every state: it opens Login Items when - // approval is pending, and in the states no run can fix it re-reports the - // same sentence — the accepted cost of having nothing to branch on, not a - // gap to close by inventing a marker. + // The helper reverses the old no-marker decision deliberately: approval, + // progress, an active sync, displacement, and failure require different + // copy and actions. The backend owns this phase so every surface sees the + // same state and no UI parses diagnostic prose. if ( !inputs.skipPermissions && inputs.helperPreference === "granted" && inputs.helperRow !== null && - inputs.helperRow.status !== "granted" && - inputs.helperGraceElapsed + inputs.helperRow.status !== "granted" ) { + const observedPhase = + inputs.helperRow.helperPhase ?? + (inputs.helperRow.canRequestProgrammatically ? "reconciling" : "approvalRequired"); + // Enable stores Granted before its pass, so for a moment the row still + // shows the earlier NoHelper/Removed (or Ready) report while the + // preference already says Granted. That is setup in progress, not a + // failure and not an opt-out; the loop publishes the real phase next. + const phase = + observedPhase === "disabled" || observedPhase === "ready" ? "reconciling" : observedPhase; banners.push({ kind: "helper-inactive", + phase, instructions: inputs.helperRow.instructions ?? null, }); } diff --git a/apps/native/src/components/widget/repair/repair.test.tsx b/apps/native/src/components/widget/repair/repair.test.tsx index 3c2f74d66..e02c9d57c 100644 --- a/apps/native/src/components/widget/repair/repair.test.tsx +++ b/apps/native/src/components/widget/repair/repair.test.tsx @@ -7,32 +7,40 @@ import { makeNixInstallState, } from "@/utils/test-fixtures"; import { initialViewModelState, useViewModel } from "@nixmac/state"; -import { act, renderHook } from "@testing-library/react"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { HELPER_GRACE_MS, useRepair } from "./repair"; +import { act, fireEvent, render, renderHook, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { RepairBanners, useRepair } from "./repair"; /** * The helper's banner is the one part of the repair plan that follows live state, - * so what is worth testing is the timing: it must stay quiet while the backend is - * still converging, speak once the condition has outlasted that, and go when the - * helper answers — without a churning report ever holding it back. + * so it must follow the backend's typed phase and clear as soon as the helper + * answers. Retry/approval actions are distinct and tested at the render surface. */ +const { mockHelperRetry, mockPermissionRequest, mockRefresh } = vi.hoisted(() => ({ + mockHelperRetry: vi.fn<() => Promise>(async () => ({})), + mockPermissionRequest: vi.fn< + (input: { permissionId: string }) => Promise + >(async () => ({})), + mockRefresh: vi.fn<() => Promise>(async () => {}), +})); + vi.mock("@/ipc/api", () => ({ tauriAPI: { - permissions: { refresh: vi.fn(async () => {}) }, + permissions: { refresh: mockRefresh }, }, })); vi.mock("@/lib/orpc", () => ({ client: { - flake: { exists: vi.fn(async () => true) }, - permissions: { request: vi.fn(async () => {}) }, + flake: { exists: vi.fn<() => Promise>(async () => true) }, + darwin: { helperRetry: mockHelperRetry }, + permissions: { request: mockPermissionRequest }, }, })); vi.mock("@/lib/env", () => ({ settings: {} })); -vi.mock("@/router", () => ({ nav: { openSettings: vi.fn() } })); +vi.mock("@/router", () => ({ nav: { openSettings: vi.fn<(tab?: string) => void>() } })); vi.mock("@/components/widget/onboarding/restart-setup", () => ({ RestartSetupConfirmation: () => null, })); @@ -67,12 +75,8 @@ function publishHelperRow(helperRow: Permission) { } describe("useRepair", () => { - beforeEach(() => { - vi.useFakeTimers(); - }); - afterEach(() => { - vi.useRealTimers(); + vi.clearAllMocks(); useViewModel.setState(initialViewModelState); }); @@ -83,88 +87,267 @@ describe("useRepair", () => { return view; } - it("stays quiet until the condition has outlasted the grace period", async () => { + it("shows approval immediately", async () => { seedStore(makeHelperRow(), "granted"); const { result } = await mount(); - expect(result.current.plan.banners).toEqual([]); - - await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS); - }); - expect(result.current.plan.banners).toEqual([ - { kind: "helper-inactive", instructions: APPROVE_IN_LOGIN_ITEMS }, + { + kind: "helper-inactive", + phase: "approvalRequired", + instructions: APPROVE_IN_LOGIN_ITEMS, + }, ]); }); - it("does not restart the clock when a still-failing report changes", async () => { - seedStore(makeHelperRow(), "granted"); + it("follows reconciling and active-sync phases without parsing copy", async () => { + seedStore( + makeHelperRow({ + helperPhase: "reconciling", + canRequestProgrammatically: true, + instructions: "nixmac is finishing unattended sync setup.", + }), + "granted", + ); const { result } = await mount(); - await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS * 0.6); + expect(result.current.plan.banners[0]).toMatchObject({ + kind: "helper-inactive", + phase: "reconciling", + }); + + act(() => { publishHelperRow( makeHelperRow({ + helperPhase: "waitingForActivation", canRequestProgrammatically: true, - instructions: "nixmac could not register the unattended sync helper.", + instructions: "Waiting for the current sync.", }), ); }); - expect(result.current.plan.banners).toEqual([]); - - // Past the grace period counted from the first report, not the second. - await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS * 0.6); - }); expect(result.current.plan.banners).toEqual([ { kind: "helper-inactive", - instructions: "nixmac could not register the unattended sync helper.", + phase: "waitingForActivation", + instructions: "Waiting for the current sync.", }, ]); }); - it("clears the banner and the clock once the helper answers", async () => { - seedStore(makeHelperRow(), "granted"); + it("clears the banner once the helper answers", async () => { + seedStore(makeHelperRow({ helperPhase: "failed" }), "granted"); const { result } = await mount(); - - await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS); - }); expect(result.current.plan.banners).toHaveLength(1); - await act(async () => { + act(() => { publishHelperRow( makeHelperRow({ status: "granted", + helperPhase: "ready", canRequestProgrammatically: true, instructions: "The unattended sync helper is installed and answering.", }), ); }); expect(result.current.plan.banners).toEqual([]); + }); - // The clock was cleared with the banner, so a later regression waits again. - await act(async () => { - publishHelperRow(makeHelperRow()); - }); + it("does not let dismissal of one phase hide a later failure", async () => { + seedStore(makeHelperRow(), "granted"); + const { result } = await mount(); + + act(() => result.current.dismissBanner("helper-inactive")); expect(result.current.plan.banners).toEqual([]); - await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS); + act(() => { + publishHelperRow( + makeHelperRow({ + helperPhase: "failed", + canRequestProgrammatically: true, + instructions: "Try again.", + }), + ); + }); + expect(result.current.plan.banners[0]).toMatchObject({ + kind: "helper-inactive", + phase: "failed", }); - expect(result.current.plan.banners).toHaveLength(1); }); it("says nothing about a helper the user did not ask for", async () => { seedStore(makeHelperRow(), "unset"); const { result } = await mount(); + expect(result.current.plan.banners).toEqual([]); + }); +}); + +describe("RepairBanners helper actions", () => { + afterEach(() => vi.clearAllMocks()); + + it("opens System Settings only for approval", async () => { + render( + {}} + onRecheck={mockRefresh} + />, + ); + fireEvent.click(screen.getByRole("button", { name: "Open System Settings" })); + await waitFor(() => expect(mockPermissionRequest).toHaveBeenCalled()); + expect(mockHelperRetry).not.toHaveBeenCalled(); + }); + + it("retries only a failed helper", async () => { + render( + {}} + onRecheck={mockRefresh} + />, + ); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + await waitFor(() => expect(mockHelperRetry).toHaveBeenCalled()); + expect(mockPermissionRequest).not.toHaveBeenCalled(); + }); + + it("reports a failed banner Retry through its diagnostic", async () => { + mockHelperRetry.mockRejectedValueOnce(new Error("IPC channel closed")); + render( + {}} + onRecheck={mockRefresh} + />, + ); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect((await screen.findByRole("alert")).textContent).toBe( + "Retrying the unattended sync helper failed: IPC channel closed Try again. If this keeps happening, report the problem.", + ); + expect(mockRefresh).not.toHaveBeenCalled(); + expect(screen.getByRole("button", { name: "Retry" })).toBeEnabled(); + + // A later attempt clears the earlier failure. + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + await waitFor(() => expect(screen.queryByRole("alert")).toBeNull()); + }); + + it("drops a failed Retry's error once the helper leaves that failure", async () => { + mockHelperRetry.mockRejectedValueOnce(new Error("IPC channel closed")); + const failed = [ + { kind: "helper-inactive" as const, phase: "failed" as const, instructions: "Try again." }, + ]; + const { rerender } = render( + {}} onRecheck={mockRefresh} />, + ); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(await screen.findByRole("alert")).toBeTruthy(); + + // Recovered: the banner goes away, then a new failure starts clean. + rerender( {}} onRecheck={mockRefresh} />); + rerender( {}} onRecheck={mockRefresh} />); + expect(screen.queryByRole("alert")).toBeNull(); + expect(mockHelperRetry).toHaveBeenCalledTimes(1); + + // Another phase in between clears it the same way. + mockHelperRetry.mockRejectedValueOnce(new Error("IPC channel closed")); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(await screen.findByRole("alert")).toBeTruthy(); + rerender( + {}} + onRecheck={mockRefresh} + />, + ); + rerender( {}} onRecheck={mockRefresh} />); + expect(screen.queryByRole("alert")).toBeNull(); + }); + + it("ignores a Retry that settles after its failure episode ended", async () => { + let rejectFirst!: (error: Error) => void; + mockHelperRetry.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectFirst = reject; + }), + ); + const failed = [ + { kind: "helper-inactive" as const, phase: "failed" as const, instructions: "Try again." }, + ]; + const { rerender } = render( + {}} onRecheck={mockRefresh} />, + ); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect(mockHelperRetry).toHaveBeenCalledTimes(1); + + // The helper recovers and fails again while the first request is pending. + rerender( {}} onRecheck={mockRefresh} />); + rerender( {}} onRecheck={mockRefresh} />); + await act(async () => { - vi.advanceTimersByTime(HELPER_GRACE_MS); + rejectFirst(new Error("IPC channel closed")); }); + expect(screen.queryByRole("alert")).toBeNull(); - expect(result.current.plan.banners).toEqual([]); + // A Retry made in the new episode still reports its own failure. + mockHelperRetry.mockRejectedValueOnce(new Error("helper action transport failed")); + fireEvent.click(screen.getByRole("button", { name: "Retry" })); + expect((await screen.findByRole("alert")).textContent).toContain( + "helper action transport failed", + ); + }); + + it("offers no repair action while waiting for an activation", () => { + render( + {}} + onRecheck={mockRefresh} + />, + ); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); + expect(screen.queryByRole("button", { name: "Open System Settings" })).toBeNull(); + }); + + it("keeps move or restart guidance instead of offering an ineffective retry", () => { + render( + {}} + onRecheck={mockRefresh} + />, + ); + expect(screen.getByText("Move nixmac to /Applications and restart it.")).toBeTruthy(); + expect(screen.queryByRole("button", { name: "Retry" })).toBeNull(); }); }); diff --git a/apps/native/src/components/widget/repair/repair.tsx b/apps/native/src/components/widget/repair/repair.tsx index 89174b754..52a3a469d 100644 --- a/apps/native/src/components/widget/repair/repair.tsx +++ b/apps/native/src/components/widget/repair/repair.tsx @@ -4,11 +4,13 @@ import { Button } from "@/components/ui/button"; import { RestartSetupConfirmation } from "@/components/widget/onboarding/restart-setup"; import { tauriAPI } from "@/ipc/api"; import { settings } from "@/lib/env"; +import { DIAGNOSTIC_CODES, diagnostics } from "@/lib/errors"; import { client } from "@/lib/orpc"; +import { cn } from "@/lib/utils"; import { nav } from "@/router"; import { useViewModel } from "@nixmac/state"; import { CircleAlert, FolderX, Loader2, RotateCcw, X } from "lucide-react"; -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { HELPER_PERMISSION_ID } from "@/lib/permissions"; import { computeRepairPlan, @@ -20,17 +22,9 @@ import { /** The inputs read once, when the (hydrated) widget mounts. */ type LaunchSnapshot = Omit< RepairInputs, - "helperRow" | "helperPreference" | "helperGraceElapsed" + "helperRow" | "helperPreference" >; -/** - * How long "the helper was asked for and is not answering" must hold before - * the banner shows. Not a cosmetic debounce: on any launch that re-registers - * the helper the condition is true at mount with the backend already working - * on it, and the grace lets those finish in silence. - */ -export const HELPER_GRACE_MS = 10_000; - /** * Post-completion prerequisite regressions: what to block on, what to banner. * @@ -57,21 +51,11 @@ export function useRepair(): { ); const helperPreference = useViewModel((s) => s.preferences?.helperPreference ?? null); - // The grace clock keys on this boolean, not on the row: every publish is a - // fresh object, and a helper whose registration keeps failing publishes one - // per pass — restarting the clock each time would keep the banner off the - // screen for exactly as long as the failure lasts. - const helperWanted = - helperPreference === "granted" && helperRow !== null && helperRow.status !== "granted"; - const [helperGraceElapsed, setHelperGraceElapsed] = useState(false); + // Dismissal belongs to the state the user saw. A later failure must not stay + // hidden because the user dismissed an earlier progress or approval notice. useEffect(() => { - if (!helperWanted) { - setHelperGraceElapsed(false); - return; - } - const timer = setTimeout(() => setHelperGraceElapsed(true), HELPER_GRACE_MS); - return () => clearTimeout(timer); - }, [helperWanted]); + setDismissed((current) => current.filter((kind) => kind !== "helper-inactive")); + }, [helperRow?.helperPhase]); const evaluate = useCallback(async () => { // Snapshot the store rather than subscribing: everything here is @@ -114,7 +98,7 @@ export function useRepair(): { }, [evaluate]); const plan = snapshot - ? computeRepairPlan({ ...snapshot, helperRow, helperPreference, helperGraceElapsed }) + ? computeRepairPlan({ ...snapshot, helperRow, helperPreference }) : { blocking: null, banners: [] }; return { @@ -128,36 +112,63 @@ export function useRepair(): { } /** - * The same Grant the helper's row in Settings → Permissions offers; it may - * open Login Items when macOS is waiting for approval there. No re-probe - * afterwards: this banner reads the row live, published on every - * reconciliation pass, so a failed call is logged rather than surfaced. The - * label keeps its box with the spinner laid over it so the running button - * never resizes (the design-system Button animates resizes). + * The helper's two repair actions are intentionally distinct: approval records + * the opt-in and may open Login Items, while Retry carries out the stored + * decision again without recording a new one. A failed Retry request is + * reported through the same diagnostic as the Permissions panel's Retry. */ -function HelperGrantButton() { - const [granting, setGranting] = useState(false); +function HelperActionButton({ + action, + onRecheck, + episode, + onError, +}: { + action: "approval" | "retry"; + onRecheck: () => Promise; + /** The failure episode a click starts in, read at click time. */ + episode: () => number; + /** Reports for the episode the request started in; obsolete ones are ignored. */ + onError: (episode: number, message: string | null) => void; +}) { + const [working, setWorking] = useState(false); + const idle = action === "approval" ? "Open System Settings" : "Retry"; + const busy = action === "approval" ? "Opening…" : "Retrying…"; return ( )} - {issue.kind === "helper-inactive" && } + {issue.kind === "helper-inactive" && issue.phase === "approvalRequired" ? ( + reportHelperError(episode, issue.phase, message)} + /> + ) : null} + {issue.kind === "helper-inactive" && issue.phase === "failed" ? ( + reportHelperError(episode, issue.phase, message)} + /> + ) : null}