README.md (1)

189-201: Consider adding a language specifier to the fenced code block.

The ASCII diagram doesn't require syntax highlighting, but adding a specifier (e.g., text or plaintext) would satisfy markdown linters and maintain consistency.

📝 Suggested fix

-```
+```text
 ┌─────────────────────────────────────────────────────────────────┐
 │  LOCAL [1]│ ● prod [2]│ ◐ staging [3]│ ○ dev [4]│      +       │
 ├─────────────────────────────────────────────────────────────────┤

src/remote/audit.ts (3)

30-33: Use dirname() instead of join(logPath, '..').

The current approach works but is less idiomatic. Using dirname() from node:path is clearer and more robust.

♻️ Suggested fix

+import { join, dirname } from 'node:path';
-import { join } from 'node:path';
 
 // In constructor:
-    this.logDir = join(logPath, '..');
+    this.logDir = dirname(logPath);

---

149-170: TOCTOU race condition in rotation logic.

There's a time-of-check to time-of-use gap between stat() and rename(). If multiple processes write concurrently, both could attempt rotation simultaneously. For a single-instance audit logger this is unlikely to cause issues, but consider documenting this limitation or using a lock file for multi-process scenarios.

---

154-154: Unnecessary dynamic import.

rename and unlink are already available from the static import at line 9. The dynamic import adds overhead and inconsistency.

♻️ Suggested fix

-import { appendFile, mkdir, readFile, stat } from 'node:fs/promises';
+import { appendFile, mkdir, readFile, stat, rename, unlink } from 'node:fs/promises';

// In rotateIfNeeded:
-        const { rename, unlink } = await import('node:fs/promises');
         const oldPath = this.logPath + '.old';

src/remote/types.ts (1)

250-258: Move imports to top of file.

The imports at line 250 break the convention of placing all imports at the top of the file. This makes dependencies harder to scan and could cause issues with some bundlers or linters.

♻️ Suggested fix

Move these imports to the top of the file after the existing imports:

import type { TrackerTask } from '../plugins/trackers/types.js';
import type {
  EngineEvent,
  EngineStatus,
  IterationResult,
  ActiveAgentState,
  RateLimitState,
  SubagentTreeNode,
} from '../engine/types.js';

website/content/docs/cli/listen.mdx (1)

29-29: Minor grammatical improvement.

Consider adding the definite article for better readability.

-On first run, a secure authentication token is generated:
+On the first run, a secure authentication token is generated:

src/commands/remote.test.ts (1)

160-171: Restore console.log via try/finally to avoid cross-test leakage.

If printRemoteHelp() throws or an assertion fails, console.log remains mocked and can taint later tests.

♻️ Suggested fix

-    const logs: string[] = [];
-    const originalLog = console.log;
-    console.log = (...args: unknown[]) => logs.push(args.join(' '));
-
-    expect(() => printRemoteHelp()).not.toThrow();
-
-    // Restore console.log
-    console.log = originalLog;
+    const logs: string[] = [];
+    const originalLog = console.log;
+    try {
+      console.log = (...args: unknown[]) => logs.push(args.join(' '));
+      expect(() => printRemoteHelp()).not.toThrow();
+    } finally {
+      console.log = originalLog;
+    }

src/tui/components/Header.tsx (1)

234-242: Guard against long remote labels to prevent header overflow.

Long aliases or hostnames can push the status and task text off-screen. Consider truncating the remote name/host in the header.

♻️ Suggested tweak

-            <text>
-              <span fg={colors.accent.primary}>{REMOTE_ICON} {remoteInfo.name}</span>
-              <span fg={colors.fg.dim}> ({remoteInfo.host}:{remoteInfo.port})</span>
+            <text>
+              <span fg={colors.accent.primary}>
+                {REMOTE_ICON} {truncateText(remoteInfo.name, 14)}
+              </span>
+              <span fg={colors.fg.dim}>
+                ({truncateText(`${remoteInfo.host}:${remoteInfo.port}`, 24)})
+              </span>
               <span fg={colors.fg.dim}> │ </span>
             </text>

src/commands/listen.test.ts (1)

108-126: Use try/finally when mocking console.log.

If the assertion fails, console.log stays mocked and can affect later tests.

♻️ Suggested fix

-    const logs: string[] = [];
-    const originalLog = console.log;
-    console.log = (...args: unknown[]) => logs.push(args.join(' '));
-
-    expect(() => printListenHelp()).not.toThrow();
-
-    console.log = originalLog;
+    const logs: string[] = [];
+    const originalLog = console.log;
+    try {
+      console.log = (...args: unknown[]) => logs.push(args.join(' '));
+      expect(() => printListenHelp()).not.toThrow();
+    } finally {
+      console.log = originalLog;
+    }

src/tui/components/Toast.tsx (1)

41-51: Add a defensive default in getVariantColors.

If a variant is ever cast from untyped input, the function could return undefined. A default keeps behaviour safe and predictable.

♻️ Suggested fix

 function getVariantColors(variant: ToastVariant): { border: string; text: string } {
   switch (variant) {
     case 'success':
       return { border: colors.status.success, text: colors.status.success };
     case 'warning':
       return { border: colors.status.warning, text: colors.status.warning };
     case 'error':
       return { border: colors.status.error, text: colors.status.error };
     case 'info':
       return { border: colors.status.info, text: colors.status.info };
+    default:
+      return { border: colors.status.info, text: colors.status.info };
   }
 }

src/remote/remote.test.ts (1)

347-381: Consider centralising the message type list to avoid drift.

The hard-coded messageTypes array can fall out of sync with protocol changes. A shared constant (or deriving from a single source) would reduce maintenance overhead.

src/remote/token.ts (2)

222-238: Constant-time comparison has a timing leak when string lengths differ.

The implementation attempts constant-time comparison, but when a.length !== b.length, it performs a different amount of work (iterating over b.length elements) before returning false. An attacker could potentially infer token length by measuring timing differences.

A more robust approach would pad both strings to the maximum expected length or use Node's built-in crypto.timingSafeEqual:

♻️ Suggested improvement using crypto.timingSafeEqual

+import { timingSafeEqual } from 'node:crypto';
+
 /**
  * Constant-time string comparison to prevent timing attacks.
  */
 function constantTimeCompare(a: string, b: string): boolean {
-  if (a.length !== b.length) {
-    // Still do a comparison to maintain constant time
-    // The _unused variable prevents timing attacks by doing work even when lengths differ
-    let _unused = 0;
-    for (let i = 0; i < b.length; i++) {
-      _unused |= b.charCodeAt(i) ^ (a.charCodeAt(i % a.length) || 0);
-    }
-    return false;
-  }
-
-  let result = 0;
-  for (let i = 0; i < a.length; i++) {
-    result |= a.charCodeAt(i) ^ b.charCodeAt(i);
-  }
-  return result === 0;
+  const aBuffer = Buffer.from(a);
+  const bBuffer = Buffer.from(b);
+  
+  if (aBuffer.length !== bBuffer.length) {
+    // Compare against self to maintain constant time, then return false
+    timingSafeEqual(aBuffer, aBuffer);
+    return false;
+  }
+  
+  return timingSafeEqual(aBuffer, bBuffer);
 }

---

280-285: Consider periodic automatic cleanup of expired connection tokens.

The activeConnectionTokens Map stores connection tokens in memory, and while cleanupExpiredTokens() exists, it must be called manually. For long-running server processes, this could lead to memory growth if cleanup isn't invoked regularly.

Consider setting up an automatic cleanup interval when the first token is issued:

♻️ Suggested automatic cleanup

let cleanupInterval: ReturnType<typeof setInterval> | null = null;

function startCleanupInterval(): void {
  if (!cleanupInterval) {
    // Clean up every hour
    cleanupInterval = setInterval(cleanupExpiredTokens, 60 * 60 * 1000);
  }
}

export function issueConnectionToken(clientId: string): ConnectionToken {
  startCleanupInterval();
  // ... rest of implementation
}

src/remote/client.ts (2)

259-262: Consider supporting secure WebSocket connections (WSS).

The client uses unencrypted ws:// for connections. For a remote monitoring and control system that transmits authentication tokens and can execute commands (pause, interrupt, config push), encrypted connections should be strongly considered, especially when connecting over untrusted networks.

♻️ Suggested improvement to support WSS

+  private useSecure: boolean;
+
   constructor(
     host: string,
     port: number,
     token: string,
     eventHandler: RemoteClientEventHandler,
-    reconnectConfig: Partial<ReconnectConfig> = {}
+    reconnectConfig: Partial<ReconnectConfig> = {},
+    useSecure = false
   ) {
     this.host = host;
     this.port = port;
     this.serverToken = token;
     this.eventHandler = eventHandler;
     this.reconnectConfig = { ...DEFAULT_RECONNECT_CONFIG, ...reconnectConfig };
+    this.useSecure = useSecure;
   }

   // In connect():
-  const url = `ws://${this.host}:${this.port}`;
+  const protocol = this.useSecure ? 'wss' : 'ws';
+  const url = `${protocol}://${this.host}:${this.port}`;

Is there a reason WSS support was not included? Please verify whether the deployment scenarios require encrypted connections.

---

268-275: Consider logging JSON parse errors for debugging.

The empty catch block silently ignores malformed messages. While ignoring invalid messages is reasonable, logging these at debug level would aid troubleshooting protocol issues or malicious input.

         this.ws.onmessage = (event) => {
           try {
             const message = JSON.parse(event.data as string) as WSMessage;
             this.handleMessage(message, resolve, reject);
-          } catch {
-            // Ignore invalid messages
+          } catch (error) {
+            // Log but don't fail on invalid messages
+            console.debug('Failed to parse WebSocket message:', error);
           }
         };

src/remote/instance-manager.ts (1)

374-379: Fragile assumption in isViewingRemote().

The method assumes the local tab is always at index 0. If tab ordering ever changes, this logic would break silently.

♻️ Suggested improvement for robustness

   isViewingRemote(): boolean {
-    return this.selectedIndex > 0;
+    const selectedTab = this.tabs[this.selectedIndex];
+    return selectedTab ? !selectedTab.isLocal : false;
   }

src/commands/run.tsx (1)

687-713: Move instance-manager initialisation to useEffect with cleanup; remove debug file writes.

This code has multiple issues:

1. Non-idiomatic React: Using useState(() => {...}) for side effects violates React conventions. The initialiser should be pure and only compute the initial state value.

2. Debug logging to /tmp/ralph-keys.log: Lines 690-692 and 709-712 write to a hardcoded file path. This is leftover debug code that should be removed before merging—it's a potential security/privacy concern and causes unnecessary I/O.

3. Missing cleanup: When the component unmounts, instanceManager.disconnectAll() is never called, which can leak connections.

🛠️ Suggested fix

+import { useEffect } from 'react';
+
   // Remote instance management
   const [instanceManager] = useState(() => new InstanceManager());
   const [instanceTabs, setInstanceTabs] = useState<InstanceTab[]>([]);
   const [selectedTabIndex, setSelectedTabIndex] = useState(0);
   const [connectionToast, setConnectionToast] = useState<ConnectionToastMessage | null>(null);

   // Initialize instance manager on mount
-  useState(() => {
-    instanceManager.onStateChange((tabs, selectedIndex) => {
-      import('fs').then(fs => {
-        fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] onStateChange: selectedIndex=${selectedIndex}, tabs=${tabs.length}\n`);
-      });
-      setInstanceTabs(tabs);
-      setSelectedTabIndex(selectedIndex);
-    });
-    instanceManager.onToast((toast) => {
-      setConnectionToast(toast as ConnectionToastMessage);
-      // Auto-clear toast after 3 seconds
-      setTimeout(() => setConnectionToast(null), 3000);
-    });
-    instanceManager.initialize().then(() => {
-      setInstanceTabs(instanceManager.getTabs());
-      setSelectedTabIndex(instanceManager.getSelectedIndex());
-    });
-  });
+  useEffect(() => {
+    instanceManager.onStateChange((tabs, selectedIndex) => {
+      setInstanceTabs(tabs);
+      setSelectedTabIndex(selectedIndex);
+    });
+    instanceManager.onToast((toast) => {
+      setConnectionToast(toast as ConnectionToastMessage);
+      setTimeout(() => setConnectionToast(null), 3000);
+    });
+    void instanceManager.initialize().then(() => {
+      setInstanceTabs(instanceManager.getTabs());
+      setSelectedTabIndex(instanceManager.getSelectedIndex());
+    });
+    return () => {
+      void instanceManager.disconnectAll();
+    };
+  }, [instanceManager]);

   // Handle tab selection
   const handleSelectTab = async (index: number): Promise<void> => {
-    const fs = await import('fs');
-    fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] handleSelectTab called with index: ${index}\n`);
     await instanceManager.selectTab(index);
-    fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] selectTab completed\n`);
   };

website/content/docs/cli/run.mdx (1)

250-253: Consider adding a note about the JSONL format.

The audit log example shows JSONL format, but users unfamiliar with this format might benefit from a brief note that each line is a separate JSON object.

📝 Suggested clarification

 ### Audit Logging

-All remote actions are logged to `~/.config/ralph-tui/audit.log`:
+All remote actions are logged to `~/.config/ralph-tui/audit.log` in JSONL format (one JSON object per line):

README.md (2)

193-201: Consider adding a language identifier to the fenced code block.

The ASCII diagram code block has no language specified. While ASCII art diagrams don't have a standard language, you can use text or leave it empty to satisfy the linter.

📝 Suggested fix

-```
+```text
 ┌─────────────────────────────────────────────────────────────────┐
 │  LOCAL [1]│ ● prod [2]│ ◐ staging [3]│ ○ dev [4]│      +       │

---

360-365: Clarify the distinction between remote.json and remotes.toml.

The configuration files table lists both remote.json (server token storage) and remotes.toml (remote server configurations). This is potentially confusing—consider clarifying that remote.json stores the local server's token when running as a listener, whilst remotes.toml stores configurations for connecting to remote servers.

📝 Suggested clarification

 | File | Purpose |
 |------|---------|
-| `~/.config/ralph-tui/remote.json` | Server token storage |
-| `~/.config/ralph-tui/remotes.toml` | Remote server configurations |
+| `~/.config/ralph-tui/remote.json` | Local server token (when running with `--listen`) |
+| `~/.config/ralph-tui/remotes.toml` | Remote server connection configs (client-side) |
 | `~/.config/ralph-tui/audit.log` | Audit log of all remote actions |
 | `~/.config/ralph-tui/listen.pid` | Daemon PID file |

src/commands/run.tsx (1)

1376-1386: Consider adding a comment explaining the intentional blocking.

The never-resolving promise pattern is valid for keeping the process alive, but the code structure is slightly confusing: await engine.dispose() on line 1388 becomes unreachable in listen mode. While the signal handlers do call engine.dispose() before process.exit(), a brief comment would clarify the intent.

💡 Optional clarification

   // In listen mode, keep process alive for remote connections
   if (listenMode) {
     logger.info('system', 'Engine idle. Waiting for remote commands (Ctrl+C to stop)...');

-    // Keep process alive until signal received
+    // Block indefinitely until SIGINT/SIGTERM triggers gracefulShutdown,
+    // which handles engine.dispose() before calling process.exit().
     await new Promise<void>((_resolve) => {
-      // The existing SIGINT/SIGTERM handlers will call process.exit()
-      // This promise just keeps the event loop alive indefinitely
     });
   }

src/commands/run.tsx (2)

681-713: Move instance-manager side-effects into useEffect and drop debug file writes.

Side-effects during render plus /tmp/ralph-keys.log writes are risky and leave no cleanup. Use useEffect with a cleanup that calls disconnectAll(), and remove the file writes.

🔧 Proposed fix

-import { useState } from 'react';
+import { useEffect, useState } from 'react';

-  // Initialize instance manager on mount
-  useState(() => {
-    instanceManager.onStateChange((tabs, selectedIndex) => {
-      import('fs').then(fs => {
-        fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] onStateChange: selectedIndex=${selectedIndex}, tabs=${tabs.length}\n`);
-      });
-      setInstanceTabs(tabs);
-      setSelectedTabIndex(selectedIndex);
-    });
-    instanceManager.onToast((toast) => {
-      setConnectionToast(toast as ConnectionToastMessage);
-      // Auto-clear toast after 3 seconds
-      setTimeout(() => setConnectionToast(null), 3000);
-    });
-    instanceManager.initialize().then(() => {
-      setInstanceTabs(instanceManager.getTabs());
-      setSelectedTabIndex(instanceManager.getSelectedIndex());
-    });
-  });
+  // Initialise instance manager on mount
+  useEffect(() => {
+    instanceManager.onStateChange((tabs, selectedIndex) => {
+      setInstanceTabs(tabs);
+      setSelectedTabIndex(selectedIndex);
+    });
+    instanceManager.onToast((toast) => {
+      setConnectionToast(toast as ConnectionToastMessage);
+      setTimeout(() => setConnectionToast(null), 3000);
+    });
+    void instanceManager.initialize().then(() => {
+      setInstanceTabs(instanceManager.getTabs());
+      setSelectedTabIndex(instanceManager.getSelectedIndex());
+    });
+    return () => {
+      void instanceManager.disconnectAll();
+    };
+  }, [instanceManager]);
@@
   const handleSelectTab = async (index: number): Promise<void> => {
-    const fs = await import('fs');
-    fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] handleSelectTab called with index: ${index}\n`);
     await instanceManager.selectTab(index);
-    fs.appendFileSync('/tmp/ralph-keys.log', `[${new Date().toISOString()}] selectTab completed\n`);
   };

---

1377-1385: Listen-mode shutdown bypasses remote and session cleanup.

In listen mode, runHeadless keeps the process alive and its signal handlers call process.exit(), so the cleanup after runHeadless (including remoteServer.stop(), endSession, and lock release) never runs. Consider a shared shutdown path (or pass remoteServer into runHeadless) so the signal flow awaits cleanup before exiting.

Also applies to: 1809-1812

src/remote/client.ts (1)

964-977: Persist the connection token on successful reconnect.
The reconnect auth_response success path still doesn’t store connectionToken/expiry or re-arm refresh, so a reconnected session can miss token refresh.

🔧 Proposed fix

           if (authResponse.success) {
             this._status = 'connected';
             this._connectedAt = new Date().toISOString();
+            // Store connection token if provided
+            if (authResponse.connectionToken && authResponse.connectionTokenExpiresAt) {
+              this.connectionToken = authResponse.connectionToken;
+              this.connectionTokenExpiresAt = authResponse.connectionTokenExpiresAt;
+              this.scheduleTokenRefresh();
+            }
             this.startMetricsInterval();
             this.eventHandler({ type: 'reconnected', totalAttempts: this.reconnectAttempts });
             this.reconnectAttempts = 0;
             this.startPingInterval();

src/remote/server.ts (2)

448-452: Return the actual bound port in getState().
When a fallback port is used, getState() still reports options.port. Prefer _actualPort to keep status accurate.

🔧 Proposed fix

-      port: this.options.port,
+      port: this._actualPort ?? this.options.port,

---

782-826: Validate iteration counts from remote clients.
count can be zero/negative or non-integer, which can lead to unexpected engine behaviour. Reject invalid values up-front.

🔧 Proposed fix

   private async handleAddIterations(
     ws: ServerWebSocket<WebSocketData>,
     message: AddIterationsMessage
   ): Promise<void> {
+    if (!Number.isInteger(message.count) || message.count <= 0) {
+      this.sendOperationError(ws, message.id, 'add_iterations', 'count must be a positive integer');
+      return;
+    }
     if (!this.options.engine) {
       this.sendOperationError(ws, message.id, 'add_iterations', 'No engine attached');
       return;
     }
@@
   private async handleRemoveIterations(
     ws: ServerWebSocket<WebSocketData>,
     message: RemoveIterationsMessage
   ): Promise<void> {
+    if (!Number.isInteger(message.count) || message.count <= 0) {
+      this.sendOperationError(ws, message.id, 'remove_iterations', 'count must be a positive integer');
+      return;
+    }
     if (!this.options.engine) {
       this.sendOperationError(ws, message.id, 'remove_iterations', 'No engine attached');
       return;
     }

Also applies to: 813-839

src/commands/run.tsx (3)

688-713: Debug logging and non-idiomatic React pattern still present.

The issues previously identified remain unaddressed:

1. Using useState(() => { ... }) for side effects is non-idiomatic and bypasses proper React lifecycle management
2. Debug file writes to /tmp/ralph-keys.log (lines 690-692, 709-712) must be removed

Move the initialisation to useEffect with a cleanup function that calls disconnectAll(), and remove all debug fs.appendFileSync calls.

---

1376-1386: Unreachable cleanup code after never-resolving promise.

When listenMode is true, this promise never resolves and the signal handlers call process.exit() directly. This means:

1. Line 1388 (engine.dispose()) is unreachable in listen mode
2. runHeadless never returns, so remoteServer.stop() in executeRunCommand (line 1816) is never called

The signal handlers properly dispose the engine, but the remote server cleanup is bypassed. Consider passing remoteServer into runHeadless or refactoring the shutdown flow to ensure remoteServer.stop() is called.

---

1814-1817: remoteServer.stop() is bypassed in listen mode.

As noted in the review of lines 1376-1386, when --listen is active, runHeadless blocks indefinitely and the signal handlers call process.exit() directly, meaning this cleanup code is never reached.

While the OS cleans up sockets on process termination, explicitly stopping the server ensures graceful shutdown logic (audit logging, client notifications) executes correctly.

src/tui/components/RemoteConfigView.tsx (1)

97-115: Clamp scroll offset to the content length.
j/G can drive scrollOffset past the end of the file, leaving blank content and making “bottom” approximate. Consider clamping against a computed max offset and reusing a single page-size constant.

♻️ Suggested refactor

-  const contentLines = currentContent?.split('\n') ?? [];
-  const visibleLines = contentLines.slice(scrollOffset, scrollOffset + 20);
-  const canScrollDown = scrollOffset + 20 < contentLines.length;
-  const canScrollUp = scrollOffset > 0;
+  const pageSize = 20;
+  const contentLines = currentContent?.split('\n') ?? [];
+  const maxOffset = Math.max(0, contentLines.length - pageSize);
+  const clampedOffset = Math.min(scrollOffset, maxOffset);
+  const visibleLines = contentLines.slice(clampedOffset, clampedOffset + pageSize);
+  const canScrollDown = clampedOffset < maxOffset;
+  const canScrollUp = clampedOffset > 0;

Also applies to: 142-146

README.md (1)

148-149: Clarify the C shortcut behaviour.

The current wording "Open settings (local) or view remote config (remote)" is ambiguous. Based on the PR description, Shift+C opens a read-only config viewer for both local and remote tabs.

✏️ Suggested doc tweak

-| `,` / `C` | Open settings (local) or view remote config (remote) |
+| `,` | Open settings |
+| `C` | View config (read-only, local or remote) |

src/remote/instance-manager.ts (1)

374-376: Consider documenting the tab ordering invariant.

The method assumes the local tab is always at index 0. While this is established in initialize() and maintained by other methods, consider adding a brief comment to document this invariant for future maintainers.

src/tui/components/LeftPanel.tsx (1)

103-106: Import ConnectionStatus from ../types.js instead of redefining locally.

This type is already exported from src/tui/types.ts (line 159-160) with identical values. Importing it would reduce duplication and ensure consistency if the type definition changes.

♻️ Proposed fix

-import type { LeftPanelProps, TaskItem } from '../types.js';
+import type { LeftPanelProps, TaskItem, ConnectionStatus } from '../types.js';

Then remove the local type definition at lines 103-106.

src/tui/types.ts (1)

157-160: Consider consolidating ConnectionStatus type definitions.

This type is defined in multiple places:

• src/tui/types.ts (here)
• src/remote/client.ts (same values, different order)
• src/tui/components/LeftPanel.tsx (local copy)

While the values are semantically identical, having a single source of truth would reduce maintenance burden. Consider either:

1. Re-exporting from src/remote/client.ts if this is the canonical source
2. Creating a shared types module for cross-layer types

src/remote/server.ts (3)

161-170: Consider using crypto.randomUUID() for stronger ID generation.

Math.random() is not cryptographically secure. While client IDs and message IDs aren't security-critical tokens, using crypto.randomUUID() (available in Bun) would be more robust and eliminate any potential for ID collisions in high-throughput scenarios.

♻️ Suggested improvement

-function generateClientId(): string {
-  return Math.random().toString(36).substring(2, 10);
-}
-
-function generateMessageId(): string {
-  return Math.random().toString(36).substring(2, 15);
-}
+function generateClientId(): string {
+  return crypto.randomUUID().slice(0, 8);
+}
+
+function generateMessageId(): string {
+  return crypto.randomUUID();
+}

---

1186-1201: Migration error is silently swallowed.

The fire-and-forget migration pattern catches errors but does nothing with them. Consider logging the failure for debugging purposes, even if the config write itself succeeded.

♻️ Suggested improvement

     // Run migration in background - don't await
     checkAndMigrate(cwd, { quiet: true }).then((result) => {
       if (result?.migrated) {
-        // Migration was performed
+        // Migration was performed - could emit an event or log
       }
     }).catch(() => {
-      // Migration failed, but config was still written successfully
+      // Migration failed, but config was still written successfully
+      // Consider: this.auditLogger.logFailure(clientId, 'push_config_migration', 'Migration failed');
     });

---

765-787: Async operation sends response after promise resolution but doesn't await audit logging.

The handleInterrupt method correctly awaits engine.stop() before sending the response. However, unlike other handlers, any audit logging for this operation appears to be missing.

src/tui/components/RemoteManagementOverlay.tsx (1)

415-419: Token masking shows fixed 8 asterisks when empty.

When the token field is empty and hidden, '*'.repeat(token.length || 8) displays 8 asterisks. This is a reasonable placeholder length, though it could mislead users about expected token length. Consider whether this is intentional UX or if showing "(hidden)" might be clearer for empty state.

♻️ Optional alternative

-          value={showToken ? token : '*'.repeat(token.length || 8)}
+          value={showToken ? token : (token.length > 0 ? '*'.repeat(token.length) : '(hidden)')}

src/tui/components/RunApp.tsx (1)

2555-2575: Consider the non-atomic nature of alias change operations.

When changing a remote's alias, the code first removes the old config and then adds the new one. If addRemote fails after removeRemote succeeds, the remote configuration is lost. While unlikely, this could leave the config in an inconsistent state.

Consider either:

1. Adding the new config first, then removing the old one (reverse order)
2. Adding a rollback mechanism if the add fails
3. Documenting this as a known limitation

♻️ Optional: Safer ordering

           if (editingRemote && editingRemote.alias !== data.alias) {
+            // Add new config first to avoid data loss if it fails
+            const result = await addRemote(data.alias, data.host, data.port, data.token);
+            if (!result.success) {
+              throw new Error(result.error || 'Failed to add remote');
+            }
             // Remove old config
             await removeRemote(editingRemote.alias);
             // Remove old tab
             instanceManager.removeTab(editingRemote.alias);
-            // Add new config
-            const result = await addRemote(data.alias, data.host, data.port, data.token);
-            if (!result.success) {
-              throw new Error(result.error || 'Failed to add remote');
-            }
             // Connect with new alias
             await instanceManager.addAndConnectRemote(data.alias, data.host, data.port, data.token);

tests/remote/remote.test.ts (4)

347-388: Consider improving or removing this low-value test.

This test merely verifies that an array of hardcoded string literals are non-empty strings, which provides negligible coverage. The comment correctly notes that actual server testing requires Bun.serve.

Consider either:

1. Removing this placeholder test entirely
2. Replacing it with an integration test that spins up a real server
3. At minimum, importing the message type literals from the actual source to catch typos

---

866-867: Consider using deterministic waiting instead of setTimeout.

Using setTimeout(resolve, 10) for synchronisation is fragile and can lead to flaky tests, especially under CI load. Consider:

• Using vi.useFakeTimers() / vi.advanceTimersByTime() (or Bun equivalents)
• Awaiting specific events or state changes
• Using a utility like waitFor that polls for a condition

This pattern appears at lines 867, 901, 929, and 1149.

---

843-844: Silent error swallowing may mask test failures.

The .catch(() => {}) pattern silently ignores all errors, which can hide legitimate failures. Consider either:

• Explicitly expecting specific error types: .catch(e => expect(e).toBeInstanceOf(ExpectedError))
• Using a more complete mock that doesn't throw unexpectedly
• Documenting which specific errors are expected and ignored

This pattern recurs throughout the mocked WebSocket tests.

---

990-1004: Consider using expect(...).resolves for cleaner async assertions.

The try/catch pattern is verbose. Consider:

await expect(manager.refresh()).resolves.not.toThrow();

Or simply:

// If refresh() resolves without throwing, this passes
await manager.refresh();