fix(gui): open minimized honours the setting - #49
Merged
Conversation
This was referenced Aug 21, 2026
The "Open minimized" preference inferred a login launch from NSApplication.launchIsDefaultUserInfoKey, which was its only input and read wrong in both directions: the window appeared at login with the setting on, and stayed hidden on a manual launch. MainWindow also left NSWindow.isRestorable at its default true, so AppKit state restoration could reopen the window without consulting the setting at all. Register a LaunchAgent shipped in the bundle instead of SMAppService.mainApp. Its ProgramArguments end in --background, so CommandLine.arguments is ground truth rather than a guess, and LaunchVisibility takes backgroundLaunch: instead of deliberateLaunch:. Existing installs migrate once on launch, gated on the old registration having been enabled so an upgrade never switches login-at-launch on for someone who had it off. build-app.sh fails the build if the plist is missing: a bundle without it registers nothing and silently stops starting at login. Refs docs/adr/0014-login-item-launch-marker.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the launch-marker change found the whole registration lifecycle untested, and three real defects in it. Registering a RunAtLoad launchd agent execs the binary immediately, and launchd — unlike the LaunchServices login item this replaced — does not care that the app is already running. Both callers of register() run while the app is up, so turning on "Open this app at login", or upgrading with it already on, left two menubar items, two Dock tiles and two state-file timers; the duplicate carries --background, so under the default "Only at login" it opened no window and there was no way to tell the icons apart. RunAtLoad has to stay true or the login launch never happens, so main.swift now exits a duplicate before the delegate installs anything. The rule is a total order over (launch date, pid) in DezhbanCore rather than a "does anyone else exist" test: two copies that each saw the other and each stood down would leave the Mac with no app. The migration inferred "already migrated" from a live mainApp.status read, which is only truthful when the unregister succeeded — and it can fail for real. It then re-ran on every launch, re-registering the agent after the user had switched login-at-launch off in Settings, with no way to turn it off again. It is now gated on a persisted flag and runs at most once per account. When the legacy item survives the attempt the agent is deliberately left unregistered: two launches at login, one with the marker and one without, is worse than the behaviour it replaces. isEnabled reports either registration, so the toggle tells the truth about whether anything starts the app at login, and switching it off retracts both. Uninstall left the agent registered. A LaunchServices login item goes away with its bundle; a per-user launchd job does not, and an orphan fails to load at every subsequent login. uninstall.sh boots it out for the console user and prints the one command other accounts need. Also: the build guard only restated what `install` under `set -e` had just proven, while the two invariants the comments called un-driftable went unchecked. It now asserts them — Label equals the filename SMAppService is given, and ProgramArguments still carry --background — both verified to fail the build when broken. And the plist declares AssociatedBundleIdentifiers, so the System Settings entry a user reaches for to stop the app starting at login reads "Dezhban" rather than a raw job label. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round found the duplicate-instance guard unsound, and it was: comparing launch dates cannot work here, because only a newly started process ever evaluates the question. The copy already serving the menubar never re-decides anything, so any rule under which the newcomer might think it wins leaves both running, and any rule under which an undatable process yields can retire both and leave the Mac with no app. NSRunningApplication.launchDate is documented as optional, so both were reachable, and the test I wrote pinned the wrong direction. Replaced with an exclusive flock: exactly one open file description holds it, and the kernel drops it when that process dies however it dies, so a crashed predecessor cannot lock its successor out. Keyed on the bundle path, not its identifier, so dist/Dezhban.app run against an installed copy — the documented GUI dev loop — is not treated as a duplicate and silently exited, which would have made every manual check on the list test the installed binary instead. A launch the user performed must not become a silent no-op either. The copy that loses the lock now focuses the winner and posts a distributed notification asking it to open its window, since the incumbent may be a --background login launch with no window to hand over. A notification rather than re-opening the bundle through NSWorkspace, which could spawn yet another copy that finds the lock held and asks again. launchctl bootout did not retract the registration it claimed to. It unloads the job for the current boot and leaves the record that recreates it at the next login, pointing into a bundle the script then deletes — precisely the orphan it was added to prevent. Only SMAppService can retract it and only the app can call that, so DezhbanMenu takes a --unregister-login-item errand flag, handled before the instance lock, and uninstall.sh runs it as the console user inside their GUI session before deleting anything. The advertised recovery from a stuck legacy login item was unreachable: toggle() branches on isEnabled, which the stuck item holds true, so every attempt took the off branch and could never reach register(). toggle() now returns an Outcome instead of a Bool, naming the one thing that does work — remove the item in System Settings — and also distinguishing "macOS is holding this for your approval", which register() reports as a status rather than an error and which otherwise looked like a switch that refuses to stay on. Also: the label assertion only tied the plist to build-app.sh's own hardcoded copy, so a consistent rename passed every check while SMAppService named a file that does not exist. It now greps LoginItem.swift and uninstall.sh too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third review round, eight findings, all in the registration lifecycle again. .requiresApproval is a live registration — it starts the app the moment the user approves it — but every unregister was guarded on .enabled, so it could not be retracted by the Settings switch or by the uninstaller's errand. The bundle would have been deleted with the registration still on file: exactly the orphan the errand exists to remove. "Is there a registration" and "will this start the app" are now separate questions and the retractions ask the first one. The switch also dead-ended there. Outcome.isOn counted awaiting-approval as on while isEnabled counted it as off, and the binding branches on isEnabled — so the user's attempt to switch it back off re-registered instead, with no way out. The displayed value and the branched-on value are one value now. The migration marked itself done before register() could fail. On an upgrade where the legacy item went away and the agent would not register, that left nothing starting the app at login, no retry, and no word to the user. It now retries, made safe by a second flag: an explicit "off" outlives every retry, so a retry can only restore what was already on. macOS has a second way to start the app at login and it carries no marker. "Reopen windows when logging back in" relaunches through LaunchServices with no arguments, which SMAppService.mainApp was reconciled with and a launchd agent is not — both would start and race for the lock, and a resume copy that won opened the window at login under the default "Only at login". That is this branch's own defect, made intermittent rather than absent. NSApp.disableRelaunchOnLogin() is the API for saying the login item is the only such path. Also: the duplicate hand-off opened a window even under "Open minimized: Always", so a second launch became the one way to make a window appear; it now consults the preference. Its observer was registered after four other calls, and distributed notifications are never queued, so a hand-off arriving during that prologue was dropped — it is the first statement now, and scoped to the bundle path so a duplicate launch of one install cannot open another install's window. The lock moved out of ~/Library/Caches, which macOS may purge from under a held descriptor, leaving the next launch to lock a fresh inode and run a second copy undetectably. And Outcome.failed no longer interpolates a raw SMAppService.Status into text a user reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth review round. The retry added last round was dead code: by the time register() is attempted the legacy item is already confirmed gone, so a retry launch asks "is there a legacy item to migrate?", reads no, marks itself migrated and returns without ever registering again. A user whose register() failed once was left with nothing starting the app at login, permanently — the exact outcome the unmarked flag was introduced to prevent. A flag recorded when the legacy item is confirmed retracted, before the register, is what tells "this account had a login item and the agent is not up yet" from "this account never had one". Two more places the .enabled/.requiresApproval distinction still leaked. disable() decided its outcome from legacyEnabled while isEnabled counts a registration, so a legacy item left at .requiresApproval reported "App will not open at login" and then came back on when the pane was reopened — the pane contradicting itself, which is what unifying those two values was supposed to end. And the toggle's setter threw away the value it was handed and re-derived from a live read, so a stale switch inverted the click: with the Settings window open, remove the login item in System Settings, come back, click the still-ON switch to turn it off, and it turned login-at-launch on. LoginItem.set(enabled:) takes the state the user asked for. The instance lock keyed on an unresolved path while the incumbent match resolved one, so a symlinked install derived two different locks and both copies ran. Both sides resolve now. The test for it is built on a real symlink because resolvingSymlinksInPath() returns the path unchanged when the leaf does not exist — the first version passed for the wrong reason. The uninstaller also left the per-user directory this branch introduced, in the very hunk whose point is not leaving orphans, and ran the retraction errand unbounded and silent before deleting the bundle. Both fixed. One finding is documented rather than fixed: unregistering the agent may have launchd terminate the app, since in a login-started session the app IS the job's process. Routing the agent through /usr/bin/open would avoid it but resolves the bundle by identifier and could launch a different copy than the one that registered — a silent wrong-bundle launch in place of a nuisance. It is an ADR risk with a manual check, to be reopened with measurements rather than guesses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ld about Fifth review round, and the sharpest finding is the migration reading the wrong question. It gated on "is there a legacy registration", and .requiresApproval is exactly what mainApp reports once the user has switched Dezhban off under System Settings — so an upgrade treated a deliberate off as something to carry forward, retracted it, registered the agent, and turned login-at-launch back on behind their back. The userDisabledKey added last round cannot help: a pre-upgrade user never set it. The migration decides on .enabled now; the unregister guards keep asking about presence, because a registration awaiting approval is still one to retract. The migration also ran from any bundle. register() records the plist of the calling bundle while the flag is shared by every copy of the app, so an upgrader who tried the app zip from ~/Downloads before moving it — or a dev running dist/ — pointed the login agent at a bundle about to move and marked the account done forever, reported only as a status nobody reads. It runs only from /Applications now, and does not mark the account otherwise, so the installed copy still does the work. Two more places the switch could lie. enable() would register the agent beside a live legacy item, which disable() and the migration both refuse by name — the stuck path led straight to it: switch reads on, click off, click on, both registered, and the next login is the two-launch race this branch exists to remove. And a failed agent unregister came back as .failed, whose isOn is false, painting the switch OFF while the registration was live and the app kept starting at login; unregister swallows its throw, so that was reachable. It has its own outcome now. The hand-off race was real beyond the ordering fix. The lock is taken before NSApplication exists, so a request posted in the gap before the observer is installed reached nobody — landing exactly at login, when someone impatient double-clicks a slow-starting app. A HandoffRequest file beside the lock now backs the notification, consumed both at observer install and on the tick the app already runs. Requests carry freshness so one the incumbent never got is not inherited by the next app to start and turned into a window nobody asked for. Uninstaller: the per-user directory was removed via a hardcoded /Users/<name>, which misses a network or relocated home and made the closing "files deleted" untrue for those accounts; it asks dscl now. And the errand watchdog leaked a sleep past the end of the script and could fire a kill -9 at a recycled pid, so completion is signalled by a file — an exited-but-unreaped child is a zombie that kill -0 still calls alive, so polling the pid would have waited out the full timeout on every successful run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sixth review round found no high-severity defect — every finding is in the seams around the mechanism, which is the point at which this is worth stopping. The hand-off answered itself twice. The file is written before the notification is posted, so in the normal case the notification handler opened the window and left the file on disk for the backstop to find a second later — and MainWindow.open() activates the app, so that is a second focus steal, or a window reopening just after the user closed it. The notification path discards the file it has just answered. The backstop also ran forever, putting a synchronous stat on the main thread on every tick: the exact hazard pollStateFile was restructured to remove, taken on for a cosmetic feature, and worst on the network and mobile homes uninstall.sh had just been taught to accommodate. The window it covers is a launch-time one — the lock is taken before NSApplication exists — so it now runs for a few seconds after launch and stops, and does its filesystem work off the main thread while it lasts. disable() wrote the user-disabled flag and then called an unregister that this ADR already records may get the process killed by launchd. UserDefaults does not write through synchronously, so losing that write while a migration retry was pending would let the next launch register the agent again — precisely what the flag exists to prevent. Flushed before the unregister. The stable-install check compared a literal string while every other path comparison in this branch resolves symlinks for stated reasons, so an install reached through a symlinked directory never migrated, ever, with a log line as the only symptom. It resolves now, and accepts ~/Applications as well as /Applications — the system-sanctioned per-user equivalent. The uninstaller's timeout killed only the wrapping subshell, leaving the DezhbanMenu it had started alive for `rm -rf "$APP"` to delete the bundle out from under — the thing the pkill above it exists to avoid, reintroduced on the one path the timeout is for. And the ADR shipped in this PR still described a toggle() that derives its direction from isEnabled, which is the control flow this branch replaced with set(enabled:) two commits ago. A new ADR that documents different control flow than the code is the failure the doc rules exist to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventh review round: no high-severity defect, and the two findings that matter are both in machinery this branch added while fixing earlier findings. The de-duplication added last round was itself racy. The notification handler discarded the file while the backstop stat-ed and removed it on a background queue, so a backstop tick that had already read the timestamp still reported success after the discard — and the window opened twice, which is what the discard was added to prevent. Both signals describe one request, so taking it is now a claim: exactly one removeItem can succeed, so exactly one caller is told it owns the request and the other is told it lost. The .lost branch has a test seam rather than a comment claiming it works, since it is the only outcome that arises purely from overlap and it is the whole point of the type. disable() retracted the agent before the legacy item, and the agent unregister is the call ADR-0014 records may get this process killed by launchd. In a login-started session with a stuck legacy item, the app died between the two lines and left the legacy registration in place — still starting the app at login, without the marker, which is the state disable() exists to clear. Legacy first, agent last, for the same reason the flush above them exists. legacyRetractedKey was only ever written by the migration, so retracting through the Settings switch destroyed the fact without recording it and reopened the dead-retry hole from the other side: switch off, switch on, register() fails, and the next launch sees no legacy item and no flag, concludes there was never anything to migrate, and marks the account done with nothing starting the app at login. One helper records it wherever the retraction happens — except the uninstall errand, where recording "the agent still needs registering" would be a lie about an app being deleted. Also: the migration ran synchronously on the main thread during applicationDidFinishLaunching — five blocking XPC round-trips and a register() that forks a process, at a slow login, which is the situation this feature is tuned around. It only affects the next login, so nothing waits on it. Three comments promised the backstop ran on the ordinary 1-second tick, which is a stronger guarantee than the bounded launch-time one it actually has. And the Settings-pane checklist still asserted the toggle sets SMAppService.mainApp to .enabled, which this branch makes false and the new block asserts the opposite of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eighth review round: no high-severity defect, and the findings are the residue of the claim protocol added last round. Refusing to act was the wrong half to tighten. The duplicate writes the file and then posts the notification, so a backstop tick landing between those two calls claims the file and opens the window, and the notification then finds nothing — and standing down on that would turn a hand-off into the silent no-op the whole mechanism exists to prevent, which is worse than a duplicate window. Same for a stale file: freshness exists to stop a dead predecessor's request being inherited, but a notification arriving is itself proof somebody is alive and asking, so the pre-file design would have honoured it. The notification now acts on everything except .lost, the backstop still acts only on .fresh, and the effect is debounced — an open within three seconds of a previous hand-off open is dropped. Debouncing what the user actually notices is cheaper and safer than making two asynchronous signals agree. set(enabled:) ran on the main thread from a SwiftUI setter, and it grew to six-to-eight blocking SMAppService round-trips plus an unregister — the same cost that had the migration moved off-main one commit ago, with the reasoning not carried across. Clicking the login switch could beachball the Settings window, and on the disable path launchd may terminate the app partway through, so the main thread is the last thing to be holding. The switch moves where the user put it immediately and is corrected from the outcome when it lands. Also O_CLOEXEC on the lock file. Foundation's Process spawns with POSIX_SPAWN_CLOEXEC_DEFAULT and every subprocess goes through it, so nothing leaks the descriptor today — but the lock IS that descriptor, a child holding it would lock the app's own successor out permanently with nothing to release it, and resting that on an unstated implementation detail of a framework is not worth the two tokens it costs to fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ninth review round, no high-severity defect, seven low ones — most of them
the two questions this branch keeps having to separate: "is there a
registration" and "will anything actually start the app".
isEnabled reported ON for a legacy item the user had switched off in System
Settings, contradicting the migration's own reading of that exact state
("their off is the answer") and showing a switch that was on while nothing
started the app. .requiresApproval means opposite things for the two
registrations — for the agent, we asked and macOS wants consent; for the
legacy item, which this app never registers any more, it is the signature
of the user's off — so isEnabled now asks about the agent's presence and
the legacy item's enablement.
enable() refused on legacy presence while its justification was "two
launches at login", and a .requiresApproval legacy item launches nothing.
That made Outcome.legacyStuck's own advice a dead end: removing the item
under System Settings leaves mainApp exactly there, so "then switch this on
again" hit the same refusal and the agent could never be registered.
Moving both the migration and the toggle off the main thread let them
interleave: the migration passes its userDisabledKey check, the user
switches login-at-launch off, disable() retracts everything, and the
migration registers the agent — login-at-launch back on right after being
turned off, the single thing that key exists to prevent. Every mutation
goes through one serial queue now.
The debounce added last round swallowed real work. A claim of .fresh means
the caller took a request nobody else had, so it is a distinct launch by
definition: two double-clicks a second apart with the window closed in
between are two requests and both must be answered. Only the ambiguous
signals are debounced now.
The uninstall errand exited 0 whichever way it went, and the script
discards its output — so a login item macOS refused to retract stayed
behind, pointing at a bundle deleted two lines later, while the script
printed "files deleted". The status is checked and the closing message says
what to do. The script also left the app's preference domain, which is not
cosmetic: the migration records that it has run, so a surviving flag means
a later install is never migrated onto the agent, silently restoring the
very defect this branch fixes.
Also seed() still read isEnabled on the main thread, at twice its former
cost, which is the same blocking XPC the last two commits moved off it. And
O_CLOEXEC insurance on the lock descriptor from the same round.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… failure
Tenth review round. The two mediums are both in the uninstaller machinery
added over the last two rounds.
The errand's completion marker was "${TMPDIR:-/tmp}/dezhban-uninstall-errand.$$",
written by root. Under `sudo sh` TMPDIR is frequently unset, so that is a
predictable name in a world-writable directory: an unprivileged local user
can pre-plant a symlink and have the echo write through it as root. The
sticky bit stops them deleting our file, not creating one first, and the
`rm -f` only unlinked their symlink and left the window open to try again.
It goes in a root-owned mktemp -d now.
The other one undid the check it sat next to. On the timeout path no marker
is ever written, so the "failed" test read false and the script printed a
clean "files deleted" while the registration was still on file — the silent
orphan that check exists to prevent, on the one path the timeout is there
for. Same for an app bundle already dragged to the Trash: only the app can
call SMAppService.unregister(), so with it gone the entry cannot be
retracted by anything, ever, and the script said everything was removed.
Three distinct outcomes now, each named in the closing warning.
seed() was moved off the main thread last round and runs on every
didBecomeActive — which macOS delivers *during* a login-item change, since
it surfaces System Settings or an approval prompt. So a read could start
before a click and land after it: click off, completion writes false, stale
read writes true, and the switch reads ON with nothing starting the app at
login. Both paths are stamped with a click revision.
And the notification path accepted a hand-off with no file behind it,
without bound. DistributedNotificationCenter has no sender authentication
and both the name and the object are derivable, so any process running as
this user could activate the app once per debounce interval indefinitely,
reopening a window the moment it was closed. The exemption exists only for
the microsecond between a duplicate writing the file and posting, so it is
now bounded to the launch window where that can happen; outside it, a file
is required.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleventh review round, four findings, all in machinery the previous round added. The mktemp failure branch announced a skip and then did not skip. Control fell through with errand_dir empty, so the marker path was "/done" — the sealed read-only system volume — which can never be written even as root. So the marker never appeared, the poll burned its full 15 seconds, the reason was overwritten from "refused" to "timeout", and the kill -9 then murdered a retraction that had very likely just succeeded. The whole errand block is one if/elif/else now instead of three separate -x tests. The bigger one: gating the fileless notification on the launch backstop contradicted the reason the fileless case was allowed at all, and broke the case this mechanism exists for. A cold login where the incumbent takes longer than 30 seconds to finish starting is exactly the impatient double-click, and there the duplicate's file was written before the observer existed, so the notification found nobody and the file was then discarded for being older than the freshness window. The user's launch did nothing. The fix is to delete the freshness window. It was there so a request its reader never got could not be inherited by the next app to start — but the session owner already discards whatever it finds at the moment it takes the lock, which does that exactly rather than by guessing at an age. With that in place a claimed file was written after this process took the lock by construction, so it is honoured however old it is, and Claim loses its .stale case entirely. The fileless exemption stays bounded to the launch window, which is the only time it can legitimately arise, so last round's unauthenticated-activate channel stays closed. isEnabled did its two status reads outside the mutation queue, so it could observe disable() half-applied — legacy retracted, agent not yet — and report ON. If its main-queue hop landed after the mutation's own completion it overwrote the correct answer, which is the failure the revision stamp was added for and could not close alone. It runs on the queue now, and seed() additionally refuses to write the switch while a mutation is outstanding, since a read started after a click shares that click's revision and passed the equality check. Also the notification handler claimed on the main thread while the backstop deliberately does the identical work off it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rmed Twelfth review round. The medium is the consequence of round nine's own fix, which is worth stating plainly: making isEnabled ask whether the legacy item is *enabled* was right for the switch, and it left the present-but-not-enabled case both invisible and unreachable. That case was deliberately left alone, on the reasoning that the user had switched Dezhban off under System Settings and their off should not be undone behind their back. But .requiresApproval is a live registration, not a dead one. Re-approving the entry later has LaunchServices start the app with no --background, breaking "Open minimized" permanently — the migration is marked done and never runs again. And the escape the comment pointed at did not exist: with isEnabled reading OFF, a click routed to enable(), never to the disable() that would have cleared it. The retraction attempt is unconditional now; whether the item was enabled still decides whether the agent is registered afterwards, so the user's off is honoured either way. Two clicks on the login switch could also settle wrong. Each dispatched its own block to a concurrent queue and those raced into LoginItem's serial queue, so click two could be applied first, leaving the registration in click one's state while the UI showed click two's outcome — the switch OFF while the app still starts at login. LoginItem gained an enqueueing form, and since its queue is serial, click order now survives. The uninstaller's completion marker was written with a plain redirect, which truncates before writing. A poll landing in that gap saw the file and read an empty string, which is not "failed", so a retraction macOS had refused was reported as a clean uninstall — one-directional, and towards the silent orphan the check exists to catch. It is written to a side name and moved into place. The no-app warning is also gated on there actually being a registration, so a .pkg install that never launched the app does not send anyone hunting for a Login Items entry that was never created. And the login completion no longer overwrites the shared Settings status line if something else has claimed it since — a window that grew from instant to seconds in this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r happened Thirteenth review round. The home-directory lookup added two rounds ago queried `dscl .`, the local node. A network, LDAP or AD account has no record there, so the lookup returned nothing, the guard skipped the delete, and the app's per-user directory survived the uninstall for exactly the accounts the lookup was written to serve. /Search resolves both, verified on this machine. Two of the uninstaller's warnings were saying things that were not true. The mktemp-failure path reported "macOS would not retract the login item" when nothing had been attempted at all, sending the user to clear an entry by hand that re-running the uninstaller would very likely clear for them; it has its own state and message now. And the already-trashed-bundle warning was gated on `launchctl print`, which reports only *loaded* jobs — so a registration sitting at .requiresApproval, the state the app documents as "the user switched Dezhban off under Login Items", printed a clean "files deleted" over a surviving entry. That gate was added last round to avoid sending .pkg-only users hunting for nothing, so the fix is not to drop it and over-claim in the other direction: the warning always prints when the bundle is missing, and its wording is conditional, because root has no way to read SMAppService status and pretending otherwise is what got this wrong twice. enable()'s catch returned .failed without re-reading the status, which is the same asymmetry .agentStuck was added to close on the disable() side: .failed has isOn == false, so a live registration painted the switch OFF while the app kept starting at login. kSMErrorAlreadyRegistered is the obvious route in — switch stale-OFF, user clicks on, register throws *because* it is already registered. Also deleted the synchronous set(enabled:), which had no callers left: a queue.sync on the type whose job is serializing the mutation paths is an invitation to reintroduce the main-thread beachball. One finding documented rather than fixed: disableRelaunchOnLogin() cannot cover the first logout after an upgrade, since the build being replaced never called it, so the window may open once at that login. Every workaround guesses — suppressing hand-offs near login would break the impatient double-click the hand-off exists for, permanently, to fix one event that has already passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stence Fourteenth review round, and my guess that it would be shell polish was wrong — two of these are correctness bugs in the Swift, one of them breaking the invariant ADR-0014 is written around. The migration's two flags had different durability. retractLegacy() writes legacyRetractedKey and flushes; markMigrated() did a plain set. This runs seconds into a login, so a session that ended before cfprefsd wrote it left a legacy item retracted — durably recorded — with the "already migrated" half gone. The next launch then fell through to register() and turned login-at-launch back ON for a user who had it off, which is precisely what must never happen; the mirror loss strands the account with nothing starting the app at login. disable() already flushes before the call that can end the process, and this path has the same obligation. enable() guarded on the legacy item being *enabled* while its own comment said any live registration. That guard was narrowed two rounds ago to keep legacyStuck's advice from being a dead end, on the reasoning that a .requiresApproval item launches nothing — but AssociatedBundleIdentifiers makes one "Dezhban" row in Login Items govern both registrations, so approving that row arms both, and then two copies start at login, one with the marker and one without, racing the lock over whether the window opens. That is the defect this branch removes, and it cannot be traded for a better message. The dead end is real and is now stated instead: if macOS will not retract the old registration, nothing the app can do makes enabling safe. The .agentStuck message was also wrong in the case its own comment names as the main way in. Mapping every throw-with-a-registration to "macOS would not remove the login item, remove it in System Settings" told a user who had just asked to switch login-at-launch ON to go and delete it. The outcome comes from the status now, not from the fact that a throw happened. seed() overwrote the status line with "Loading…" and "Seeded from …" — and it runs on didBecomeActive, which macOS delivers during a login-item change because it surfaces the approval prompt. So the user came back from that prompt and the awaitingApproval and legacyStuck guidance was swallowed: the entire reason Outcome carries a message. loginPending already protected loginEnabled from that race; the status line has its own hold now. Also two checklist items asserted behaviour the code contradicts: launching a fully-started app from Finder is a reopen, not a hand-off, so it never reaches the instance lock and correctly opens the window in every "Open minimized" mode — the old text expected "Always" to suppress it and would have been filed as a regression. Both are rewritten to test what is reachable, and a duplicated sentence is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fifteenth review round, three findings. legacyStuck was returned from both directions while carrying isOn == true, and the two directions are opposites. From disable() the old item is still enabled, so something IS starting the app and the switch must stay on. From enable() nothing is registered at all — enable() is only entered when the switch read off — so an on-ish outcome snapped the switch ON over a state where nothing was registered, and the next seed() flipped it back. That switch-versus-isEnabled disagreement is the one thing isEnabled's own docstring says must never exist. Sharing the case also had it telling a user who clicked OFF what switching it ON would do. enable() returns blockedByLegacy now, isOn == false, worded for the click that was made. The uninstaller skipped its entire per-user teardown when there is no non-root console user — run at the login window, or over ssh on a Mac nobody is logged into — and every LOGIN_ITEM_STUCK assignment lived inside that block, so it printed an unqualified "files deleted" over an agent registration that survives, an entry that fails to load at every subsequent login, and the migration flag that makes a later reinstall skip the migration. The same silent-clean-report the other states were added to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nores Sixteenth review round, five findings, and the medium reverts something I added in round nine. The hand-off was gated on "Open minimized", so under Always a double-click that lost the instance lock posted nothing and the launch was a visible no-op. The gate was added on the reasoning that Always must mean always — but the preference governs the LAUNCH, and a user-initiated launch of an already-running app is not one. A second later the identical gesture opens the window, because LaunchServices turns it into a reopen and applicationShouldHandleReopen answers unconditionally in every mode, by design and by documentation. So the gate gave one gesture opposite answers depending on whether the incumbent had finished starting yet, which is worse than either behaviour on its own, and it cost the single launch that had no other route to a window. claim() mapped every removeItem failure to .lost, so a permanent failure — an unwritable Application Support directory with a request file in it — was indistinguishable from the benign race. Every claimer stood down forever, discard() could not clear it either, and the hand-off was dead for every future launch with nothing said. ENOENT is the race; anything else is .blocked, which callers still stand down on (acting on a file that cannot be removed would repeat on every check) but now log. post() swallowed its write error, and the loser then exited assuming one of the two signals would land. If the write fails during the gap the file exists to cover, neither does. It returns a Result now; the caller cannot repair it but can say so. The .unavailable branch called discard() without having taken the lock, against that method's stated contract — so a transient open() failure in a third launch deleted a request the real session owner was about to claim. And the status-line hold was taken even when the message it protects had been declined, suppressing seed()'s updates for ten seconds to defend a line that was never shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seventeenth review round, four findings, all low. The home-directory lookup scraped dscl's plain output with sed, which puts a value containing a space on a continuation line — so for a home like "/Volumes/Home Dirs/jsmith" it came back empty, the guard skipped the cleanup, and the script printed an unqualified "files deleted" over a surviving instance lock. That is the untruth the /Search fix two rounds ago was written to end, and a space is exactly what the network and relocated homes it exists for tend to have. Read as a plist now, verified on this machine. isInStableInstallLocation compared only the immediate parent, so /Applications/Utilities/Dezhban.app never migrated — an ordinary thing for someone to do with an app, and certainly a location it stays in. That user's legacy item kept starting the app with no marker and "Open minimized" stayed broken for them permanently, reported only in a log line. Anywhere under an Applications directory counts now. The status line had one writer left unguarded. The login completion checks before writing and seed() respects the hold, but the service toggle's completion cleared unconditionally — and the login toggle is not disabled during that sequence, so a user could flip it mid-install, get the awaitingApproval guidance they must act on for the switch not to be lying, and have it wiped a moment later. Also dropped a leftover clause in ADR-0014 that still described the hand-off as gated on "Open minimized", one paragraph above the text explaining that it is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eighteenth review round, four findings. Last round widened where the migration may run from — anywhere under an Applications directory, so filing the app into /Applications/Utilities is supported — and left the uninstaller looking only at /Applications/Dezhban.app. That install therefore got "the app bundle was already gone", an agent unloaded for this boot only, an rm -rf that deleted nothing, and a bundle that kept launching at every subsequent login, while all three printed messages said it had been removed. The bundle is searched for now, in the same places the app is allowed to live. The label-drift guard used grep without -F, so the pattern is a regex and `.` is a wildcard: a label renamed to "…app-login" in one consumer still matched "…app.login", and the check passed over exactly the drift it exists to catch — leaving SMAppService naming a plist that does not exist, which surfaces only as the .notFound status nobody reads. enable() was the one write to the three coupled login-item flags without a flush, while disable(), retractLegacy() and markMigrated() all have one and say why. Clearing the explicit-off in memory only meant a session that ended first left it reading true, so the next launch marked the account migrated and permanently cancelled the register() retry added so nobody is stranded with nothing starting the app at login. And "instance lock" collided with the glossary's existing Single-instance lock, which is the daemon's lock over the state directory — one name for two unrelated mechanisms, in the file CLAUDE.md makes the authority for a term. The GUI type is SessionLock now, and the glossary gained entries for it and for launch marker, login agent and hand-off request, with the distinction stated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nineteenth review round, five findings, and the medium is collateral from
last round's rename. A blanket substitution turned "the single-instance
lock's mutex" in a released CHANGELOG entry into "single-session lock" — the
daemon's Windows mutex, which is called single-instance everywhere else in
the tree including the glossary entry this branch just added, which points
the reader at it by that name. `grep -rn single-session` matched exactly one
line in the repo: the one I had broken. Restored.
blockedByLegacy was returned whenever any legacy registration survived a
failed retraction, with isOn false — but when the survivor is *enabled* the
app is still starting at login, so the switch snapped OFF over a live login
launch and the next seed() flipped it back. The argument for why that was
impossible ("enable() is only entered when the switch read OFF, so isEnabled
was false") assumes a fresh switch, and the rest of this code deliberately
treats it as stale — which is why set(enabled:) takes the state the user
asked for instead of re-reading. The outcome comes from what actually
survived now.
The login toggle and the service toggle were competing for one status line,
and guarding one direction only moved the loss to the other: the in-progress
write was unguarded, so flipping the login toggle mid-install destroyed the
install's progress message, and the install's completion then declined to
clear it. They are two facts with different lifetimes, so the login item has
its own line under the toggle and the hold machinery is gone.
Requiring a hand-off file outside the launch window — which is what keeps an
unauthenticated notification from being an activate-on-demand channel — made
a failed post() a visible no-op, reachable on a full or read-only home. The
poster is the only party that knows its file did not land, so it says so in
the notification rather than the receiver inferring it from a timer. The
exemption weakens nothing already reachable: anything able to forge that
notification could equally run `open -a Dezhban`.
And the .blocked test manufactured its condition with a 0o500 directory,
which root ignores — so it would have failed under sudo for a reason
unrelated to the code, on a checklist that routinely asks for privileged
on-host runs. It skips instead of lying.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twentieth review round, four findings. enable() had no install-location gate, only the migration did, and the waiver was "an explicit toggle from Settings is the user's own call". That misses who can undo it: register() records the *calling* bundle, and only that bundle can ever unregister(). So switching login-at-launch on from dist/Dezhban.app — the documented dev loop, which SessionLock is path-keyed specifically to permit — leaves a launchd registration pointing into a bundle the next build deletes. It fails to load at every login, orphans a row in Login Items, and nothing in the product can retract it, since uninstall.sh only searches the Applications directories. Same for a zip copy run once from ~/Downloads, which is the case the migration's gate already exists to avoid. The toggle is gated the same way now and says why. A copy that could not take the session lock and started anyway — the .unavailable path, which exists so a broken support directory cannot stop the app launching — still installed the hand-off observer and backstop. Both it and the real owner would answer one double-click, each with its own debounce, so neither could suppress the other: two windows and two activations. Only the owner answers now. loginMessage was never cleared, so guidance outlived the condition it described: "macOS is holding this for your approval" stayed on screen after the user went to System Settings, approved it, and came back — which is the very activation that re-seeds the switch to ON. A fresh status read clears it and lets the switch speak for itself. And a comment on the login completion still described a claim check against the pane's shared status line, which the split removed two commits ago — inviting a future reader to restore a guard that would undo the split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-first review round, four findings, three of them mediums in machinery added over the last few rounds. The label-drift guard could not fail for LoginItem.swift. It grepped for the label as a bare substring, and LoginItem's own dispatch-queue label — "…app.loginitem" — contains it, so the check passed no matter what plistName said. Renaming plistName alone would have shipped an SMAppService call naming a plist that does not exist, reported only as the .notFound status nobody reads: exactly the drift ADR-0014 calls this assertion load-bearing for. Both consumers are now matched against their exact declaration, and the negative case is verified to fail the build. retractLegacy() wrote its flag after the unregister, which is the ordering disable() explicitly avoids two hundred lines away, for the same reason: SMAppService.mainApp is a launchd job, the migration's main case is a pre-agent install with login-at-launch on, so the running app IS that job's process and launchd may kill it as the job unloads. That kill left the legacy item retracted with nothing recorded, and the next launch — no legacy item, no flag — concluded there had never been one, marked the account migrated and returned. Nothing starting the app at login, permanently, which is the hole the flag was added to close. It now records the attempt, flushed first, so a retraction that succeeded unrecorded is not mistaken for one that never happened and a failed one is still re-attempted. disable() tested whether the legacy item was enabled while enable() tested whether one was registered, so the two directions disagreed about a .requiresApproval leftover that refuses to retract: disable reported a clean "App will not open at login", and every later click to turn it on was refused permanently, with nothing having warned them. Both derive from what actually survived now. And seed()'s loginMessage clear was not merely possible but guaranteed: its status read is a queue.sync behind the in-flight mutation, so it always completes after it, by which time loginPending is false — nilling the one line that tells the user to approve Dezhban in System Settings a moment after it appeared. The completion bumps the revision, so a read already in flight is discarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-second review round, two findings — the fewest yet, and the medium is the other half of last round's fix. Recording the retraction attempt durably before the unregister was right, but the fact that decides what happens *next* was still only a local. A pre-upgrade user who switched Dezhban off under System Settings leaves mainApp at .requiresApproval and never touched Dezhban's own switch, so nothing records their intent. If the process ended between the retraction and markMigrated() — a quit or logout during the status read that follows — the next launch saw no legacy item, saw the attempt flag set, fell through, and registered the agent: login-at-launch switched back on for someone who had deliberately turned it off, which ADR-0014 says must never happen, and userDisabledKey could not catch because it was never set. It is set now, flushed, before anything is retracted. The uninstaller's home-directory lookup was best-effort and every consumer degraded quietly. If it resolved to nothing while a console user existed — a directory record without NFSHomeDirectory, or any change in dscl/plutil output shape — the ~/Applications half of the bundle search was skipped, so an app installed there was declared "already gone" while it sat in place with its login agent intact, and the per-user cleanup was skipped under a closing message that said "files deleted". It falls back to the conventional path, and if even that is not there it warns instead of claiming a clean removal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-third review round, seven findings. Both "the legacy item survived" branches answered from their own branch — legacyEnabled ? .legacyStuck : .blockedByLegacy — which ignores the agent. With a dormant .requiresApproval legacy item that will not retract AND a live agent registration, that returned isOn false and a message asserting "it has been left off", so the switch snapped off, told the user nothing starts the app at login, and the next seed() flipped it back — while isEnabled said true. Both branches now derive from what is actually live, through one helper, so isOn agrees with isEnabled by construction and the two directions cannot drift apart again. This is the third time these two have disagreed about the same state; deriving rather than deciding per-branch is what finally makes that structural. The notification handler stood down on .blocked, which is right for the backstop (it would repeat every tick) and wrong here: this is a one-shot event tied to a real user launch, so standing down made that launch — and every later one, since an unremovable request stays — the silent no-op the mechanism exists to prevent. Notable that the mirror case, a file that could not be *written*, got a whole notification flag last round and this one was left. The debounce could not cover the ordering it was written for. When the backstop claims .fresh and the notification therefore sees .absent, both hop to main; if the .absent hop lands first it opens, and the .fresh hop then bypasses the debounce entirely. A definite open is now suppressed when the open it follows was indefinite — that pairing is two signals for one request — while two genuine double-clicks both claim .fresh and so never suppress each other. Four in the uninstaller: the no-home warning claimed the saved preferences survived when they are deleted unconditionally; a console user whose uid could not be looked up was told "nobody is logged in — re-run from a graphical session", which is false and useless since it fails the same way; the timeout's pkill was unscoped and reached every logged-in account's menubar app on a Mac using fast user switching; and a comment still asserted the Settings-toggle waiver that this same branch removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-fourth review round, four findings. seed() cleared loginMessage unconditionally, and it runs on every didBecomeActive — so clicking away from the app and back was enough to erase it. That was justified for "macOS is holding this for your approval", whose condition really does expire, but it also wiped the messages that explain a *refusal* and have no other channel: run dist/Dezhban.app, click the login toggle, watch it snap back with "Dezhban has to live in Applications to open at login", click another app and back, and the reason is gone. A switch that snaps back with no explanation is indistinguishable from a bug, which is the thing Outcome was introduced to prevent. Outcomes now say whether they are transient, and only those are cleared. .blockedByLegacy's message described a click the user may not have made — "switching this on could start Dezhban twice at login, so it has been left off" — and disable() reaches it too, when the agent goes away but a dormant legacy registration will not. Describing the wrong direction is the mistake splitting .legacyStuck out was supposed to end, so the wording is now neutral about which way was clicked. The uninstaller's leftover-home condition shared LOGIN_ITEM_STUCK, which holds one value — so whenever an earlier step had recorded timeout/refused/not-attempted, an unresolvable home was swallowed and the session lock survived under a report mentioning only the other problem. It has its own flag, and both warnings print. And ADR-0014 still carried the sentence "an explicit toggle from Settings is the user's own call and is not gated" fifteen lines after explaining why that waiver was wrong and removed. That is the sentence a future reader would act on to re-add it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-fifth review round, three findings, and the medium is the cost of round twenty-three's fix. Deriving the outcome from live state stopped enable() and disable() disagreeing, but it made the answer direction-blind — and a registered agent means opposite things to the two callers. To someone disabling, it means the unregister failed; liveOutcome reported .enabled, "App will open at login.", to a user who had just clicked off. That made .agentStuck unreachable from disable() whenever a legacy registration also survived, which matters because .agentStuck is the only outcome carrying the line telling them to clear it in System Settings and the only non-transient one — so the message was wiped by the next refresh and the switch sat back ON explaining nothing. The same blindness had .awaitingApproval telling someone who clicked off to go and enable Dezhban. liveOutcome takes the direction now. The debounce left lastHandoffOpenWasDefinite false after suppressing a definite open, so the rest of the 3s window kept swallowing definite opens and a genuine second double-click a second later was dropped — the no-op the mechanism exists to prevent, reached through the machinery that prevents it. Suppressing closes the pair. And the transient flag was never reset when a click began, so a progress line inherited the previous refusal's "keep me" and became un-clearable if its completion was superseded. A refusal also outstayed its truth: the user cleared the condition in System Settings, came back, the switch moved, and the old refusal sat there contradicting it. Messages now remember what the switch read when they were written, and go when it no longer matches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…must stay Twenty-sixth review round, three findings. .awaitingApproval was classed transient, on the reasoning that a fresh status read lets the switch speak for itself. It cannot: an awaiting-approval registration counts as registered, so the switch reads ON while nothing actually starts the app at login. Clearing the message on the next activation therefore left exactly the switch-versus-reality lie this type exists to prevent — and the activation in question is the one the approval prompt itself causes, so it happened every time. The message now expires on the status changing rather than on a refresh, which needed the pane to read both facts in one call so they describe the same instant. registered() maps .notFound to "not registered", which is the honest answer about this bundle's plist and the wrong one for "is there anything to retract". retractAll() returned true on it, so the uninstall errand exited 0 and the script printed a clean removal over a registration an earlier valid copy of the bundle may still hold — the orphan the errand exists to remove. disable() had the same shape, reporting "App will not open at login" without having attempted anything. The .blocked hand-off opened as indefinite, so the debounce dropped the second of two launches inside three seconds — and since a request that cannot be removed stays that way, every later pair too. There is no other claimant to pair with there (the backstop stands down on .blocked), so it is definite. Also removed isEnabled, which had no callers left once the pane needed the approval fact beside it. Its reasoning is cited by several comments and moved onto the accessor that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nnot tell" Twenty-seventh review round, two highs — both of them last round's fix, which rested on an assumption about SMAppService that is false on this OS. .notFound is what `SMAppService.agent(plistName:).status` returns for an agent that was never registered, not only for a plist it cannot resolve. Verified here: with no Dezhban login item and no agent job present, the shipped ad-hoc bundle's `--unregister-login-item` errand exited 1. So treating that status as "nothing can be asserted" fired on the ordinary path, twice over. disable() returned .agentStuck on it, which is isOn true and non-transient — so the common case, a pre-agent install whose switch reads ON via the legacy item and whose agent was never registered, had the user click off, watch the switch snap back ON, and be told "macOS would not remove the login item, remove it in System Settings" over a retraction that had just succeeded. That is the switch-versus-reality lie the Outcome type exists to prevent, reached from the other side. retractAll() returned false on it, so the uninstaller recorded "refused" and printed the System Settings warning on every uninstall of an install where login-at-launch was never switched on — crying wolf on the ordinary path and eroding the signal on the path the warning was written for. The errand now exits 0 there, checked against the rebuilt bundle. The distinction those changes wanted is simply not available from a status read, so nothing infers it any more and a comment says why, so it is not attempted a third time. What stays truthful is an unregister that actually fails. Third finding: seed() read `awaitingApproval` only to decide whether to clear a message a click had left behind, so opening Settings with no preceding click — on an install whose agent the user had switched off under Login Items — painted the switch ON with nothing said while nothing started the app at login. The switch counts an awaiting-approval registration, so it cannot express that; the pane now states it unprompted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-eighth review round, four findings. The debounce is gone. Three versions of it tried to tell "two signals for one request" from "two requests" using elapsed time plus a definite/indefinite flag, and every one both let a duplicate window through in one ordering and swallowed a genuine second launch in another — because elapsed time does not carry that information. Each request now carries a token, written into the file and repeated in the notification, so whichever signal arrives second is recognised and dropped while a new launch always opens. That deletes lastHandoffOpenAt, lastHandoffOpenWasDefinite, handoffDebounce and the definite parameter, and the two remaining failures this round named — a fileless pair always counting as indefinite, and the pair-closing branch consuming a real new request — cannot be expressed any more. The migration's "user turned this off" guard sat above the legacy block, so it short-circuited the retraction that the comment twenty lines below insists is unconditional. It gates the register only now. A live mainApp registration left on file is re-armed by the user approving the single Dezhban row under Login Items and then starts the app with no marker, permanently, since the migration is marked done and never runs again. That was reachable because disable() wrote the account-wide flag from any bundle: a dev build or a ~/Downloads copy switching login-at-launch off recorded "off" for the *installed* app, whose migration then skipped its retraction forever, while this bundle's own retraction acted on something else. Retracting from anywhere is fine; speaking for the account is not. post()'s nil case defaulted to success, so a nil request path would have sent a notification without the fileless marker and dropped the launch with nothing logged. Unreachable today, and the safe reading of "no file to write" is the same as a failed write. And the uninstaller's bundle search was depth-limited while isInStableInstallLocation accepts any depth under an Applications directory — so an install the app would register the login agent from was one the uninstaller could not find, printing "the app is gone" over a bundle still launching at every login. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Twenty-ninth review round, five findings, all low. `state` and `liveOutcome` each queried SMAppService two or three times and derived a decision from the mix, so a status changing between reads selected a branch matching neither instant. The reachable case: a user approving Dezhban in System Settings while the pane read it, getting "macOS is holding this for your approval" written under a switch painted OFF. One read now feeds every branch, which also makes .agentStuck unreachable from the enabling direction — it tells the user to go and *remove* the login item, the opposite of what they asked for, and it was only ever reachable through that same split read. claim() read the token before unlinking, which is a TOCTOU on a claim-by-unlink design: post() is an atomic replace, so a second duplicate's request landing between the read and the unlink had this caller delete T2 while reporting T1 — the token already handled, so the caller matched it, declined to open, and that launch became a silent no-op with its request gone. Renaming aside is the claim and the read barrier at once. Reports of a failed retraction rested on a status read taken microseconds after unregister(), which is not documented to update synchronously. If it lags, disable() snaps the switch back ON over a retraction that succeeded and the uninstall errand exits non-zero so the script warns over a clean removal — the crying-wolf failure the .notFound predicate was deleted for two rounds ago, reached from the other side. A refusal is now confirmed over ~150ms before being reported; a success still returns on the first read. And build-app.sh asserted four of the six facts of its class: BundleProgram against the executable actually installed, and the bundle identifier across Info.plist, the agent's AssociatedBundleIdentifiers and uninstall.sh's APP_BUNDLE_ID, were unchecked — so renaming the executable target or the bundle id passed every test and this build while the login job failed to load, the Login Items row lost its name, and uninstall left the migration flag behind. Both new checks are verified to fail the build when broken. One process note: the first attempt at these build assertions was lost to an interrupted command, and the negative test I ran afterwards is what caught that they were never written. Worth remembering that a green build proves nothing about an assertion you have not seen fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirtieth review round, two findings — and the first is last round's fix applied to half its sites. stillRegistered() was introduced because SMAppService.status is not documented to keep up with unregister(), and then wired into only disable()'s agent check and retractAll(). The three legacy checks kept their single un-retried reads, and the worst of them is in migrateLocked(): on an upgrading account with login-at-launch ON, a status lagging past that read logged "the legacy login item could not be retracted", called markMigrated() and returned — so the agent was never registered, and because the flag is now set, the register-retry whose whole justification is "nothing starts the app at login, and with the flag set that would never be retried" can never run. Nothing starting the app at login, permanently, reported in an NSLog. All three go through the retried read now; the pre-checks stay single reads, since nothing has happened yet for a status to lag behind. disable() gated only its account-wide flag write on the install location, leaving the retraction itself ungated. The agent is registered under a label every copy of the app shares, so a dev build shows the switch ON because the *installed* copy registered the agent, and one click off retracts the installed copy's registration — silently, because the "off" is not recorded from an unstable location either, so the installed app's migration never sees that the user asked for it. No legitimate registration originates outside an Applications directory, so there is nothing there for such a copy to turn off, and it now refuses exactly as enable() does. The checklist gains the disable direction, which it only covered for enable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… request Thirty-first review round, five findings. stillRegistered() was the retried read for the unregister side; the register side had none, and took three separate status reads on top. A status that had not caught up with a register() that did not throw gave .failed — isOn false — so the switch snapped OFF saying "macOS did not keep the registration" while the agent was registered and the app would start at login: the same switch-versus-reality lie stillRegistered exists to kill, on the other side. Three reads also let the status change between them, producing the self-contradicting "Could not change the login item: the login item is enabled." One settled read now feeds every branch. The .blocked hand-off was acted on unconditionally. Since the claim is a rename, .blocked means the directory is unwritable — which is also why no poster could have written that file: it is a leftover, and discard() cannot clear it either. So every later notification reactivated the app, fresh token each time, window reopening indefinitely with no in-product recovery. A real launch in that state cannot write its file either, so it arrives with the fileless marker and is accepted on that basis instead. And the backstop now actually stands down as its comment claimed, rather than returning from one tick and logging the same permanent condition eleven times. The uninstaller searched only the console user's ~/Applications, and CONSOLE_HOME is resolved only when somebody is logged in there — so over ssh or at the login window an install in a ~/Applications was never found, nothing was deleted, and the script still printed "files deleted" and "Nothing will start Dezhban (the app is gone)". It searches every account's now. And the "switch moved" clear-clause wiped the message its own comment names as the motivating case: .unstableLocation says this copy may not touch the login item, which stays true however the switch reads — and on a dev build beside an installed copy the switch reads ON permanently, so the disagreement never resolved and the explanation went on every activation. Outcomes now say whether they describe the switch's state at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng a path Thirty-second review round, three findings. (The round before this one stalled without producing a review; a stalled agent is not a clean pass, so it was relaunched rather than counted.) The token dedupe kept one slot, which three interleaved signals defeat. Two duplicates launching inside the backstop window: the second post() atomically replaces the first's file, so a backstop tick can claim T2 while both notifications still arrive, and the sequence T2, T1, T2 answered T2 twice — the second activation half a second later that the token design exists to prevent, on the exact gesture the checklist asks testers to perform. It remembers the last sixteen now. uninstall.sh took the bundle path from `find … | head -1`, which truncates at the first newline — legal in a macOS directory name — and the result is handed to `rm -rf` as root and used to exec the retraction errand. Demonstrated here: a bundle under "My<newline>Apps" gave /Applications/My. find writes the path itself now, into a root-owned mktemp directory, and printf without a trailing newline means the command substitution reproduces it exactly. Verified that the new form resolves to the real directory where the old one did not. And ADR-0014 still described the superseded timing design — a three-second debounce, "once per debounce interval", "the debounce is a rate limit, not a gate" — three paragraphs before correctly describing what actually ships, and two rounds after the debounce was deleted. Editing it is right while it is unmerged; once shipped it would have to be superseded instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirty-third review round, three findings. enable() was the one writer in LoginItem that retracted before recording the user's intent, inverting the rule the rest of the file states twice. retractLegacy() unloads a launchd job and in a login-started session that job's process is this app, so a kill between the retraction and the write left the "user turned this off" flag still reading true with the attempt flag now set — and the next launch's migration fell straight through to markMigrated(), retiring the account with nothing registered, moments after the user asked for login-at-launch ON. The write is flushed first now, like its two siblings. The uninstaller's bundle search only ran when /Applications/Dezhban.app was absent. SessionLock is path-keyed and isInStableInstallLocation accepts any Applications directory precisely so two installs can coexist, so the copy holding the agent registration need not be the one at the default path: the errand then ran from a bundle that had never registered — where the status is truthfully "nothing to do" — rm -rf deleted the wrong copy, and the script closed with an unqualified "files deleted" while the survivor kept starting the app at every login. The search now runs unconditionally and records every match, and each bundle's errand runs from that bundle before it is removed, because only the registering bundle can retract its own registration. And .heldByAnother exited unconditionally, even when no live copy of this install owns the lock. flock is released by the kernel when its holder dies, so locally "held" implies "alive" — but over a network home the server emulates it, and an advisory lock can outlive its process. Every launch then posted a hand-off nobody would claim and exited: permanently unstartable, silently, which is the opposite of how every other failure here degrades. It retries the lookup, since at login the incumbent is often launchd-exec'd and not yet LaunchServices-registered — the ordinary reason to find nobody, and one that must still yield — and starts anyway only when nobody turns up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirty-fourth review round, five findings, all in last round's fix. The bounded incumbent probe was the wrong shape. At login the incumbent is launchd-exec'd and often not yet registered with LaunchServices, so 600ms times out on a *live* one — and the fallback then started a second copy of the app, which is the failure the session lock exists to prevent, on the common path. It also discarded the hand-off it had just posted, which the .unavailable case thirty lines above refuses to do for exactly the reason stated there. Locality decides it instead. Locally flock is the kernel's, so held means alive and the launch yields whether or not anyone has appeared in LaunchServices; the request it posts is claimed by the incumbent's own backstop. Only on a network home, where the server emulates the lock and it can outlive its holder, is "nobody there" allowed to mean stale — and there the app now also sets itself as the session owner, because otherwise it started, became the incumbent, and then ignored every later launch's hand-off: the silent no-op again, permanently. claim() renames the request aside before reading it, so a process killed in between — which this branch documents as reachable, since retracting a login item can have launchd terminate the app — orphaned a .claiming-* file that discard() never touches. Swept by whoever takes the lock, alongside the discard, since anything there then belongs to a predecessor. .unstableLocation asserted isOn == false from the disable direction, so a dev build beside an installed copy painted the switch OFF while login-at-launch was still live through the installed agent. Only outcomes that describe the switch's own state may move it. And the uninstaller deleted bundles in any account's ~/Applications while cleaning up only the console user's, leaving other users' session lock and hand-off file behind under a closing "files deleted". Root can reach those, so it removes them for every home; the preferences and the login item cannot be reached, and the closing warning now names the migration flag rather than only the Login Items entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Behnam-RK
force-pushed
the
fix/open-minimized-launch-marker
branch
from
August 22, 2026 06:47
fbd9da4 to
6e8a1b5
Compare
…p walk Thirty-fifth review round, five findings. .failed was excluded from describesSwitchState alongside .unstableLocation, which was wrong in the dangerous direction. The binding moves the switch optimistically to where the user put it and only corrects it from an outcome that describes the switch — so a failed *enable* left it reading ON with nothing registered until the next activation. A failure is very much a statement about the switch; only .unstableLocation genuinely is not. sweepAbandonedClaims() deleted every .claiming-* in the support directory, which every install shares on purpose — SessionLock is path-keyed precisely so a dev build may run beside the installed copy. So one install's launch could delete the other's in-flight claim: the victim's read then returned nothing, its claim reported a nil token, nothing was recorded, and the paired notification opened the window a second time. The duplicate activation the token design exists to eliminate, caused by cleanup meant to be harmless. Claim files are named per request now, and the sweep is scoped to that prefix. The bundle search walked *inside* every installed application — millions of inodes on a Mac with Xcode, repeated per home directory, any of which may be a network mount that blocks — silently, after panic had torn the rules down and before anything was deleted. It prunes at every .app boundary now. The first attempt at that was wrong and the fixture caught it: `expr -a \( … \) -prune` evaluates the prune only when the whole conjunction is true, so a non-matching bundle fell through and was descended into anyway, which showed up as finding a Dezhban.app nested inside an Xcode.app fixture. Two pruned branches instead. Also: three doc comments had been merged onto `enum Direction`, leaving liveOutcome and describe undocumented while Direction's docs described neither of them; and the comment above the find still claimed `-quit` stops at the first match, two rounds after it was changed to record every match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
"Open minimized" (Never / Always / Only at login) decided between a login launch and a user launch entirely from
NSApplication.launchIsDefaultUserInfoKey. That key was the only input, and it reported wrong in both directions: the window appeared at login with the setting on, and stayed hidden on a manual Finder launch.There was nothing better to consult.
LoginItemregisteredSMAppService.mainApp, which relaunches the bundle with no arguments and no marker of any kind.Separately,
MainWindow'sNSWindowleftisRestorableat its defaulttrue, so AppKit state restoration could reopen the window at launch without consulting the setting at all.What changed
Contents/Library/LaunchAgents/com.behnam-rk.dezhban.app.login.plist, installed bybuild-app.shand registered viaSMAppService.agent(plistName:). ItsProgramArgumentsend in--background, soCommandLine.argumentsis ground truth rather than a guess.LaunchVisibility.opensWindow(deliberateLaunch:)becomesopensWindow(backgroundLaunch:), fed byLaunchVisibility.isBackgroundLaunch(arguments:). An absent marker reads as a user launch — the safe direction, since the worst case is a window nobody asked for, and the Dock icon and "Open Dezhban…" open the window unconditionally in every mode.MainWindowsetsisRestorable = false. Frame and sidebar position still persist; those go through frame autosave, not restoration.LoginItem.migrateFromMainAppRegistration()moves existing installs across, once, on launch. It is gated on the old registration having been enabled, so an upgrade never switches login-at-launch on for someone who had it off. If the unregister fails it still registers the agent: a duplicate entry in System Settings is cosmetic, whereas skipping it would leave a login launch that never sets--background.build-app.shfails the build if the plist is missing. A bundle without it registers nothing and the app silently stops starting at login.Rationale
ADR-0014 records why an explicit marker rather than a better heuristic (
NSApp.isActivewas the alternative), why the three-way setting was kept rather than collapsed to a boolean, and why an in-bundle agent plist beats a separate login-item helper app.Verification
go build ./...,go vet ./...,go test ./...— pass.swift test— 194 tests pass, including the rewrittenLaunchVisibilityTests.build-app.shassembles the bundle with the plist present;codesign --verify --deep --strictpasses.Not verified by CI, and added to docs/contribute/testing.md as on-host checks: the actual login/logout behaviour in each of the three modes, that the login launch carries
--background(ps -o args=), the migration in both the enabled and disabled directions, and that state restoration can no longer reopen the window.🤖 Generated with Claude Code