Code Review

This pull request introduces several Windows-specific fixes and improvements, including support for the ia32-win32 platform, updating ripgrep to version 15.1.0, and addressing file system issues on Windows by increasing retry attempts and implementing a platform-faking workaround for Arborist. Additionally, test suites were refactored for better cross-platform compatibility and reliability. Feedback suggests avoiding global side effects in packages/opencode/src/npm/index.ts by making platform-specific values injectable rather than relying on top-level environment variable modifications.

Review Summary

Overall this PR addresses Windows compatibility issues well. The changes are appropriate for fixing the unit test failures on Windows. However, I found several issues that should be addressed before merge.

P0 (Blocking)

None found.

P1 (Recommend fixing before merge)

1. ripgrep.ts: inconsistent executable suffix handling - The which() call now explicitly checks for rg.exe, but this should be verified against Bun's actual behavior on Windows.

2. npm/index.ts: global side effect at module load time - Setting process.env.__FAKE_PLATFORM__ affects the entire process. Consider wrapping this or documenting as a known compromise.

P2 (Nice to fix, optional)

3. worktree/index.ts: magic retry number - The 6x jump from 5 to 30 retries lacks justification.

4. workspace-adaptor.test.ts: silent error masking - The catch handler returns "0" which could hide real issues.

P3 (Minor nitpicks)

5. ripgrep.ts: version upgrade lacks changelog reference - Adding a comment would help future maintenance.

6. runtime-namespace.test.ts: expectRoot incomplete - Only validates 4 of 6 fields.

7. cross-spawn-spawner.test.ts: concurrency change lacks explanation - No comment clarifying intent.

Overall assessment

The PR is well-structured with clear intent. Most issues are minor (P2/P3). The P1 issues are worth addressing for code quality but may not block the Windows tests from passing. Recommend addressing P1 items or documenting why the current approach is acceptable.

Actionable comments posted: 2

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/test/global/runtime-namespace.test.ts`:
- Around line 4-9: The tests create a shared hard-coded roots object (named
roots) with /tmp paths; replace this with per-test tmpdir-managed directories by
importing tmpdir from fixture/fixture.ts and using `await using` to obtain
isolated temp dirs for data, cache, config, and state, then assign those paths
into the roots object (or directly replace usages of roots) so each test gets
its own lifecycle and automatic cleanup; update any setup/teardown that
references roots in runtime-namespace.test.ts to use the tmpdir handles returned
by tmpdir().
- Around line 13-19: The test currently only sets
process.env.PAWWORK_RUNTIME_NAMESPACE when namespace is provided, which allows
inherited values to leak; update the setup to explicitly clear or unset
PAWWORK_RUNTIME_NAMESPACE when namespace is undefined (e.g., delete
process.env.PAWWORK_RUNTIME_NAMESPACE or set it to undefined) so the child
process starts clean. Locate the test setup where process.env.XDG_* are assigned
and modify the logic around process.env.PAWWORK_RUNTIME_NAMESPACE and the local
variable namespace to ensure the env var is removed when namespace is not
passed.

Nitpick pass on the Windows stabilization PR. Nothing blocking — all P2/P3. Details inline.

• P2: src/npm/index.ts mutates a Bun-global env var at module import time.
• P2: waitForCounter silently returns last value on timeout; inconsistent with the waitFor helper right above it.
• P3: test/fixture/fixture.ts still uses maxRetries: 5 for the same Windows-EBUSY reason.
• P3: The ripgrep 14.1.1 → 15.1.0 bump is outside the stated scope of this PR.
• P3: The 3_000 → 10_000 test timeout is applied to all platforms, not just Windows.

Actionable comments posted: 2

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/src/worktree/index.ts`:
- Around line 361-373: Replace the direct import/use of "fs/promises" in the
cleanup block with the injected AppFileSystem.Service instance used earlier
(refer to the AppFileSystem.Service symbol and the variable used at line 179)
and call its fs.remove(target, { recursive: true }) method instead of fsp.rm;
then wrap that call with Effect.retry (or a Schedule) to implement the
platform-specific retry behavior (use maxRetries = process.platform === "win32"
? 30 : 5 and retryDelay 100ms) following the pattern used at the other cleanup
site (see the usage around line 497) so the operation honors the service
abstraction and applies the same retry semantics.

In `@packages/opencode/test/plugin/workspace-adaptor.test.ts`:
- Around line 60-68: The waitForCounter helper currently returns the last read
value when the 5_000ms loop times out, which hides failure context; update
waitForCounter so that after the timeout (end loop controlled by Date.now() and
end) it throws an Error instead of returning value and include contextual
details (file identifier, expected min, and last observed value) in the error
message; locate the loop using read(), wait(50), value, end and replace the
final return value with a thrown Error that contains file, min and value for
easier debugging.

Nitpick review for Windows stabilization PR. Issues graded P1-P3 per project convention.

Additional nitpick comments for PR #148.