Skip to content

fix: Windows codex runner path quoting and log encoding#453

Closed
peteromallet wants to merge 2 commits intomainfrom
fix/windows-codex-runner-442
Closed

fix: Windows codex runner path quoting and log encoding#453
peteromallet wants to merge 2 commits intomainfrom
fix/windows-codex-runner-442

Conversation

@peteromallet
Copy link
Owner

Summary

  • Quote executable paths containing spaces when building cmd /c commands on Windows, preventing subprocess spawn failures
  • Add encoding="utf-8", errors="replace" to read_text() calls in log recovery (io.py), preventing UnicodeDecodeError on Windows where the default encoding is cp1252
  • Bump version to 0.9.10

Fixes #442

Test plan

  • Full test suite passes (5364 passed, 3 skipped)
  • Manual verification on Windows with a path containing spaces
  • Manual verification with a log file containing non-cp1252 bytes

🤖 Generated with Claude Code

…overy

Fixes #442

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…quoting

The previous fix pre-quoted the executable path, but the actual breakage was
in argument paths (-C repo_root, -o output_file) containing spaces. Pre-embedding
quotes in a subprocess list causes double-quoting because Popen's list2cmdline()
adds its own quotes.

The real issue: cmd /c concatenates everything after /c and re-parses it with its
own tokeniser. The fix introduces _wrap_cmd_c() which uses subprocess.list2cmdline()
to build the inner command as a single properly-quoted string, then passes that as
one token after /c: ["cmd", "/c", "codex exec -C \"path with spaces\" ..."].

- Revert incorrect executable pre-quoting in _resolve_executable
- Add _wrap_cmd_c() to properly collapse cmd /c commands
- Apply _wrap_cmd_c in codex_batch_command after building the full arg list
- Keep correct encoding="utf-8", errors="replace" fix in io.py
- Add tests for _wrap_cmd_c and Windows codex_batch_command path quoting

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@peteromallet
Copy link
Owner Author

Fix corrected: proper cmd /c argument quoting

The previous commit pre-quoted the executable path in _resolve_executable, but the actual breakage was in argument paths (-C repo_root, -o output_file) containing spaces. The error unexpected argument 'Copy' found confirms -C core_project - Copy was being split at the space.

Why pre-quoting was wrong

subprocess.Popen with a list calls list2cmdline() which adds its own quoting. Pre-embedding " around the executable caused double-quotinglist2cmdline would produce "\"C:\...\codex.CMD\"".

The real problem

cmd /c concatenates everything after /c into a single string and re-parses it with its own tokeniser. Even though list2cmdline quotes individual arguments with spaces, cmd's re-parsing has edge cases where those quotes don't survive properly.

The fix

Introduced _wrap_cmd_c() which:

  1. Takes the full command list ["cmd", "/c", "codex.cmd", "exec", "-C", "path with spaces", ...]
  2. Uses subprocess.list2cmdline() on everything after /c to build one properly-quoted string
  3. Returns ["cmd", "/c", "codex.cmd exec -C \"path with spaces\" ..."]

This way, cmd /c receives exactly the command string we intended as a single token.

What's kept

  • The encoding="utf-8", errors="replace" fix in io.py is correct and unchanged.

Tests

  • Added test_wrap_cmd_c_collapses_arguments_into_single_string — verifies the collapsing logic
  • Added test_codex_batch_command_on_windows_collapses_cmd_c — end-to-end test mocking Windows platform
  • Full suite: 5366 passed, 3 skipped

@peteromallet peteromallet deleted the fix/windows-codex-runner-442 branch March 16, 2026 21:22
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.

desloppify review --run-batches --runner codex fails on Windows, then crashes during log recovery

1 participant