Alex Newman
18 lines
4.6 KiB
Markdown
18 lines
4.6 KiB
Markdown
# Phase 05: Windows Stability Batch
|
|
|
|
These PRs fix Windows-specific issues. They should be reviewed in order since some may conflict.
|
|
|
|
## Tasks
|
|
|
|
- [x] Review and merge PR #972 (`Fix Windows path handling for usernames with spaces` by @farikh). File: `plugin/scripts/bun-runner.js`. This fixes bun-runner.js (just added in v9.0.17) failing when Windows usernames contain spaces. The `shell: IS_WINDOWS` option in `spawn()` causes cmd.exe to split at spaces. Fix: remove `shell: true` on Windows. Steps: (1) `gh pr checkout 972` (2) Review the spawn change — verify it removes shell option or properly quotes paths (3) Test that the change doesn't break non-space paths (4) Run `npm run build` (5) This is a direct bug in code we just shipped — high priority. If clean: `gh pr merge 972 --rebase --delete-branch`
|
|
> **Completed 2025-02-05**: Merged via rebase onto main. Fix replaces `shell: IS_WINDOWS` with `windowsHide: true` — removes cmd.exe routing that split paths at spaces, adds windowsHide to prevent console popups. The `shell: IS_WINDOWS` on line 29 (for `where` command lookup) is correctly preserved since `where` needs shell mode. Build passes clean.
|
|
|
|
- [x] Review PR #935 (`fix(worker): guard ProcessTransport writes on Windows startup` by @jayvenn21). Files: `package.json`, `patches/@anthropic-ai+claude-agent-sdk+0.1.77.patch`. This patches the Claude Agent SDK to guard stdin/stdout transport writes during startup on Windows. Steps: (1) `gh pr checkout 935` (2) **CAUTION**: This adds a patch file for the SDK. Review whether patching a dependency is the right approach vs. guarding at the application layer. (3) Check if the SDK version matches what we use (4) If the patch is invasive or fragile, request changes to implement the guard in our code instead. (5) Run `npm run build` to verify.
|
|
> **Closed 2026-02-05**: PR patches the SDK to silently swallow `ProcessTransport is not ready for writing` errors — changing a `throw` to a silent `return`. Rejected for three reasons: (1) Silently dropping writes causes subtle data loss bugs, violating Fail Fast principles (2) `patch-package` approach is fragile, tied to exact SDK v0.1.77 line numbers, breaks on any upgrade (3) Already superseded by fail-open architecture (PRs #973 and #959 merged) — worker crashes are handled gracefully without blocking Claude Code. Proper fix would be application-layer readiness checks, not SDK patching.
|
|
|
|
- [x] Review PR #931 (`Prevent repeated worker spawn popups on Windows when startup fails` by @jayvenn21). File: `src/services/worker-service.ts`. Prevents hooks from repeatedly trying to spawn the worker when startup fails, causing visible terminal popups on Windows. Steps: (1) `gh pr checkout 931` (2) Review the spawn-once logic — should track spawn attempt and not retry within a cooldown period (3) Run `npm run build` (4) If clean: `gh pr merge 931 --rebase --delete-branch`
|
|
> **Closed 2026-02-05**: PR had non-trivial merge conflicts with current main — startup logic was refactored from inline `main()` into `ensureWorkerStarted()` since this PR was written. The concept (file-based spawn cooldown lock) was sound and needed: every hook invocation runs `worker-service start`, so repeated failures on Windows produce visible terminal popups. Implemented the spawn guard directly in `ensureWorkerStarted()` with: (1) `.worker-start-attempted` lock file in claude-mem data dir (2) 2-minute cooldown skips re-spawn after failure (3) Lock cleared on successful start (4) Windows-only guards (no-op on other platforms). Build passes clean. Commit: `0ecb387f`.
|
|
|
|
- [x] Review PR #930 (`Fix blocking startup by deferring worker initialization` by @jayvenn21). File: `src/cli/handlers/context.ts`. Defers worker init so Claude UI isn't blocked for 1-2 minutes on WSL2/slow systems. Steps: (1) `gh pr checkout 930` (2) Review that deferred init still ensures context is available when needed (3) Verify this doesn't conflict with #959 (fail-open context inject) (4) Run `npm run build` (5) If clean and compatible with Phase 04 changes: `gh pr merge 930 --rebase --delete-branch`
|
|
> **Closed 2026-02-05**: PR fully superseded by Phase 04 fail-open architecture. Current main already implements non-blocking startup: `ensureWorkerRunning()` does a quick health check returning `false` without blocking (PR #959), and the context handler returns empty context gracefully. PR #930 would have regressed by removing `ensureWorkerRunning()` entirely — skipping the health check and relying solely on fetch try/catch. The three Phase 04 PRs (#959 fail-open context, #973 graceful hook failures, #931 spawn guard) collectively solve the blocking startup issue this PR targeted.
|