[malpern]

Part of #1353 (release-blocker). It does not close the issue: the clean-VM run from 1.0.1 has not happened, and nothing in the release process enforces it yet.

What a user upgrading from 1.0.1 now sees

The session runtime still refuses to start beside 1.0.1's root kanata daemon. While that refusal stands, Settings → Status shows it as the main message and replaces Fix it with Remove Old Service. One administrator prompt removes the old stack, then KeyPath starts. The toast says whether the Karabiner driver is still installed.

How it works

• InstallerEngine.removeLegacyPrivilegedRuntime(using:) runs inside the installer transaction. The published refusal issue carries AutoFixAction.removeLegacyPrivilegedRuntime, and runSingleAction routes it. The wizard cannot detect 1.0.1 yet, so Settings is the only entry point (ADR-045 updated).
• LegacyPrivilegedRuntimeCleanup (new, KeyPathInstallationWizard/Core):
  1. Read-only inspection: launchctl print on system/ and gui/<uid>/ for each com.keypath.* label 1.0.1 used, root-owned KeyPath processes (owner UID read with sysctl), and KeyPath's own system files. If nothing is found, no prompt is shown.
  2. One root batch: kp_timeout-bounded launchctl bootout for every target, SIGTERM to the observed root PIDs only after re-checking UID 0 and the command line as root, then rm -f of KeyPath's own files.
  3. Verification: fresh inspection, polled up to about 5 s for launchd teardown. Success needs no loaded legacy service, no root KeyPath process, and no legacy file. An unreadable service state never counts as gone. A cancelled or failed batch is never reported as success.
• The Karabiner VirtualHID driver is not removed, because Karabiner-Elements shares it. The user is told whether it remains and is pointed to the new guide.
• After a verified removal, the daemon evidence caches (10 s PID cache, SMAppService status) are invalidated, so the refusal clears on fresh evidence before KeyPath starts.
• This is not a recipe. Privileged recipes stay unavailable, and a test confirms a forged plan naming the removal is refused.
• 1.0.1's ServiceManagement records are deliberately left. Release builds do not ship those plists, so SMAppService cannot resolve them. A stale Login Items entry may remain. Whether a reboot brings anything back is step 12 of the VM plan.

Testing

• New LegacyPrivilegedRuntimeCleanupTests (18 tests) cover: no prompt when nothing is left, one batch for the whole cleanup, polling for teardown, cancelled authorization, a failed batch never counting as success, verification failure, unreadable state, the driver never being touched, engine routing and completion states, smuggled-recipe refusal, every 1.0.1 process path matching the pattern, bash -n on the real root script, and a real-process ADR-040 grounding test (PID 1 is root and returns EPERM, this process is not root).
• New LegacyRuntimeUpgradeFlowTests cover every Settings outcome. Existing refusal, MainAppStateController, InstallerEngine, and all Lint suites updated and passing.
• swift test --filter 'LegacyPrivilegedRuntimeCleanupTests|LegacyRuntimeUpgradeFlowTests|SessionLegacyDaemonRefusalTests|SessionFailureRecoveryMessageTests|MainAppStateControllerTests|InstallerEngine|Lint': 215 XCTest and 42 Swift Testing tests pass, no warnings. The full suite was not run locally.
• python3 Scripts/check-accessibility.py passes.
• Not run: the manual clean-VM test in docs/testing/upgrade-from-1-0-1.md. It needs vm-lab on the mini, which is offline. Nothing has exercised a real 1.0.1 upgrade end to end, and nothing was deployed to /Applications.

Review gate

./Scripts/review-gate.sh ran locally three times. The first two passes found real defects, all fixed here: the GUI launchd domain was missed, the SMJobBless helper path didn't match, another user's processes could be killed, and the button was never rendered (Settings does not draw StatusDetail.actions). The third pass: REVIEW-VERDICT: PASS. Remaining notes, all PLAUSIBLE or lower, none a false success:

• Log files count toward "clean", so a leftover log alone triggers a prompt or a retry.
• An unreadable process owner is treated as not root, the opposite of how unreadable service state is treated. The start-time daemon check still guards.
• A start-intent race could show the "still detects it" message after a clean removal.
• invalidateEvidenceCaches() has no direct test of its effect on evaluateStatus.

Still open on #1353

• Run docs/testing/upgrade-from-1-0-1.md on clean macOS 26 and 27 VMs starting from 1.0.1, and record the results there.
• Make the release process block shipping until that run is recorded (release-doctor.sh has no such check).
• Publish guides/upgrading-from-1-0-1.md to gh-pages and link it from docs.md. Check the driver deactivate command it gives on the VM.
• Optional: let the wizard detect the refusal (SystemValidator session snapshot) so it can offer the fix too.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-05T05:05:32.727084Z	21c8461	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Review (diff was truncated at 60KB, so the tail of WizardTypes.swift and all tests/docs went unreviewed)

1. processPattern is too broad and can SIGTERM unrelated root daemons (LegacyPrivilegedRuntimeCleanup.swift, LegacyPrivilegedRuntime.processPattern)

• The pattern is Library/(KeyPath|HelperTools)/|PrivilegedHelperTools/com\.keypath\.helper.
• The Library/HelperTools/ alternative has no KeyPath anchor. It matches any root process whose command line contains that path, including third-party vendors' helpers in /Library/HelperTools/.
• Those processes pass the root-owner filter. They end up in rootProcesses, so isClean is false.
• The root batch's re-check uses the same pattern, so it would kill -TERM them.
• Because we can't kill them for good, verification would also fail after the kill. The user would see "not fully removed" for a process KeyPath doesn't own.
• Suggested fix: anchor on KeyPath-specific paths, such as KeyPath.app/Contents/Library/HelperTools/ or Library/KeyPath/bin/, plus the com.keypath.helper path.

2. Unreadable launchctl state makes removal fail

• In inspectLive, any launchctl print exit code other than 0 or 113 goes to unreadableServices, and isClean treats that as not clean.
• A gui/<uid>/… target can return a different non-zero code when there is no GUI session, for example over SSH or at the login window. If so, the flow fails with "could not check" even though everything was removed.
• Consider limiting the strict check to the system/ domain, or treating the known "no such domain" statuses as absent for gui/.

3. Minor

• kp_timeout is used in the batch script. Please confirm PrivilegedCommandRunner defines it for every batch. Otherwise each bootout line becomes a silent no-op (|| true), and the stack is only removed by file deletion and the PID kill.
• The diff includes many unrelated brace-style reformatting hunks, such as single-line if … { return } expanded to multiple lines. They add noise to review. Please confirm they come from the pinned SwiftFormat 0.61.1 and not a different version.

No other correctness issues found in the reviewed portion.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21c84611f7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Restrict root process matching to KeyPath-owned executables

On a Mac running another app's root helper from Contents/Library/HelperTools/, this pattern also classifies that unrelated helper as part of KeyPath 1.0.1. inspectLive() includes it in rootProcesses, and batch(stopping:userID:) sends it SIGTERM after rechecking the same overly broad pattern. For example, /Applications/Other.app/Contents/Library/HelperTools/OtherHelper matches without containing any KeyPath identifier. Consequently, Remove Old Service can terminate unrelated privileged services, and a helper that respawns can also prevent cleanup verification from succeeding. Restrict the matches to the known KeyPath executable paths or identities.

Useful? React with 👍 / 👎.

[malpern]

Anchored the pattern on KeyPath executables only: KeyPath.app/Contents/Library/(KeyPath|HelperTools)/, /Library/KeyPath/bin/, and /Library/PrivilegedHelperTools/com.keypath.helper. Added tests that another vendor's HelperTools helper and a non-KeyPath privileged helper do not match.

🤖 Addressed by Claude Code

[chatgpt-codex-connector]

Make the upgrade guide discoverable in KeyPath Help

After cleanup on a Mac without Karabiner-Elements, this notice directs the user to an article that cannot be found in KeyPath Help. Repository-wide search finds no navigation link to the new guide, and HelpBrowserView.filteredTopics searches only the hard-coded HelpTopic.allTopics, which does not include it. Although build-offline-help.py packages the Markdown, packaging alone does not expose it in the browser. Add the guide to the help topics or provide a direct action that opens it so users can reach the promised driver-removal instructions.

Useful? React with 👍 / 👎.

[malpern]

Added "Upgrading from KeyPath 1.0.1" to HelpTopic.allTopics (Getting Started), so the guide the notice names appears and is searchable in the Help browser.

🤖 Addressed by Claude Code

[malpern]

Re the claude[bot] review:

1. Too-broad process pattern: fixed in the latest commit (anchored on KeyPath paths, with tests).
2. Unreadable gui/<uid>/ state: kept strict on purpose. Remove Old Service runs only inside the signed-in user's app session, where that GUI domain exists, so a missing job returns 113. Treating other codes as absent could report a still-loaded daemon as removed. If the VM run shows a spurious "could not check", I'll relax it for gui/ only.
3. kp_timeout is defined in PrivilegedCommandRunner.Batch.scriptPrelude, which every batch script starts with, and a bash -n test parses the real script. The reformatting comes from the pinned SwiftFormat 0.61.1. CI lints whole changed files at that version, so files this PR touches must be fully formatted.

🤖 Addressed by Claude Code

[claude]

Review of PR #1362 (the supplied diff was truncated at 60KB, so I did not see the rest of the InstallerEngine/wizard wiring or the tests and docs.)

No blocking correctness problems in the part I saw. Cleanup is only reported as success after a fresh read-only check. It re-checks the PID and uid as root before sending a signal. A failed batch is never reported as success.

Points to check:

1. kp_timeout in the root batch. Each launchctl bootout is written as kp_timeout 15 /bin/launchctl bootout … || true. The diff doesn't show where kp_timeout is defined. If PrivilegedCommandRunner.Batch doesn't prepend that helper, every bootout exits 127 and the || true hides it. The cleanup would then delete the plists while the jobs stay loaded, and the 5s verification would fail with "still loaded". Please confirm the helper is defined, or add a test that the batch script includes it.
2. Only SIGTERM is sent to root processes. If a legacy kanata ignores or is slow to handle TERM, the user gets a failure after a full admin prompt and has to retry. Consider a short wait followed by a re-checked SIGKILL inside the same batch.
3. Unrelated formatting churn. Many hunks only expand one-line if x { return } bodies into multiple lines: ServiceLifecycleCoordinator+SessionRuntime, MainAppStateController, HelpBrowserView, and SettingsView+StatusDetails (which also merges some guards). This isn't related to the feature and makes the diff harder to review. AGENTS.md asks for minimal diffs, so I'd drop these or move them into a separate PR.
4. Stale Login Items entry. The code comment accepts that the SMAppService records are never unregistered. If the user can see this entry, the Help guide should say what it is, or tell them to ignore it.
5. Tests. The new unit tests use injected dependencies. A test on the generated batch string would cover point 1, and one for a root-owned process that survives TERM would cover point 2.

[malpern]

Re the second claude[bot] review:

1. kp_timeout is defined in PrivilegedCommandRunner.Batch.scriptPrelude, which starts every batch script. A test now asserts the generated script contains kp_timeout() {, and bash -n parses the real script.
2. Root processes that ignore TERM: the batch now sends TERM, waits 2s, and sends KILL to any PID that is still root-owned with a KeyPath command line. Both checks are re-run as root before each signal. A test asserts the KILL step exists.
3. Formatting churn: left as is. CI runs the pinned SwiftFormat 0.61.1 --lint on every changed file in full, so any file this PR touches must be fully formatted or the format job fails.
4. Stale Login Items entry: the guide now says it is a leftover 1.0.1 record that runs nothing and can be ignored.
5. Covered by the tests in points 1 and 2. A real root process surviving TERM needs the VM run (docs/testing/upgrade-from-1-0-1.md).

🤖 Addressed by Claude Code

[claude]

Review note: the diff was truncated at 60KB, so I only reviewed the part that was supplied. That covers the session-runtime changes, the upgrade flow, the settings UI, the InstallerEngine changes, and most of LegacyPrivilegedRuntimeCleanup.swift. I did not see the rest of that file (processOwnerUID and later), the tests, or the docs.

No blocking issues found. The design is sound: root-owned PIDs are re-checked inside the root script before each signal, success is only reported after a fresh observation, and a failed batch is never reported as success.

Two non-blocking points:

1. unreadableServices blocks success. isClean is false whenever any launchctl print exits with a code other than 0 or 113. If a gui/<uid>/… target returns something else (for example, no GUI domain when the app runs over SSH or at the login window), cleanup would prompt for admin and then fail verification on every run, even though nothing is installed. Consider treating the other "domain/service not found" exit codes as absent for the gui/ targets, or add a test that pins the intended behaviour.
2. kp_timeout is not defined in this diff. The batch calls kp_timeout 15 /bin/launchctl bootout …. Please confirm that PrivilegedCommandRunner.Batch defines kp_timeout in the generated script. If it doesn't, every bootout fails and is hidden by || true, and the legacy services would then be reported as "still loaded".

[malpern]

Re the third claude[bot] review (both points answered in the previous reply):

1. Unreadable gui/<uid>/ state blocking success is intended, and tests already pin it. testUnreadableServiceStateIsNeverTreatedAsGone covers verification, and testLiveInspectionReadsLaunchctlExitCodesAndFiles maps a non-0, non-113 exit to unreadable. Remove Old Service runs only inside the signed-in user's app session, where the GUI domain exists. Over SSH or at the login window the button is never offered. Counting an unreadable state as absent could report a still-loaded daemon as removed.
2. kp_timeout is defined in PrivilegedCommandRunner.Batch.scriptPrelude. Since 4585e47, a test asserts the generated script contains kp_timeout() {.

🤖 Addressed by Claude Code

[malpern]

Correction to my earlier replies about formatting churn. The churn did not come from the pinned SwiftFormat. I had run Homebrew's 0.64.5 by mistake. Commit 048fd66 rebuilds each touched file as master plus this PR's changes only, formatted with the pinned 0.61.1. The unrelated hunks are gone, and that churn was also what broke SessionTerminalDiagnosticPlacementTests in CI.

🤖 Addressed by Claude Code

[claude]

Review of PR #1362 (1.0.1 legacy runtime removal)

The diff I received was truncated at 60 KB, so I did not see the rest of the test changes or the docs and guides. I found no blocking defects in the part I did see. The design is sound: one admin prompt, a re-check of owner and command line as root before each signal, and success only after a fresh read-only observation. The Karabiner driver is reported but never removed.

Non-blocking points to check:

1. Unreadable launchd state can make the cleanup fail permanently. inspectLive treats any launchctl print exit code other than 0 or 113 as unreadableServices, and isClean requires that list to be empty. If gui/<uid>/… returns a different status when the user has no GUI session (for example over SSH, or at the login window), the removal can never report success even though nothing is left. Please confirm which code launchd returns for a missing domain. If it differs from 113, treat that code as absent for gui/ targets, or add a test for it.

2. runSingleAction bypasses the usual action checks. The early return for .removeLegacyPrivilegedRuntime skips the inspectSystem() and intent-planning path, which looks intentional. Please make sure the wizard still never offers this action outside the legacy-refusal state. The autoFixAction is currently set only for .legacyDaemon in MainAppStateController, which looks right.

3. The kill script depends on a kp_timeout function defined by PrivilegedCommandRunner.Batch. The tests check that the script contains kp_timeout() { and that it passes bash -n, but nothing runs it. A small test that executes the signal and rm section against a stub would catch quoting regressions in the heredoc (\\ line continuation, and the \(processPattern) interpolation inside single quotes).

4. A PID can be missed if launchd respawns the daemon between the read-only probe and the batch. bootout runs first and the batch signals only the PIDs captured earlier, so a respawned process under a new PID would be caught only by the post-check, which then fails with a message to retry. That is acceptable, but a one-line hint in the failure text would help.

[malpern]

Re the fourth claude[bot] review:

1. Measured on macOS 27.3: launchctl print gui/<uid>/<label> exits 112 when the GUI domain does not exist, and 113 for a missing service. 112 now counts as absent, since a domain that does not exist holds no job. Other codes stay unreadable, and the live-inspection test pins all three.
2. Confirmed: only MainAppStateController sets .removeLegacyPrivilegedRuntime, and only for the legacy-daemon refusal. The wizard's planner never produces it, and recipeForAction returns nil for it. The early return in runSingleAction is the only path to it, and the removal re-inspects state itself before prompting.
3. Added testScriptPatternSurvivesShellQuoting. It takes the grep -E argument from the generated script and runs it through bash on KeyPath, non-KeyPath, and literal-dot cases. The signal and rm steps aren't executed, because they act on root-owned system paths. The VM run covers them.
4. The verification failure now ends with "Choose Remove Old Service to try again."

🤖 Addressed by Claude Code

[claude]

Automated review of the supplied diff. The diff was truncated at 60000 bytes, so the tail of LegacyPrivilegedRuntimeCleanupTests, LegacyRuntimeUpgradeFlowTests, the other test changes and the docs were not reviewed.

I found no blocking issues in the reviewed part. The design is sound: one administrator batch, a re-check of PID owner and command line as root before each signal, success only on fresh read-only evidence, and unreadable launchd state never counted as absent.

Minor notes:

1. testRootProcessFilterAgainstRealProcesses assumes the test runner is not root. It asserts canSignalProcess(pid: 1) == false and processOwnerUID(getpid()) == getuid(). The first assertion fails if the suite runs as root or in an unusual sandbox. Consider skipping when getuid() == 0.
2. runSingleAction(.removeLegacyPrivilegedRuntime) calls removeLegacyPrivilegedRuntimeWithinTransaction() directly. The public removeLegacyPrivilegedRuntime wraps the same call in withInstallerTransaction. Please confirm the wizard path already holds the installer transaction, or two removals could overlap.
3. The 1.0.1 SMAppService records are deliberately left registered. invalidateEvidenceCaches resets the sample counters, so this looks fine. Check that refreshLegacyDaemonRefusalAfterCleanup cannot still report .enabled with no process on a Mac that keeps a stale record. That would show the .stillDetected message even though everything was removed.

🤖 Generated with Claude Code

[malpern]

Re the fifth claude[bot] review:

1. testRootProcessFilterAgainstRealProcesses now skips when run as root (86e6d58).
2. Already serialized. The public runSingleAction wraps runSingleActionWithinTransaction in withInstallerTransaction, and the early return for this action is inside that wrapper. The gate is reentrant through a task-local, so two removals cannot overlap. This is the same path every other single action takes.
3. A stale .enabled record with no process cannot read as running. After invalidateEvidenceCaches() the sample count is 0 and lastObservedState is .stopped. The first samples return .unknown, and after three it falls through to the TCP probe, then .failed. Neither counts as running, so refreshLegacyDaemonRefusalAfterCleanup clears the refusal. Release builds don't ship the plist, so that record usually reads .notFound anyway, which counts as stopped when no process exists.

🤖 Addressed by Claude Code

[claude]

Review of PR 1362 (1.0.1 upgrade cleanup)

I found no blocking correctness issues in the supplied diff. The diff was truncated at 60 KB, so I did not see the rest of the tests or the final files. These are minor points to confirm:

1. Stale SMAppService registration can make stillDetected permanent. LegacyPrivilegedRuntimeCleanup deliberately does not unregister 1.0.1's ServiceManagement records. refreshLegacyDaemonRefusalAfterCleanup then re-reads through legacyDaemonRefusalForStartup / isLegacyDaemonRunning. If that detector still counts an .enabled registration with no process as running, the user would get "KeyPath still detects it. Restart your Mac" after a successful removal. invalidateEvidenceCaches resets the sample counter, but a test with a lingering .enabled status and no process would confirm the flow clears.
2. Auto-fix may run the removal without a deliberate click. MainAppStateController now puts .removeLegacyPrivilegedRuntime on the published WizardIssue.autoFixAction. Confirm that no wizard "Fix all" or auto-repair path runs autoFixAction automatically, since this action shows an administrator prompt and deletes files. The code comment says it only runs when the user asks.
3. Cleanup is not idempotent against a recreating service. The rm -f of /var/log/* and the plists happens after bootout. If bootout times out (kp_timeout 15) and the job is still alive, it can recreate its logs. Verification would then report "files remain" and the user would have to retry. This is acceptable, but the failure message could point to a restart.
4. Test coverage. The tests cover the batch script, the evidence parsing and the engine routing well. I'd add a LegacyRuntimeUpgradeFlow test where start fails and lastError is nil. That case should produce the fallback "Turn KeyPath on with the switch above." message.

[malpern]

Re the sixth claude[bot] review:

1. Added testLingeringEnabledRegistrationIsNotARunningDaemonAfterCleanup. A fake .enabled registration with no process and no TCP never reads as running across five samples after invalidateEvidenceCaches(), so the flow cannot get stuck on .stillDetected for that reason.
2. Nothing runs it automatically. The only consumers of an issue's autoFixAction are user-pressed Fix buttons. WizardConflictsPage reads wizard issues, which never carry this action. Fix All runs run(intent: .repair), whose planner cannot produce it (recipeForAction returns nil). In Settings, Remove Old Service is the only trigger.
3. The verification failure now says to restart if retrying keeps failing.
4. Already covered: testRemovedButNotStartedSaysWhy checks that a failed start with nil lastError gives "Turn KeyPath on with the switch above."

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60 KB, so the tail of the tests and the docs were not reviewed)

I found no blocking issues in the visible portion. The cleanup is well-structured:

• It re-checks the owner and command line as root before every signal.
• It never treats an unreadable launchd state as "gone".
• It never reports success when the admin batch failed.
• It polls for launchd teardown.
• Nothing from the Karabiner driver is removed.

Minor, non-blocking notes:

1. InstallerEngine.runSingleAction(.removeLegacyPrivilegedRuntime) calls removeLegacyPrivilegedRuntimeWithinTransaction() directly. The public removeLegacyPrivilegedRuntime(using:) wraps the same call in withInstallerTransaction. If wizard callers don't already hold the installer transaction, the runSingleAction path skips the serialization. Please confirm that runSingleAction callers are already serialized.
2. processOwnerUID uses e_pcred.p_ruid, the real UID. This is fine for launchd-spawned root jobs. The batch script, however, checks the effective UID via ps -o uid=. A process whose real and effective UIDs differ would pass one check and fail the other. This is unlikely for the 1.0.1 stack, but a comment would help.
3. The test testRootProcessFilterAgainstRealProcesses assumes PID 1 is launchd owned by root. That holds on macOS, but the test is environment-dependent and skips only for root.

Overall, this looks good to me.

[malpern]

Re the seventh claude[bot] review:

1. Already serialized. The public runSingleAction wraps runSingleActionWithinTransaction in withInstallerTransaction, and the early return for this action is inside that wrapper (answered in the fifth review too).
2. Fixed in 5fe8534. processOwnerUID now reads the effective UID (e_ucred.cr_uid), the same value the root script checks with ps -o uid=.
3. That test asserts macOS facts: PID 1 is launchd, owned by root. It skips when run as root, the one setup where the premise fails. KeyPath tests only run on macOS.

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60 KB, so the tail of LegacyPrivilegedRuntimeCleanupTests and the later test and doc files were not reviewed).

I found no blocking correctness or security issues in the supplied portion. The cleanup is well-guarded:

• The root script re-checks PID owner and command line before every signal.
• A failed batch is never reported as success.
• Unreadable launchd state is treated as not clean.
• Verification runs against fresh evidence.

Two non-blocking things to confirm:

1. removeLegacyPrivilegedRuntime wraps the work in withInstallerTransaction, but the runSingleAction short-circuit calls removeLegacyPrivilegedRuntimeWithinTransaction() directly. Please confirm runSingleAction already holds the installer transaction at that point. If it doesn't, the wizard path runs unserialized. If it does, the wrapper must not be reentered.
2. kp_timeout is referenced in the generated script, and the tests assert the script defines it. Please confirm that PrivilegedCommandRunner.Batch.script really prepends that definition, because testBatchScriptIsValidShell only runs bash -n and would not catch an undefined function.

Test coverage for the cleanup logic, the shell quoting and the live probes looks solid.

[malpern]

Re the eighth claude[bot] review. Both points repeat earlier rounds, so the answers are now comments in the code (e7db5ec):

1. The runSingleAction early return is already inside withInstallerTransaction. The public wrapper calls runSingleActionWithinTransaction within it, and the gate is reentrant through a task-local.
2. kp_timeout is defined in PrivilegedCommandRunner.Batch.scriptPrelude, which Batch.script prepends. testOneAuthorizationRemovesTheStackAndWaitsForLaunchdTeardown asserts the generated script contains kp_timeout() {, which bash -n alone would not catch.

🤖 Addressed by Claude Code

[claude]

Review of PR #1362 (the supplied diff was truncated after about 60KB, so the tail of the tests and the docs were not reviewed).

I found no blocking issues in the part I could see.

What I checked:

• The cleanup runs as one administrator batch and boots out the system and GUI domains.
• It re-checks each PID's owner and command line as root before signalling.
• It reports success only after fresh read-only evidence shows nothing left.
• Unreadable launchd state counts as not clean, and the Karabiner driver is reported but never removed.
• The new tests cover shell syntax, regex quoting, the cancel path, and teardown polling.

Minor notes, none blocking:

• LegacyPrivilegedRuntimeCleanup.run() only passes the PIDs found before the batch to the kill step. A root process that appears after the first inspection is only stopped if its launchd bootout works. The post-batch verification would catch it and report failure, so the user would be told to retry. This is acceptable.
• /var/log/* files are in isClean. If a lingering process recreates a log after the rm -f, the cleanup reports a failure. The retry path covers it.

[malpern]

Re the ninth claude[bot] review: no change needed. Both notes are the accepted cases, and in both the post-batch verification reports a failure instead of a false success. That failure message tells the user to try again or restart first. I'm keeping the log files in isClean, so a recreated log means something is still running.

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60KB; tests/docs tail not reviewed)

Findings:

1. LegacyPrivilegedRuntimeCleanup.run() – verification polling skipped on batch failure. When the batch exits nonzero (e.g. a single rm or kill hiccup) but launchd teardown is merely slow, after is read once with no retry, so a partially successful cleanup is reported as failure. Acceptable, but retry-on-failure is cheap if you want fewer false failures.

2. Batch script kp_timeout dependency. The script relies on kp_timeout from PrivilegedCommandRunner.Batch.scriptPrelude; the test only checks the string kp_timeout() { exists. Good, but bash -n won't catch it if the prelude is absent in a future refactor of the runner. Low risk.

3. InstallerEngine.removeLegacyPrivilegedRuntime(using _:) ignores the broker. The admin prompt goes through WizardDependencies.executePrivilegedBatch, so the PrivilegeBroker parameter is vestigial; callers (SettingsView) pass a fresh PrivilegeBroker(). Consider dropping it to avoid implying broker-mediated privilege.

4. KanataDaemonService.invalidateEvidenceCaches() resets lastObservedState = .stopped unconditionally; if the daemon is still legitimately running (cleanup failed, never called in that path since it runs after success) this is fine, but worth a comment that it must only be called post-verified removal.

5. Process pattern scope. KeyPath\.app/Contents/Library/(KeyPath|HelperTools)/ matches any root process under any KeyPath.app path; the root-uid re-check mitigates this. Looks safe.

No blocking correctness or security issues found in the reviewed portion. Test coverage of the cleanup flow (cancellation, partial failure, PID recycling guard, shell-quoting) is strong.

[malpern]

Re the tenth claude[bot] review (d43c872):
3. Dropped the unused PrivilegeBroker parameter from removeLegacyPrivilegedRuntime().
4. invalidateEvidenceCaches() now says it must only be called after a verified removal. Its one caller runs only on success.

1. Left as is on purpose. A non-zero batch is reported as a failure however the later evidence looks, so polling longer would not change the result. Retrying is cheap, and the message offers it.
   2 and 5: agreed, low risk. The root-UID re-check covers the pattern scope, and a test pins the kp_timeout definition.

🤖 Addressed by Claude Code

[claude]

Automated review of the supplied diff. The diff was truncated at 60KB, so the rest of the test files and the docs and guide changes were not reviewed.

No blocking findings in the reviewed portion.

• The cleanup design is sound. It uses one admin batch. It re-checks owner and command line as root before each signal. It treats unreadable launchd state as not clean. It never reports success after a failed batch. It only removes files KeyPath owns, and it reports the Karabiner driver without removing it.
• Minor, non-blocking: LegacyPrivilegedRuntime.processPattern matches KeyPath.app/Contents/Library/KeyPath/ for any owner. The ownerUID == 0 filter and the root-side ps -o uid= re-check make this safe. If a future build runs a root process from that path, it would be treated as legacy, so keep the pattern tied to 1.0.1 artifacts.
• Minor, non-blocking: testRootProcessFilterAgainstRealProcesses and the bash -n tests assume macOS. That is fine for this repo's CI, but they will fail on a non-macOS runner.

🤖 Generated with Claude Code

[malpern]

Re the eleventh claude[bot] review: no change needed.

• Pattern scope: no build on this branch runs any root process from those paths, since ADR-045 retires the privileged stack. The root-UID check plus the root-side ps -o uid= re-check are what keep the match on 1.0.1 artifacts.
• macOS-only tests: KeyPath is macOS-only and its CI runs only on macOS.

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60KB; test files and docs after the cut were not reviewed)

No blocking issues found. Notes:

1. Process-kill safety (good): the root script re-checks the UID and command line before each TERM/KILL, so a recycled PID is not signalled. processPattern is narrow enough that other vendors' helpers are not matched.

2. invalidateEvidenceCaches (KanataDaemonService.swift): it resets lastObservedState = .stopped. The doc comment says to call it only after verified removal, and the only call site is refreshLegacyDaemonRefusalAfterCleanup, which runs after report.success. That is fine today, but the internal method has no guard, so a future caller could hide a running daemon. Consider an assertion or a more specific name.

3. Verification window: verificationAttempts of 10 at 500ms gives about 5s for launchd teardown. A job that is slow to boot out would produce a "not fully removed" failure even though the removal would soon complete. The retry message covers this, so it is acceptable.

4. describeBatchFailure cancel detection: output.contains("-128") can match unrelated output, for example a path or PID containing "-128". Matching (-128) would be tighter.

5. Test coverage: the new tests cover the cleanup script, the process filter, and the routing well. I did not see a test that the Settings button replaces "Fix it" in the UI. offersLegacyRuntimeRemoval is trivial, so this is minor.

[malpern]

Re the twelfth claude[bot] review (36968c5):
4. Cancel detection now matches (-128) instead of any -128. A test checks that unrelated output containing -128 is not reported as a cancelled prompt.
2. Kept the name. Its doc comment requires a verified removal first, and its only caller, refreshLegacyDaemonRefusalAfterCleanup, runs only after one. A check inside the function would need the same daemon probe the caller makes right after.
3. Agreed it's acceptable. The failure message offers a retry and suggests a restart.
5. The button sits in a SwiftUI view with no snapshot harness for that state. It is shown exactly when offersLegacyRuntimeRemoval is true, which is tested, and the manual VM plan's step 6 checks it on screen.

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60KB; tests and docs tail not reviewed).

Findings:

1. LegacyPrivilegedRuntimeCleanup.run() — verification can pass while the batch failed partially, and the user is told "cancelled" or a generic error. This is minor. Behavior is conservative (a failed batch is never success), so no change is required.

2. processPattern is broad for root kill -KILL. The pattern /Library/KeyPath/bin/ and KeyPath\.app/Contents/Library/(KeyPath|HelperTools)/ matches any process whose command line contains those strings, e.g. an editor or tail with that path as an argument. The root script re-checks uid==0 and the same regex, but both checks use the same loose pattern. A root process like vim /Library/KeyPath/bin/kanata would be KILLed, and would also be counted as a leftover root process. Consider anchoring the pattern to the start of the command (^) or matching the executable path rather than the whole command line.

3. inspectLive treats any launchctl print exit code other than 0, 112 or 113 as unreadable. That blocks isClean, so the user can get a persistent "could not check" failure with no recovery path other than a restart. Worth confirming that gui/<uid> returns 112/113 on macOS versions KeyPath supports when the user has no GUI session (some versions return 125/"Domain does not support specified action").

4. invalidateEvidenceCaches() sets lastObservedState = .stopped unconditionally. It is called only after removeLegacyPrivilegedRuntime, but refreshLegacyDaemonRefusalAfterCleanup is also called when removal succeeded, so this is fine. Make sure no other caller can reach it while the daemon is live.

5. Settings flow: removeLegacyRuntimeAndStart calls kanataManager.startKanata even when refusalStillStands cleared only because the coordinator's evidence cache was stale-then-fresh. This is fine, but .removedButNotStarted shows an info toast with lastError, which may still hold the old legacy-refusal text. Clear it, or prefer a fixed message when lastError == legacyDaemonRefusalMessage.

6. Test coverage: the flow tests are good (shell syntax check, regex quoting check, PID-recycle guard). There is no test for the cache-invalidation path in KanataDaemonService.invalidateEvidenceCaches.

No blocking correctness or security issues found in the reviewed portion. Item 2 is the one I'd address before merge.

[malpern]

Re the thirteenth claude[bot] review (0da1376):
2. Fixed. Each alternative in the pattern is anchored at the start of the command line, so only the executable path can match. vim /Library/KeyPath/bin/kanata and tail -f …KeyPath.app/… no longer match, in the regex test and in the test that runs the real script's grep -E through bash. A live check confirmed pgrep -f honours the ^ anchor.
5. Can't happen today: clearing the refusal also calls onError?(nil). Guarded anyway, and if lastError is still the refusal text, the flow shows the fixed message instead. There's a test for that.
3. Kept strict. On macOS 27.3 I measured 113 for a missing service and 112 for a missing domain, and both count as absent. I haven't seen 125 from print. Any other code means the state is unknown, and treating unknown as gone could hide a running daemon. If the VM run turns up a code that means 'absent', it gets added to that list.
4. Its only caller runs after a verified removal, and its doc comment says so.
6. Already covered: testLingeringEnabledRegistrationIsNotARunningDaemonAfterCleanup exercises invalidateEvidenceCaches() against a real KanataDaemonService.

🤖 Addressed by Claude Code

[claude]

Review of the visible diff. It was truncated at 60KB, so I did not see the rest of the tests or the docs.

No blocking issues found. The root script is guarded: it re-checks the owner and command line of each PID as root, it bounds bootout with kp_timeout, and it only deletes KeyPath-specific paths. Success requires fresh evidence that nothing is left, and a failed batch is never reported as success.

Non-blocking:

1. Wizard path leaves the refusal in place. runSingleAction(.removeLegacyPrivilegedRuntime) removes the stack but does not call refreshLegacyDaemonRefusalAfterCleanup() or start the runtime. Only the Settings button does that, through LegacyRuntimeUpgradeFlow. MainAppStateController now attaches this autoFixAction to the issue, so any wizard UI that surfaces the issue could remove the daemon and still show the stale refusal until the next start attempt. Either route that action through the same flow, or confirm the wizard never shows this issue.
2. leftoverFiles makes isClean strict. The /var/log/* and /usr/local/bin/keypath-logrotate.sh entries count as leftovers. If anything recreates a log after rm -f, removal reports failure even though the daemon is gone. That is unlikely once the jobs are booted out, but it is worth keeping out of the isClean criteria. Logs could be best-effort instead.
3. Process pattern overlaps the current build. processPattern also matches current KeyPath.app/Contents/Library/KeyPath/... paths. The root-owner filter makes this safe today, but a comment saying so would help future edits.

[malpern]

Re the fourteenth claude[bot] review: no change needed.

1. The wizard never shows this issue. Wizard pages read SystemInspector issues, which never carry .removeLegacyPrivilegedRuntime, and the planner cannot produce it. Settings is the only entry point, and it runs the full flow: remove, re-check the refusal, start. ADR-045 records this. The runSingleAction route stays as tested plumbing.
2. Deliberate, as in earlier rounds. A failure here is reported as a failure, never as a false success, and the message offers a retry.

That concludes review rounds on this PR. Further changes will only be for a CI failure or a real defect.

🤖 Addressed by Claude Code

[claude]

Review (diff truncated at 60KB; test file tail not reviewed)

Overall the cleanup design looks solid: single admin prompt, fresh read-only verification, PID/owner/command re-check as root before signalling, and the driver is reported rather than removed. Findings:

1. Orphan-process gap (minor): the root script only signals PIDs found in the pre-batch inspection. A legacy root process that respawns or starts between inspect and batch (e.g. launchd KeepAlive before bootout lands) isn't killed; verification will then fail with 'still running' and ask the user to retry. Consider re-enumerating inside the root script (e.g. pgrep -f with the same pattern, filtered to uid 0) after the bootouts, rather than relying only on pre-collected PIDs.
2. Verification wait only on success: when operationFailure != nil no polling occurs, so a batch that exited non-zero only because of a trailing step (e.g. rmdir/timeouts) gets a single immediate inspect. Fine for cancel, but the message may be misleading for partial-success cases; consider polling when exit code != -128 case too.
3. invalidateEvidenceCaches sets lastObservedState = .stopped: safe after verified removal, but refreshLegacyDaemonRefusalAfterCleanup is public on the coordinator and could be called without that precondition; a doc note or guard would help.
4. Test coverage: SettingsView.removeLegacyRuntimeAndStart wiring (toast mapping, removingLegacyRuntime guard) and the primaryIssueDetail override are only indirectly covered via offersLegacyRuntimeRemoval.

No blocking issues found in the supplied portion.