test: address review feedback on mock removal changes

- Remove redundant CACHE_DIR alias, use tmpDir directly
- Add force:true to afterAll rmSync for robustness
- Wrap chmod test in try/finally to prevent leaked read-only state
- Remove vacuous conditional assertion in resolve-ui-dist-dir
- Switch get-trace to beforeEach/afterEach for test isolation consistency

Author: Hweinstock

src/cli/__tests__/update-notifier.test.ts

@@ -1,45 +1,37 @@
 import { type UpdateCheckResult, checkForUpdate, printUpdateNotification } from '../update-notifier.js';
 import { rmSync } from 'fs';
-import { mkdir, readFile, writeFile } from 'fs/promises';
+import { chmod, mkdir, readFile, writeFile } from 'fs/promises';
 import { join } from 'path';
 import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
-const tmpDir = vi.hoisted(() => {
-  /* eslint-disable @typescript-eslint/no-require-imports */
-  const fs = require('fs');
-  const os = require('os');
-  const path = require('path');
-  /* eslint-enable @typescript-eslint/no-require-imports */
-  return fs.mkdtempSync(path.join(os.tmpdir(), 'update-notifier-test-'));
-});
+/* eslint-disable @typescript-eslint/no-require-imports -- vi.hoisted runs before ESM imports are available */
+const tmpDir = vi.hoisted(() =>
+  require('fs').mkdtempSync(require('path').join(require('os').tmpdir(), 'update-notifier-test-'))
+);
+/* eslint-enable @typescript-eslint/no-require-imports */
+
+const mockFetchLatestVersion = vi.hoisted(() => vi.fn());
 
 vi.mock('../../lib/schemas/io/global-config.js', () => ({
   GLOBAL_CONFIG_DIR: tmpDir,
 }));
 
-vi.mock('../constants.js', async importOriginal => {
-  const actual = await importOriginal<typeof import('../constants.js')>();
-  return { ...actual, PACKAGE_VERSION: '1.0.0' };
-});
-
-const { mockFetchLatestVersion } = vi.hoisted(() => ({
-  mockFetchLatestVersion: vi.fn(),
+vi.mock('../constants.js', async importOriginal => ({
+  ...(await importOriginal<typeof import('../constants.js')>()),
+  PACKAGE_VERSION: '1.0.0',
 }));
 
 vi.mock('../commands/update/action.js', async importOriginal => {
   const actual = await importOriginal<typeof import('../commands/update/action.js')>();
   return { ...actual, fetchLatestVersion: mockFetchLatestVersion };
 });
 
-const CACHE_DIR = tmpDir;
 const CACHE_FILE = join(tmpDir, 'update-check.json');
 
 describe('checkForUpdate', () => {
   beforeEach(() => {
     vi.spyOn(Date, 'now').mockReturnValue(1708646400000);
-    try {
-      rmSync(CACHE_DIR, { recursive: true });
-    } catch {}
+    rmSync(tmpDir, { recursive: true, force: true });
   });
 
   afterEach(() => {
@@ -48,7 +40,7 @@ describe('checkForUpdate', () => {
   });
 
   afterAll(() => {
-    rmSync(tmpDir, { recursive: true });
+    rmSync(tmpDir, { recursive: true, force: true });
   });
 
   it('fetches from registry when no cache exists', async () => {
@@ -61,7 +53,7 @@ describe('checkForUpdate', () => {
   });
 
   it('uses cache when last check was less than 24 hours ago', async () => {
-    await mkdir(CACHE_DIR, { recursive: true });
+    await mkdir(tmpDir, { recursive: true });
     await writeFile(CACHE_FILE, JSON.stringify({ lastCheck: 1708646400000 - 1000, latestVersion: '2.0.0' }), 'utf-8');
 
     const result = await checkForUpdate();
@@ -71,7 +63,7 @@ describe('checkForUpdate', () => {
   });
 
   it('fetches from registry when cache is expired', async () => {
-    await mkdir(CACHE_DIR, { recursive: true });
+    await mkdir(tmpDir, { recursive: true });
     await writeFile(
       CACHE_FILE,
       JSON.stringify({ lastCheck: 1708646400000 - 25 * 60 * 60 * 1000, latestVersion: '1.5.0' }),
@@ -119,7 +111,7 @@ describe('checkForUpdate', () => {
   });
 
   it('returns null on cache parse error and fetch error', async () => {
-    await mkdir(CACHE_DIR, { recursive: true });
+    await mkdir(tmpDir, { recursive: true });
     await writeFile(CACHE_FILE, 'invalid json', 'utf-8');
     mockFetchLatestVersion.mockRejectedValue(new Error('network error'));
 
@@ -129,18 +121,19 @@ describe('checkForUpdate', () => {
   });
 
   it('succeeds even when cache write fails', async () => {
-    await mkdir(CACHE_DIR, { recursive: true });
+    await mkdir(tmpDir, { recursive: true });
     await writeFile(CACHE_FILE, '', 'utf-8');
-    const { chmod } = await import('fs/promises');
-    await chmod(CACHE_DIR, 0o444);
+    await chmod(tmpDir, 0o444);
 
-    mockFetchLatestVersion.mockResolvedValue('2.0.0');
-
-    const result = await checkForUpdate();
+    try {
+      mockFetchLatestVersion.mockResolvedValue('2.0.0');
 
-    expect(result).toEqual({ updateAvailable: true, latestVersion: '2.0.0' });
+      const result = await checkForUpdate();
 
-    await chmod(CACHE_DIR, 0o755);
+      expect(result).toEqual({ updateAvailable: true, latestVersion: '2.0.0' });
+    } finally {
+      await chmod(tmpDir, 0o755);
+    }
   });
 });
 

src/cli/operations/dev/web-ui/__tests__/resolve-ui-dist-dir.test.ts

@@ -39,12 +39,4 @@ describe('resolveUIDistDir', () => {
 
     expect(resolveUIDistDir()).toBe(tmpDir);
   });
-
-  it('returns a bundled candidate when no env var is set and bundled path exists', () => {
-    const result = resolveUIDistDir();
-    // If a bundled candidate has index.html, it should be returned
-    if (result) {
-      expect(result).toContain('dist-assets');
-    }
-  });
 });

src/cli/operations/traces/__tests__/get-trace.test.ts

@@ -3,7 +3,7 @@ import type { FetchTraceRecordsOptions } from '../types';
 import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
-import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 const { mockSend } = vi.hoisted(() => ({
   mockSend: vi.fn(),
@@ -181,16 +181,15 @@ describe('fetchTraceRecords', () => {
 describe('getTrace', () => {
   let tmpDir: string;
 
-  beforeAll(() => {
+  beforeEach(() => {
     tmpDir = mkdtempSync(join(tmpdir(), 'get-trace-test-'));
   });
 
-  afterAll(() => {
+  afterEach(() => {
+    vi.clearAllMocks();
     rmSync(tmpDir, { recursive: true, force: true });
   });
 
-  afterEach(() => vi.clearAllMocks());
-
   it('calls fetchTraceRecords and writes result to disk', async () => {
     mockSend.mockResolvedValueOnce({ queryId: 'q-1' }).mockResolvedValueOnce({
       status: 'Complete',