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/runtime/types.ts (1)

23-26: Consider making launchd metadata required when launchStrategy is "launchd".

launchdLabel and launchdLogDir are optional for all strategies, so type checks won’t prevent incomplete launchd manifests. A discriminated union would catch this earlier.

♻️ Suggested typing refinement

 export type RuntimeUnitManifest = {
   id: RuntimeUnitId;
   label: string;
   kind: RuntimeUnitKind;
   launchStrategy: RuntimeUnitLaunchStrategy;
@@
-  launchdLabel?: string;
-  launchdLogDir?: string;
+  launchdLabel?: string;
+  launchdLogDir?: string;
@@
 };
+
+export type RuntimeUnitManifestTyped =
+  | (RuntimeUnitManifest & {
+      launchStrategy: "launchd";
+      launchdLabel: string;
+      launchdLogDir: string;
+    })
+  | (RuntimeUnitManifest & {
+      launchStrategy: Exclude<RuntimeUnitLaunchStrategy, "launchd">;
+    });

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/runtime/types.ts` around lines 23 - 26, Refine the runtime
config types so that when launchStrategy is "launchd" the launchd-specific
fields are required: replace the single interface with a discriminated union
keyed on launchStrategy (e.g. type RuntimeConfig = LaunchdRuntimeConfig |
OtherRuntimeConfig), where LaunchdRuntimeConfig has launchStrategy: "launchd"
and required launchdLabel and launchdLogDir, and OtherRuntimeConfig has the
other strategy literal(s) and keeps those fields optional; update any references
expecting the old type to use the new RuntimeConfig/discriminant
(launchStrategy) for exhaustive checks.

tests/desktop/plist-generator.test.ts (1)

20-27: Add assertions for the newly introduced plist environment keys.

mockEnv now includes new controller/openclaw path fields, but the tests don’t verify those keys are present in generated plist output yet.

🧪 Suggested assertion additions

   const plist = generatePlist("controller", mockEnv);
@@
   expect(plist).toContain("<key>PORT</key>");
   expect(plist).toContain("<string>50800</string>");
+  expect(plist).toContain("<key>WEB_URL</key>");
+  expect(plist).toContain("<string>http://127.0.0.1:50801</string>");
+  expect(plist).toContain("<key>OPENCLAW_SKILLS_DIR</key>");
+  expect(plist).toContain("<key>SKILLHUB_STATIC_SKILLS_DIR</key>");
+  expect(plist).toContain("<key>PLATFORM_TEMPLATES_DIR</key>");
+  expect(plist).toContain("<key>OPENCLAW_BIN</key>");
+  expect(plist).toContain("<key>OPENCLAW_EXTENSIONS_DIR</key>");
+  expect(plist).toContain("<key>SKILL_NODE_PATH</key>");
+  expect(plist).toContain("<key>OPENCLAW_TMP_DIR</key>");

🤖 Prompt for AI Agents

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

In `@tests/desktop/plist-generator.test.ts` around lines 20 - 27, The test's
mockEnv now includes new controller/openclaw keys (webUrl, openclawSkillsDir,
skillhubStaticSkillsDir, platformTemplatesDir, openclawBinPath,
openclawExtensionsDir, skillNodePath, openclawTmpDir) but the plist-generator
test doesn't assert they appear in the generated plist; update the test that
consumes mockEnv (the plist generator test) to add assertions that the generated
plist output contains each of those keys and that their values match mockEnv
(e.g., assert presence of "webUrl" and its value, "openclawSkillsDir", etc.),
using the same generator invocation currently in the test to produce the plist
string/object.

tests/desktop/state-migration.test.ts (1)

25-41: Minor: Parent temp directory not cleaned up.

The base directory created by mkdtempSync (e.g., /tmp/nexu-migration-XXXXXX/) isn't tracked for cleanup in afterEach. Only sourceDir and targetDir (subdirectories) are deleted, leaving the empty parent behind. Over many test runs this accumulates garbage in /tmp.

♻️ Suggested fix to clean up parent directory

+let testBaseDir: string;
 let sourceDir: string;
 let targetDir: string;
 const logMessages: string[] = [];
 const log = (msg: string) => logMessages.push(msg);

 beforeEach(() => {
-  const base = mkdtempSync(join(tmpdir(), "nexu-migration-"));
-  sourceDir = join(base, "source");
-  targetDir = join(base, "target");
+  testBaseDir = mkdtempSync(join(tmpdir(), "nexu-migration-"));
+  sourceDir = join(testBaseDir, "source");
+  targetDir = join(testBaseDir, "target");
   mkdirSync(sourceDir, { recursive: true });
   mkdirSync(targetDir, { recursive: true });
   logMessages.length = 0;
 });

 afterEach(() => {
   try {
-    rmSync(sourceDir, { recursive: true, force: true });
-    rmSync(targetDir, { recursive: true, force: true });
+    rmSync(testBaseDir, { recursive: true, force: true });
   } catch {
     // cleanup best effort
   }
 });

🤖 Prompt for AI Agents

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

In `@tests/desktop/state-migration.test.ts` around lines 25 - 41, The temp parent
created by mkdtempSync in beforeEach isn't removed; change the setup to store
that parent path in a scoped variable (e.g., base or baseDir) accessible to
afterEach, and in afterEach call rmSync on that parent (with { recursive: true,
force: true }) in addition to removing sourceDir and targetDir; update
beforeEach/afterEach to reference the same variable names (created in beforeEach
and cleaned in afterEach) so the mkdtempSync parent folder is removed after each
test.

tests/desktop/launchd-bootstrap.test.ts (1)

57-61: Mock missing port property that production code expects.

The EmbeddedWebServer interface (in embedded-web-server.ts) now includes port: number, but this mock only returns { close: ... }. While current tests may pass because effectivePorts comes from env defaults, this could mask bugs if tests later check the actual web server port resolution.

♻️ Suggested fix to include port in mock

 vi.mock("../../apps/desktop/main/services/embedded-web-server", () => ({
   startEmbeddedWebServer: vi.fn().mockResolvedValue({
+    port: 50810,
     close: vi.fn().mockResolvedValue(undefined),
   }),
 }));

🤖 Prompt for AI Agents

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

In `@tests/desktop/launchd-bootstrap.test.ts` around lines 57 - 61, The test mock
for startEmbeddedWebServer is missing the required port property from the
EmbeddedWebServer interface; update the vi.mock for
"../../apps/desktop/main/services/embedded-web-server" so the mocked
startEmbeddedWebServer (used in launchd-bootstrap.test.ts) resolves to an object
that includes both port: <number> (use the same default/effective port your
tests expect) and close: fn(), e.g. return { port: <expectedPort>, close:
fn().mockResolvedValue(undefined) }; ensure the mocked shape matches the
EmbeddedWebServer interface so port resolution logic in code under test behaves
the same as production.

apps/desktop/main/services/quit-handler.ts (1)

211-231: Minor inconsistency in error logging severity.

In the window close handler (line 144), bootoutService errors use console.error, but here both bootout and waitForExit errors use console.warn. Consider aligning the severity for consistency.

♻️ Suggested alignment

       try {
         await opts.launchd.bootoutService(label);
       } catch (err) {
-        console.warn(
+        console.error(
           `bootout ${label} failed: ${err instanceof Error ? err.message : String(err)}`,
         );
       }

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/services/quit-handler.ts` around lines 211 - 231, The catch
for opts.launchd.bootoutService is using console.warn here but elsewhere (window
close handler) bootoutService errors use console.error; update the catch for
bootoutService to use console.error (and optionally change the waitForExit catch
to console.error as well) inside the decision === "quit-completely" block so
error severity is consistent; target the catch blocks around
opts.launchd.bootoutService and opts.launchd.waitForExit to make this change.

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

179-179: Remove unused variable _callCount.

The variable _callCount is declared and incremented but never used. This appears to be debug remnant.

♻️ Suggested fix

-      let _callCount = 0;
       mockExecFile.mockImplementation(
         (
           _cmd: string,
           args: string[],
           callback: (
             error: Error | null,
             result: { stdout: string; stderr: string },
           ) => void,
         ) => {
-          _callCount++;
           if (args.includes("print")) {

🤖 Prompt for AI Agents

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

In `@tests/desktop/launchd-manager-ops.test.ts` at line 179, Remove the unused
debug variable _callCount: delete its declaration (let _callCount = 0;) and
remove any increments or references to _callCount within the test (e.g., places
that do _callCount++), leaving the rest of the test logic intact; this cleans up
the test file and eliminates an unused variable warning.

---

73-77: Consider restoring process.platform after tests.

Modifying process.platform via Object.defineProperty in beforeEach without restoring the original value in afterEach could leak state to other test files if running in the same process. While Vitest typically isolates test files, it's safer to capture and restore:

♻️ Suggested fix

 describe("LaunchdManager", () => {
+  const originalPlatform = process.platform;
+
   beforeEach(() => {
     vi.clearAllMocks();
     Object.defineProperty(process, "platform", { value: "darwin" });
   });
+
+  afterEach(() => {
+    Object.defineProperty(process, "platform", { value: originalPlatform });
+  });

🤖 Prompt for AI Agents

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

In `@tests/desktop/launchd-manager-ops.test.ts` around lines 73 - 77, Save the
original process.platform to a variable and restore it after each test: declare
a variable (e.g., let originalPlatform: NodeJS.Platform) in the describe scope,
set originalPlatform = process.platform in beforeEach before you call
Object.defineProperty, and add an afterEach that restores process.platform using
Object.defineProperty(process, "platform", { value: originalPlatform }); keep
the existing vi.clearAllMocks() behavior.

apps/desktop/main/index.ts (2)

585-594: Direct mutation of runtimeConfig works but could be fragile.

The runtime config is mutated directly after launchd bootstrap to reflect effective ports. This works because the same object reference is passed to registerIpcHandlers. Consider adding a comment explaining this dependency, or refactoring to make the flow more explicit.

📝 Suggested documentation

   // Always sync runtimeConfig with actual effective ports — these may differ
   // from the initial config if ports were recovered from a previous session or
   // OS-assigned due to conflicts.
+  // NOTE: This mutates the same object passed to registerIpcHandlers(), which
+  // is gated by coldStartReady, so IPC callers always see the final values.
   const { controllerPort, openclawPort, webPort } =
     launchdResult.effectivePorts;

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/index.ts` around lines 585 - 594, The code directly mutates
runtimeConfig after reading launchdResult.effectivePorts (controllerPort,
openclawPort, webPort) and relies on the fact that the same object reference was
previously passed to registerIpcHandlers; make this explicit by either (A)
adding a short comment above the mutation referencing registerIpcHandlers and
that it relies on object identity, or (B) refactoring to avoid implicit mutation
by creating a new config object (copying existing runtimeConfig, updating
ports/urls) and passing that updated object into registerIpcHandlers (or calling
registerIpcHandlers only after updating runtimeConfig) so the flow is explicit
and robust. Ensure references to runtimeConfig, launchdResult.effectivePorts,
and registerIpcHandlers are updated accordingly.

---

509-521: Migration code is synchronous but marked for removal—optional refactor only if proven problematic.

The migration correctly runs before services start and uses synchronous operations (cpSync) to migrate critical agent sessions and identity files. However, this migration is explicitly temporary—the code contains a TODO: "Remove this migration after v0.3+ when all users have upgraded past v0.1.6."

Since this is a one-time upgrade path scheduled for removal, converting to async is not urgent. Only refactor if startup responsiveness during v0.1.6→current upgrades becomes a reported issue.

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/index.ts` around lines 509 - 521, The migration block runs
synchronously (using migrateOpenclawState) during startup which is intentional
and temporary; if you want to avoid blocking startup, make migrateOpenclawState
asynchronous and await it here instead of calling it synchronously: change
migrateOpenclawState to return a Promise, call await migrateOpenclawState({
targetStateDir: openclawStateDir, sourceStateDir: legacyStateDir, log: (msg) =>
logColdStart(`state-migration: ${msg}`) }) inside this non-dev branch (ensure
the surrounding function is async), otherwise leave the current synchronous
behavior as-is until the TODO removal.

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

273-281: Port probing could be slow if many consecutive ports are occupied.

findFreePort probes up to 10 ports sequentially using detectPortOccupier (which spawns lsof). If ports 50800-50809 are all occupied, this spawns 10 lsof processes before falling back to port 0. Consider probing fewer ports or using a faster check:

♻️ Reduce probing attempts or use socket-based check

-async function findFreePort(preferred: number): Promise<number> {
-  for (let offset = 0; offset < 10; offset++) {
+async function findFreePort(preferred: number): Promise<number> {
+  // Try fewer ports — if preferred is taken, likely so are adjacent ones
+  for (let offset = 0; offset < 3; offset++) {
     const port = preferred + offset;
     const occupier = await detectPortOccupier(port);
     if (!occupier) return port;
   }
-  // All 10 ports occupied — let OS assign
+  // Ports occupied — let OS assign
   return 0;
 }

The current approach is acceptable for correctness; this is just an optimization suggestion.

🤖 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 273 - 281, The
findFreePort function currently probes up to 10 ports sequentially using
detectPortOccupier (which shells out to lsof); to avoid slowdowns when many
ports are occupied, either reduce the max attempts (e.g., try 2–3 offsets
instead of 10) or replace detectPortOccupier with a socket-bind check that
attempts to listen on the candidate port (using Node's net.createServer/listen
and immediately close on success) and returns free/busy as a Promise; update
findFreePort to call the new/changed helper and fall back to 0 if none succeed.

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

670-695: execFileSync in refreshLaunchdUnit blocks the main thread.

Using execFileSync in the refresh path means getRuntimeState() blocks the main process while waiting for launchctl. With a 3s timeout, this could cause UI jank if launchctl is slow (e.g., system under load).

Consider whether this refresh needs to be synchronous, or if the orchestrator could use cached state with async updates:

However, I understand the sync requirement comes from getRuntimeState() being called synchronously. The 3s timeout is a reasonable safeguard. If this causes issues in practice, consider debouncing refreshes or making the IPC handler async.

🤖 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 670 - 695,
refreshLaunchdUnit currently uses execFileSync which blocks the main thread;
change it to a non-blocking implementation: replace execFileSync with the async
child_process.execFile (or util.promisify(execFile)) inside refreshLaunchdUnit,
return a Promise (make refreshLaunchdUnit async) and propagate async behavior to
callers like getRuntimeState or the IPC handler that invokes it; if you cannot
change callers to async, implement a cached-state approach where
refreshLaunchdUnit performs an async refresh and updates an in-memory cache
(e.g., runtimeStateCache) that synchronous getRuntimeState reads, and ensure
callers of the IPC handler are updated to await the async refresh or read the
cached value as appropriate. Ensure you modify function names refreshLaunchdUnit
and getRuntimeState and the IPC handler that calls them when applying the fix.

---

851-863: Fixed 1-second delay after startService could be replaced with active polling.

The 1-second setTimeout delay before refreshing status is a workaround. A more responsive approach would poll getServiceStatus in a tight loop with backoff until running or timeout. However, this is a minor optimization.

🤖 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 851 - 863,
Replace the fixed 1-second setTimeout after launchdManager.startService with an
active polling loop: repeatedly call refreshLaunchdUnit(record) (or directly
query the manager via a getServiceStatus/getService method if available) with a
short interval and exponential backoff until record.phase === ("running" as
RuntimeUnitRecord["phase"]) or a configurable timeout elapses; then call
logStateChange with reasonCode "start_succeeded" or "start_failed" based on the
final state. Ensure the loop aborts on errors, respects a max timeout, and does
not block the event loop (use await with sleep intervals). Keep the existing
symbols: launchdManager.startService, refreshLaunchdUnit, logStateChange, and
the RuntimeUnitRecord["phase"] check.

---

740-805: Log tailing implementation is solid but has a subtle edge case.

The byte-offset tracking and line-buffering logic is well-implemented. However, if a log file is truncated/rotated (size becomes smaller than prevOffset), fileSize <= prevOffset will skip reading entirely. Consider detecting truncation:

♻️ Handle log rotation/truncation

         const prevOffset = this.launchdLogOffsets.get(logFile.path) ?? 0;
         const fileSize = stat.size;
 
-        if (fileSize <= prevOffset) continue;
+        if (fileSize < prevOffset) {
+          // File was truncated (rotated) — reset to start
+          this.launchdLogOffsets.set(logFile.path, 0);
+        } else if (fileSize === prevOffset) {
+          continue; // No new content
+        }
+        const currentOffset = this.launchdLogOffsets.get(logFile.path) ?? 0;

🤖 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 740 - 805,
tailLaunchdLogs currently skips reading when fileSize <= prevOffset which misses
the truncation/rotation case; change the logic in tailLaunchdLogs (where
prevOffset is read from this.launchdLogOffsets) so that if fileSize < prevOffset
you treat the file as truncated: reset prevOffset to 0 (and update
this.launchdLogOffsets.set(logFile.path, 0) if desired) and continue processing
from the start of the file, whereas only when fileSize === prevOffset should you
continue (no new data). Adjust the readStart computation to use the updated
prevOffset so truncation will be handled correctly and you still apply the
existing maxRead cap.

apps/desktop/main/services/state-migration.ts (2)

158-166: Potential issue: cpSync without error handling for individual session files.

If copying a single session file fails (e.g., permission denied), the entire migration won't be marked complete. This is probably desired behavior, but consider whether logging the specific failed file would help debugging.

♻️ Suggested improvement for debuggability

     for (const file of sessionFiles) {
       const sourceFile = join(sourceSessionsDir, file);
       const targetFile = join(targetSessionsDir, file);
 
       if (!existsSync(targetFile)) {
-        cpSync(sourceFile, targetFile);
-        count++;
+        try {
+          cpSync(sourceFile, targetFile);
+          count++;
+        } catch (err) {
+          log(`failed to copy session file ${file}: ${err instanceof Error ? err.message : String(err)}`);
+          throw err; // Re-throw to prevent stamp from being written
+        }
       }
     }

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/services/state-migration.ts` around lines 158 - 166, Wrap
the per-file copy inside a try/catch so a single failing session file doesn't
abort the whole migration and so you can log which file failed; specifically, in
the loop that iterates sessionFiles and calls cpSync(sourceFile, targetFile)
(using sourceSessionsDir, targetSessionsDir, and updating count), catch errors
from cpSync, increment no count for failed files, and call the module's logger
(or console.error) with the file path and the caught error to aid debugging
while continuing to the next file.

---

191-196: safeReaddir silently swallows all errors.

While filtering out dotfiles is good, catching all errors without logging could hide issues like permission denied. Consider logging unexpected errors:

♻️ Suggested improvement

-function safeReaddir(dir: string): string[] {
-  try {
-    return readdirSync(dir).filter((name) => !name.startsWith("."));
-  } catch {
-    return [];
-  }
-}
+function safeReaddir(dir: string, log?: (msg: string) => void): string[] {
+  try {
+    return readdirSync(dir).filter((name) => !name.startsWith("."));
+  } catch (err) {
+    // ENOENT is expected if dir doesn't exist; log other errors
+    if (err instanceof Error && "code" in err && err.code !== "ENOENT" && log) {
+      log(`warning: could not read directory ${dir}: ${err.message}`);
+    }
+    return [];
+  }
+}

🤖 Prompt for AI Agents

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

In `@apps/desktop/main/services/state-migration.ts` around lines 191 - 196,
safeReaddir currently swallows all exceptions which can hide issues (e.g.,
permission errors); modify the function (safeReaddir) to catch the error into a
variable (catch (err)) and only silently return [] for expected cases like
ENOENT/ENOENT-like "not found" while logging unexpected errors (use
console.warn/console.error or the app logger) with context including the dir and
the error before returning []; keep the existing readdirSync usage and dotfile
filter but ensure unexpected errors are surfaced in logs instead of being
silently ignored.