fix: Windows compatibility + cross-platform e2e testing harness

[spencermarx]

Summary

• Fixes the Windows dashboard crash reported in Dashboard fails on Windows: ESM loader rejects Windows absolute paths #23 — ESM loader rejects bare C:\ paths, .cmd shims fail without shell
• Introduces @open-code-review/platform shared Nx workspace library with cross-platform utilities (importModule, execBinary, spawnBinary)
• Adds e2e smoke tests for both CLI and dashboard as dedicated Nx projects
• Establishes CI with GitHub Actions — unit tests on Ubuntu, e2e on a 3-OS matrix (Ubuntu, macOS, Windows)
• Adds manual cross-platform testing via workflow_dispatch with optional SSH debug session

Changes

Platform fixes

• importModule() wraps pathToFileURL + import() — fixes Received protocol 'c:' on Windows
• execBinary() wraps execFileSync with shell: true on Windows — fixes .cmd shim detection
• spawnBinary() wraps spawn with shell: true + windowsHide: true on Windows
• Build step rm -rf/cp -r replaced with cross-platform fs.rmSync/fs.cpSync

Testing infrastructure

• @nx/vitest:test executor across all packages (replaces raw nx:run-commands)
• tsconfig.spec.json + v8 coverage config for each package
• packages/cli-e2e/ — 7 CLI smoke tests (version, help, init, doctor, state, unknown command)
• packages/dashboard-e2e/ — 8 dashboard smoke tests (health, auth, config, sessions, stats, commands, reviewers)
• /api/health moved above auth middleware (accessible without token for readiness probes)

CI workflows

• ci.yml — automated on push/PR with cross-platform e2e matrix
• cross-platform-test.yml — manual workflow_dispatch with OS picker + optional tmate SSH debug

Test plan

• nx run-many -t test — 362 unit tests pass (platform, cli, dashboard)
• nx e2e cli-e2e — 7 CLI smoke tests pass
• nx e2e dashboard-e2e — 8 dashboard smoke tests pass
• nx build cli — build succeeds with new platform imports
• CI workflow runs on all 3 OS after merge

Closes #23

🤖 Generated with claude-flow

[spencermarx]

Code Review: PR #24 — Windows compatibility + cross-platform e2e testing harness

Date: 2026-04-03
Reviewers: @principal-1, @Quality-1, @architect-1, @infrastructure-1, @testing-1
Mode: Full (with discourse)

---

Verdict

REQUEST CHANGES

Two blockers must be resolved: post-handler.ts still uses raw execFile for gh CLI calls (the exact pattern this PR eliminates), and CI jobs have no timeout-minutes setting (6-hour default on expensive macOS runners). The core platform fix is sound, the testing infrastructure is excellent, and the remaining issues are addressable without major rework.

---

Blockers

1. post-handler.ts bypasses platform lib for gh invocations

Flagged by: @principal-1
Type: Incomplete migration
Location: packages/dashboard/src/server/socket/post-handler.ts:9,23,62,125,491

Issue: post-handler.ts imports raw execFile from node:child_process and calls the gh CLI binary in three places (auth check, PR lookup, comment posting) without shell: true. On Windows, gh installed via npm is a .cmd shim — execFile('gh', ...) without shell: true will fail identically to the original dashboard crash this PR fixes.

Why this blocks: The PR's stated goal is to route all binary execution through the platform lib. This file was missed, leaving Windows gh posting broken.

Suggested fix: Add execBinaryAsync to @open-code-review/platform (since these calls run in the async Socket.IO server context where execFileSync would block the event loop), then migrate the three execFileAsync('gh', ...) calls:

// packages/shared/platform/src/index.ts — add:
export async function execBinaryAsync(
  binary: string,
  args: string[],
  opts?: ExecFileOptions,
): Promise<{ stdout: string; stderr: string }> {
  return execFilePromise(binary, args, { ...opts, shell: isWindows });
}

---

2. No job-level timeout in CI workflows

Flagged by: @infrastructure-1
Type: Operational risk
Location: .github/workflows/ci.yml:17-61 (all jobs), .github/workflows/cross-platform-test.yml

Issue: No CI job has a timeout-minutes setting. GitHub Actions defaults to 6 hours. If a test harness hangs (health poll never succeeds, server deadlocks), the CI job blocks an expensive runner for the full window.

Why this blocks: A single hung job on a macOS runner costs ~6x a Linux job and occupies a concurrent runner slot. This is a preventable operational risk.

Suggested fix: Add timeout-minutes to every job:

jobs:
  test:
    timeout-minutes: 20
    # ...
  e2e-api:
    timeout-minutes: 30
    # ...
  e2e-ui:
    timeout-minutes: 30
    # ...

---

Should Fix

1. execBinary return type is a type lie

Flagged by: @principal-1, @Quality-1, @architect-1
Location: packages/shared/platform/src/index.ts:46-49

The function declares string as the return type but execFileSync returns string | Buffer depending on the encoding option. The as string cast suppresses the type error without enforcing that callers pass encoding. A future caller omitting encoding will get a Buffer at runtime with no TypeScript warning. All current callers pass encoding: 'utf-8' — make this explicit in the signature via overloads or by requiring encoding: BufferEncoding in the options type.

2. Stale-port regression test has a race condition

Flagged by: @Quality-1, @testing-1
Location: packages/dashboard-api-e2e/src/server-api.test.ts:106

The 500ms setTimeout is a timing assumption. On a heavily loaded CI runner, 500ms may not be enough for the fork to start — the assertion vacuously passes. On a fast machine, the server may already be healthy — the stale-value check is also vacuous. Replace with a polling loop or IPC-based synchronization.

3. Playwright webServer requires .ocr/ init in CI

Flagged by: @infrastructure-1
Location: packages/dashboard-ui-e2e/playwright.config.ts:13-18

The webServer.command (npx nx dev dashboard) runs scripts/dev.ts which exits immediately if no .ocr/ directory exists. In CI (fresh clone), the e2e-ui job has no setup step to create this structure. Playwright will spend 2 minutes polling a dead process before failing with a confusing timeout. Add a CI setup step that creates the .ocr/ structure before the Playwright e2e job.

4. stdio: 'pipe' in test harness can deadlock on Windows

Flagged by: @infrastructure-1
Location: packages/dashboard-api-e2e/src/helpers/server-harness.ts:75

The forked server is started with stdio: 'pipe' but the pipe is never drained. On Windows, a full pipe buffer (64KB) will cause the child process to block, making waitForHealth time out because the server is deadlocked — not because it's unhealthy. Use stdio: process.env.CI ? 'inherit' : 'pipe' or drain the pipes explicitly.

5. Port counter collision risk in test harness

Flagged by: @infrastructure-1, @testing-1
Location: packages/dashboard-api-e2e/src/helpers/server-harness.ts:34

portCounter randomizes across a 1000-port range. Concurrent CI runs (matrix jobs, re-runs) can collide. EADDRINUSE will surface as a confusing 15s health timeout. Use OS port 0 allocation or add a pre-flight port check with retry.

6. spawnCli timeout/error handling masks failures

Flagged by: @Quality-1, @testing-1
Location: packages/cli-e2e/src/helpers/spawn-cli.ts:40-47

The err as { ... } cast and e.code ?? 1 fallback make timeouts, permission errors, and genuine exit-1 failures indistinguishable. Check e.killed for timeouts and throw descriptively. Add an existence check for CLI_BIN before tests run.

7. Ghost packages/dashboard-e2e/ directory

Flagged by: @architect-1
Location: packages/dashboard-e2e/

The old pre-split e2e directory with only node_modules/ matches the workspace glob. Delete it to prevent install warnings and nx show projects confusion.

---

Suggestions

Code Quality

• "Use type instead of interface for CliResult and TempProject — project convention" — @Quality-1
• "Extract duplicated git repo init (4 commands, 3 locations) into a shared helper" — @Quality-1, @testing-1
• "Replace page.waitForTimeout(2_000) with a condition-based wait — Playwright anti-pattern" — @Quality-1, @testing-1, @infrastructure-1
• "Add expect(result.stdout).toContain('No active session') to state show test" — @testing-1

Architecture

• "Remove ../shared/platform/src/**/*.ts from dashboard tsconfig.json include — bypasses non-buildable boundary" — @architect-1
• "Add explicit devDependencies to e2e package.json files so nx graph can trace them" — @architect-1

Infrastructure

• "Consider running 3-OS matrix only on push to main, ubuntu-only for PRs — macOS runners ~10x cost" — @infrastructure-1
• "Add comment explaining FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 env var purpose" — @infrastructure-1

---

Consensus & Dissent

Topic: execBinary return type

Reviewers: @principal-1, @Quality-1, @architect-1

Reviewer	Position
@principal-1	"Use overloads or constrained generic"
@Quality-1	"Require encoding: BufferEncoding in options type"
@architect-1	"Require it — contract should be honest from day one"

Consensus: All agree the as string cast must be fixed. Preferred approach: require encoding in the type signature.

Topic: Playwright waitForTimeout

Reviewers: @Quality-1, @testing-1, @infrastructure-1
Consensus: All agree this should be replaced with a condition-based wait.

Topic: Dashboard tsconfig.json direct include

Reviewers: @architect-1
Context: The ../cli/src/lib/db/**/*.ts include already existed on the same line — the pattern predates this PR. Removing the platform include without investigating the root cause could break type-checking.
Resolution: Valid concern; investigate and fix in follow-up rather than blocking this PR.

---

What's Working Well

• "Platform lib is a model of focused, well-documented utility code. The JSDoc explains not just what but why." — @Quality-1
• "Non-buildable pattern is correctly implemented. Canonical Nx approach." — @architect-1
• "The packages/shared/ directory is an extensible pattern for future shared code." — @architect-1
• "Port-file race fix is well-engineered — mtime guard, 30s timeout, clear error." — @principal-1, @Quality-1
• "Health endpoint correctly placed above auth middleware for readiness probes." — @infrastructure-1
• "Detroit school compliance is strong — no mocks of internal code anywhere." — @testing-1
• "The stale-port regression test is creative and valuable (despite the timing fragility)." — @testing-1
• "Detection pattern in adapters is clean and uniform across all consumers." — @principal-1

---

Requirements Assessment

Requirement	Status	Notes	Reviewer
Fix ESM loader Received protocol 'c:'	✓ Met	importModule() wraps pathToFileURL	@principal-1
Fix .cmd shim execution	⚠ Partial	execBinary/spawnBinary done, but post-handler.ts missed	@principal-1
Cross-platform build	✓ Met	fs.rmSync/fs.cpSync replaces rm -rf/cp -r	@Quality-1
E2e tests for CLI	✓ Met	7 CLI smoke tests	@testing-1
E2e tests for dashboard	✓ Met	8 API tests + 2 Playwright UI tests	@testing-1
CI with cross-platform matrix	✓ Met	3-OS matrix in ci.yml	@infrastructure-1
Manual debug workflow	✓ Met	cross-platform-test.yml with SSH	@infrastructure-1

Gaps identified: 1 requirement partially met (post-handler.ts migration incomplete)

---

Clarifying Questions

Every question below was raised by a reviewer. Please address each.

From @principal-1

• "Was post-handler.ts intentionally excluded from the migration, or is this an oversight?"
• "Is execBinary sync-only by design, or was execBinaryAsync not added yet?"

From @architect-1

• "Why does dashboard/tsconfig.json include platform sources directly via glob? Is there a build constraint?"
• "Is packages/dashboard-e2e/ intentionally preserved?"

From @infrastructure-1

• "Has pnpm nx e2e dashboard-ui-e2e been run successfully end-to-end in CI? What provides the .ocr/ directory?"
• "Is there a plan to enable Nx remote caching?"

From @testing-1

• "Should ocr state show exit non-zero when no session exists, or is exit 0 intentional?"
• "Why not use port 0 for OS-assigned ports in the test harness?"

---

Individual Reviews

Full reviews available in session directory:

Reviewer	Blockers	Should Fix	Suggestions	File
@principal-1	1	2	4	rounds/round-1/reviews/principal-1.md
@Quality-1	0	4	6	rounds/round-1/reviews/quality-1.md
@architect-1	0	2	4	rounds/round-1/reviews/architect-1.md
@infrastructure-1	2	4	3	rounds/round-1/reviews/infrastructure-1.md
@testing-1	0	4	6	rounds/round-1/reviews/testing-1.md

Session: .ocr/sessions/2026-04-03-hotfix-issue-23-windows-and-cross-platform-compatibility/
Discourse: rounds/round-1/discourse.md

[spencermarx]

Code Review: PR #24 — Windows compatibility + cross-platform e2e testing harness

Date: 2026-04-03
Reviewers: @principal-1, @Quality-1, @architect-1, @infrastructure-1, @testing-1
Mode: Full (with discourse)
Round: 2

---

Verdict

APPROVE

No blockers identified. The PR correctly fixes the Windows ESM loader crash (issue #23) and introduces a well-designed cross-platform testing infrastructure. Seven should-fix items are recommended for follow-up — none block merge.

---

Blockers

None.

---

Should Fix

1. dev.ts uses raw spawn instead of spawnBinary

Flagged by: @principal-1, @Quality-1, @architect-1
Location: packages/dashboard/scripts/dev.ts:11-86

The dev script imports spawn directly from node:child_process and passes shell: true unconditionally, rather than using spawnBinary from @open-code-review/platform. This breaks the centralization pattern this PR establishes. The platform lib exists to be the single gateway for cross-platform process spawning — having the dev script bypass it weakens the architectural invariant and introduces unnecessary shell injection surface on POSIX.

Fix: Import spawnBinary from @open-code-review/platform.

2. dev.ts cleanup can fire twice

Flagged by: @Quality-1
Location: packages/dashboard/scripts/dev.ts:93-109

Both server.on("exit") and client.on("exit") call cleanup(), which kills both processes. Signal handlers (SIGINT, SIGTERM) also call cleanup(). This cascading cleanup can call process.exit() multiple times, potentially producing confusing exit codes on some platforms.

Fix: Add a let shuttingDown = false guard at the top of cleanup().

3. execBinaryAsync has zero test coverage

Flagged by: @testing-1, @principal-1
Location: packages/shared/platform/src/__tests__/platform.test.ts

The platform library exports 4 functions but only 3 are tested. execBinaryAsync is used in production (post-handler.ts for GitHub CLI operations) with zero unit test coverage. The testing pattern for the other 3 functions is already established — this should be consistent.

Fix: Add a describe("execBinaryAsync") block with happy-path and error-path tests.

4. Stale port file test relies on hardcoded 500ms sleep

Flagged by: @testing-1
Location: packages/dashboard-api-e2e/src/server-api.test.ts:106

The test waits 500ms and assumes the server's synchronous init has completed. On slow CI runners (especially Windows GitHub Actions), the process may not have started within 500ms. This timing-dependent test is a flaky test risk.

Fix: Replace the sleep with a polling loop that checks until the port file changes from 9999 or disappears, with a reasonable timeout (e.g., 5s).

5. SSH debug workflow has no timeout guard

Flagged by: @infrastructure-1
Location: .github/workflows/cross-platform-test.yml:50-54

The tmate SSH step holds the runner open until manual disconnect or the 30-minute job timeout. A forgotten macOS debug session costs the equivalent of 300 Linux runner minutes (10x cost multiplier).

Fix: Add timeout-minutes: 15 to the tmate step.

6. No artifact upload on e2e failure

Flagged by: @infrastructure-1
Location: .github/workflows/ci.yml:33-71

When e2e tests fail, no logs, screenshots, or Playwright traces are preserved. Playwright is configured with trace: "on-first-retry" but the trace directory is never uploaded. Diagnosing Windows-specific failures requires the expensive SSH debug workflow.

Fix: Add actions/upload-artifact@v4 with if: failure() for e2e jobs.

7. Dashboard vitest.config.ts hardcodes path to platform source

Flagged by: @architect-1
Location: packages/dashboard/vitest.config.ts:12

The vitest config manually aliases @open-code-review/platform with a hardcoded relative path, duplicating the resolution already in tsconfig.base.json. If the platform lib's structure changes, this alias breaks silently in tests while TypeScript compilation continues.

Fix: Investigate using vite-tsconfig-paths plugin, or document this as the standard pattern.

---

Suggestions

Architecture & Patterns

• "No @nx/enforce-module-boundaries ESLint rule — tags are decorative without it. Consider adding enforcement as a follow-up." — @architect-1
• "packages/shared/ introduces a new depth level with only one package. Fine at current scale, but monitor for anti-pattern growth." — @architect-1
• "Consider extracting duplicated git setup logic from test harnesses into a shared helper." — @Quality-1

CI & Infrastructure

• "CI e2e-api job runs cli-e2e and dashboard-api-e2e sequentially — if CLI fails, API results are hidden. Consider run-many or continue-on-error." — @principal-1
• "Playwright UI e2e runs only on Ubuntu. No automated UI regression testing on Windows. Document the decision." — @infrastructure-1
• "Concurrency group cancel-in-progress: true may cancel 10-30 min e2e runs on rapid pushes. Consider splitting concurrency groups." — @infrastructure-1

Code Quality

• "Magic numbers for timeouts (15000, 30000) and port ranges (14000) should be named constants." — @Quality-1
• "build.mjs remains a .mjs file despite project convention requiring TypeScript. Pre-existing but touched in this PR." — @Quality-1
• "Remove concurrently from dashboard devDependencies — dead dependency after dev.ts replacement." — @principal-1

Testing

• "Server harness cleanup should send SIGKILL after 5-second timeout if SIGTERM is ignored." — @testing-1
• "Playwright auth test silently catches window.__OCR_TOKEN__ failures. Test degrades without failing if the global is renamed." — @testing-1
• "Add boundary tests for importModule with paths containing spaces — common on Windows." — @testing-1
• "e2e test harnesses use raw execFileSync for git instead of execBinary. Functional but inconsistent." — @principal-1, @Quality-1

Informational

• "Port allocation uses 14000 + random(0-999). Acceptable now with separate CI runners per OS. Switch to port 0 if moving to self-hosted." — @testing-1, @infrastructure-1
• "process.kill(-pid) in server/index.ts is POSIX-only — pre-existing code, not introduced by this PR. Flag for future Windows hardening." — @infrastructure-1
• "Server harness fork() + child.kill() becomes hard kill on Windows (no graceful shutdown). Pre-existing pattern." — @infrastructure-1

---

Consensus & Dissent

Topic: dev.ts bypasses platform lib

Reviewers: @principal-1, @Quality-1, @architect-1, @infrastructure-1

Reviewer	Position
@principal-1	"Breaks centralization pattern. Should use spawnBinary."
@Quality-1	"Inconsistent with PR's own rationale. Unnecessary shell surface on POSIX."
@architect-1	"Weakens architectural invariant. Low priority for dev-only script."
@infrastructure-1	"Harmless for hardcoded commands, but pattern should be uniform."

Consensus: All agree — use spawnBinary for consistency.

Topic: Test harness duplication

Reviewers: @Quality-1, @architect-1, @testing-1

Reviewer	Position
@Quality-1	"Extract shared createTempGitRepo() helper."
@architect-1	"Shared test utility package warranted at two packages."
@testing-1	"Some duplication in e2e is acceptable. Prefer local refactoring."

Consensus: Partial — extraction preferred but not urgent. Author's discretion.

---

What's Working Well

• "Platform lib design is excellent — minimal contract surface, four focused functions, enforced encoding in type signatures. Impossible to misuse." — @principal-1, @architect-1
• "Non-buildable workspace lib pattern correctly implemented. No redundant build step, consumers inline source via bundlers." — @architect-1
• "Detroit school testing philosophy faithfully applied throughout. Real processes, real HTTP, real filesystems. No mocks." — @testing-1, @Quality-1
• "Dev proxy race condition fix with port-file polling and timestamp-based freshness check is significantly more robust than the previous sleep 2 approach." — @principal-1, @Quality-1
• "Conditional 3-OS CI matrix (push = full, PR = ubuntu-only) is a smart cost optimization." — @infrastructure-1, @principal-1
• "JSDoc comments explain why each function exists, not just what it does." — @Quality-1
• "Release configuration correctly excludes shared and e2e packages from publishing." — @architect-1
• "The existsSync guard in spawn-cli.ts fails fast with a clear error message — excellent DX." — @testing-1
• "Port discovery regression test targets the exact race condition from the original bug." — @testing-1

---

Requirements Assessment

Requirement	Status	Notes	Reviewer
Fix ESM loader Received protocol 'c:'	✓ Met	importModule() wraps pathToFileURL	@principal-1
Fix .cmd shim execution on Windows	✓ Met	execBinary/spawnBinary set shell: isWindows	@principal-1
Replace Unix-only shell commands	✓ Met	fs.rmSync/fs.cpSync in build script	@Quality-1
E2e smoke tests for CLI	✓ Met	7 CLI test cases	@testing-1
E2e smoke tests for dashboard API	✓ Met	8 API test cases	@testing-1
Playwright browser tests for UI	✓ Met	4 Playwright tests	@testing-1
CI with cross-platform OS matrix	✓ Met	Conditional 3-OS matrix on push	@infrastructure-1
Manual cross-platform testing workflow	✓ Met	workflow_dispatch with SSH debug	@infrastructure-1

All requirements met. No gaps identified.

---

Clarifying Questions

From @principal-1

• "Does esbuild resolve the platform lib source correctly through tsconfig paths when bundling the dashboard server?"
• "Is there a reason concurrently was kept in devDependencies?"

From @architect-1

• "CLI lists @open-code-review/platform under dependencies, dashboard under devDependencies. Intentional for Nx graph resolution, or incidental?"

From @infrastructure-1

• "Is this repository on a GitHub plan with free macOS/Windows minutes, or are those billed?"
• "Is FORCE_JAVASCRIPT_ACTIONS_TO_NODE24 a real GitHub env var? Could not find it in docs."

From @testing-1

• "Should platform unit tests also run on the 3-OS matrix, or is e2e coverage sufficient?"

---

Individual Reviews

Full reviews available in session directory:

Reviewer	Should Fix	Suggestions	File
@principal-1	2	6	rounds/round-2/reviews/principal-1.md
@Quality-1	3	6	rounds/round-2/reviews/quality-1.md
@architect-1	1	4	rounds/round-2/reviews/architect-1.md
@infrastructure-1	4	4	rounds/round-2/reviews/infrastructure-1.md
@testing-1	2	6	rounds/round-2/reviews/testing-1.md

Session: .ocr/sessions/2026-04-03-hotfix-issue-23-windows-and-cross-platform-compatibility/
Discourse: rounds/round-2/discourse.md