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

349-359: Consider making the hint text configurable.

The hard‑coded “Pattern: prd*.json” hint will be misleading if this component is reused with different filters. Exposing a prop (or deriving it from fileExtension/filenamePrefix) would keep the UI accurate.

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

22-51: Make onFilePath required in file-prompt mode to prevent silent no-ops.

When mode='file-prompt', the component delegates to FileBrowser and calls onFilePath?.(path) using optional chaining. If onFilePath is omitted, file selection silently fails. A discriminated union type would enforce this contract at compile time, ensuring that onFilePath is always provided whenever mode='file-prompt' is set.

🔧 Example type-safe shape

+type EpicLoaderOverlayBaseProps = {
+  visible: boolean;
+  trackerName: string;
+  onCancel: () => void;
+};
+
-export interface EpicLoaderOverlayProps {
-  /** Whether the overlay is visible */
-  visible: boolean;
-  /** Mode: 'list' for beads-style selection, 'file-prompt' for json-style */
-  mode: EpicLoaderMode;
-  /** Available epics (for list mode) */
-  epics: TrackerTask[];
-  /** Whether epics are loading */
-  loading: boolean;
-  /** Error message if loading failed */
-  error?: string;
-  /** Tracker name for display */
-  trackerName: string;
-  /** Current epic ID (for highlighting) */
-  currentEpicId?: string;
-  /** Callback when an epic is selected */
-  onSelect: (epic: TrackerTask) => void;
-  /** Callback when user cancels (Escape) */
-  onCancel: () => void;
-  /** Callback when file path is submitted (file-prompt mode) */
-  onFilePath?: (path: string) => void;
-}
+export type EpicLoaderOverlayProps =
+  | (EpicLoaderOverlayBaseProps & {
+      mode: 'list';
+      epics: TrackerTask[];
+      loading: boolean;
+      error?: string;
+      currentEpicId?: string;
+      onSelect: (epic: TrackerTask) => void;
+      onFilePath?: never;
+    })
+  | (EpicLoaderOverlayBaseProps & {
+      mode: 'file-prompt';
+      onFilePath: (path: string) => void;
+      onSelect?: never;
+      epics?: never;
+      loading?: never;
+      error?: string;
+      currentEpicId?: string;
+    });

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

168-178: Consider user feedback on invalid paths.

When navigateToPath receives an invalid path that causes isDirectory to return false (including when the path doesn't exist), the error message says "Not a directory" which may be misleading for non-existent paths.

💡 Optional improvement for clearer error messaging

 const navigateToPath = useCallback(async (path: string) => {
   const expanded = expandPath(path);
-  const isDir = await isDirectory(expanded);
-  if (isDir) {
-    setCurrentPath(expanded);
-    setEditingPath(false);
-    setEditedPath('');
-  } else {
-    setError(`Not a directory: ${path}`);
+  try {
+    const isDir = await isDirectory(expanded);
+    if (isDir) {
+      setCurrentPath(expanded);
+      setEditingPath(false);
+      setEditedPath('');
+    } else {
+      setError(`Not a directory or does not exist: ${path}`);
+    }
+  } catch {
+    setError(`Cannot access: ${path}`);
   }
 }, [expandPath]);

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

180-192: Simplify callback wrapper.

The arrow function wrapper (path) => onFilePath(path) is redundant since onFilePath already has the correct signature.

♻️ Simplify to direct reference

     return (
       <FileBrowser
         visible={visible}
         fileExtension=".json"
         filenamePrefix="prd"
         trackerLabel={trackerName}
-        onSelect={(path) => onFilePath(path)}
+        onSelect={onFilePath}
         onCancel={onCancel}
       />
     );

src/sandbox/detect.test.ts (1)

14-21: Consider renaming the describe block or testing actual module exports.

The test suite is named "sandbox detection constants" but only verifies that Node.js's platform() function returns expected values. This tests stdlib behaviour rather than any code from the sandbox detection module itself. If the intent is to have a placeholder test suite, consider either:

1. Renaming to something more accurate (e.g., "platform utility behaviour")
2. Importing and testing actual constants or exports from the sandbox detection module

Given the header comment explains this file is intentionally minimal, this is a minor clarity improvement.

💡 Suggested rename for clarity

-describe('sandbox detection constants', () => {
-  test('platform returns expected values', () => {
+describe('platform utility', () => {
+  test('returns a known OS identifier', () => {
     const currentPlatform = platform();
-    // platform() should return a known value
     expect(['darwin', 'linux', 'win32', 'freebsd', 'openbsd', 'sunos', 'aix']).toContain(
       currentPlatform
     );
   });
 });

tests/sandbox/detect.test.ts (2)

18-36: Consider adding a comment about keeping this in sync with production code.

The local implementation mirrors src/sandbox/detect.ts but uses Bun.spawn to avoid mock pollution. This creates a maintenance risk if the production logic changes.

📝 Suggested documentation addition

 /**
  * Local implementation of commandExists using Bun.spawn to bypass mock pollution.
  * Mirrors the logic in src/sandbox/detect.ts but uses Bun's native spawn.
+ *
+ * IMPORTANT: Keep this implementation in sync with src/sandbox/detect.ts.
+ * If the production logic changes, update this function accordingly.
  *
  * NOTE: Uses process.platform instead of platform() from node:os to avoid
  * mock pollution from other tests that mock the node:os module.
  */

---

45-53: Use test.skipIf() for clearer platform-specific test skipping.

Early return; statements in conditional blocks mask issues in test reporting. Bun's test runner supports test.skipIf() to make the intent explicit and improve test output clarity.

♻️ Example refactor

-  test('returns true for which command', async () => {
-    // 'which' doesn't exist on Windows (uses 'where' instead)
-    if (process.platform === 'win32') {
-      return; // Skip on Windows
-    }
-    // 'which' should exist on Linux/macOS
-    const result = await commandExists('which');
-    expect(result).toBe(true);
-  });
+  test.skipIf(process.platform === 'win32')('returns true for which command', async () => {
+    // 'which' should exist on Linux/macOS (Windows uses 'where' instead)
+    const result = await commandExists('which');
+    expect(result).toBe(true);
+  });

Also applies to: lines 131–144, 146–159, 161–168

tests/plugins/agents/base-execute.test.ts (1)

458-600: Guard env mutations with try/finally to prevent leakage between tests.
If a spawn/expectation throws before the restore block, process.env changes can bleed into later tests. A small helper keeps cleanup consistent.

♻️ Example helper to guarantee env restore

+async function withEnv(
+  key: string,
+  value: string,
+  run: () => Promise<void>
+): Promise<void> {
+  const original = process.env[key];
+  process.env[key] = value;
+  try {
+    await run();
+  } finally {
+    if (original === undefined) {
+      delete process.env[key];
+    } else {
+      process.env[key] = original;
+    }
+  }
+}
@@
-      const originalValue = process.env.TEST_KEPT_VAR;
-      process.env.TEST_KEPT_VAR = 'should_remain';
-
-      let stdout = '';
-      const handle = agent.execute('TEST_KEPT_VAR', [], {
-        onStdout: (data) => {
-          stdout += data;
-        },
-      });
-
-      await handle.promise;
-
-      if (originalValue === undefined) {
-        delete process.env.TEST_KEPT_VAR;
-      } else {
-        process.env.TEST_KEPT_VAR = originalValue;
-      }
-
-      await agent.dispose();
+      let stdout = '';
+      await withEnv('TEST_KEPT_VAR', 'should_remain', async () => {
+        const handle = agent.execute('TEST_KEPT_VAR', [], {
+          onStdout: (data) => {
+            stdout += data;
+          },
+        });
+
+        await handle.promise;
+      });
+
+      await agent.dispose();

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

24-44: Reduce maintenance drift in the pass-through mock.
Manually listing every export is easy to forget when new exports are added. Consider spreading the real module and override only when needed.

♻️ Proposed refactor

-  mock.module('../setup/skill-installer.js', () => ({
-    listBundledSkills: realSkillInstaller.listBundledSkills,
-    isSkillInstalledAt: realSkillInstaller.isSkillInstalledAt,
-    resolveSkillsPath: realSkillInstaller.resolveSkillsPath,
-    installViaAddSkill: realSkillInstaller.installViaAddSkill,
-    resolveAddSkillAgentId: realSkillInstaller.resolveAddSkillAgentId,
-    buildAddSkillInstallArgs: realSkillInstaller.buildAddSkillInstallArgs,
-    expandTilde: realSkillInstaller.expandTilde,
-    computeSkillsPath: realSkillInstaller.computeSkillsPath,
-    getBundledSkillsDir: realSkillInstaller.getBundledSkillsDir,
-    isEloopOnlyFailure: realSkillInstaller.isEloopOnlyFailure,
-    getSkillStatusForAgent: realSkillInstaller.getSkillStatusForAgent,
-    AGENT_ID_MAP: realSkillInstaller.AGENT_ID_MAP,
-  }));
+  mock.module('../setup/skill-installer.js', () => ({
+    ...realSkillInstaller,
+  }));

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

158-188: Resolve typed relative paths against the current directory for more intuitive navigation.

Right now, relative inputs resolve against process CWD; using currentPath is more consistent with the browser context and matches user expectations when typing paths relative to the currently displayed directory.

♻️ Proposed change

  const expandPath = useCallback((path: string): string => {
    let expanded = path.trim();
    if (expanded.startsWith('~')) {
      expanded = homedir() + expanded.slice(1);
    }
-   return resolve(expanded);
-  }, []);
+   return resolve(currentPath, expanded);
+  }, [currentPath]);