test: remove unnecessary mocks, use real filesystem

[Hweinstock]

Description

Remove unnecessary mocks from 4 test files, replacing them with real filesystem operations using temp directories. This improves test reliability by verifying actual behavior rather than mock call sequences.

This change refactors a few tests to no longer mock the file system (there are many others to go), and adds to the reviewer system prompt to flag this in future PRs.

Related Issue

N/A — test quality improvement

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Other (please describe): Test quality improvement — removing unnecessary mocks

Testing

How have you tested the change?

• I ran npm run test:unit and npm run test:integ
• I ran npm run typecheck
• I ran npm run lint
• If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

• I have read the CONTRIBUTING document
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

[agentcore-cli-automation]

Nice cleanup overall — swapping in real tmp dirs makes these tests exercise more real behavior. I have a few concerns that I think are worth addressing before merging:

1. The succeeds even when cache write fails test no longer reliably triggers a write failure (chmod on the directory doesn't prevent overwriting an existing file on Linux, is bypassed by root, and is a no-op on Windows). As written, the test passes even when the cache write silently succeeds, so it no longer validates the error-handling path.
2. resolve-ui-dist-dir.test.ts dropped the returns null when no candidate has index.html and returns the first candidate that has index.html tests. The new replacement for the "skips AGENT_INSPECTOR_PATH..." case only asserts result !== tmpDir, which is a significantly weaker guarantee than toBeNull() and will pass for unrelated reasons (e.g., if any bundled candidate is present in the test environment).

Details inline.

[agentcore-cli-automation]

Thanks for addressing the earlier feedback — the update-notifier cache-write-failure test using a file-as-directory is a solid cross-platform fix, and restoring the missing resolve-ui-dist-dir test cases is appreciated.

One remaining concern worth addressing before merging: the two it.skipIf(bundledExists) tests in resolve-ui-dist-dir.test.ts will be skipped in practice in every CI run (details inline), so they effectively provide zero coverage of the paths they claim to test. Suggesting a narrow existsSync spy as a path forward.

Otherwise the refactor looks good.

[agentcore-cli-automation]

Took a pass on the latest commit (022c09b). The two substantive concerns from prior reviews — the cache-write-failure test not reliably triggering the error path, and the missing coverage for resolveUIDistDir returning null — have both been addressed:

• succeeds even when cache write fails now points AGENTCORE_CONFIG_DIR at a regular file, so mkdir/writeFile reliably fail cross-platform without depending on permission bits or uid.
• resolve-ui-dist-dir.test.ts restores the returns null when no candidate has index.html and skips AGENT_INSPECTOR_PATH when env var is set but dir lacks index.html cases with a narrow fs.existsSync spy, which is consistent with the "mock only at true I/O boundaries" guidance added to review.md.

The refactor is a clear improvement — real tmp dirs exercise actual fs behavior, and dropping the compareVersions mock means the cache-path tests now verify real version comparison end-to-end. Nothing blocking from my side; approving.

[tejaskash]

LGTM