Skip to content

feat(helper)!: wire reconciliation into startup and apply - #636

Draft
Alex Shabalin (alex-sparus) wants to merge 1 commit into
alex/fix-helper-e1cfrom
alex/fix-helper-e1d
Draft

feat(helper)!: wire reconciliation into startup and apply#636
Alex Shabalin (alex-sparus) wants to merge 1 commit into
alex/fix-helper-e1cfrom
alex/fix-helper-e1d

Conversation

@alex-sparus

@alex-sparus Alex Shabalin (alex-sparus) commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

not done yet

  • after the helper upgrade, it might (very likely) need to be re-approved by the user. This should show a banner in the main app.

Summary

  • activates all the E1 commits which enable automatic helper upgrade
    internal code E1d

Test Plan

  • Manual testing of a CI-build artifact
  • Dev-built and signed version replaces the current prod helper

Docs

  • Docs updated (companion PR in darkmatter/nixmac-web: #___)
  • No docs update needed

@alex-sparus

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by jj-ryu.

@darkmatter

darkmatter Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🎨 Storybook preview

Open Storybook preview

Updated for 6764b13


⚠️ Detected UI changes (1)

These stories' HTML snapshots changed. I've added screenshots + links to the changed stories below. Review them carefully then accept the changes to regenerate baselines and include them in this PR:

Flows/Evolve › Playground

Flows/Evolve › Playground


Accept UI changes

  • Click here to accept these changes

Alternatively, you can run bun run test:update-snapshots locally to re-generate the baselines and then push the changes to this PR.

What does this do?

The screenshots above show UI changes detected by the Storybook
snapshot tests run on this PR. Each image is the rendered output of
a Storybook story from the code in this PR branch; the snapshot
test compared it against the committed baseline in
__snapshots__/ and flagged the difference.

Checking the box tells the darkmatter[bot] to regenerate the
baselines from this PR's current code and commit them directly to
this branch. The new baselines become the source of truth for
future runs — only accept after confirming the visual changes are
intentional.

Comparison baseline: the committed __snapshots__/ files on this
PR branch (carried forward from develop). Accept updates them in
place on this branch.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
Warnings
⚠️

No Linear issue ID found in this PR's title, description, or branch name (expected something like ENG-123). Add one so this work is traceable in Linear, or add #no-linear to the PR description to acknowledge it's intentionally untracked.

⚠️ PR is marked WIP / draft — do not merge until ready for review.
⚠️ ❗ Big PR (2848 lines changed). Consider splitting it into smaller, focused changes.

📋 PR Overview

Lines changed 2848 (+2167 / -681)
Files 3 added, 16 modified, 0 deleted
Draft / WIP yes
Has Test Plan yes
Linear issue no
No Test Plan Needed no
New UI components no
New Storybook stories no
New Rust modules yes (2)
New TS source files no
New tests yes (1)
package.json touched no
Cargo.toml touched no
Infra / CI touched no

🔬 Coverage

Report Lines Statements Functions Branches
apps/native/coverage/coverage-summary.json 36.2% 35.8% 31.0% 30.6%

Generated by 🚫 dangerJS against 6764b13

use std::time::Duration;

const CLIENT_TIMEOUT: Duration = Duration::from_secs(30);
/// The one generous bound in this client, and deliberately not one of the short

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This good comment makes me think the line below should be changed to a from_mins for emphasis and future-proofing.

@@ -0,0 +1,958 @@
// Which of the two activation paths one apply may use.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #620 (comment)

So of the files in play, // is mandatory only in protocol.rs, peer_auth.rs, client.rs, helper_runtime.rs, and rebuild/out_link.rs. socket_probe.rs, reconcile.rs, and activation_path.rs are style-matching their neighbors.

stdout: String,
stderr: String,
///
/// Also how a refused activation is reported: `success: false` with the reason

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious if we knew all the possible things that could happen if the condition of the worker changed in the middle of a long-running activation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing that can disrupt the activation is the helper being suddenly killed

AI comment:

Closed set, by construction: the path is chosen before any bytes are dispatched and nothing after re-reads the preference, status, or displacement gates — so post-dispatch the only inputs are that one connection's reply (reply_refusal, total over HelperReply) and its failure (exchange_failure, total over HelperClientError). Helper killed → ClosedBeforeReply; 30-min timeout → Io; both become UnknownOutcome and are reported, never a password fallback. A concurrent replacement can't cut the activation short: retirement latches and the in-flight activation finishes (helper_runtime.rs:125), with reconcile waiting on Busy unbounded. Residual we accept: if the helper dies mid-run, nixmac can't know whether activation completed, and says so.

One reconciliation function decides everything about the installed
helper. Apply chooses between it and the administrator-password prompt
before any bytes reach the helper, from reconciled service state, and
reports every refusal instead of substituting a prompt. Startup, Grant,
Disable and the permission refresh each drive the function until the
stored decision is carried out, then stop: one run is often not enough
and nothing re-ran it, so convergence depended on relaunches and user
gestures — the manual test took six runs across five launches.

- delete the App Management bypass: skipping the helper for applies that
  touch managed app bundles was a silent password substitution while an
  enabled registration could still admit sync-agent work
- delete the status-probe admission gate and the pre-dispatch error
  fallback, both of which could select the password path after an
  exchange had been attempted
- do not count a pass that attempted nothing: a pending approval may
  last weeks and registers nothing, and a caller turned away never
  began, so only passes that try to change something spend the bound
- publish by replacing the helper's row rather than refreshing every
  permission, which would run a second, unbudgeted reconciliation
- refuse to start when permissions are reported granted without probing,
  so a build running on that fiction cannot register a real helper
- offer one action per permission row, at a size that does not change
  while it runs: the helper's is a toggle of the standing decision,
  except while macOS holds the registration pending approval, where the
  row deep-links into Login Items instead
- open Login Items from the click rather than from a report, so a
  background pass that reports first cannot swallow it

BREAKING CHANGE: helperRegister and helperUnregister are replaced by
helperGrant and helperDisable; helperStatus returns the reconciliation
report.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants