feat(profiles): one server per purpose, with a Server panel you can read (SPEC-50) - #162
Merged
Conversation
Every daemon start/stop/restart failure was invisible. Two defects stacked: `makit start` reports why it failed on stdout, not stderr -- the daemon is spawned detached with its output redirected into the log, so the parent's only diagnostic is deps.out() -> console.log. The app read res.stderr alone, so a real failure surfaced as the bare string "makit start exited 1: ". Worse, DaemonActionResult was discarded by all four call sites, so even a correct message went nowhere. _failureMessage() now reads both streams (stderr first, it carries the lower-level cause), dedupes, and drops the separator when both are empty so the message can never end in a dangling colon. All four call sites post a StatusCenter failure. Proven on a real port collision: exit 1, reason on stdout, stderr empty. Also adds SPEC-50 and its mockup (12 cards) as ground truth for the rest.
A profile is now a thing the user owns rather than plumbing derived from a file
path: it can be named, listed, started, stopped, and deleted, and several run at
once. Proven live -- two daemons on 7861/7862 with separate MAKIT_HOMEs, each
minting its own TLS fingerprint, which is what makes per-profile QR pairing work.
Identity is persisted. ProfileRegistry mints the id once into
~/.makit/profiles.json and re-binds a rebuilt or moved dev build by its stored
origin. Before this, id = fnv1a(repoRoot) was recomputed every launch, so moving
a worktree minted a NEW profile and orphaned the old one's home, pairings,
projects and prefs. Measured on this machine: 27 of 33 dev homes (732 MB) were
already unreachable that way, each still holding a device pairing and a TLS
keypair. They are now listed, sized, and offered for bulk reclaim -- offered,
never reaped, because auto-deletion would also have destroyed transcripts the
first time a worktree moved.
isDefault split into name (a UI fact, editable) and storage (legacy|namespaced, a
compatibility fact, frozen). storage:legacy pins the shipped key layout and
implies protected, so ~/.makit is renameable yet undeletable and no shipped
user's prefs move -- the effective key stays byte-identical, asserted by test.
Ports are allocated by probing upward from the hash guess and persisted, skipping
ports other profiles claim, since a probe cannot see a stopped profile's port.
100 slots for unbounded worktrees made collisions inevitable and nothing
reallocated.
Server & Devices drops from ~10 controls to four rows: active profile, one
reachability question, pair-a-phone, and a Diagnostics disclosure holding
pid/CLI/fingerprint/log plus Advanced. The two-phase "Save & restart server" is
gone. Four exclusive bind modes become Reachability{thisMacOnly,myDevices} plus
allowLanFallback, because Auto and LAN were one behaviour wearing two labels:
chooseBindHost prefers Tailscale before it consults allowLan, so --lan is a
fallback. chooseBindHost is unchanged. Every persisted generation migrates, and
the newest wins when all three are present -- otherwise a stale bind mode would
re-open a server the user had restricted.
Deleting a profile erases all four stores (home dir, secure-store namespace,
prefs, registry entry last -- omit the entry and it resurrects empty) and says
what it KEEPS: worktrees and repos are never touched. It refuses rather than
throws, and the refusals are the interesting part. Three findings from review, all
reproduced before fixing:
- Orphan reclaim could never work. stopAndConfirm polled the control socket FILE,
but a SIGKILLed daemon never unlinks it and `makit stop` on a dead daemon
removes only the pid file -- verified against the real binary. So every crashed
profile looked alive forever, and exactly the orphans this feature exists to
reclaim were permanently undeletable. It now polls liveness.
- A rogue registry entry could erase the APNs key. The guard trusted the entry's
own storage flag, but profiles.json is user-writable. Fixed once, then defeated
again by a single trailing slash (~/.makit/ is not == to ~/.makit yet satisfies
a startsWith check). Paths are now canonicalised before any comparison, and a
fourth rule that mutation testing showed could never bite was deleted rather
than left as reassurance.
- Concurrent instances could lose a profile. save() now re-reads and merges by
id, remembers deletions so a merge cannot resurrect them, and uses a
pid-suffixed temp file.
profiles.json is written 0600 inside a 0700 directory, matching the guarantee the
server makes for a directory holding an APNs auth key and a TLS private key;
Dart's defaults would have left it 0644 in 0755.
D10 (in-place switching) is deferred, with the reason and the exact remaining
refactor written into the spec. Its foundations are here and tested
(ProfileScopedPrefs, verified byte-identical to the setPrefix keys it replaces),
but adopting them across WorkspaceController's 20 files could not be done safely
in one pass, and a partial adoption would switch the server while still showing
another profile's panes -- a subtler failure than not switching at all. The badge
therefore names the active profile (it used to hide it) but does not yet change
it.
Also: the pair QR carries optional &n= and &id= so a phone can label each server
instead of showing a bare IP, byte-identical when absent; Install CLI moved to
General.
app 2256 tests pass, 0 real failures (loading-stage flakes are pre-existing and
random); analyze clean under --fatal-infos; format clean.
server 1283 tests pass, tsc clean.
Brings the profiles work in from the isolated worktree it was built in. See 5e38116 for the full rationale.
Prefs (SPEC-50 D11). ServerConfigController and GroupsController now take a ScopedPrefs instead of SharedPreferences, and desktop_app no longer calls SharedPreferences.setPrefix -- which throws once getInstance() has run and so blocked in-place switching outright. Keys are composed by us and compose byte-identically, so this is a no-op rather than a migration. Appearance, shortcuts, recent models and cached commands become shared across profiles, which is intended: the old blanket prefix is why a worktree build opened with a default theme and empty shortcuts. Correction to the deferral rationale: WorkspaceController holds NO preferences (its only mention of SharedPreferences is a doc comment); pane layouts persist through GroupsController. My earlier '20 files / 154 references' figure counted files that merely mention the class, so D10 was materially cheaper than claimed. UI, found by rendering it. Built tool/profiles_demo.dart and captured the real macOS window with cua-driver: stale profiles appeared TWICE, once as ordinary rows and once in the stale group. On this machine that is 5 of 9 rows -- and 27 of 33 on a real one -- so dead profiles crowd the live ones off the screen. The main list now excludes them; the stale group is their only home. tool/profiles_demo.dart renders both surfaces from in-memory fakes with every delete refused, so a design review cannot touch real data. app 1108 tests pass in the touched trees, 0 real failures; analyze clean.
The last gap. Picking a profile from the title-bar pill now confirms, verifies the target, and hands the window over -- no relaunch. ProfileRuntime holds the whole per-profile object graph (control client, daemon controller, scoped prefs controllers, lifecycle, deleter) behind one disposable, so a second one can exist. Switching is a key change on the ProviderScope, which makes Riverpod dispose the entire old container -- there is no hand-written teardown list to forget an entry later. Dispose cancels the poll timer before closing the client, because a poll firing against a closed client throws. Order is the point: the target is started and confirmed answering WHILE the current profile is still live. If it cannot come up, nothing changes and the reason is reported. Success and failure both surface through StatusCenter, so a switch can never silently not happen. The tray was the trap. It closed over one DesktopController, so after a switch the menubar would drive a disposed object; it now reads through a holder and its listener is re-attached on each switch. The pill became the switcher and degrades gracefully: many surfaces (and most widget tests) mount the badge with no profile wiring, so both new providers default to null and the badge falls back to a calm label rather than crashing -- which is also the honest UI when there is nothing to switch to. That kept ~50 sidebar tests untouched. 'Switch away & delete' is now a real action instead of a disabled item with an apology. Last-active profile persists in profiles.json, honoured ONLY for the installed app: a dev build always opens its own profile, or building a worktree would silently reopen Work and look like the build did nothing. The spec records the correction rather than hiding it: I deferred D10 claiming WorkspaceController needed a 20-file refactor, but it holds no preferences at all -- I had counted files that merely mention the class. app 2195 pass, 0 real failures; analyze clean under --fatal-infos; format clean. server 1283 pass, tsc clean.
…witch Two defects in the previous commit, both found by asking whether it was really finished. 'Switch away & delete' did not switch. I enabled the menu item but left it calling the delete sheet directly, so for the ACTIVE profile ProfileDeleter refused it every time -- an honest disabled item replaced by a broken promise. It now picks a landing profile (preferring the protected one, which always exists), confirms both consequences in ONE sheet because it is one intent, and hands the delete to the host: the widget offering it cannot do the work, because the ProviderScope it lives in is disposed by the switch. The host survives that rebuild and deletes through the NEW runtime's deleter, which correctly sees the old profile as inactive. The switch sequence had no tests and had never executed. Extracted it as verifyThenHandOver, where the irreversible half is injected, so the property that matters is assertable: does it hand the window over at all? Eight tests now cover a running target (no spawn), a stopped one (start then hand over), a target that refuses to start, and -- the subtler case -- one where makit: already running makit: running pid 82738 listening 100.119.58.97:7808 fingerprint d3e73b15ac380e583d588c5f8d7fa1b8a097d10c853cdbd34ecc4f890f088651 paired 8 device(s) sessions 3 running uptime 138373s version 0.0.0 exits 0 but nothing is listening, where trusting the exit code would hand the window to a dead server. All three guards bite under mutation (1, 1 and 2 failures). That test also caught a weak message: with empty CLI output the failure read 'makit start exited 1' and never named the profile, which is useless where the detail is surfaced alone. It always names the profile now. ProfilesController gains notifyRegistryChanged, because the host mutates the shared registry directly and notifyListeners is protected to subclasses. app 2278 pass, 0 real failures; analyze clean under --fatal-infos; format clean.
…mment Both findings from an open-code-review pass over the three unreviewed commits. The switch sheets had no test file. confirmProfileSwitch was exercised only incidentally through the badge, and confirmSwitchAwayAndDelete -- which gates a delete that erases a profile's database, media, pairings and TLS identity -- had no coverage at all. Twelve tests now cover both: that they name the target, that they carry BOTH halves of the consequence, that a start is promised only for a stopped target, and that Cancel and outside-tap dismissal each return false. That last one matters most: a dismissed dialog pops null, and reading null as consent would switch or delete on a stray click. Mutations bite -- 'result ?? true' fails 2, always-promising-a-start fails 1, renaming the kept block fails 1. The demo harness comment was wrong about its own safety mechanism. It claimed every delete is refused because everything is 'active', but activeProfileId 'ALL-REFUSED' matches nothing: a namespaced profile like 'personal' would pass both that check and isProtected. The real guard is homeDir '/nonexistent', which makes _unsafeHomeReason reject every seeded home. The comment now says so, since a guard nobody understands is one somebody will remove. Note the review itself was PARTIAL: 7 of 21 files failed in the run that produced these findings (19 of 21 in a first attempt), so this is not a clean bill of health for the diff -- only for what was actually read. app 2272 pass, 0 real failures; analyze clean under --fatal-infos; format clean.
Contributor
📝 WalkthroughWalkthroughThis PR adds persistent server profiles with profile-scoped preferences, independent daemon lifecycles, profile switching, safe deletion, profile settings, reachability configuration, and pairing URL metadata. It also adds tests, specifications, mockups, and a profile design harness. ChangesProfile management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProfilesSection
participant ProfileRegistry
participant ProfileRuntime
participant ProfileLifecycle
participant ProviderScope
User->>ProfilesSection: Select profile
ProfilesSection->>ProfileRegistry: Resolve target profile
ProfilesSection->>ProfileRuntime: Verify target handover
ProfileRuntime->>ProfileLifecycle: Start and probe target daemon
ProfileLifecycle-->>ProfileRuntime: Running status
ProfileRuntime-->>ProfilesSection: Handover result
ProfilesSection->>ProviderScope: Replace profile-scoped providers
ProviderScope-->>User: Show active profile
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6a12a6d2-1310-40aa-b166-980334b13921) |
Owner
Author
|
@macroscope-app review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Review started! I'm analyzing the changes now. Results will be posted as check runs when complete. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 15 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Correctness fixes from the PR #162 review, each with a regression test: - server_profile: reject out-of-range persisted ports (>65535) so a hand-edited profiles.json falls back instead of silently failing to bind. - profile_registry: serialise save()'s read-merge-write under an inter-process advisory lock so two instances can't drop each other's new profile; preserve a newer on-disk lastActive unless this instance set it. - profile_deleter: guard diskUsage() with a containment check (a corrupt home:"/" no longer walks the whole disk) and make the size walk resilient to unreadable dirs; make delete() best-effort so a store failure is recorded and always yields a result instead of a partial delete. - profile_lifecycle: stopAndConfirm now waits for the daemon PID to exit, not just the control socket, closing the window where MAKIT_HOME could be deleted under a still-live daemon writing makit.db-wal. - reconnecting_control_client: close() awaits an in-flight connect so it can't install and leak a live socket after teardown. - desktop_app: serialise profile switches at switchTo() so two interleaved switches can't leave the window, persistence and title disagreeing. - profiles_section: omit/disable Start-Stop for the active profile (never stop the daemon this window talks to); add profiles.* deep-link anchors. - profile_delete_sheet / reclaim_sheet: stop claiming prefs are deleted when they're skipped; wrap deletions in try/catch so one failure can't abort the flow. - server_config / server_devices_section: setReachability clears customHost so the choice takes effect; the Address line reflects the effective bind rather than the client's loopback connection. - server_devices_section: auto-expand Diagnostics when a deep-link targets one of its (otherwise unmounted) rows.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b56f9adf-d398-4874-ac92-e717ed2429e3) |
Nine more review findings, each with a regression test:
- server_profile: reject a relative `home` (must be an absolute MAKIT_HOME so
spawned CLIs don't resolve it against their cwd).
- profile_registry: harden multi-instance save()/load() —
- honour deletions across windows via persisted `deletedIds` tombstones so a
stale window can't resurrect a profile whose stores are already erased;
- only override an on-disk profile this instance actually modified
(tracked like `_deleted`), so an unrelated save no longer reverts another
window's rename/port/origin edit;
- reconcile duplicate ports after the merge (and on load), so two instances
that independently allocate the same free port — or a hand-edited/fallback
7777 that collides with the legacy profile — don't hit EADDRINUSE;
- drop a second `legacy` profile on load (D2: at most one may own the
unprefixed prefs keys and unsuffixed secure store).
- profile_lifecycle: stopAndConfirm returns false if `stop()` itself failed
(CLI missing / command failed) instead of proceeding as if shut down.
- profile_deleter (Critical): resolve symlinks before the containment check so
a symlinked ancestor (e.g. ~/.makit/profiles -> external) can't route a
recursive delete outside ~/.makit*; both sides are resolved to avoid false
positives on symlinked temp roots.
- profiles_section: the active profile's danger-zone button now runs
switch-away-&-delete (it was permanently disabled despite its label).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_71c98d69-e19d-43c4-b686-27df8a12fbd9) |
# Conflicts: # .gitignore
- profile_registry: _uniqueId now avoids tombstoned ids too. Reusing a just- deleted slug made save() drop the new profile (it honours the tombstone) and orphan its home; minting a fresh id keeps it persistable. - profiles_controller / profiles_section: keep an active profile in the main list even when it is stale, and exclude it from the reclaim group (whose deleter refuses the active profile). Otherwise a stale active profile had no reachable delete at all; now its row's switch-away-&-delete works. - profile_deleter_live_test: moved from app/lib/ to app/test/ so `flutter test` (which only scans test/) actually runs the real-filesystem deletion-safety checks (symlink escape, traversal, legacy-home protection) in CI. - profile_lifecycle_test: inject statusProbe in the socket-disappearance test so isRunning is genuinely driven by socketExists and the polling path is exercised, instead of short-circuiting on the default probe. Regression tests added for the tombstoned-id reuse and the active-stale-profile cases.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d6078cc1-22ed-4cf0-8ad1-dec4512b0e53) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/desktop/settings/server_config.dart (1)
250-260: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPersist the current-schema discriminator in both setters.
On a fresh profile,
setAllowLanFallbackandsetCustomHostdo not write_kReachabilityKey.loadthen treats the stored fallback or custom-host value as absent after restart. LAN fallback resets tofalse, and the custom host is ignored.Write
state.reachability.nameto_kReachabilityKeyin both setters. Add reload tests for each setter before the production fix.Proposed fix
Future<void> setAllowLanFallback(bool allow) async { state = state.copyWith(allowLanFallback: allow); + await _prefs.setString(_kReachabilityKey, state.reachability.name); await _prefs.setBool(_kAllowLanFallbackKey, allow); } Future<void> setCustomHost(String host) async { final h = host.trim(); state = state.copyWith(customHost: h); + await _prefs.setString(_kReachabilityKey, state.reachability.name); await _prefs.setString(_kCustomHostKey, h); }As per coding guidelines: “Use test-driven development: write a failing test before production logic” and “Never leave a verified bug unfixed.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/desktop/settings/server_config.dart` around lines 250 - 260, Update tests first to verify that calling setAllowLanFallback and setCustomHost persists the current reachability discriminator and that a reload restores each setting. Then update both setters to write state.reachability.name to _kReachabilityKey alongside their existing preference writes, preserving the current state updates and persistence behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/desktop/chat/server_profile_badge.dart`:
- Around line 44-68: Update the row filtering in the server profile badge to
retain the active profile even when its row is stale, while continuing to
exclude other stale profiles. Base the exception on the current profile identity
used by _MenuRow and _switch, so the active profile remains listed and
rows.length > 1 correctly enables switching to another profile.
In `@app/lib/desktop/daemon/daemon_lifecycle.dart`:
- Around line 136-145: Extract the duplicated _failureMessage helper from
app/lib/desktop/daemon/daemon_lifecycle.dart lines 136-145 into a shared library
in the same directory and export it. Remove the duplicate definition from
app/lib/desktop/daemon/profile_lifecycle.dart lines 46-55 and import the shared
helper there, preserving the existing behavior and call sites.
In `@app/lib/desktop/daemon/profile_deleter.dart`:
- Around line 346-349: Remove the stale first paragraph from the doc comment
above _unsafeHomeReason, including the incorrect boolean-contract description,
and retain only documentation consistent with its reason-string-or-null return
value.
In `@app/lib/desktop/daemon/profile_lifecycle.dart`:
- Around line 210-220: Convert _posixProcessAlive from synchronous to
asynchronous by using Process.run and returning Future<bool>, preserving the
Windows and ProcessException false results. Update the injected _processAlive
callback and all stopAndConfirm call sites to await Future<bool> checks,
including the polling loop, so profile deletion never blocks the UI isolate.
In `@app/lib/desktop/daemon/profile_registry_perms_test.dart`:
- Around line 21-26: Replace the subprocess-based implementation of modeOf with
a portable Dart filesystem API that reads the permission bits for the given path
and returns the mode as the expected octal string. Remove the dependency on
/usr/bin/stat and preserve the existing callers’ mode assertion format.
In `@app/lib/desktop/daemon/profile_registry.dart`:
- Around line 597-622: Update withLock so FileSystemException handling covers
only lock-file creation, opening, and locking, not body(). Acquire the lock in a
separate guarded section, then invoke body exactly once afterward; if
acquisition fails, continue unlocked, while propagating any FileSystemException
raised by body without retrying it.
- Around line 648-671: Update writeAtomic to wrap the temporary-file write,
chmod, and rename operations in failure cleanup that deletes the tmp file when
any step throws, while preserving the original failure propagation. Use the
existing tmp File instance and avoid affecting successfully renamed files;
_chmod should remain best-effort as currently defined.
In `@app/lib/desktop/daemon/profile_runtime.dart`:
- Around line 189-196: Update ProfileRuntime.dispose to explicitly dispose the
externally-created profilesController before completing teardown, while
preserving the existing controller.dispose-before-client.close ordering. Ensure
the same profilesController exposed by profilesControllerProvider and
switcherProfilesProvider is disposed exactly once.
In `@app/lib/desktop/daemon/profiles_controller_test.dart`:
- Around line 1-6: Move the ProfilesController unit test containing
ProfilesController, ProfileRegistry, and ServerProfile coverage from the lib
location into the app/test/desktop/daemon test tree so flutter test --coverage
discovers it, then remove the now-unnecessary depend_on_referenced_packages
ignore directive.
In `@app/lib/desktop/daemon/profiles_controller.dart`:
- Around line 141-149: Make ProfilesController.refresh isolate failures from
each RunningProbe and DiskProbe invocation, continue probing all profiles, and
always notify listeners. In app/lib/desktop/daemon/profiles_controller_test.dart
lines 105-135, first add a failing test with a throwing DiskProbe that verifies
later profiles are probed and listeners are notified. In
app/lib/desktop/settings/sections/profile_delete_sheet.dart lines 58-61, prevent
refresh failure from being reported as deletion failure; in
app/lib/desktop/settings/sections/profile_reclaim_sheet.dart line 59, guard the
refresh call so bulk-delete results are still reported.
In `@app/lib/desktop/daemon/server_profile.dart`:
- Around line 197-222: Remove the unused ProfileRuntime prefsPrefix getter after
updating its equivalence test in profile_registry_test.dart to validate
prefsKeyPrefix directly or removing the obsolete assertion. Revise
prefsKeyPrefix documentation to describe its active production role, and update
any remaining references so ProfileRuntime.create continues using prefsKeyPrefix
without relying on prefsPrefix.
In `@app/lib/desktop/desktop_app.dart`:
- Around line 479-517: Update _switchTo to return a result type or record with
separate switched status and deleteFailure fields instead of encoding delete
errors as a switch failure. Preserve switched=true after the handover succeeds,
populate deleteFailure only when deleting deleteAfter fails, and update callers
such as ServerProfileBadge._switch to show the successful switch and separate
deletion warning.
- Around line 382-399: Update attachTray to track the exact controller
associated with _trayListener when the listener is attached, and remove the
previous listener from that stored controller rather than runtime.controller.
Refresh the stored controller reference whenever attaching the new listener,
while preserving the existing best-effort disposal handling and tray updates.
In `@app/lib/desktop/settings/sections/profile_switch_sheet.dart`:
- Around line 64-96: Wrap the dialog body Columns in both _SwitchSheet and
confirmSwitchAwayAndDelete with SingleChildScrollView, following the existing
pattern in profile_delete_sheet.dart. Preserve the current SizedBox width,
spacing, content, and layout alignment while allowing the bodies to scroll when
text scaling exceeds the available height.
In `@app/lib/desktop/settings/sections/profiles_section.dart`:
- Around line 707-715: Update the candidate sort comparator in the profiles
section to use a derived protected-status key, ensuring candidates with equal
protection status compare as equal and retain deterministic ordering. Preserve
protected candidates before unprotected candidates.
- Around line 265-267: Update the overflow menu documentation above the relevant
class to match the shipped behavior: delete is absent for protected profiles,
while the active profile has an enabled delete item that invokes
switchAwayAndDelete. Remove the stale claims that it is disabled and that
switching is not wired yet.
In `@app/lib/desktop/settings/sections/server_devices_section.dart`:
- Around line 228-236: Serialize daemon lifecycle operations at the shared
DesktopController lifecycle boundary, covering start, stop, and restart so
concurrent actions cannot interleave PID/socket updates or daemon processes.
Ensure Reachability, fallback, Host, Port, Restart, and Stop controls use this
guarded path, either queuing operations or rejecting overlapping requests
consistently, and add a regression test exercising overlapping actions.
In `@app/test/desktop/daemon/profile_deleter_live_test.dart`:
- Around line 240-246: The test comments in
app/test/desktop/daemon/profile_deleter_live_test.dart at lines 240-246 and
414-419 misidentify the guards responsible for refusal. Update the first comment
to reference _unsafeHomeReason rule 2, requiring at least one path segment below
~/.makit/, and update the second to reference rule 3, which refuses entries
claiming the shared home; make no production-code changes.
- Around line 531-559: Update the symlinked-home test around
build(profile).deleter.delete(profile) to assert the deletion is refused, using
the deleter’s existing outcome or error contract tied to _unsafeHomeReason. Keep
the existing keepme.txt survival assertion, and verify both the refusal result
and preservation of the outside data.
In `@app/test/desktop/settings/server_devices_section_test.dart`:
- Around line 429-443: Update the `_pump` helper to accept an optional
`NavigatorObserver`, forwarding it to `MaterialApp`. Replace this test’s
duplicated provider scope, viewport configuration, and initial pump with
`_pump(observer: observer)`, while retaining the observer reset and existing
assertions; do not add a `serverProfileProvider` override.
In `@app/tool/profiles_demo.dart`:
- Around line 94-99: Override FileSystemAdapter.withLock in _NoWriteFs with a
no-op implementation so save() cannot create lock files or write to disk,
preserving the adapter’s no-write guarantee while leaving readOrNull and
writeAtomic unchanged.
In `@docs/specs/2026-08-10-SPEC-50-profiles.md`:
- Line 179: Update the verification step in the documented validation checklist
to replace the direct node_modules/.bin/tsc invocation with the repository’s
pnpm typecheck command, while preserving the existing pnpm test requirement and
pre-existing count expectation.
- Line 149: Update the P4 Pair-URL params (D12) entry in the profiles
specification to reference delta 19 instead of delta 18, matching the mockup
delta table.
- Around line 136-139: Update D12 to document that the optional n profile name
is limited to 64 Unicode code points and values exceeding that limit are
silently truncated, matching MAX_PROFILE_NAME_CODE_POINTS in pairing/url.ts.
In `@mockups/server-settings-and-profiles.html`:
- Around line 795-798: Correct the two stale statements in the “Explicitly not
changed” section: update the per-profile preferences statement to reflect that
SharedPreferences.setPrefix(profile.prefsPrefix) is no longer called and the
legacy prefix behavior is not preserved, and change the pair-URL reference in
the wire-protocol statement from `#15` to the correct delta row `#19`.
---
Outside diff comments:
In `@app/lib/desktop/settings/server_config.dart`:
- Around line 250-260: Update tests first to verify that calling
setAllowLanFallback and setCustomHost persists the current reachability
discriminator and that a reload restores each setting. Then update both setters
to write state.reachability.name to _kReachabilityKey alongside their existing
preference writes, preserving the current state updates and persistence
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a93fd852-1fad-43eb-b77a-a68e5861c6ed
📒 Files selected for processing (52)
.gitignoreapp/integration_test/desktop/control_e2e_test.dartapp/integration_test/desktop/settings_repo_test.dartapp/lib/control/reconnecting_control_client.dartapp/lib/control/reconnecting_control_client_test.dartapp/lib/desktop/chat/groups/groups_controller.dartapp/lib/desktop/chat/server_profile_badge.dartapp/lib/desktop/daemon/daemon_lifecycle.dartapp/lib/desktop/daemon/daemon_lifecycle_test.dartapp/lib/desktop/daemon/profile_deleter.dartapp/lib/desktop/daemon/profile_deleter_test.dartapp/lib/desktop/daemon/profile_lifecycle.dartapp/lib/desktop/daemon/profile_lifecycle_test.dartapp/lib/desktop/daemon/profile_registry.dartapp/lib/desktop/daemon/profile_registry_perms_test.dartapp/lib/desktop/daemon/profile_registry_test.dartapp/lib/desktop/daemon/profile_runtime.dartapp/lib/desktop/daemon/profile_runtime_test.dartapp/lib/desktop/daemon/profiles_controller.dartapp/lib/desktop/daemon/profiles_controller_test.dartapp/lib/desktop/daemon/server_profile.dartapp/lib/desktop/daemon/server_profile_paths.dartapp/lib/desktop/daemon/server_profile_test.dartapp/lib/desktop/desktop_app.dartapp/lib/desktop/settings/registry/settings_registry.dartapp/lib/desktop/settings/sections/general_section.dartapp/lib/desktop/settings/sections/profile_delete_sheet.dartapp/lib/desktop/settings/sections/profile_reclaim_sheet.dartapp/lib/desktop/settings/sections/profile_switch_sheet.dartapp/lib/desktop/settings/sections/profiles_format.dartapp/lib/desktop/settings/sections/profiles_providers.dartapp/lib/desktop/settings/sections/profiles_section.dartapp/lib/desktop/settings/sections/server_devices_section.dartapp/lib/desktop/settings/server_config.dartapp/lib/store/prefs/profile_scoped_prefs.dartapp/test/desktop/chat/groups/groups_controller_test.dartapp/test/desktop/chat/server_profile_badge_test.dartapp/test/desktop/daemon/profile_deleter_live_test.dartapp/test/desktop/server_config_test.dartapp/test/desktop/server_control_integration_test.dartapp/test/desktop/settings/general_section_test.dartapp/test/desktop/settings/profile_switch_sheet_test.dartapp/test/desktop/settings/profiles_section_test.dartapp/test/desktop/settings/server_devices_section_test.dartapp/test/desktop/settings/settings_registry_test.dartapp/test/desktop/settings/settings_window_test.dartapp/test/store/prefs/profile_scoped_prefs_test.dartapp/tool/profiles_demo.dartdocs/specs/2026-08-10-SPEC-50-profiles.mdmockups/server-settings-and-profiles.htmlserver/src/pairing/url.test.tsserver/src/pairing/url.ts
💤 Files with no reviewable changes (1)
- app/lib/desktop/daemon/server_profile_test.dart
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b3ebf7ff-658f-4d02-a0c0-fe59729682ae) |
Completes the CodeRabbit/Macroscope round begun in 288b7b2. - desktop_controller: serialize start/stop/restart. They share the daemon's PID file and control socket (and restart is a stop+start), so overlapping requests could drop a fresh PID, launch a second daemon, or stop one another action had just started. Each action now chains after the previous, with a regression test asserting they never overlap. - profile_lifecycle: keep the pid-file read synchronous. Making it async added real filesystem microtasks that a widget test's pumpAndSettle can never settle, which hung the delete tests. Only `processAlive` (polled ~100x per stop) needed to be async, and it stays async. - test doubles: override `withLock` in the two "writes nothing" FileSystemAdapter fakes. They inherited the real implementation, which created `<path>.lock` on the real disk under a non-existent /Users/test — the actual cause of the hang. - profiles_section_test: replace `await Future.delayed(...)` with `tester.pump(...)`; inside testWidgets the clock only advances when pumping, so the bare delay deadlocked. - profile_registry: keep the already-persisted profile's port when reconciling a post-merge collision (its daemon may be running on it) and reassign the newcomer instead; test asserts which side moves. - profile_runtime_test: assert dispose() disposes profilesController. - server_devices_section_test: reuse `_pump` via an optional NavigatorObserver. - docs/mockup: record the 64-code-point cap on the pair-URL `n` param, fix the delta cross-references (18→19, 17→18), use `pnpm typecheck` as the documented server check, and correct two stale "explicitly not changed" claims.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_54e2e523-d4ac-4618-a665-2798f09fe466) |
Owner
Author
|
✅ #28 (B, High): CLI path per-profile via MakitCliResolver.overridePath + ProfileLifecycle wiring. ✅ #29 (A, High): Prefs store purged via ProfileScopedPrefs.clearScope() hook in ProfileDeleter—fixes 'prefs left behind' for all non-active deletes. ✅ #30 (C, Medium): home-at-file case uses deleteFile instead of deleteDirectory. ✅ #31 (D, Medium): Guard promptCreateProfile against controller.create throw. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_eae84129-8281-420b-a6ea-d5207c8014a6) |
Four more review findings: - profile_lifecycle / profile_runtime: lifecycle actions target arbitrary profiles, but the CLI path and endpoint args came from the ACTIVE profile's config. Starting profile B from A's window therefore used A's configured binary and, since `start` passed no `--port`, launched B's daemon on the CLI default (7777) — colliding with the legacy daemon or landing on an endpoint B's own ServerConfig disagreed with. ProfileLifecycle now takes `cliPathFor` and `serveArgsFor`, wired to the target's scoped ServerConfig, and MakitCliResolver.resolve() accepts a per-call override. - profile_deleter: actually purge the profile's preference keys (store 3). The unconditional skip was based on the obsolete `SharedPreferences.setPrefix` assumption; prefs are now scoped by key prefix, so `ProfileScopedPrefs .clearScope()` can purge a non-active profile. Injected as `purgePrefs`, so contexts with no prefs still report the store honestly. The delete sheet lists prefs under "Will be deleted" again, and its caveat note is gone. - profile_deleter: a regular file at `home` is no longer reported as removed while `deleteDirectory` silently no-ops on it — `ProfileFileSystem` gains `isDirectory`, and the file case is erased with `deleteFile` and named in the result. - profiles_section: `promptCreateProfile` catches a throwing `create` (unwritable registry, failed port allocation) and reports it, instead of leaking an unhandled async error with no user feedback.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_94606996-7727-47c6-958e-178a315d7b23) |
Owner
Author
|
✅ Found in PRRT_kwDOTNcrJs6YZbAo: |
`promptRenameProfile` called `controller.rename` without catching persistence failures. `rename` mutates the registry's in-memory list and then saves, so an unwritable registry left the row showing the new name for the rest of the session while the change was silently lost on restart — and the exception escaped as an unhandled async error with no user feedback. It now reports the failure and reverts the in-memory name (without saving) so the row shows what is actually persisted. Regression test drives Rename through a registry whose writes throw and asserts both the failure event and the revert.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a399a96a-96fb-4c5e-a9ca-d05057fba625) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements SPEC-50 — Profiles. A profile becomes a first-class, named, persisted noun (its own
MAKIT_HOME, daemon, port, devices, projects) instead of invisible plumbing derived from the running.app's filesystem path.app/lib/desktop/daemon/): profiles are chosen and persisted, not derived.~/.makitbecomes a named, renameable profile;isDefaultis split intoname(mutable) +storage(legacy/namespaced, never mutable).app/lib/store/prefs/profile_scoped_prefs.dart): preferences no longer leak across profiles.app/lib/desktop/settings/sections/): full UI for the four operations —server_devices_section.dart,general_section.dart,server_config.dart): the ten-control panel is cut down to model the user's purpose rather than the daemon's configuration.server/src/pairing/url.ts): two optional query params to support profile-aware pairing. No wire-protocol change.Why
Two complaints with one root cause: the Server panel exposed ten controls to do one thing (run the server), and profiles existed only as invisible, non-persisted plumbing that broke when a worktree moved and could never be named, stopped, or discarded. On the author's machine, 27 of 33 dev profiles (82%) were orphaned and unreachable by any UI, each still holding a device pairing and TLS keypair. The fix is to model the profile as the missing noun.
No change to: the wire protocol (frames,
v:1, bearer auth),MAKIT_HOMEas the isolation boundary,chooseBindHost(), the daemon spawn path, or any server storage module.How tested
app/test/**,app/lib/desktop/daemon/*_test.dart).server/src/pairing/url.test.tsfor the new pairing query params.dart format,flutter analyze, TypeScript typecheck.app/tool/profiles_demo.dartadded for manual UI verification.See
docs/specs/2026-08-10-SPEC-50-profiles.mdandmockups/server-settings-and-profiles.htmlfor the design ground truth.Note
High Risk
Major architectural change to profile identity, prefs scoping, daemon lifecycle, and destructive multi-store deletion (including path/symlink safety). Also reshapes Server config persistence and pairing URL metadata.
Overview
Turns path-derived server isolation into first-class, persisted profiles — each with its own
MAKIT_HOME, port, prefs scope, and daemon — so worktrees no longer orphan homes when moved, and users can name, switch, and delete them.Profile core. Adds
ProfileRegistry(profiles.jsonwith merge/lock/tombstones),ProfileRuntime(swappable per-profile object graph),ProfileLifecycle, andProfileDeleter(four-store erase with path/symlink safety guards).ServerProfileidentity is persisted rather than re-derived;isDefaultsplits into mutablename+ frozenstorage(legacy/namespaced). In-window switching verifies the target is reachable before tearing down the old runtime.Settings & prefs. New Profiles section covers create, rename, start/stop, delete, switch-away-and-delete, and stale reclaim. Title-bar badge becomes a switcher. Server panel collapses to Active Profile / Reachability / Pair / Diagnostics;
ServerBindModebecomes two-valueReachabilityplusallowLanFallback. CLI install moves to General.ProfileScopedPrefsreplaces globalsetPrefixso only server-bound prefs are namespaced and switching works in-process.Also. Pairing URLs optionally carry
n/id; daemon errors read stdout (wheremakit startreports failures);close()awaits in-flight connects; lifecycle actions are serialized.Reviewed by Cursor Bugbot for commit a6bcb12. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add per-profile server management with a Profiles settings section and restructured Server settings panel
ServerProfilewithProfileRegistryfor persisted, multi-profile server management; each profile gets its own scoped prefs, control socket, lifecycle, and config viaProfileRuntime.ServerBindModewith a two-valueReachabilityenum (thisMacOnly/myDevices) plus a separateallowLanFallbackflag; settings apply immediately and trigger a daemon restart without an explicit Save button.ProfileDeleter.ServerProfileBadgenow always renders and opens a switcher menu when multiple profiles exist; switches are confirmed before executing and failures are reported to the status center.ReconnectingControlClient.close()to await in-flight connections before disposal, preventing socket leaks across profile switches.ServerConfigload migrates legacybindModeandhostkeys to the new schema on first load; previously persistedauto/lan/loopback/customvalues are mapped to the new enum and may not round-trip back to the old format.Macroscope summarized a6bcb12.
Summary by CodeRabbit
New Features
Bug Fixes