# Investigation: Fix Test Infrastructure (#252 + #378)

**Issues**: #252 (https://github.com/dynamous-community/remote-coding-agent/issues/252), #378 (https://github.com/dynamous-community/remote-coding-agent/issues/378)
**Type**: BUG
**Investigated**: 2026-02-17

### Assessment

| Metric     | Value  | Reasoning                                                                                                                     |
| ---------- | ------ | ----------------------------------------------------------------------------------------------------------------------------- |
| Severity   | MEDIUM | 3 tests fail consistently in CI; the hang (#252) no longer reproduces but may be intermittent                                 |
| Complexity | MEDIUM | Touches 3 files across 2 packages; requires understanding Bun's mock.module leak behavior                                    |
| Confidence | HIGH   | Root cause for #378 is definitively proven (reproduced by running the two test files together); #252 has clear preventive fix |

---

## Problem Statement

**Issue #378**: Three `versionCommand` tests fail when run as part of the full test suite (`bun test`) but pass in isolation. The `fs/promises` module is globally mocked by `github-context.test.ts`, causing `readFile` to return an empty string instead of actual package.json content.

**Issue #252**: The test suite was reported to hang after completion due to `mock.module('../db/connection')` in `executor.test.ts`. This no longer reproduces on the current codebase (full suite completes in ~40s), likely due to the `commitAllChanges` spy that prevents `sendCriticalMessage` retry delays. Preventive cleanup should still be added.

---

## Analysis

### Root Cause: Issue #378 (Version Test Failures)

**The 5 Whys:**

WHY 1: Why do the 3 version tests fail?
-> Because `JSON.parse(content)` throws with "package.json is malformed"
-> Evidence: `packages/cli/src/commands/version.ts:42-44` - `JSON.parse(content)` fails when `content` is empty string

WHY 2: Why is `content` an empty string?
-> Because `readFile()` from `fs/promises` returns `''` instead of actual file content
-> Evidence: `packages/cli/src/commands/version.ts:29` - `content = await readFile(pkgPath, 'utf-8')`

WHY 3: Why does `readFile` return an empty string?
-> Because `github-context.test.ts` globally mocks `fs/promises` with `readFile: mock(async () => '')`
-> Evidence: `packages/server/src/adapters/github-context.test.ts:98-104`

WHY 4: Why does this mock affect the version test in a different file?
-> Because Bun's `mock.module()` persists globally across all test files in the same `bun test` run
-> Evidence: Multiple comments throughout codebase acknowledge this (e.g., `orchestrator.test.ts:11-12`, `command-handler.test.ts:4-6`)

ROOT CAUSE: `github-context.test.ts:98` calls `mock.module('fs/promises', ...)` with a `readFile` that returns empty string. This mock leaks to `version.test.ts`, corrupting the `readFile` call in `version.ts:29`.

**Reproduction:**
```bash
# Fails (mock leaks):
bun test packages/server/src/adapters/github-context.test.ts packages/cli/src/commands/version.test.ts
# Result: 5 pass, 3 fail

# Passes (no leak):
bun test packages/cli/src/commands/version.test.ts
# Result: 3 pass, 0 fail
```

### Root Cause: Issue #252 (Test Suite Hang)

**Current state**: Cannot reproduce. Full suite (1305 tests) completes in ~40s.

**Original cause** (from issue description): `mock.module('../db/connection')` in `executor.test.ts:47-51` without cleanup. The mock only provides `pool.query`, missing `pool.end()` and other exports. Combined with `sendCriticalMessage` retry delays using `setTimeout`, pending timers could keep the process alive.

**Why it no longer reproduces**: The `commitAllChangesSpy` mock at `executor.test.ts:109-111` prevents `sendCriticalMessage` from entering its retry loop (returns `false` immediately), so no `setTimeout` timers are created.

**Risk**: The underlying lack of cleanup in executor.test.ts could cause hangs again if test behavior changes or Bun's behavior changes. Preventive cleanup should be added.

### Affected Files

| File                                                    | Lines    | Action | Description                                          |
| ------------------------------------------------------- | -------- | ------ | ---------------------------------------------------- |
| `packages/server/src/adapters/github-context.test.ts`   | 98-104   | UPDATE | Replace `mock.module('fs/promises')` with `spyOn`    |
| `packages/core/src/workflows/executor.test.ts`          | 133-140  | UPDATE | Add `afterAll` cleanup for module mocks              |
| `packages/core/src/test/setup.ts`                       | 1-13     | UPDATE | Add global mock cleanup to prevent future leaks      |

### Integration Points

- `packages/cli/src/commands/version.ts:29` - calls `readFile` from `fs/promises` (victim of the mock leak)
- `packages/cli/src/commands/version.ts:10-11` - imports from `@archon/core` (also mocked by `github-context.test.ts:38`)
- `packages/core/src/workflows/executor.ts:262-264` - `delay()` function using `setTimeout` (potential timer leak source)
- `packages/core/src/db/connection.ts:97-104` - `pool` export mocked in executor test

### Git History

- **fs/promises mock introduced**: `e4b30dd` (2026-01-30) - GitHub context test with heavy module mocking
- **Executor test last modified**: `c7cd67e` (2026-02-17) - Pino logging infrastructure
- **Version tests created**: `68e7db0` - Phase 5 CLI binary distribution

---

## Implementation Plan

### Step 1: Fix `github-context.test.ts` - Replace `mock.module('fs/promises')` with `spyOn`

**File**: `packages/server/src/adapters/github-context.test.ts`
**Lines**: 98-104
**Action**: UPDATE

**Current code:**
```typescript
// Line 98-104
mock.module('fs/promises', () => ({
  readdir: mock(async () => []),
  access: mock(async () => {}),
  stat: mock(async () => ({ isDirectory: () => true })),
  readFile: mock(async () => ''),
  mkdir: mock(async () => {}),
}));
```

**Required change:**

Replace the global `mock.module('fs/promises')` with `spyOn` on the imported `fs/promises` module. This follows the pattern already used in `command-handler.test.ts:48` which explicitly avoids `mock.module` for `fs/promises` to prevent global pollution.

```typescript
import * as fsPromises from 'fs/promises';

// Then in the describe block or beforeEach:
let readdirSpy: ReturnType<typeof spyOn>;
let accessSpy: ReturnType<typeof spyOn>;
let statSpy: ReturnType<typeof spyOn>;
let readFileSpy: ReturnType<typeof spyOn>;
let mkdirSpy: ReturnType<typeof spyOn>;

beforeEach(() => {
  readdirSpy = spyOn(fsPromises, 'readdir').mockResolvedValue([] as any);
  accessSpy = spyOn(fsPromises, 'access').mockResolvedValue(undefined);
  statSpy = spyOn(fsPromises, 'stat').mockResolvedValue({ isDirectory: () => true } as any);
  readFileSpy = spyOn(fsPromises, 'readFile').mockResolvedValue('');
  mkdirSpy = spyOn(fsPromises, 'mkdir').mockResolvedValue(undefined);
});

afterEach(() => {
  readdirSpy.mockRestore();
  accessSpy.mockRestore();
  statSpy.mockRestore();
  readFileSpy.mockRestore();
  mkdirSpy.mockRestore();
});
```

**Important consideration**: The `mock.module('fs/promises')` is placed before `import { GitHubAdapter } from './github'` (line 108). This means the mock needs to intercept module-level code in `github.ts`. If `spyOn` doesn't work because the import is cached, an alternative approach is to import `fs/promises` in `github.ts` via a wrapper that can be spied on, OR to move the mock into the `describe` block's `beforeEach` if the adapter re-reads on each call.

**If `spyOn` doesn't work for module-level code**: Keep the `mock.module` but add `readFile` as a pass-through to the real `readFile` for non-test paths:

```typescript
import { readFile as realReadFile, readdir as realReaddir, access as realAccess, stat as realStat, mkdir as realMkdir } from 'fs/promises';

const mockReadFile = mock(async (path: string, ...args: unknown[]) => {
  // Only mock for paths the test cares about - pass through for everything else
  if (typeof path === 'string' && path.includes('.archon/')) {
    return '';
  }
  return realReadFile(path, ...(args as [any]));
});

mock.module('fs/promises', () => ({
  readdir: mock(async () => []),
  access: mock(async () => {}),
  stat: mock(async () => ({ isDirectory: () => true })),
  readFile: mockReadFile,
  mkdir: mock(async () => {}),
}));
```

**Why**: The global `mock.module('fs/promises')` leaks to all other test files, causing `version.test.ts` (and potentially others) to get empty strings from `readFile`.

---

### Step 2: Add `afterAll` cleanup to `executor.test.ts`

**File**: `packages/core/src/workflows/executor.test.ts`
**Lines**: After line 140 (after the existing `afterEach`)
**Action**: UPDATE

**Current code (line 133-140):**
```typescript
  afterEach(async () => {
    commitAllChangesSpy.mockRestore();
    getDefaultBranchSpy.mockRestore();
    try {
      await rm(testDir, { recursive: true, force: true });
    } catch {
      // Ignore cleanup errors
    }
  });
```

**Required change - add afterAll after the afterEach block:**
```typescript
  afterEach(async () => {
    commitAllChangesSpy.mockRestore();
    getDefaultBranchSpy.mockRestore();
    try {
      await rm(testDir, { recursive: true, force: true });
    } catch {
      // Ignore cleanup errors
    }
  });

  afterAll(() => {
    // Clear any pending timers that could keep the process alive
    // (e.g., from sendCriticalMessage retry delays)
    mock.restore();
  });
```

**Why**: Preventive cleanup to ensure no module mocks or pending state from executor tests can leak to other test files or keep the process alive.

---

### Step 3: Add/Update Tests to Verify Fix

No new test files needed. The existing 3 version tests (`version.test.ts`) serve as the regression test. After the fix, running the full suite should show 0 failures:

```bash
bun test
# Expected: 1308 pass, 5 skip, 0 fail
```

**Verification steps:**
1. Run `bun test packages/server/src/adapters/github-context.test.ts packages/cli/src/commands/version.test.ts` - should now pass all 8 tests
2. Run `bun test` - full suite should have 0 failures
3. Run `bun test packages/core/src/workflows/executor.test.ts` - should still pass all 116 tests and exit cleanly

---

## Patterns to Follow

**From codebase - mirror the spyOn pattern used in command-handler.test.ts:**

```typescript
// SOURCE: packages/core/src/handlers/command-handler.test.ts:48-55
// Pattern for avoiding global mock.module pollution with fs/promises
import * as fsPromises from 'fs/promises';

// Spies for fs/promises (avoid global mock.module pollution)
let readFileSpy: Mock<typeof fsPromises.readFile>;
let readdirSpy: Mock<typeof fsPromises.readdir>;
let accessSpy: Mock<typeof fsPromises.access>;
```

**From codebase - existing comments acknowledging the mock.module leak issue:**

```typescript
// SOURCE: packages/core/src/orchestrator/orchestrator.test.ts:11-12
// Using mock.module() for internal modules causes test isolation issues since
// Bun's mock.module() persists globally across test files.

// SOURCE: packages/core/src/handlers/command-handler.test.ts:4-6
// Note: We avoid using mock.module() for internal modules (utils/git, utils/path-validation)
// since Bun's mock.module() persists globally across test files.
```

---

## Edge Cases & Risks

| Risk/Edge Case                                            | Mitigation                                                                                 |
| --------------------------------------------------------- | ------------------------------------------------------------------------------------------ |
| `spyOn(fsPromises, 'readFile')` may not intercept module-level code in github.ts | Use the pass-through mock approach (Step 1 alternative) that delegates to real readFile for non-test paths |
| Other test files may also mock `fs/promises` via `mock.module` | `worktree.test.ts:21` mocks `node:fs/promises` (different specifier) - check if this also leaks |
| `mock.restore()` in afterAll may be too aggressive | Only restores mocks created in this file's scope; should not affect other files |
| Executor hang may reappear in different environments | The `afterAll` cleanup + `mock.restore()` provides defense in depth |

---

## Validation

### Automated Checks

```bash
# Type safety
bun run type-check

# Run the specific failing tests with their conflict partner
bun test packages/server/src/adapters/github-context.test.ts packages/cli/src/commands/version.test.ts

# Run the executor test (should complete without hanging)
bun test packages/core/src/workflows/executor.test.ts

# Full suite (should be 0 failures)
bun test

# Lint
bun run lint
```

### Manual Verification

1. Verify `bun test` reports 0 failures (previously 3)
2. Verify `bun test` exits cleanly (no hanging)
3. Verify no new test failures introduced by the changes

---

## Scope Boundaries

**IN SCOPE:**
- Fix the `mock.module('fs/promises')` leak in `github-context.test.ts`
- Add preventive cleanup to `executor.test.ts`
- Ensure all existing tests still pass

**OUT OF SCOPE (do not touch):**
- Refactoring other test files that use `mock.module` (there are 50+ instances)
- Changing the version command implementation itself
- Modifying Bun's test configuration or adding test isolation features
- Addressing `worktree.test.ts:21` `mock.module('node:fs/promises')` (different specifier, may not leak to `fs/promises`)
- Changing `packages/core/src/test/setup.ts` beyond what's needed (it's currently empty placeholders)

---

## Metadata

- **Investigated by**: Claude
- **Timestamp**: 2026-02-17T12:00:00Z
- **Artifact**: `.claude/PRPs/issues/issue-252-378.md`
