Fix #609: Windows coding phase not starting after spec/planning #1347
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughConditionally switch final execution in the spec runner: on Windows use Changes
Sequence Diagram(s)sequenceDiagram
participant Electron as Electron (UI)
participant Spec as SpecRunner
participant Python as Python Runtime
participant Run as run.py Process
Electron->>Spec: start task / planning completes
Spec->>Python: build run_cmd
alt Windows (win32)
Spec->>Run: subprocess.run(run_cmd)
Run-->>Spec: return code or raises FileNotFoundError/OSError/KeyboardInterrupt
Spec->>Electron: propagate exit status / error message
else Unix/macOS
Spec->>Python: os.execv(sys.executable, run_cmd)
Note right of Python: current process replaced by run.py (no return)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @TamerineSky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug on Windows where the transition from the planning phase to the coding phase would fail, leading to an "unknown error" and preventing task resumption. The fix involves adapting the process execution mechanism to maintain the necessary inter-process communication on Windows, while preserving the optimized approach for other operating systems. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a critical issue on Windows where the coding phase would fail to start. The approach of using subprocess.run() on Windows while retaining os.execv() for Unix-like systems is a solid platform-specific fix. The added exception handling also improves the robustness of the script. I have one suggestion to improve code consistency by using the existing platform abstraction layer.
apps/backend/runners/spec_runner.py
Outdated
| os.execv(sys.executable, run_cmd) | ||
| # Execute run.py - use subprocess on Windows to maintain connection with Electron | ||
| # Fix for issue #609: os.execv() breaks connection on Windows | ||
| if sys.platform == "win32": |
There was a problem hiding this comment.
For consistency with the project's platform abstraction layer, it's better to use the is_windows() function from core.platform instead of checking sys.platform directly. This centralizes platform-specific logic.
You'll need to add from core.platform import is_windows. Since argparse is also imported inside main(), you can import is_windows locally before this line.
from core.platform import is_windows
if is_windows():![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/backend/runners/spec_runner.py`:
- Around line 375-390: Add unit tests for the platform-specific branch in
spec_runner.py that mock sys.platform and the side-effecting calls: create a
test that patches sys.platform to "win32" and patches subprocess.run to return a
MagicMock(returncode=0) and assert subprocess.run was called and sys.exit
behavior is handled (or return code propagated), and create a test that patches
sys.platform to a non-win32 value (e.g., "darwin" or "linux") and patches
os.execv to assert os.execv was called with sys.executable and the same run_cmd;
also add a test for the Windows exception path by having subprocess.run raise
FileNotFoundError or OSError to assert the error-handling branches are executed
(and KeyboardInterrupt to assert exit code 130).
- Around line 373-390: Replace direct sys.platform checks with the core.platform
abstraction: add "from core.platform import is_windows" near the top of
spec_runner.py, then change both occurrences of "if sys.platform == 'win32':"
(the Windows branch that runs subprocess.run(...) and the earlier platform
check) to use "if is_windows():" so the code uses the centralized platform
detection; keep the existing exception handling and subprocess/os.execv behavior
unchanged.
apps/backend/runners/spec_runner.py
Outdated
| if sys.platform == "win32": | ||
| try: | ||
| result = subprocess.run(run_cmd) | ||
| sys.exit(result.returncode) | ||
| except FileNotFoundError: | ||
| print("Error: Could not start coding phase - executable not found") | ||
| sys.exit(1) | ||
| except OSError as e: | ||
| print(f"Error starting coding phase: {e}") | ||
| sys.exit(1) | ||
| except KeyboardInterrupt: | ||
| print("\n\nCoding phase interrupted.") | ||
| sys.exit(130) | ||
| else: | ||
| # On Unix/macOS, os.execv() works correctly - replaces current process | ||
| os.execv(sys.executable, run_cmd) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for platform-specific execution paths.
Based on learnings, platform-specific code should have tests that mock sys.platform to verify behavior on all platforms (Windows, macOS, Linux). This ensures the fix doesn't regress and both execution paths are validated.
Example test structure:
`@patch`('sys.platform', 'win32')
`@patch`('subprocess.run')
def test_windows_uses_subprocess(mock_run, ...):
mock_run.return_value = MagicMock(returncode=0)
# ... verify subprocess.run is called
`@patch`('sys.platform', 'darwin')
`@patch`('os.execv')
def test_unix_uses_execv(mock_execv, ...):
# ... verify os.execv is called🤖 Prompt for AI Agents
In `@apps/backend/runners/spec_runner.py` around lines 375 - 390, Add unit tests
for the platform-specific branch in spec_runner.py that mock sys.platform and
the side-effecting calls: create a test that patches sys.platform to "win32" and
patches subprocess.run to return a MagicMock(returncode=0) and assert
subprocess.run was called and sys.exit behavior is handled (or return code
propagated), and create a test that patches sys.platform to a non-win32 value
(e.g., "darwin" or "linux") and patches os.execv to assert os.execv was called
with sys.executable and the same run_cmd; also add a test for the Windows
exception path by having subprocess.run raise FileNotFoundError or OSError to
assert the error-handling branches are executed (and KeyboardInterrupt to assert
exit code 130).
|
Thank you for the review feedback! Addressed:
Note about the early startup check (line 55): Regarding unit tests: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/runners/spec_runner.py`:
- Around line 374-391: The error handlers in the Windows branch (around
is_windows(), subprocess.run(run_cmd), sys.exit(...)) should include the
command/context that failed to start so logs are actionable; update the
FileNotFoundError and OSError print calls to include run_cmd (or run_cmd[0] for
the executable) and the exception message, keeping the same exit codes and
preserving the KeyboardInterrupt handling, while leaving the non-Windows
os.execv(sys.executable, run_cmd) behavior unchanged.
TamerineSky
force-pushed
the
fix/windows-coding-phase-609
branch
from
ce1de12 to
611c277
Compare
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 1 structural/other issue(s) require attention.
Branch is out of date with base branch. Update branch first - if no conflicts arise, you can merge. If merge conflicts arise, resolve them and run follow-up review again. 3 non-blocking suggestion(s) to consider.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
Findings Summary
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (3 selected of 3 total)
🔵 [4e5418f66e42] [LOW] Exit code 130 inconsistent with other handlers in file
📁 apps/backend/runners/spec_runner.py:388
The new KeyboardInterrupt handler uses exit code 130, while the existing handler on line 401 (for spec creation interruption) uses exit code 1. Other runners (roadmap_runner.py, ideation_runner.py, github/runner.py) also use exit code 1 for KeyboardInterrupt. While 130 (128+SIGINT) is technically correct per Unix convention, it creates inconsistency within the codebase.
Suggested fix:
For consistency with the rest of the codebase, consider changing to sys.exit(1), or alternatively update other handlers to use 130 for better Unix compliance.
🔵 [d3d094c694b2] [LOW] Uses print() instead of print_status() for error output
📁 apps/backend/runners/spec_runner.py:381
The new error handling code uses raw print('Error: ...') statements, while the same file uses print_status(message, 'error') for similar error conditions (see line 321). This creates visual inconsistency in error output formatting.
Suggested fix:
Replace print() calls with print_status() for consistent UI formatting: print_status("Could not start coding phase - executable not found", "error")
🔵 [99a0d6d3aedc] [LOW] Missing debug_error() logging for exception handlers
📁 apps/backend/runners/spec_runner.py:377
The new exception handlers only print user-facing messages but do not log to the debug system. Other error paths in this file consistently use debug_error() before printing (e.g., lines 319, 396). Adding debug logging would help troubleshoot Windows-specific issues.
Suggested fix:
Add debug_error() calls before each print: debug_error("spec_runner", "Could not start coding phase - executable not found", run_cmd=run_cmd)
This review was generated by Auto Claude.
On Windows, os.execv() breaks the connection with the Electron parent process when spec_runner.py transitions to run.py for the coding phase. This causes the coding phase to never start and shows 'encountered unknown error' in the UI. Solution: - Use subprocess.run() on Windows to maintain the parent-child connection - Keep os.execv() on Unix/macOS (more efficient, replaces process) - Added import subprocess Tested on Windows 10 - coding phase now starts correctly after planning. Unix/macOS behavior unchanged (continues using os.execv() as before).
Addresses Auto Claude PR Review feedback (PR AndyMik90#743): 1. MEDIUM issue - Missing exception handling: - Added try-except for FileNotFoundError with clear error message - Added OSError handler for permission/system issues - Prevents unhelpful stack traces during coding phase startup 2. LOW issue - Misleading KeyboardInterrupt message: - Added specific KeyboardInterrupt handler for coding phase - Shows "Coding phase interrupted" instead of "Spec creation interrupted" - Exits with code 130 (standard for SIGINT) These defensive programming improvements ensure graceful error handling consistent with other subprocess.run() usage in the codebase (e.g., apps/backend/services/orchestrator.py lines 295-328). Implements suggestions from @AndyMik90's Auto Claude PR Review.
Addresses ruff F541 error on line 351:
- Changed f-string to regular string (no variable interpolation needed)
- Line 354 keeps f-string (has {e} placeholder)
Fixes CI linting check failure.
Use the centralized platform abstraction helper instead of direct sys.platform check for the execution path selection. The early startup check (line 55) must remain as sys.platform since it runs before core.platform can be imported. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
TamerineSky
force-pushed
the
fix/windows-coding-phase-609
branch
from
e2af6f3 to
efafdca
Compare
- Use exit code 1 instead of 130 for KeyboardInterrupt (matches codebase) - Use print_status() instead of print() for error messages (consistent UI) - Add debug_error() logging before each error (matches existing patterns) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addressed Review FeedbackRebased onto latest develop ✅ Consistency fixes:
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/backend/runners/spec_runner.py`:
- Around line 374-394: The PR introduces an OS-specific code path in spec_runner
(the is_windows() branch using subprocess.run and the else branch using
os.execv), so ensure CI runs/tests cover Windows, macOS, and Linux: add or
enable platform-specific CI jobs that execute the spec_runner run path (invoking
run_cmd) and validate exit codes/behavior, and add tests or test-matrix entries
that exercise subprocess.run and os.execv paths (including FileNotFoundError,
OSError and KeyboardInterrupt handling) so regressions across platforms are
caught; update CI configuration and test harness to run these jobs and surface
failures for the spec_runner platform split.
- Around line 374-391: The CI failure is due to ruff formatting changes needed
in the Windows subprocess handling block; run the formatter (e.g., ruff format
apps/backend/) to apply the style fixes, ensure the try/except alignment and
f-strings around debug_error/print_status remain intact, verify functions
referenced (is_windows(), subprocess.run, debug_error, print_status, sys.exit)
still compile, then re-commit the formatted changes.
| # Execute run.py - use subprocess on Windows to maintain connection with Electron | ||
| # Fix for issue #609: os.execv() breaks connection on Windows | ||
| if is_windows(): | ||
| try: | ||
| result = subprocess.run(run_cmd) | ||
| sys.exit(result.returncode) | ||
| except FileNotFoundError: | ||
| debug_error("spec_runner", "Could not start coding phase - executable not found") | ||
| print_status("Could not start coding phase - executable not found", "error") | ||
| sys.exit(1) | ||
| except OSError as e: | ||
| debug_error("spec_runner", f"Error starting coding phase: {e}") | ||
| print_status(f"Error starting coding phase: {e}", "error") | ||
| sys.exit(1) | ||
| except KeyboardInterrupt: | ||
| debug_error("spec_runner", "Coding phase interrupted by user") | ||
| print("\n\nCoding phase interrupted.") | ||
| sys.exit(1) | ||
| else: | ||
| # On Unix/macOS, os.execv() works correctly - replaces current process | ||
| os.execv(sys.executable, run_cmd) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Please verify Windows/macOS/Linux CI coverage for this platform split.
This change is OS-specific; confirm all three OS jobs pass to avoid regressions. Based on learnings, ...
🧰 Tools
🪛 GitHub Actions: Lint
[error] 378-385: ruff format check failed. 1 file would be reformatted (apps/backend/). Please run 'ruff format apps/backend/' to fix code style issues.
🤖 Prompt for AI Agents
In `@apps/backend/runners/spec_runner.py` around lines 374 - 394, The PR
introduces an OS-specific code path in spec_runner (the is_windows() branch
using subprocess.run and the else branch using os.execv), so ensure CI
runs/tests cover Windows, macOS, and Linux: add or enable platform-specific CI
jobs that execute the spec_runner run path (invoking run_cmd) and validate exit
codes/behavior, and add tests or test-matrix entries that exercise
subprocess.run and os.execv paths (including FileNotFoundError, OSError and
KeyboardInterrupt handling) so regressions across platforms are caught; update
CI configuration and test harness to run these jobs and surface failures for the
spec_runner platform split.
|
@AndyMik90 All issues from your review have been addressed:
All CI checks passing. Ready for re-review when you have a chance. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/runners/spec_runner.py`:
- Around line 393-396: The KeyboardInterrupt handler in spec_runner currently
calls print directly; change it to use the existing print_status UI helper to
keep formatting consistent: inside the except KeyboardInterrupt block (where
debug_error("spec_runner", "Coding phase interrupted by user") and sys.exit(1)
are used), replace the raw print("\n\nCoding phase interrupted.") with a call to
print_status(... ) that outputs the same message (e.g., "Coding phase
interrupted.") so the interrupt path matches other error paths' UI formatting.
| except KeyboardInterrupt: | ||
| debug_error("spec_runner", "Coding phase interrupted by user") | ||
| print("\n\nCoding phase interrupted.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Use print_status for KeyboardInterrupt to keep UI formatting consistent.
Minor UX consistency issue: other error paths here use print_status, but the interrupt path uses raw print.
Proposed fix
- except KeyboardInterrupt:
- debug_error("spec_runner", "Coding phase interrupted by user")
- print("\n\nCoding phase interrupted.")
- sys.exit(1)
+ except KeyboardInterrupt:
+ debug_error("spec_runner", "Coding phase interrupted by user")
+ print_status("Coding phase interrupted.", "warning")
+ sys.exit(1)🤖 Prompt for AI Agents
In `@apps/backend/runners/spec_runner.py` around lines 393 - 396, The
KeyboardInterrupt handler in spec_runner currently calls print directly; change
it to use the existing print_status UI helper to keep formatting consistent:
inside the except KeyboardInterrupt block (where debug_error("spec_runner",
"Coding phase interrupted by user") and sys.exit(1) are used), replace the raw
print("\n\nCoding phase interrupted.") with a call to print_status(... ) that
outputs the same message (e.g., "Coding phase interrupted.") so the interrupt
path matches other error paths' UI formatting.
There was a problem hiding this comment.
The existing KeyboardInterrupt handler in the same file (line 403-406) also uses print() rather than print_status():
except KeyboardInterrupt:
debug_error("spec_runner", "Spec creation interrupted by user")
print("\n\nSpec creation interrupted.")
print(...)The new handler follows the same pattern for consistency with the existing code. Changing this one would create inconsistency - if we want to update to print_status(), both handlers should be updated together in a separate PR.
There was a problem hiding this comment.
@TamerineSky, you're absolutely right! I verified the existing handler at lines 403-407 and it does use print() directly. Internal consistency within the file is more important here, and your new code correctly follows the established pattern.
If you'd like to update both KeyboardInterrupt handlers to use print_status() for better UI consistency across the codebase, that could be done as a separate cleanup PR, but it's definitely not necessary for this fix.
Thanks for catching that! 👍
🧠 Learnings used
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T09:10:31.702Z
Learning: When fixing platform-specific bugs, ensure the fix doesn't break other platforms and rely on multi-platform CI to validate (Windows, macOS, Linux)
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ## ✅ Follow-up Review: Ready To Merge
✅ Ready to merge - All checks passing and findings addressed.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Verdict
All 21 CI checks passing. All 3 previous LOW severity findings have been verified as resolved: (1) exit code now consistent at sys.exit(1), (2) print_status() now used for error output, (3) debug_error() added to all exception handlers. Only 1 new LOW severity quality suggestion identified (subprocess encoding consistency), which is non-blocking. No unanswered contributor questions. The PR properly addresses issue #609 with appropriate Windows-specific subprocess handling while maintaining Unix behavior.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
💡 Suggestions (1)
These are non-blocking suggestions for consideration:
🔵 [NEW-001] [LOW] subprocess.run without encoding parameters for consistency
📁 apps/backend/runners/spec_runner.py:378
The subprocess.run call does not specify encoding='utf-8' or errors='replace'. While the parent process has configured UTF-8 encoding for its own stdout/stderr (lines 53-80), this could be made more explicit for consistency with the extensive Windows encoding handling already in the file.
Suggested fix:
For consistency, consider: result = subprocess.run(run_cmd, text=True, encoding='utf-8', errors='replace') - though this is minor since stdout/stderr are inherited and already configured.
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
2 participants
Problem
On Windows,
os.execv()breaks the connection with the Electron parent process whenspec_runner.pytransitions torun.pyfor the coding phase. This causes:Root Cause
os.execv()on Windows spawns a new process and exits the current one, severing the IPC connection with the Electron main process.Solution
subprocess.run()on Windows to maintain the parent-child process connectionos.execv()on Unix/macOS for efficiency (replaces the current process)FileNotFoundError,OSError, andKeyboardInterruptChanges
File:
apps/backend/runners/spec_runner.pyimport subprocessTesting
Notes
This is a clean rebase of the fix from #743, which had accumulated 100+ commits and 395 changed files. This PR contains only the 3 relevant commits.
Fixes #609
Supersedes #743
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.