Skip to content

feat(codex): add Windows support and compatibility for Codex plugin#101

Open
SparshGarg999 wants to merge 3 commits into
microsoft:mainfrom
SparshGarg999:fix/46-windows-support-codex
Open

feat(codex): add Windows support and compatibility for Codex plugin#101
SparshGarg999 wants to merge 3 commits into
microsoft:mainfrom
SparshGarg999:fix/46-windows-support-codex

Conversation

@SparshGarg999

Copy link
Copy Markdown
Contributor

Root Cause

The Codex integration for skillopt-sleep was previously designed primarily for Unix-like environments, using bash runner scripts (run-sleep.sh), unix-style mkdir -p directory creation in the scheduler, shell profile export paths, and a Unix bash installer (install.sh). This caused installation and execution to fail when running under Windows CMD or PowerShell. Additionally, the Codex CLI backend did not support Windows-style tool shims (.cmd), unlike the Copilot backend.

Solution

This PR adds comprehensive Windows compatibility and support for the Codex plugin:

  1. Added Windows Install/Runner Scripts:
    • plugins/codex/install.ps1: A PowerShell installer for Windows that replicates the exact logic of install.sh (copies skill files, retires legacy prompts, and suggests User-level environment variable setup).
    • plugins/run-sleep.cmd: A CMD runner wrapper.
    • plugins/run-sleep.ps1: A PowerShell runner wrapper.
  2. Exposed Windows execution steps:
    • Updated the Codex skill definition plugins/codex/skills/skillopt-sleep/SKILL.md to include Windows CMD and PowerShell commands, allowing Codex to run correctly in Windows terminals.
    • Updated plugins/codex/README.md with Windows installation instructions.
  3. Cross-Platform Backend Shims:
    • Updated resolve_codex_path in skillopt_sleep/backend.py to search Windows global Node/npm/NVM directories (e.g. %APPDATA%\npm\codex.cmd).
    • Updated CodexCliBackend.attempt_with_tools in skillopt_sleep/backend.py to write Windows-style .cmd tool shims when running on Windows (os.name == "nt").
  4. Scheduler Portability:
    • Updated _runner_cmd in skillopt_sleep/scheduler.py to produce a Windows CMD-compatible directory creation and launch string when sys.platform == "win32".

Files Changed

  • plugins/codex/README.md
  • plugins/codex/skills/skillopt-sleep/SKILL.md
  • skillopt_sleep/backend.py
  • skillopt_sleep/scheduler.py
  • tests/test_sleep_engine.py
  • plugins/codex/install.ps1 (NEW)
  • plugins/run-sleep.cmd (NEW)
  • plugins/run-sleep.ps1 (NEW)

Testing/Benchmark Performed

  1. Added unit tests test_codex_resolve_path_windows and test_codex_attempt_with_tools_windows to tests/test_sleep_engine.py to verify path resolution and .cmd shim generation.
  2. Normalized unix/windows path formats in test_sleep_engine.py to ensure cross-platform test parity.
  3. Successfully ran the entire core sleep engine test suite:
    python -m pytest tests/test_sleep_engine.py tests/test_json_utils.py tests/test_mcp_schema.py tests/test_plugin_sync.py tests/test_types.py
    (129 passed, 4 skipped)
  4. Ran the deterministic sleep experiment locally to verify improvements:
    python -m skillopt_sleep.experiments.run_experiment --persona researcher --assert-improves
    (PASS: lift +0.6667, gate blocks harmful edits)

Fixes #46

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for working on Windows support — the Windows installer/runner and Codex tool shim remain useful even after #98, so this PR has merge value. We ran it against the latest main; a few blockers remain.

Current result:

pytest tests/ -q
223 passed, 6 skipped, 1 failed

The new path-resolution test patches sys.platform to Windows while still using POSIX os.path.join, so Linux CI produces mixed separators and fails. Beyond that:

  • run-sleep.ps1 should exit $LASTEXITCODE; otherwise a failed Python run may be reported as success.
  • The scheduler still manages only crontab; returning a CMD command on Windows does not install a Windows Scheduled Task.
  • Prefer shutil.which("codex") before enumerating a few npm/NVM paths, so PATH, pnpm, Volta, and custom installs work.
  • Bring the Windows runners in line with fix(plugin): fall back to pip/uv-installed skillopt-sleep in run-sleep.sh #107 by supporting an installed skillopt-sleep CLI/module when no source checkout is available.
  • Please add real Windows CI or Pester coverage for runner execution, paths containing spaces, failure-code propagation, and installed-package fallback.
  • git diff --check currently reports trailing whitespace in the test patch.

Please rebase onto current main, address these points, and rerun both Linux and Windows tests. We prefer to merge the corrected work through #101 so your original contribution and PR attribution are preserved.

@Yif-Yang Yif-Yang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes based on the reproducible Linux test failure and the Windows runner, exit-code, scheduler, discovery, and CI gaps detailed in the maintainer comment above. The feature remains welcome after those fixes.

@SparshGarg999 SparshGarg999 force-pushed the fix/46-windows-support-codex branch from c7a1604 to 79d6117 Compare July 13, 2026 15:50
@SparshGarg999

Copy link
Copy Markdown
Contributor Author

Hello @Yif-Yang! Thank you for the detailed feedback. I have rebased onto main and addressed all your points:

  1. Path-resolution test separators: Fixed test_codex_resolve_path_windows by mocking shutil.which and using ntpath.join internally to avoid mixed separators on POSIX platforms.
  2. Propagating exit code in PowerShell runner: Updated run-sleep.ps1 to explicitly call exit $LASTEXITCODE.
  3. Windows Scheduler Tasks: Enhanced skillopt_sleep/scheduler.py to support schtasks.exe on Windows. It now registers, deletes, and lists daily Scheduled Tasks matching the Linux/macOS crontab integration.
  4. Binary path resolution: Updated resolve_codex_path to check shutil.which("codex") first, before enumerating global Node/NVM paths.
  5. Installed package fallback: Brought run-sleep.ps1 and run-sleep.cmd in sync with Unix runner (PR fix(plugin): fall back to pip/uv-installed skillopt-sleep in run-sleep.sh #107) by supporting the installed CLI command and importable module fallbacks when no source checkout is available.
  6. Windows Pester & Unit coverage:
    • Added a full Pester test suite (tests/test_runners.Tests.ps1) covering PowerShell runner execution, spaces in paths, exit-code propagation, and installed-package fallback.
    • Added Python unit tests (tests/test_scheduler_windows.py) to verify Windows task scheduling creation/deletion parameters.
    • Skipped the Unix-specific runner tests on Windows host where bash isn't fully POSIX-compliant.
  7. Trailing Whitespace: Resolved trailing whitespace issues (git diff --check passes cleanly).

All 216 python tests and the 4 Pester runner tests pass successfully on Windows. Please let me know if you need anything else!

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for the substantial update. We re-ran the latest head against current main: the Linux suite is now green (220 passed, 6 skipped), and your changes clearly address most of the original review.

We will not ask you to spend another round on minor cleanup. After merge, maintainers can handle the trailing whitespace and the small schtasks /query normalization needed for root task names such as \SkillOpt-Sleep-*.

Two Windows-specific blockers still need verification/fixes from your Windows environment before we can merge:

  1. run-sleep.cmd control flow. In the source-checkout block, PY is assigned inside parentheses but later read with %PY% rather than delayed expansion. In particular, the explicit SKILLOPT_SLEEP_PYTHON branch does not take the goto py_found path, so %PY% can remain the value expanded before the block and incorrectly report that no Python was found. Separately, :notfound exits before the installed CLI/module fallback, making that fallback unreachable in the CMD runner. Please refactor this flow (preferably without labels inside parenthesized blocks, or consistently using delayed expansion) and add a real .cmd smoke test for both source-checkout and installed-CLI fallback modes.

  2. Scheduled-task command execution with spaces. _runner_cmd() does not quote the Python executable, and the nested cmd.exe /c command relies on \" quoting that is not reliable CMD escaping. Please verify an actual create → run → delete cycle where the Python, repository, and project paths contain spaces, and adjust the /TR command construction accordingly.

Once those two Windows runtime paths pass, we can merge this original PR to preserve your contribution history, then take care of the minor deterministic cleanup ourselves.

@SparshGarg999

Copy link
Copy Markdown
Contributor Author

Hello! I have addressed the two outstanding blockers for Windows support:

  1. run-sleep.cmd control flow and exit code propagation:

    • Refactored plugins/run-sleep.cmd to resolve label jumps out of parenthesized blocks and properly handle variable expansion.
    • Used delayed expansion exit /b !errorlevel! to correctly propagate exit codes in all runner flow scenarios.
    • Added comprehensive Pester test cases in tests/test_runners.Tests.ps1 specifically for the .cmd runner to verify all fallback branches, space handling, failure exits, and module imports.
  2. Scheduled-task execution with spaces in paths:

    • Task Scheduler /TR now targets a generated helper run.cmd script under project/.skillopt-sleep/ containing properly quoted paths. This cleanly handles spaces in the Python executable, project directory, and repository paths, completely avoiding complex and unreliable CMD/subprocess escaping issues.
    • Validated the full schedule → run → unschedule cycle end-to-end on a Windows host with spaces in all paths (e.g. repo, python, and project directories). Verified that tasks ran successfully and cron.log got correctly created.

All 12 Pester tests and all 220 Python tests are passing successfully. Please let me know if any further tweaks are needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: Add Windows support / compatibility for Codex plugin

2 participants