test: remove unnecessary mocks, use real filesystem (#1156)

* test(resolve-ui-dist-dir): replace fs mock with real temp directory

* test(BaseRenderer): remove APP_DIR and fs mocks, use real temp directories

* test(get-trace): remove fs mock, verify file output with real filesystem

* test(update-notifier): remove fs and compareVersions mocks, use real filesystem

- Replace fs/promises mock with real temp directory via GLOBAL_CONFIG_DIR
- Remove compareVersions mock, let real pure function run
- Refactor source to use existing GLOBAL_CONFIG_DIR (supports AGENTCORE_CONFIG_DIR env var)
- Only mock fetchLatestVersion (network) and PACKAGE_VERSION (constant)

* 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

* review.md: specify I/O boundary examples (network, AWS SDK, HTTP)

Author: Hweinstock

.github/harness/prompts/review.md

@@ -16,3 +16,12 @@ choose. Skip style nits and minor suggestions — only flag things that actually
 
 When finished, submit a formal PR review (approve or request changes) with individual and inline comments in it. Be
 specific with line numbers.
+
+If all serious issues have already been raised in existing comments, or if you found no new issues, post a single
+comment on the PR saying it looks good to merge (or that all issues have already been flagged).
+
+## Patterns to look out for
+
+- **Excessive mocking** — Avoid excessive mocking; it couples tests to implementation details, provides weaker
+  guarantees, and often points to mismanaged dependencies. Prefer real dependencies (e.g. temp directories over fs
+  mocks) and only mock at true I/O boundaries (e.g., network calls, AWS SDK clients, HTTP requests).

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

@@ -1,146 +1,124 @@
+import { ONE_DAY_MS, ONE_HOUR_MS, ONE_SECOND_MS } from '../../lib/time-constants.js';
+import * as action from '../commands/update/action.js';
+import * as constants from '../constants.js';
 import { type UpdateCheckResult, checkForUpdate, printUpdateNotification } from '../update-notifier.js';
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
-
-const { mockReadFile, mockWriteFile, mockMkdir } = vi.hoisted(() => ({
-  mockReadFile: vi.fn(),
-  mockWriteFile: vi.fn(),
-  mockMkdir: vi.fn(),
-}));
-
-vi.mock('fs/promises', () => ({
-  readFile: mockReadFile,
-  writeFile: mockWriteFile,
-  mkdir: mockMkdir,
-}));
-
-vi.mock('../constants.js', () => ({
-  PACKAGE_VERSION: '1.0.0',
-}));
-
-const { mockFetchLatestVersion, mockCompareVersions } = vi.hoisted(() => ({
-  mockFetchLatestVersion: vi.fn(),
-  mockCompareVersions: vi.fn(),
-}));
-
-vi.mock('../commands/update/action.js', () => ({
-  fetchLatestVersion: mockFetchLatestVersion,
-  compareVersions: mockCompareVersions,
-}));
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
+import { mkdir, readFile, writeFile } from 'fs/promises';
+import { tmpdir } from 'os';
+import { join } from 'path';
+import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+const NOW = 1708646400000;
+const tmpDir = mkdtempSync(join(tmpdir(), 'update-notifier-test-'));
+const CACHE_FILE = join(tmpDir, 'update-check.json');
 
 describe('checkForUpdate', () => {
+  let originalConfigDir: string | undefined;
+
   beforeEach(() => {
-    vi.spyOn(Date, 'now').mockReturnValue(1708646400000);
-    mockWriteFile.mockResolvedValue(undefined);
-    mockMkdir.mockResolvedValue(undefined);
+    originalConfigDir = process.env.AGENTCORE_CONFIG_DIR;
+    process.env.AGENTCORE_CONFIG_DIR = tmpDir;
+    vi.spyOn(Date, 'now').mockReturnValue(NOW);
+    vi.spyOn(constants, 'PACKAGE_VERSION', 'get').mockReturnValue('1.0.0');
+    rmSync(tmpDir, { recursive: true, force: true });
   });
 
   afterEach(() => {
     vi.restoreAllMocks();
-    mockReadFile.mockReset();
-    mockWriteFile.mockReset();
-    mockMkdir.mockReset();
-    mockFetchLatestVersion.mockReset();
-    mockCompareVersions.mockReset();
+    if (originalConfigDir === undefined) {
+      delete process.env.AGENTCORE_CONFIG_DIR;
+    } else {
+      process.env.AGENTCORE_CONFIG_DIR = originalConfigDir;
+    }
+  });
+
+  afterAll(() => {
+    rmSync(tmpDir, { recursive: true, force: true });
   });
 
   it('fetches from registry when no cache exists', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockResolvedValue('2.0.0');
-    mockCompareVersions.mockReturnValue(1);
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('2.0.0');
 
     const result = await checkForUpdate();
 
     expect(result).toEqual({ updateAvailable: true, latestVersion: '2.0.0' });
-    expect(mockFetchLatestVersion).toHaveBeenCalled();
   });
 
   it('uses cache when last check was less than 24 hours ago', async () => {
-    const cache = JSON.stringify({
-      lastCheck: 1708646400000 - 1000, // 1 second ago
-      latestVersion: '2.0.0',
-    });
-    mockReadFile.mockResolvedValue(cache);
-    mockCompareVersions.mockReturnValue(1);
+    await mkdir(tmpDir, { recursive: true });
+    await writeFile(CACHE_FILE, JSON.stringify({ lastCheck: NOW - ONE_SECOND_MS, latestVersion: '2.0.0' }), 'utf-8');
 
     const result = await checkForUpdate();
 
     expect(result).toEqual({ updateAvailable: true, latestVersion: '2.0.0' });
-    expect(mockFetchLatestVersion).not.toHaveBeenCalled();
   });
 
   it('fetches from registry when cache is expired', async () => {
-    const cache = JSON.stringify({
-      lastCheck: 1708646400000 - 25 * 60 * 60 * 1000, // 25 hours ago
-      latestVersion: '1.5.0',
-    });
-    mockReadFile.mockResolvedValue(cache);
-    mockFetchLatestVersion.mockResolvedValue('2.0.0');
-    mockCompareVersions.mockReturnValue(1);
+    await mkdir(tmpDir, { recursive: true });
+    await writeFile(
+      CACHE_FILE,
+      JSON.stringify({ lastCheck: NOW - ONE_DAY_MS - ONE_HOUR_MS, latestVersion: '1.5.0' }),
+      'utf-8'
+    );
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('2.0.0');
 
     const result = await checkForUpdate();
 
     expect(result).toEqual({ updateAvailable: true, latestVersion: '2.0.0' });
-    expect(mockFetchLatestVersion).toHaveBeenCalled();
   });
 
   it('writes cache after fetching', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockResolvedValue('2.0.0');
-    mockCompareVersions.mockReturnValue(1);
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('2.0.0');
 
     await checkForUpdate();
 
-    expect(mockMkdir).toHaveBeenCalled();
-    expect(mockWriteFile).toHaveBeenCalledWith(
-      expect.stringContaining('update-check.json'),
-      JSON.stringify({ lastCheck: 1708646400000, latestVersion: '2.0.0' }),
-      'utf-8'
-    );
+    const cached = JSON.parse(await readFile(CACHE_FILE, 'utf-8'));
+    expect(cached).toEqual({ lastCheck: NOW, latestVersion: '2.0.0' });
   });
 
   it('returns updateAvailable: false when versions match', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockResolvedValue('1.0.0');
-    mockCompareVersions.mockReturnValue(0);
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('1.0.0');
 
     const result = await checkForUpdate();
 
     expect(result).toEqual({ updateAvailable: false, latestVersion: '1.0.0' });
   });
 
   it('returns updateAvailable: false when current is newer', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockResolvedValue('0.9.0');
-    mockCompareVersions.mockReturnValue(-1);
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('0.9.0');
 
     const result = await checkForUpdate();
 
     expect(result).toEqual({ updateAvailable: false, latestVersion: '0.9.0' });
   });
 
   it('returns null on fetch error', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockRejectedValue(new Error('network error'));
+    vi.spyOn(action, 'fetchLatestVersion').mockRejectedValue(new Error('network error'));
 
     const result = await checkForUpdate();
 
     expect(result).toBeNull();
   });
 
   it('returns null on cache parse error and fetch error', async () => {
-    mockReadFile.mockResolvedValue('invalid json');
-    mockFetchLatestVersion.mockRejectedValue(new Error('network error'));
+    await mkdir(tmpDir, { recursive: true });
+    await writeFile(CACHE_FILE, 'invalid json', 'utf-8');
+    vi.spyOn(action, 'fetchLatestVersion').mockRejectedValue(new Error('network error'));
 
     const result = await checkForUpdate();
 
     expect(result).toBeNull();
   });
 
   it('succeeds even when cache write fails', async () => {
-    mockReadFile.mockRejectedValue(new Error('ENOENT'));
-    mockFetchLatestVersion.mockResolvedValue('2.0.0');
-    mockCompareVersions.mockReturnValue(1);
-    mockWriteFile.mockRejectedValue(new Error('EACCES'));
+    // Point config dir at a regular file — mkdir/writeFile will fail because
+    // a file can't be used as a directory. Works cross-platform and as root.
+    mkdirSync(tmpDir, { recursive: true });
+    const blocker = join(tmpDir, 'not-a-dir');
+    writeFileSync(blocker, '');
+    process.env.AGENTCORE_CONFIG_DIR = blocker;
+
+    vi.spyOn(action, 'fetchLatestVersion').mockResolvedValue('2.0.0');
 
     const result = await checkForUpdate();
 
@@ -157,7 +135,6 @@ describe('printUpdateNotification', () => {
 
     const output = stderrSpy.mock.calls.map(c => c[0]).join('');
     expect(output).toContain('Update available:');
-    expect(output).toContain('1.0.0');
     expect(output).toContain('2.0.0');
     expect(output).toContain('npm install -g @aws/agentcore@latest');
 

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

@@ -1,62 +1,50 @@
 import { resolveUIDistDir } from '../web-server.js';
 import fs from 'node:fs';
-import path from 'node:path';
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
-vi.mock('node:fs');
-
-const existsSync = vi.mocked(fs.existsSync);
-
 describe('resolveUIDistDir', () => {
   const originalEnv = process.env;
+  let tmpDir: string;
 
   beforeEach(() => {
     process.env = { ...originalEnv };
+    tmpDir = mkdtempSync(join(tmpdir(), 'resolve-ui-test-'));
     delete process.env.AGENT_INSPECTOR_PATH;
-    existsSync.mockReturnValue(false);
   });
 
   afterEach(() => {
     process.env = originalEnv;
+    rmSync(tmpDir, { recursive: true, force: true });
     vi.restoreAllMocks();
   });
 
   it('returns null when no candidate has index.html', () => {
+    vi.spyOn(fs, 'existsSync').mockReturnValue(false);
+
     expect(resolveUIDistDir()).toBeNull();
   });
 
   it('returns AGENT_INSPECTOR_PATH when env var is set and dir has index.html', () => {
-    const customPath = '/custom/inspector/dist';
-    process.env.AGENT_INSPECTOR_PATH = customPath;
-
-    existsSync.mockImplementation(p => p === path.join(customPath, 'index.html'));
+    process.env.AGENT_INSPECTOR_PATH = tmpDir;
+    writeFileSync(join(tmpDir, 'index.html'), '');
 
-    expect(resolveUIDistDir()).toBe(customPath);
+    expect(resolveUIDistDir()).toBe(tmpDir);
   });
 
   it('skips AGENT_INSPECTOR_PATH when env var is set but dir lacks index.html', () => {
-    process.env.AGENT_INSPECTOR_PATH = '/missing/inspector';
-    existsSync.mockReturnValue(false);
+    process.env.AGENT_INSPECTOR_PATH = tmpDir;
+    vi.spyOn(fs, 'existsSync').mockReturnValue(false);
 
     expect(resolveUIDistDir()).toBeNull();
   });
 
-  it('returns the first candidate that has index.html', () => {
-    existsSync.mockImplementation(p => {
-      return String(p).endsWith(path.join('agent-inspector', 'index.html'));
-    });
-
-    const result = resolveUIDistDir();
-    expect(result).not.toBeNull();
-    expect(result!).toMatch(/agent-inspector$/);
-  });
-
   it('prefers AGENT_INSPECTOR_PATH over bundled candidates', () => {
-    const customPath = '/custom/path';
-    process.env.AGENT_INSPECTOR_PATH = customPath;
-
-    existsSync.mockReturnValue(true);
+    process.env.AGENT_INSPECTOR_PATH = tmpDir;
+    writeFileSync(join(tmpDir, 'index.html'), '');
 
-    expect(resolveUIDistDir()).toBe(customPath);
+    expect(resolveUIDistDir()).toBe(tmpDir);
   });
 });

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

@@ -1,6 +1,9 @@
 import { fetchTraceRecords, getTrace } from '../get-trace';
 import type { FetchTraceRecordsOptions } from '../types';
-import { afterEach, describe, expect, it, vi } from 'vitest';
+import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 const { mockSend } = vi.hoisted(() => ({
   mockSend: vi.fn(),
@@ -22,13 +25,6 @@ vi.mock('../../../aws', () => ({
   getCredentialProvider: vi.fn().mockReturnValue({}),
 }));
 
-vi.mock('node:fs', () => ({
-  default: {
-    mkdirSync: vi.fn(),
-    writeFileSync: vi.fn(),
-  },
-}));
-
 const baseOptions: FetchTraceRecordsOptions = {
   region: 'us-west-2',
   runtimeId: 'runtime-123',
@@ -183,11 +179,18 @@ describe('fetchTraceRecords', () => {
 });
 
 describe('getTrace', () => {
-  afterEach(() => vi.clearAllMocks());
+  let tmpDir: string;
 
-  it('calls fetchTraceRecords and writes result to disk', async () => {
-    const fs = await import('node:fs');
+  beforeEach(() => {
+    tmpDir = mkdtempSync(join(tmpdir(), 'get-trace-test-'));
+  });
+
+  afterEach(() => {
+    vi.clearAllMocks();
+    rmSync(tmpDir, { recursive: true, force: true });
+  });
 
+  it('calls fetchTraceRecords and writes result to disk', async () => {
     mockSend.mockResolvedValueOnce({ queryId: 'q-1' }).mockResolvedValueOnce({
       status: 'Complete',
       results: [
@@ -198,36 +201,38 @@ describe('getTrace', () => {
       ],
     });
 
+    const outputPath = join(tmpDir, 'test-trace.json');
     const result = await getTrace({
       region: 'us-west-2',
       runtimeId: 'runtime-123',
       agentName: 'my-agent',
       traceId: 'abc123def456',
-      outputPath: '/tmp/test-trace.json',
+      outputPath,
       startTime: 1000000,
       endTime: 2000000,
     });
 
     expect(result.success).toBe(true);
     expect(result.filePath).toContain('test-trace.json');
-    expect(fs.default.mkdirSync).toHaveBeenCalled();
-    expect(fs.default.writeFileSync).toHaveBeenCalledWith('/tmp/test-trace.json', expect.stringContaining('"traceId"'));
+    const content = JSON.parse(readFileSync(outputPath, 'utf-8'));
+    expect(content).toHaveLength(1);
+    expect(content[0]['@message']).toEqual({ traceId: 'abc123' });
   });
 
   it('returns error from fetchTraceRecords without writing file', async () => {
-    const fs = await import('node:fs');
-
+    const outputPath = join(tmpDir, 'should-not-exist.json');
     const result = await getTrace({
       region: 'us-west-2',
       runtimeId: 'runtime-123',
       agentName: 'my-agent',
       traceId: 'invalid!@#$',
+      outputPath,
       startTime: 1000000,
       endTime: 2000000,
     });
 
     expect(result.success).toBe(false);
     expect(result.error).toContain('Invalid trace ID format');
-    expect(fs.default.writeFileSync).not.toHaveBeenCalled();
+    expect(existsSync(outputPath)).toBe(false);
   });
 });