Recovered: test: make the Windows CLI test harness portable (#207 by @Yazan-O)

CANDIDATES.md

@@ -0,0 +1,53 @@
+# Candidate Improvements
+
+## Candidates
+
+1. Picked: make the test suite portable on Windows.
+   Evidence: `npm test -- src/lib/credentials.test.ts` failed on Windows before the patch because POSIX mode assertions saw `0o666` instead of `0o600`, and the default credentials path assertion assumed `/` separators. The full suite also failed where subprocess tests used `HOME` but not `USERPROFILE`, tried to unset inherited env vars with `undefined`, spawned `npm` directly, and compared POSIX path strings. Fixed in `src/lib/credentials.test.ts:22`, `test/cli.subprocess.test.ts:379`, `test/helpers/npm.ts:3`, `src/lib/skill-nudge.test.ts:11`, `src/lib/agent-targets.test.ts:36`, and `src/lib/bundle.test.ts:594`.
+
+2. `doctor` bypasses the shared `--output` validator.
+   Evidence: `src/commands/doctor.ts:260` assigns `globals.output ?? 'text'` directly, while the shared validator at `src/lib/output.ts:32` rejects invalid modes with a typed `VALIDATION_ERROR`.
+
+3. Empty `TESTSPRITE_PROFILE` is not normalized like the other env vars.
+   Evidence: `src/lib/config.ts:20` normalizes empty env values, but `src/lib/config.ts:42` reads `env.TESTSPRITE_PROFILE` raw before `readProfile`; malformed profile names are rejected at `src/lib/credentials.ts:38`.
+
+4. `--password-file` strips leading and trailing spaces from passwords.
+   Evidence: project create/update both use `readFileSync(...).trim()` at `src/commands/project.ts:205` and `src/commands/project.ts:326`, which changes a password whose value intentionally starts or ends with whitespace.
+
+5. `resolveBundleDir` trims only POSIX trailing slashes.
+   Evidence: `src/lib/bundle.ts:328` checks only `rawPath.endsWith('/')`; `src/lib/junit-report.ts:201` shows the safer local pattern of accepting both `/` and `\\`.
+
+## Picked Rationale
+
+I picked the Windows test portability issue because it was directly reproducible, blocked the documented `npm test` contributor loop on this workspace, and could be fixed entirely in tests without changing CLI behavior.
+
+## Diff Summary
+
+- Added `test/helpers/npm.ts` so subprocess-style tests run `npm run build` through the current npm entrypoint, with a Windows `.cmd` fallback.
+- Isolated subprocess tests from the real Windows user profile by setting both `HOME` and `USERPROFILE`, and by removing inherited TestSprite API env vars case-insensitively.
+- Made permission-bit assertions POSIX-only where Node/Windows cannot represent `0o600` reliably.
+- Made path and CRLF-sensitive test assertions separator/line-ending neutral.
+
+## Validation
+
+- `npx -y -p node@22 -p npm@10 npm test` - passed, 50 files / 1846 tests.
+- `npx -y -p node@22 -p npm@10 npm run lint:fix` - passed.
+- `npx -y -p node@22 -p npm@10 npm run typecheck` - passed.
+- `npx -y -p node@22 -p npm@10 npm run build` - passed.
+
+## PR Title
+
+test: make Windows CLI test harness portable
+
+## PR Body
+
+Summary:
+- make path and CRLF-sensitive tests portable across Windows and POSIX
+- isolate subprocess tests from the real Windows user profile and inherited TestSprite env vars
+- run subprocess build setup through the active npm entrypoint instead of assuming `npm` is directly spawnable
+
+Tests:
+- `npm test`
+- `npm run lint:fix`
+- `npm run typecheck`
+- `npm run build`

src/lib/agent-targets.test.ts

@@ -36,7 +36,8 @@ import {
 function parseFrontmatterDescription(content: string): string | undefined {
   const lines = content.split('\n');
   let inFrontmatter = false;
-  for (const line of lines) {
+  for (const rawLine of lines) {
+    const line = rawLine.replace(/\r$/, '');
     if (line.trim() === '---') {
       if (!inFrontmatter) {
         inFrontmatter = true;

src/lib/bundle.test.ts

@@ -10,7 +10,7 @@
 
 import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
-import { join } from 'node:path';
+import { join, resolve } from 'node:path';
 import { describe, expect, it } from 'vitest';
 import {
   applyFailedOnly,
@@ -593,8 +593,7 @@ describe('resolveBundleDir', () => {
 
   it('resolves a relative path against cwd', () => {
     const out = resolveBundleDir('./tmp/x');
-    expect(out.endsWith('/tmp/x')).toBe(true);
-    expect(out.startsWith('/')).toBe(true);
+    expect(out).toBe(resolve('./tmp/x'));
   });
 
   it('strips a trailing slash', () => {

src/lib/credentials.test.ts

@@ -1,5 +1,5 @@
 import { mkdtempSync, statSync, readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs';
-import { tmpdir } from 'node:os';
+import { homedir, tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import {
@@ -19,6 +19,11 @@ import { ApiError } from './errors.js';
 let tmpRoot: string;
 let credentialsPath: string;
 
+function expectPosixMode(path: string, expected: number): void {
+  if (process.platform === 'win32') return;
+  expect(statSync(path).mode & 0o777).toBe(expected);
+}
+
 beforeEach(() => {
   tmpRoot = mkdtempSync(join(tmpdir(), 'testsprite-creds-'));
   credentialsPath = join(tmpRoot, 'credentials');
@@ -139,8 +144,7 @@ describe('writeProfile', () => {
   it('creates the file with mode 0600 and writes the profile', () => {
     writeProfile(DEFAULT_PROFILE, { apiKey: 'sk-new' }, { path: credentialsPath });
     expect(existsSync(credentialsPath)).toBe(true);
-    const mode = statSync(credentialsPath).mode & 0o777;
-    expect(mode).toBe(0o600);
+    expectPosixMode(credentialsPath, 0o600);
     expect(readProfile(DEFAULT_PROFILE, { path: credentialsPath })).toEqual({ apiKey: 'sk-new' });
   });
 
@@ -192,14 +196,13 @@ describe('ensureRestrictiveMode', () => {
     mkdirSync(tmpRoot, { recursive: true });
     writeFileSync(credentialsPath, 'data', { mode: 0o644 });
     ensureRestrictiveMode(credentialsPath);
-    const mode = statSync(credentialsPath).mode & 0o777;
-    expect(mode).toBe(0o600);
+    expectPosixMode(credentialsPath, 0o600);
   });
 });
 
 describe('defaultCredentialsPath', () => {
   it('points at ~/.testsprite/credentials', () => {
-    expect(defaultCredentialsPath().endsWith('/.testsprite/credentials')).toBe(true);
+    expect(defaultCredentialsPath()).toBe(join(homedir(), '.testsprite', 'credentials'));
   });
 });
 

src/lib/skill-nudge.test.ts

@@ -9,28 +9,36 @@ import {
   type SkillNudgeContext,
 } from './skill-nudge.js';
 
+function posixPath(path: string): string {
+  return path.replace(/\\/g, '/');
+}
+
 // ---------------------------------------------------------------------------
 // isVerifySkillInstalled
 // ---------------------------------------------------------------------------
 
 describe('isVerifySkillInstalled', () => {
   it('true when the claude own-file SKILL.md exists', () => {
-    const existsSync = (p: string) => p.endsWith('.claude/skills/testsprite-verify/SKILL.md');
+    const existsSync = (p: string) =>
+      posixPath(p).endsWith('.claude/skills/testsprite-verify/SKILL.md');
     expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true);
   });
 
   it('true for the cursor .mdc landing file', () => {
-    const existsSync = (p: string) => p.endsWith('.cursor/rules/testsprite-verify.mdc');
+    const existsSync = (p: string) =>
+      posixPath(p).endsWith('.cursor/rules/testsprite-verify.mdc');
     expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true);
   });
 
   it('true for the cline landing file', () => {
-    const existsSync = (p: string) => p.endsWith('.clinerules/testsprite-verify.md');
+    const existsSync = (p: string) =>
+      posixPath(p).endsWith('.clinerules/testsprite-verify.md');
     expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true);
   });
 
   it('true for the antigravity landing file', () => {
-    const existsSync = (p: string) => p.endsWith('.agents/skills/testsprite-verify/SKILL.md');
+    const existsSync = (p: string) =>
+      posixPath(p).endsWith('.agents/skills/testsprite-verify/SKILL.md');
     expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true);
   });
 
@@ -73,7 +81,7 @@ describe('isVerifySkillInstalled', () => {
         return false;
       },
     });
-    expect(seen.every(p => p.startsWith('/some/proj'))).toBe(true);
+    expect(seen.every(p => posixPath(p).startsWith('/some/proj'))).toBe(true);
     // One probe per target landing path.
     expect(seen).toHaveLength(Object.keys(TARGETS).length);
   });
@@ -198,6 +206,6 @@ describe('maybeEmitSkillNudge', () => {
     });
     maybeEmitSkillNudge(ctx);
     expect(probed.length).toBeGreaterThan(0);
-    expect(probed.every(p => p.startsWith('/work/here'))).toBe(true);
+    expect(probed.every(p => posixPath(p).startsWith('/work/here'))).toBe(true);
   });
 });

test/cli.subprocess.test.ts

@@ -7,14 +7,15 @@
  * and runs `auth whoami` against the mock."
  */
 
-import { execFileSync, spawn } from 'node:child_process';
-import { existsSync, mkdtempSync, statSync } from 'node:fs';
+import { spawn } from 'node:child_process';
+import { existsSync, mkdtempSync, renameSync, statSync } from 'node:fs';
 import type { IncomingMessage, Server, ServerResponse } from 'node:http';
 import { createServer } from 'node:http';
 import { tmpdir } from 'node:os';
 import { dirname, join, resolve } from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { runNpmScript } from './helpers/npm.js';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 const REPO_ROOT = resolve(__dirname, '..');
@@ -31,13 +32,18 @@ let server: Server;
 let baseUrl: string;
 let tmpHome: string;
 
+function expectPosixMode(path: string, expected: number): void {
+  if (process.platform === 'win32') return;
+  expect(statSync(path).mode & 0o777).toBe(expected);
+}
+
 beforeAll(async () => {
   // Always rebuild — `npm run build` is fast and a stale `dist/index.js`
   // would silently mask ESM/import regressions in this suite. The
   // existsSync skip we used to do here let `dist` rot under
   // refactors and gave false-green on `project list` once
   // already.
-  execFileSync('npm', ['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' });
+  runNpmScript('build', REPO_ROOT);
   server = createServer((req: IncomingMessage, res: ServerResponse) => {
     const url = req.url ?? '/';
     if (url.startsWith('/api/cli/v1/projects/')) {
@@ -349,7 +355,7 @@ beforeAll(async () => {
 }, 60_000);
 
 afterAll(async () => {
-  await new Promise<void>(resolveClose => server.close(() => resolveClose()));
+  if (server) await new Promise<void>(resolveClose => server.close(() => resolveClose()));
 });
 
 interface SpawnResult {
@@ -362,13 +368,7 @@ function runCli(args: string[], envOverrides: Record<string, string> = {}): Prom
   return new Promise((resolveResult, rejectResult) => {
     const child = spawn('node', [BIN_PATH, ...args], {
       cwd: REPO_ROOT,
-      env: {
-        ...process.env,
-        HOME: tmpHome,
-        TESTSPRITE_API_KEY: undefined,
-        TESTSPRITE_API_URL: undefined,
-        ...envOverrides,
-      } as NodeJS.ProcessEnv,
+      env: childEnv(envOverrides),
     });
     let stdout = '';
     let stderr = '';
@@ -379,6 +379,21 @@ function runCli(args: string[], envOverrides: Record<string, string> = {}): Prom
   });
 }
 
+function childEnv(envOverrides: Record<string, string>): NodeJS.ProcessEnv {
+  const env = { ...process.env };
+  for (const key of Object.keys(env)) {
+    if (key.toUpperCase() === 'TESTSPRITE_API_KEY' || key.toUpperCase() === 'TESTSPRITE_API_URL') {
+      delete env[key];
+    }
+  }
+  return {
+    ...env,
+    HOME: tmpHome,
+    USERPROFILE: tmpHome,
+    ...envOverrides,
+  };
+}
+
 describe('auth status subprocess (+ deprecated whoami alias)', () => {
   it('prints JSON me and exits 0 against the local server', async () => {
     const result = await runCli(['auth', 'status', '--output', 'json'], {
@@ -897,7 +912,7 @@ describe('setup --from-env subprocess', () => {
     expect(result.exitCode).toBe(0);
     const credentialsPath = join(tmpHome, '.testsprite', 'credentials');
     expect(existsSync(credentialsPath)).toBe(true);
-    expect(statSync(credentialsPath).mode & 0o777).toBe(0o600);
+    expectPosixMode(credentialsPath, 0o600);
   }, 30_000);
 
   it('exits 5 with VALIDATION_ERROR when --from-env is set without TESTSPRITE_API_KEY', async () => {
@@ -1044,7 +1059,7 @@ describe('--dry-run subprocess smoke', () => {
     // skipped the prompt.
     const credPath = join(tmpHome, '.testsprite', 'credentials');
     // Make sure any previous test didn't leave one behind.
-    if (existsSync(credPath)) execFileSync('rm', [credPath]);
+    if (existsSync(credPath)) renameSync(credPath, `${credPath}.bak-${process.pid}-${Date.now()}`);
     const result = await runCli(['setup', '--dry-run', '--no-agent', '--output', 'json']);
     expect(result.exitCode).toBe(0);
     expect(existsSync(credPath)).toBe(false);

test/help.snapshot.test.ts

@@ -13,6 +13,7 @@ import { execFileSync } from 'node:child_process';
 import { dirname, join, resolve } from 'node:path';
 import { fileURLToPath } from 'node:url';
 import { beforeAll, describe, expect, it } from 'vitest';
+import { runNpmScript } from './helpers/npm.js';
 
 const __dirname = dirname(fileURLToPath(import.meta.url));
 const REPO_ROOT = resolve(__dirname, '..');
@@ -47,7 +48,7 @@ const cases: Array<[string, string[]]> = [
 
 describe('--help snapshots', () => {
   beforeAll(() => {
-    execFileSync('npm', ['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' });
+    runNpmScript('build', REPO_ROOT);
   });
 
   for (const [name, args] of cases) {

test/helpers/npm.ts

@@ -0,0 +1,13 @@
+import { execFileSync } from 'node:child_process';
+
+export function runNpmScript(script: string, cwd: string): void {
+  const npmExecPath = process.env.npm_execpath;
+  if (npmExecPath) {
+    execFileSync(process.execPath, [npmExecPath, 'run', script], { cwd, stdio: 'pipe' });
+    return;
+  }
+  execFileSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['run', script], {
+    cwd,
+    stdio: 'pipe',
+  });
+}