refactor: add dependency injection for clean code and TDD

[scaryrawr]

Summary

This PR introduces dependency injection across all core modules to improve testability and follow clean code/SOLID principles.

Changes

New Interfaces (src/interfaces/)

• FileSystem: Abstraction for file operations
• ProcessEnv: Abstraction for environment and process info
• Shell: Abstraction for spawning commands

New Adapters (src/adapters/)

• Production: BunFileSystem, NodeProcess, BunShell
• Testing: MemoryFileSystem, MockProcess, MockShell

Refactored Modules

• config.ts: ConfigDependencies with fs/process
• scanner.ts: ScannerDependencies with fs/process
• marketplace.ts: MarketplaceDependencies with fs/shell/paths
• cache.ts: CacheInstance factory, removed global state
• plugin.ts: PluginDependencies with scanner/config/output
• translator.ts: TranslatorDependencies with cache/fs
• executor.ts: ExecutorDependencies with shell/env

Test Improvements

• 104 tests (up from 63)
• No tests modify process.cwd() or process.env.HOME
• Unit tests use in-memory mocks (fast, isolated)
• Integration tests separated into *.integration.test.ts

Documentation

• Updated AGENTS.md with testing patterns and examples

Breaking Changes

None - all changes are backward compatible. Functions accept optional deps parameter with production defaults.

[Copilot]

Pull request overview

This PR introduces comprehensive dependency injection across all core modules to improve testability and follow SOLID principles. The refactoring adds interface abstractions for FileSystem, ProcessEnv, and Shell operations, with production adapters using Bun/Node.js APIs and test adapters using in-memory implementations.

Changes:

• Created three core interfaces (FileSystem, ProcessEnv, Shell) with production and test adapters
• Refactored 7 core modules to accept optional dependency injection parameters with production defaults
• Increased test count from 63 to 104 with in-memory mocks replacing file system modifications
• Added comprehensive test utilities and separated integration tests

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/interfaces/file-system.ts	New FileSystem interface for file operations abstraction
src/interfaces/process.ts	New ProcessEnv interface for environment/system info abstraction
src/interfaces/shell.ts	New Shell interface for command spawning abstraction
src/adapters/bun-file-system.ts	Production FileSystem implementation using Bun/Node.js
src/adapters/node-process.ts	Production ProcessEnv implementation using Node.js APIs
src/adapters/bun-shell.ts	Production Shell implementation using Bun.spawnSync
src/adapters/memory-file-system.ts	In-memory FileSystem for testing
src/adapters/mock-process.ts	Mock ProcessEnv for testing
src/adapters/mock-shell.ts	Mock Shell for testing
src/test-utils/index.ts	Centralized test utility exports
src/translator.ts	Added TranslatorDependencies with cache/fs injection
src/scanner.ts	Added ScannerDependencies with fs/process injection
src/plugin.ts	Added PluginDependencies with scanner/config/output injection
src/marketplace.ts	Added MarketplaceDependencies with fs/shell/paths injection
src/config.ts	Added ConfigDependencies with fs/process injection
src/cache.ts	Added CacheInstance factory pattern, removed global state
src/executor.ts	Added ExecutorDependencies with shell/env injection
src/scanner.test.ts	New unit tests using memory mocks
src/plugin.test.ts	Refactored to use mock dependencies
src/marketplace.test.ts	Refactored to use memory file system and mock shell
src/config.test.ts	New unit tests with memory file system
src/cache.unit.test.ts	New unit tests with memory file system
src/cache.integration.test.ts	Integration tests using real file system
AGENTS.md	Updated documentation with testing patterns and DI examples

Comments suppressed due to low confidence (1)

src/marketplace.ts:44

• The getMarketplacesRoot function still directly accesses process.env.HOME and calls homedir() from node:os instead of using injected ProcessEnv dependency. This breaks the dependency injection pattern and makes the function not fully testable. Consider accepting a ProcessEnv parameter similar to how scanner.ts handles it in getKnownMarketplacesPath.

function getMarketplacesRoot(deps?: MarketplaceDependencies): string {
  if (deps?.marketplacesRoot) {
    return deps.marketplacesRoot;
  }
  const homeDir = process.env.HOME ?? homedir();
  return join(homeDir, ".claude", "plugins", "marketplaces");
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[scaryrawr]

All review feedback has been addressed in commits cf4148e through 6672b4d:

Fixes Applied:

• bun-shell.ts stdout/stderr (cf4148e): Fixed to use result.stdout ?? new Uint8Array() directly
• marketplace.ts process.exit (e59364e): Changed to throw Error for testability, removed unused shell variable
• cache.unit.test.ts unused variables (106459e): Removed 4 unused fs destructurings
• marketplace.test.ts async handlers (147de49): Added sync methods (writeFileSync, mkdirSync) to MemoryFileSystem and updated tests to use them
• AGENTS.md & cache.ts docs (6672b4d): Updated DI pattern example, added comment explaining sync imports for backward compatibility

New Tests Added:

• 8 unit tests for MemoryFileSystem.writeFileSync and mkdirSync
• 1 test for addMarketplace throwing error when cloned repo has no marketplace.json

All 113 tests pass ✅

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (4)

src/marketplace.ts:44

• This function still uses global process.env.HOME and homedir() directly instead of using the injected dependencies. This is inconsistent with the dependency injection pattern used throughout the PR. Consider accepting a ProcessEnv dependency or using the injected file system's environment access pattern.

function getMarketplacesRoot(deps?: MarketplaceDependencies): string {
  if (deps?.marketplacesRoot) {
    return deps.marketplacesRoot;
  }
  const homeDir = process.env.HOME ?? homedir();
  return join(homeDir, ".claude", "plugins", "marketplaces");
}

src/marketplace.ts:150

• This function directly calls process.exit() which makes it non-testable and breaks the dependency injection pattern. The tests work around this by mocking process.exit, but it would be cleaner to either throw an error that the caller can handle, or accept an exit function in the dependencies similar to the pattern used in plugin.ts.

function runGitCommand(args: string[], deps?: MarketplaceDependencies): void {
  const { shell } = { ...defaultDeps, ...deps };
  let result;
  try {
    result = shell.spawnSync(["git", ...args], {
      stdout: "pipe",
      stderr: "pipe",
    });
  } catch (error) {
    console.error(
      `Error: Failed to clone/update marketplace: ${error instanceof Error ? error.message : String(error)}`,
    );
    process.exit(1);
  }

  if (result.exitCode !== 0 || result.exitCode === null) {
    const details =
      decodeOutput(result.stderr) ||
      decodeOutput(result.stdout) ||
      `exit code ${result.exitCode ?? "unknown"}`;
    console.error(`Error: Failed to clone/update marketplace: ${details}`);
    process.exit(1);
  }
}

src/marketplace.ts:96

• Similar to runGitCommand, this function directly calls process.exit() which breaks testability and the dependency injection pattern. Consider throwing an error instead that can be caught and handled by the caller, or accepting an exit function in the dependencies.

async function writeKnownMarketplaces(
  data: KnownMarketplacesFile,
  deps?: MarketplaceDependencies,
): Promise<void> {
  const { fs } = { ...defaultDeps, ...deps };
  const filePath = deps?.knownMarketplacesPath ?? getKnownMarketplacesPath();
  try {
    await fs.mkdir(dirname(filePath), { recursive: true });
    await fs.writeFile(filePath, JSON.stringify(data, null, 2));
  } catch (error) {
    console.error(
      `Error: Failed to write known marketplaces file at "${filePath}": ${
        error instanceof Error ? error.message : String(error)
      }`,
    );
    process.exit(1);
  }
}

src/marketplace.ts:77

• This function also calls process.exit() directly. The entire marketplace.ts file has multiple process.exit() calls that break the dependency injection pattern established elsewhere. For consistency with plugin.ts which properly injects exit/log/error functions, consider adding these to MarketplaceDependencies.

async function readKnownMarketplaces(
  deps?: MarketplaceDependencies,
): Promise<KnownMarketplacesFile> {
  const { fs } = { ...defaultDeps, ...deps };
  const filePath = deps?.knownMarketplacesPath ?? getKnownMarketplacesPath();

  if (!(await fs.exists(filePath))) {
    return {};
  }

  try {
    const content = await fs.readFile(filePath);
    const data = JSON.parse(content);
    if (typeof data === "object" && data !== null) {
      return data as KnownMarketplacesFile;
    }
  } catch (error) {
    console.error(
      `Error: Failed to read known marketplaces: ${error instanceof Error ? error.message : String(error)}`,
    );
    process.exit(1);
  }

  return {};
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The condition dir !== '.' && dir !== '..' may not cover all cases where mkdir should be skipped. For example, if filePath is just a filename without a directory (like "file.txt"), dirname returns ".", but an absolute path at root like "/file.txt" would have dirname "/". Consider a more robust check or simply letting mkdir handle these cases since it's called with recursive: true.

Suggested change

	if (dir !== '.' && dir !== '..') {
	await this.mkdir(dir, { recursive: true });
	}
	await this.mkdir(dir, { recursive: true });

[Copilot]

Unused import createMemoryFileSystem.

Suggested change

	import { MemoryFileSystem, createMemoryFileSystem } from "./memory-file-system";
	import { MemoryFileSystem } from "./memory-file-system";