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".

apps/desktop/main/services/launchd-bootstrap.ts (1)

725-743: Consider resolving pgrep path programmatically.

The pgrep command is invoked via execFileAsync("pgrep", ...) which relies on the system PATH. Per coding guidelines for apps/desktop/**/*.ts, shell commands should not rely on the user's PATH. While pgrep is typically at /usr/bin/pgrep on macOS, consider using an absolute path for robustness in packaged apps.

🔧 Suggested path resolution

+const PGREP_PATH = "/usr/bin/pgrep";
+
 async function findNexuProcessPids(): Promise<number[]> {
   const allPids = new Set<number>();
 
   for (const pattern of NEXU_PROCESS_PATTERNS) {
     try {
-      const { stdout } = await execFileAsync("pgrep", ["-f", pattern]);
+      const { stdout } = await execFileAsync(PGREP_PATH, ["-f", pattern]);
       for (const line of stdout.trim().split("\n")) {

As per coding guidelines: "Desktop packaged app: never use npx, npm, pnpm, or any shell command that relies on the user's PATH."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/main/services/launchd-bootstrap.ts` around lines 725 - 743, The
function findNexuProcessPids currently calls execFileAsync("pgrep", ...) which
relies on the user's PATH; update it to call pgrep via an absolute, validated
path (e.g. define a PGREP_PATH constant like "/usr/bin/pgrep"), use that
constant in the execFileAsync call instead of "pgrep", and validate
existence/readability of PGREP_PATH (fs.access or similar) before calling;
reference the function name findNexuProcessPids, the execFileAsync invocation,
and NEXU_PROCESS_PATTERNS when making the change so the replacement is localized
and robust for packaged desktop apps.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

scripts/desktop-stop-smoke.sh (1)

107-112: Anchor runtime-ports.json check to script location, not caller CWD.

Line 107 uses a relative path; invoking this script outside repo root can produce false results.

Proposed change

-RUNTIME_PORTS=".tmp/launchd/runtime-ports.json"
+RUNTIME_PORTS="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/.tmp/launchd/runtime-ports.json"
 if [ ! -f "$RUNTIME_PORTS" ]; then
   pass "no stale runtime-ports.json"
 else

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/desktop-stop-smoke.sh` around lines 107 - 112, The check for
RUNTIME_PORTS uses a relative path and must be anchored to the script's
directory; compute the script base directory (e.g., using dirname/realpath of
"$0") and build RUNTIME_PORTS from that base instead of the current working
directory so the if [ ! -f "$RUNTIME_PORTS" ] check always targets
.tmp/launchd/runtime-ports.json inside the repository; update the RUNTIME_PORTS
variable assignment in scripts/desktop-stop-smoke.sh and ensure subsequent
references use that anchored path.

tests/desktop/quit-handler.test.ts (2)

87-104: The “full sequence” test name promises order, but order is not asserted.

Line 87 describes strict sequencing, yet Lines 96-104 only assert that calls happened. Add explicit ordering assertions to catch regressions in teardown flow.

✅ Example assertion pattern

   it("quit-completely calls onBeforeQuit → webServer.close → teardown → exit", async () => {
+    const calls: string[] = [];
     const opts = createQuitOpts();
+    opts.onBeforeQuit = vi.fn(async () => {
+      calls.push("onBeforeQuit");
+    });
+    opts.webServer.close = vi.fn(async () => {
+      calls.push("webServer.close");
+    });
+    mockTeardown.mockImplementation(async () => {
+      calls.push("teardown");
+    });
+    mockApp.exit.mockImplementation(() => {
+      calls.push("exit");
+    });
@@
     expect(mockApp.exit).toHaveBeenCalledWith(0);
+    expect(calls).toEqual([
+      "onBeforeQuit",
+      "webServer.close",
+      "teardown",
+      "exit",
+    ]);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/quit-handler.test.ts` around lines 87 - 104, Test
"quit-completely calls onBeforeQuit → webServer.close → teardown → exit" claims
a strict call order but only checks presence; modify the test around
quitWithDecision to assert call ordering between opts.onBeforeQuit,
opts.webServer.close, mockTeardown, and mockApp.exit (e.g. compare their mock
invocationCallOrder or inspect mock.calls indices) so the test explicitly
verifies that onBeforeQuit ran before webServer.close, which ran before
mockTeardown, which ran before mockApp.exit.

---

61-70: Replace unsafe as never casts with proper QuitHandlerOptions typing.

The createQuitOpts function uses as never casts to suppress type errors instead of properly typing its return value. Since QuitHandlerOptions is exported from the quit-handler module, the function should be explicitly typed to return QuitHandlerOptions, and test calls should pass the result without casts.

♻️ Proposed refactor

-function createQuitOpts(overrides?: Record<string, unknown>) {
+function createQuitOpts(overrides?: Partial<QuitHandlerOptions>): QuitHandlerOptions {
   return {
-    launchd: {} as never,
+    launchd: {} as QuitHandlerOptions["launchd"],
     labels: { controller: "io.nexu.controller", openclaw: "io.nexu.openclaw" },
     plistDir: "/tmp/test-plist",
     webServer: { close: vi.fn().mockResolvedValue(undefined) },
     onBeforeQuit: vi.fn().mockResolvedValue(undefined),
     onForceQuit: vi.fn(),
     ...overrides,
-  };
+  } as QuitHandlerOptions;
 }

-    await quitWithDecision("quit-completely", opts as never);
+    await quitWithDecision("quit-completely", opts);

Also applies to lines 94–95, 121–122, 135–136, 152–153, 173–174, 189–190, 205–206, 222–223.

Run pnpm typecheck and pnpm lint after making these changes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/quit-handler.test.ts` around lines 61 - 70, The createQuitOpts
helper currently uses unsafe `as never` casts; change its signature to return
the exported QuitHandlerOptions type (e.g. function createQuitOpts(overrides?:
Partial<QuitHandlerOptions>): QuitHandlerOptions) and replace the casted fields
(launchd, webServer, onBeforeQuit, onForceQuit, labels, plistDir, etc.) with
properly typed mock values that satisfy QuitHandlerOptions (use Partial for
overrides and merge into a fully typed object), then remove any call-site casts
that relied on `as never` (the other createQuitOpts usages referenced in the
comment). After updating createQuitOpts and all test usages, run pnpm typecheck
and pnpm lint to confirm types and linting pass.

tests/desktop/daemon-supervisor.test.ts (1)

309-326: Cover the two suffix fallbacks as well.

This only exercises command === process.execPath. The production guard also relies on /Electron and /electron suffix matches, so those new heuristics can regress without any test failure.

Suggested test shape

-it("forces ELECTRON_RUN_AS_NODE=1 when command is process.execPath", async () => {
+it.each([
+  process.execPath,
+  "/Applications/Electron.app/Contents/MacOS/Electron",
+  "/usr/local/bin/electron",
+])("forces ELECTRON_RUN_AS_NODE=1 when command is %s", async (command) => {
   const { RuntimeOrchestrator } = await import(
     "../../apps/desktop/main/runtime/daemon-supervisor"
   );

   const orchestrator = new RuntimeOrchestrator([
     makeManagedManifest("controller", {
-      command: process.execPath,
+      command,
       // Deliberately omit ELECTRON_RUN_AS_NODE from manifest env
       env: { PORT: "50800" },
     }),
   ] as never[]);

   await orchestrator.startAutoStartManagedUnits();

   const spawnEnv = mockSpawn.mock.calls[0][2].env;
   expect(spawnEnv.ELECTRON_RUN_AS_NODE).toBe("1");
-});
+});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/daemon-supervisor.test.ts` around lines 309 - 326, The test
only asserts the exact match case (command === process.execPath) but misses the
two suffix-fallback heuristics that also should set ELECTRON_RUN_AS_NODE; update
the test in daemon-supervisor.test.ts (the "forces ELECTRON_RUN_AS_NODE=1 when
command is process.execPath" case) to include two additional scenarios where
RuntimeOrchestrator is constructed with manifests whose command strings end with
"/Electron" and "/electron" respectively (using makeManagedManifest to build
each manifest), call startAutoStartManagedUnits for each, and assert
mockSpawn.mock.calls[].env.ELECTRON_RUN_AS_NODE === "1" for those calls so all
three heuristics (exact process.execPath, "/Electron" suffix, "/electron"
suffix) are covered.

apps/desktop/main/runtime/daemon-supervisor.ts (1)

1007-1019: Extend the Electron fallback to utility-process launches (defensive measure).

Currently, no managed units use runner: "utility-process", but this predicate checks only manifest.command, which is not consulted by utilityProcess.fork(). For future safety, extend the Electron detection to also cover the utility-process runner.

Suggested change

     const isElectronBinary =
+      manifest.runner === "utility-process" ||
       manifest.command === process.execPath ||
       manifest.command?.endsWith("/Electron") ||
       manifest.command?.endsWith("/electron");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/desktop/main/runtime/daemon-supervisor.ts` around lines 1007 - 1019, The
Electron-detection predicate (isElectronBinary) only checks manifest.command but
misses cases where the unit uses the utility-process runner; update
isElectronBinary to also return true when the manifest indicates runner:
"utility-process" (e.g., manifest.runner === "utility-process" or presence of
manifest.utilityProcess) so that the ELECTRON_RUN_AS_NODE fallback is applied to
utilityProcess.fork launches as well; change the isElectronBinary expression in
daemon-supervisor.ts to include this runner check while keeping the existing
command-based checks.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/desktop/embedded-web-server.test.ts (2)

33-35: Temp directory is not cleaned up after tests.

The webRoot temp directory created via mkdtempSync is never removed, leaving orphan files on disk. Consider cleaning up in afterAll.

🧹 Proposed fix to clean up temp directory

+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
-import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs";

   afterAll(async () => {
     await server?.close();
+    if (webRoot) {
+      rmSync(webRoot, { recursive: true, force: true });
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/embedded-web-server.test.ts` around lines 33 - 35, The test
creates a temporary webRoot directory via mkdtempSync but never cleans it up;
update the afterAll block (where server?.close() is called) to also remove the
temporary directory (webRoot) when tests finish — e.g., check that webRoot is
defined and then remove it recursively/forcefully using the fs API (fs.rmSync or
fs.rmdirSync with recursive) to avoid orphaned files; reference the webRoot
variable and the existing afterAll teardown to place the cleanup.

---

17-17: Fixed port may cause test flakiness in CI.

If port 51777 is already in use by another process or parallel test run, startEmbeddedWebServer will fail. Consider using port 0 to let the OS assign an available port, then read the actual port from the server instance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/embedded-web-server.test.ts` at line 17, The test uses a fixed
TEST_PORT (const TEST_PORT = 51777) which can cause CI flakiness; change the
test to pass port 0 to startEmbeddedWebServer so the OS picks an available port,
then read the actual bound port from the returned server instance (or
server.address().port) and use that dynamic port in subsequent
requests/assertions; update any references to TEST_PORT and the
startEmbeddedWebServer call to use 0 and replace usages with the server's actual
port.

tests/desktop/quit-handler-full.test.ts (1)

75-88: Tighten createQuitOpts typing to eliminate as never casts.

The loose Record<string, unknown> return type in createQuitOpts forces 14 as never casts across the test file. Since QuitHandlerOptions is exported from quit-handler.ts, type the helper or fixture to match it directly—either by importing QuitHandlerOptions or using Parameters<typeof installLaunchdQuitHandler>[0]—so type mismatches fail at compile time.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/quit-handler-full.test.ts` around lines 75 - 88, The
createQuitOpts helper currently returns a loose Record and uses `as never`
casts; change its signature to return the concrete type used by the handler
(e.g. import and use `QuitHandlerOptions` or use `Parameters<typeof
installLaunchdQuitHandler>[0]`) and type the `overrides` parameter as
`Partial<QuitHandlerOptions>` (or the equivalent Parameters-based type); update
the function return type accordingly and remove all `as never` casts inside the
fixture so TypeScript will catch mismatches at compile time while preserving the
mock shapes (e.g. keep vi.fn() values but typed to match the corresponding
properties).

tests/desktop/launchd-manager-ops-extended.test.ts (1)

83-377: Consolidate overlapping suites to reduce maintenance drift.

This file largely duplicates scenarios already covered in tests/desktop/launchd-manager-ops.test.ts (Line 348-573) for the same five methods. Consider keeping one detailed suite and trimming the other to non-overlapping smoke coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/launchd-manager-ops-extended.test.ts` around lines 83 - 377,
This test file duplicates full coverage of the same five LaunchdManager
behaviors (uninstallService, stopServiceGracefully, restartService,
rebootstrapFromPlist, hasPlistFile); please consolidate by trimming this file to
non-overlapping smoke tests only (or remove redundant it blocks) and keep one
authoritative, detailed suite elsewhere: identify the describe blocks named
"uninstallService", "stopServiceGracefully", "restartService",
"rebootstrapFromPlist", and "hasPlistFile" in this file, remove or reduce
duplicate it(...) cases that mirror the other suite, and retain only minimal
assertions that exercise happy-path behavior (using existing setupExecFile and
fs mocks) or unique edge cases not covered elsewhere so maintenance won’t drift.

tests/desktop/update-manager-full.test.ts (1)

513-600: Add coverage before the first periodic timer fires.

Both timer specs only interact after initialDelayMs has elapsed. A regression that double-schedules the startup timeout on repeated startPeriodicCheck() calls, or forgets to cancel that startup timeout in stopPeriodicCheck(), would still pass here.

🧪 Suggested cases

 describe("startPeriodicCheck", () => {
+  it("is a no-op when called twice before initialDelayMs elapses", async () => {
+    vi.useFakeTimers();
+    mockAutoUpdater.checkForUpdates.mockResolvedValue({
+      updateInfo: { version: "0.2.0", releaseDate: "2026-03-15" },
+    });
+
+    const { mgr } = await createManager(undefined, {
+      initialDelayMs: 1000,
+      checkIntervalMs: 5000,
+    });
+
+    mgr.startPeriodicCheck();
+    mgr.startPeriodicCheck();
+
+    await vi.advanceTimersByTimeAsync(1000);
+    expect(mockAutoUpdater.checkForUpdates).toHaveBeenCalledTimes(1);
+  });
+
   it("calling startPeriodicCheck twice after timer is set is a no-op", async () => {
     vi.useFakeTimers();
     mockAutoUpdater.checkForUpdates.mockResolvedValue({
       updateInfo: { version: "0.2.0", releaseDate: "2026-03-15" },
     });
@@
 describe("stopPeriodicCheck", () => {
+  it("cancels the startup timeout before the first check", async () => {
+    vi.useFakeTimers();
+    mockAutoUpdater.checkForUpdates.mockResolvedValue({
+      updateInfo: { version: "0.2.0", releaseDate: "2026-03-15" },
+    });
+
+    const { mgr } = await createManager(undefined, {
+      initialDelayMs: 1000,
+      checkIntervalMs: 5000,
+    });
+
+    mgr.startPeriodicCheck();
+    mgr.stopPeriodicCheck();
+
+    await vi.advanceTimersByTimeAsync(1000);
+    expect(mockAutoUpdater.checkForUpdates).not.toHaveBeenCalled();
+  });
+
   it("clears the interval and prevents further checks", async () => {
     vi.useFakeTimers();
     mockAutoUpdater.checkForUpdates.mockResolvedValue({
       updateInfo: { version: "0.2.0", releaseDate: "2026-03-15" },
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/desktop/update-manager-full.test.ts` around lines 513 - 600, Add tests
in tests/desktop/update-manager-full.test.ts that assert behavior before the
initialDelayMs fires: (1) calling mgr.startPeriodicCheck() twice before the
initial delay should not schedule duplicate startup timeouts (advance timers by
less than initialDelayMs then by initialDelayMs and verify checkForUpdates
called only once, then advance by checkIntervalMs to verify only a single
interval), and (2) mgr.stopPeriodicCheck() called after startPeriodicCheck but
before the initialDelayMs should cancel the pending startup timeout (start, call
stopPeriodicCheck(), advance past initialDelayMs and ensure
mockAutoUpdater.checkForUpdates was never called). Reference the
startPeriodicCheck and stopPeriodicCheck methods on mgr and the
initialDelayMs/checkIntervalMs options when adding these specs.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes