diff --git a/.github/harness/prompts/review.md b/.github/harness/prompts/review.md index 67cb54fec..a9749eb65 100644 --- a/.github/harness/prompts/review.md +++ b/.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). diff --git a/src/cli/__tests__/update-notifier.test.ts b/src/cli/__tests__/update-notifier.test.ts index 3713e51f2..28f0d90ef 100644 --- a/src/cli/__tests__/update-notifier.test.ts +++ b/src/cli/__tests__/update-notifier.test.ts @@ -1,107 +1,83 @@ +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(); @@ -109,9 +85,7 @@ describe('checkForUpdate', () => { }); 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(); @@ -119,8 +93,7 @@ describe('checkForUpdate', () => { }); 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(); @@ -128,8 +101,9 @@ describe('checkForUpdate', () => { }); 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(); @@ -137,10 +111,14 @@ describe('checkForUpdate', () => { }); 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'); diff --git a/src/cli/operations/dev/web-ui/__tests__/resolve-ui-dist-dir.test.ts b/src/cli/operations/dev/web-ui/__tests__/resolve-ui-dist-dir.test.ts index 710281293..6308a1f24 100644 --- a/src/cli/operations/dev/web-ui/__tests__/resolve-ui-dist-dir.test.ts +++ b/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); }); }); diff --git a/src/cli/operations/traces/__tests__/get-trace.test.ts b/src/cli/operations/traces/__tests__/get-trace.test.ts index c6fda22f4..8f833cc0a 100644 --- a/src/cli/operations/traces/__tests__/get-trace.test.ts +++ b/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); }); }); diff --git a/src/cli/templates/__tests__/BaseRenderer.test.ts b/src/cli/templates/__tests__/BaseRenderer.test.ts index 083c20aa5..ebac2150f 100644 --- a/src/cli/templates/__tests__/BaseRenderer.test.ts +++ b/src/cli/templates/__tests__/BaseRenderer.test.ts @@ -1,22 +1,15 @@ import { BaseRenderer } from '../BaseRenderer.js'; -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockCopyAndRenderDir = vi.fn(); -const mockExistsSync = vi.fn(); vi.mock('../render.js', () => ({ copyAndRenderDir: (...args: unknown[]) => mockCopyAndRenderDir(...args), })); -vi.mock('node:fs', async () => { - const actual = await vi.importActual('node:fs'); - return { ...actual, existsSync: (...args: unknown[]) => mockExistsSync(...args) }; -}); - -vi.mock('../../../lib', () => ({ - APP_DIR: 'app', -})); - class TestRenderer extends BaseRenderer { constructor(config: any, sdkName: string, baseTemplateDir: string, protocol?: string) { super(config, sdkName, baseTemplateDir, protocol); @@ -28,7 +21,16 @@ class TestRenderer extends BaseRenderer { } describe('BaseRenderer', () => { - afterEach(() => vi.clearAllMocks()); + let tmpDir: string; + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'base-renderer-test-')); + }); + + afterEach(() => { + vi.clearAllMocks(); + rmSync(tmpDir, { recursive: true, force: true }); + }); it('getTemplateDir joins language, protocol, and sdk name', () => { const renderer = new TestRenderer( @@ -63,19 +65,18 @@ describe('BaseRenderer', () => { it('render copies base template', async () => { mockCopyAndRenderDir.mockResolvedValue(undefined); - mockExistsSync.mockReturnValue(false); const renderer = new TestRenderer( { targetLanguage: 'Python', name: 'MyAgent', hasMemory: false }, 'strands', - '/templates' + tmpDir ); await renderer.render({ outputDir: '/output' }); expect(mockCopyAndRenderDir).toHaveBeenCalledTimes(1); expect(mockCopyAndRenderDir).toHaveBeenCalledWith( - '/templates/python/http/strands/base', + join(tmpDir, 'python', 'http', 'strands', 'base'), '/output/app/MyAgent', expect.objectContaining({ projectName: 'MyAgent', Name: 'MyAgent', hasMcp: false }) ); @@ -83,19 +84,19 @@ describe('BaseRenderer', () => { it('render copies memory capability when hasMemory and dir exists', async () => { mockCopyAndRenderDir.mockResolvedValue(undefined); - mockExistsSync.mockReturnValue(true); + mkdirSync(join(tmpDir, 'typescript', 'http', 'langchain', 'capabilities', 'memory'), { recursive: true }); const renderer = new TestRenderer( { targetLanguage: 'TypeScript', name: 'Agent', hasMemory: true }, 'langchain', - '/templates' + tmpDir ); await renderer.render({ outputDir: '/out' }); expect(mockCopyAndRenderDir).toHaveBeenCalledTimes(2); expect(mockCopyAndRenderDir).toHaveBeenCalledWith( - '/templates/typescript/http/langchain/capabilities/memory', + join(tmpDir, 'typescript', 'http', 'langchain', 'capabilities', 'memory'), '/out/app/Agent/memory', expect.objectContaining({ projectName: 'Agent', hasMemory: true }) ); @@ -103,13 +104,8 @@ describe('BaseRenderer', () => { it('render skips memory capability when dir does not exist', async () => { mockCopyAndRenderDir.mockResolvedValue(undefined); - mockExistsSync.mockReturnValue(false); - const renderer = new TestRenderer( - { targetLanguage: 'Python', name: 'Agent', hasMemory: true }, - 'strands', - '/templates' - ); + const renderer = new TestRenderer({ targetLanguage: 'Python', name: 'Agent', hasMemory: true }, 'strands', tmpDir); await renderer.render({ outputDir: '/out' }); diff --git a/src/cli/update-notifier.ts b/src/cli/update-notifier.ts index 98c468296..dca990c15 100644 --- a/src/cli/update-notifier.ts +++ b/src/cli/update-notifier.ts @@ -1,12 +1,15 @@ +import { ONE_DAY_MS } from '../lib/time-constants.js'; import { compareVersions, fetchLatestVersion } from './commands/update/action.js'; import { PACKAGE_VERSION } from './constants.js'; import { mkdir, readFile, writeFile } from 'fs/promises'; import { homedir } from 'os'; import { join } from 'path'; -const CACHE_DIR = join(homedir(), '.agentcore'); -const CACHE_FILE = join(CACHE_DIR, 'update-check.json'); -const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000; // every 24 hours +function getConfigDir(): string { + return process.env.AGENTCORE_CONFIG_DIR ?? join(homedir(), '.agentcore'); +} + +const CHECK_INTERVAL_MS = ONE_DAY_MS; interface CacheData { lastCheck: number; @@ -20,7 +23,7 @@ export interface UpdateCheckResult { async function readCache(): Promise { try { - const data = await readFile(CACHE_FILE, 'utf-8'); + const data = await readFile(join(getConfigDir(), 'update-check.json'), 'utf-8'); return JSON.parse(data) as CacheData; } catch { return null; @@ -29,8 +32,9 @@ async function readCache(): Promise { async function writeCache(data: CacheData): Promise { try { - await mkdir(CACHE_DIR, { recursive: true }); - await writeFile(CACHE_FILE, JSON.stringify(data), 'utf-8'); + const dir = getConfigDir(); + await mkdir(dir, { recursive: true }); + await writeFile(join(dir, 'update-check.json'), JSON.stringify(data), 'utf-8'); } catch { // Silently ignore cache write failures } diff --git a/src/lib/index.ts b/src/lib/index.ts index cab20d720..d21baec86 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -27,3 +27,4 @@ export * from './utils'; // Schema I/O utilities export * from './schemas/io'; +export * from './time-constants'; diff --git a/src/lib/time-constants.ts b/src/lib/time-constants.ts new file mode 100644 index 000000000..679ddc2b2 --- /dev/null +++ b/src/lib/time-constants.ts @@ -0,0 +1,4 @@ +export const ONE_SECOND_MS = 1000; +export const ONE_MINUTE_MS = ONE_SECOND_MS * 60; +export const ONE_HOUR_MS = ONE_MINUTE_MS * 60; +export const ONE_DAY_MS = ONE_HOUR_MS * 24;