fix(test): guard the CASES_DIR delete + harden the data-dir teardown

PR #356 stopped the remote-hosts.json fixture write from clobbering prod.
Two holes in the same file remain:

1. The quick-start afterEach still ran rmSync(CASES_DIR, recursive).
   CASES_DIR is join(homedir(), 'codeman-cases'), and on Linux builds
   where os.homedir() reads /etc/passwd instead of $HOME it resolves to
   the PROD case tree - so a full-suite run deleted the real
   ~/codeman-cases. Add a shared safeRmHomeTree() containment gate that
   only deletes a path under the redirected test HOME.

2. setup.ts teardown did rmSync(process.env.CODEMAN_DATA_DIR ?? '') AFTER
   restoring the env - if a pre-existing prod CODEMAN_DATA_DIR was set,
   that deleted prod. Capture the throwaway dir in a const and clean that.

A broader test-isolation sweep (10 files: cli-skill-target, edge-cases,
integration-flows, operation-lightspeed, ralph-integration,
case-clone-routes, voice-routes, session-cleanup, sse-events,
sse-subscription-filter) also applies the same containment gates to every
per-case delete. It is intentionally NOT included here to keep this PR
skinny; it is identified and available on request.
This commit is contained in:
timkjr
2026-09-02 20:41:24 -05:00
parent 2694d3f74a
commit ff88b6957e
4 changed files with 53 additions and 6 deletions
+1 -1
View File
@@ -7,5 +7,5 @@
export { MockSession, createMockSession, terminalOutputs } from './mock-session.js';
export { MockStateStore } from './mock-state-store.js';
export { waitForEvent, createDeferred } from './test-helpers.js';
export { waitForEvent, createDeferred, safeRmHomeTree, isUnderTestHome } from './test-helpers.js';
export { createMockRouteContext, type MockRouteContext } from './mock-route-context.js';
+39
View File
@@ -2,6 +2,9 @@
* Reusable async test helpers.
*/
import { rmSync } from 'node:fs';
import { resolve } from 'node:path';
/** Wait for an EventEmitter to emit a specific event, with timeout */
export function waitForEvent(
emitter: { once: (event: string, listener: (...args: unknown[]) => void) => void },
@@ -34,3 +37,39 @@ export function createDeferred<T = void>(): {
});
return { promise, resolve, reject };
}
/**
* Delete a directory tree, but ONLY when it lives inside the test HOME.
*
* SAFETY (2026-08-29): `test/setup.ts` redirects `process.env.HOME` to a
* throwaway dir, but code that resolves paths via `os.homedir()` does NOT
* follow that redirect on every Linux build/Node version — some read
* /etc/passwd instead of $HOME. A test that `rmSync(CASES_DIR, recursive)` can
* therefore delete the PRODUCTION `~/codeman-cases` (or any home-anchored
* tree) on those platforms. This gate refuses to delete anything not under the
* (redirected) `process.env.HOME`. Lexical `resolve()` is used because the leaf
* often does not exist and `realpathSync` would throw.
*/
export function safeRmHomeTree(path: string): void {
const home = process.env.HOME;
if (!home) return;
const target = resolve(path);
const root = resolve(home);
if (target === root || target.startsWith(root + '/')) {
rmSync(target, { recursive: true, force: true });
}
}
/**
* True when `path` resolves strictly inside `process.env.HOME` (or to it).
* Same rationale as `safeRmHomeTree`; use for guarded non-recursive deletes
* (single files like `linked-cases.json`) so they never touch prod state on
* platforms where `os.homedir()` ignores `$HOME`.
*/
export function isUnderTestHome(path: string): boolean {
const home = process.env.HOME;
if (!home) return false;
const target = resolve(path);
const root = resolve(home);
return target === root || target.startsWith(root + '/');
}
@@ -26,7 +26,7 @@ import { mkdtemp, rm, readFile, mkdir, writeFile } from 'node:fs/promises';
import { existsSync } from 'node:fs';
import { join } from 'node:path';
import { tmpdir } from 'node:os';
import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js';
import { createMockRouteContext, safeRmHomeTree, type MockRouteContext } from '../mocks/index.js';
import { installRouteErrorHandler } from '../../src/web/route-error-handler.js';
import { registerSessionRoutes } from '../../src/web/routes/session-routes.js';
import { generateHooksConfig, applyWorkspaceHooks } from '../../src/hooks-config.js';
@@ -259,7 +259,11 @@ describe('POST /api/quick-start workspace hooks', () => {
// Docker fixtures + case dirs must not leak into the next test.
await rm(join(getDataDir(), 'docker-hosts.json'), { force: true });
await rm(join(getDataDir(), 'docker-cases.json'), { force: true });
await rm(CASES_DIR, { recursive: true, force: true });
// SAFETY (2026-08-29): CASES_DIR is `join(homedir(), 'codeman-cases')`, and
// on environments where `os.homedir()` ignores `$HOME` it resolves to the
// PROD case tree. `safeRmHomeTree` refuses to delete anything not under the
// redirected test HOME, so a run can never nuke the real `~/codeman-cases`.
safeRmHomeTree(CASES_DIR);
});
it('installs hooks into an EXISTING case directory (a linked case / cloned repo)', async () => {
+7 -3
View File
@@ -20,6 +20,7 @@ const originalVitest = process.env.VITEST;
const originalPlaywrightBrowsersPath = process.env.PLAYWRIGHT_BROWSERS_PATH;
const originalCodemanDataDir = process.env.CODEMAN_DATA_DIR;
const testHome = mkdtempSync(join(tmpdir(), 'codeman-vitest-'));
const testDataDir = join(tmpdir(), `codeman-vitest-data-${process.pid}`);
if (originalPlaywrightBrowsersPath === undefined && originalHome) {
process.env.PLAYWRIGHT_BROWSERS_PATH =
@@ -41,7 +42,7 @@ process.env.VITEST = 'true';
// overwrote prod `remote-hosts.json` with an `h1/box/10.0.0.5` fixture during a
// bare full-suite run, wiping every user-defined remote host and emptying the
// launch case dropdown). Point every test at a throwaway data dir instead.
process.env.CODEMAN_DATA_DIR = join(tmpdir(), `codeman-vitest-data-${process.pid}`);
process.env.CODEMAN_DATA_DIR = testDataDir;
delete process.env.CODEMAN_PASSWORD;
delete process.env.CODEMAN_USERNAME;
@@ -61,7 +62,9 @@ afterAll(async () => {
// "onUserConsoleLog" call is still pending, and that single unhandled
// EnvironmentTeardownError fails the run after every test has passed
// (observed twice on the PR #175/#176 merge commit; never locally).
await new Promise((resolve) => setTimeout(resolve, 50));
const { promise: drained, resolve: drainDone } = Promise.withResolvers<void>();
setTimeout(drainDone, 50);
await drained;
if (originalHome === undefined) delete process.env.HOME;
else process.env.HOME = originalHome;
@@ -79,7 +82,7 @@ afterAll(async () => {
else process.env.CODEMAN_DATA_DIR = originalCodemanDataDir;
rmSync(testHome, { recursive: true, force: true });
rmSync(process.env.CODEMAN_DATA_DIR ?? '', { recursive: true, force: true });
rmSync(testDataDir, { recursive: true, force: true });
});
// afterAll never fires for a fully-skipped test file (no tests execute), which
@@ -87,4 +90,5 @@ afterAll(async () => {
// with force is a no-op when afterAll already removed it.
process.on('exit', () => {
rmSync(testHome, { recursive: true, force: true });
rmSync(testDataDir, { recursive: true, force: true });
});