fix(windows): isolate detached launch stdio handles - #38
Conversation
- document the inherited-handle root cause and stable boundary - plan TDD, Windows regression coverage, and independent review
- clear inheritance from valid runner standard handles before detached spawns - cover process-mode policy and redirected stdout EOF behavior
- promote task headings and remove trailing blank lines
- clear inheritance without a query race and ignore ERROR_INVALID_HANDLE - bound EOF regression cleanup and expose taskkill failures
WalkthroughВ Windows добавлена изоляция наследуемых стандартных handle перед запуском ChangesИзоляция detached stdio в Windows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsTest
participant V8Runner
participant WindowsAPI
participant DetachedChild
participant StdoutReader
WindowsTest->>V8Runner: Запуск Detached или ManagedDetached
V8Runner->>WindowsAPI: Очистка HANDLE_FLAG_INHERIT
WindowsAPI-->>V8Runner: Результат изоляции
V8Runner->>DetachedChild: Spawn
V8Runner-->>WindowsTest: Возврат PID
StdoutReader->>V8Runner: Чтение stdout pipe
V8Runner-->>StdoutReader: EOF до завершения DetachedChild
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
- preserve Windows standard-handle isolation for detached modes - retain master managed-wait command construction
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/platform/process.rs`:
- Around line 586-624: Update isolate_inherited_standard_handles to detect real
Windows console handles before calling SetHandleInformation, using the
appropriate console-handle check. Ignore SetHandleInformation errors for console
handles, while preserving error propagation for non-console pipe/handle values
so inheritance isolation remains enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e4a9d41-4172-41bf-960e-894dac89e499
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomldocs/superpowers/plans/2026-07-22-windows-detached-stdio-isolation.mddocs/superpowers/specs/2026-07-22-windows-detached-stdio-isolation-design.mdsrc/platform/process.rs
- ignore SetHandleInformation failures only for console handles\n- keep non-console handle failures actionable\n- cover console-only failure policy
- ignore only stale ERROR_INVALID_HANDLE failures - cover redirected stdout EOF for both detached launch modes - run targeted regressions in Windows contract CI
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/platform/process.rs (1)
587-613: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftПроверить обработку
SetHandleInformationдля реальных консольных handle-ов.Прошлый обзор уже указывал, что
SetHandleInformationможет завершаться ошибкой на консольных handle-ах с кодом, отличным отERROR_INVALID_HANDLE. В показанном коде логика не изменилась:is_invalid_standard_handle_errorпо-прежнему игнорирует только код 6, а обнаружение консольных handle-ов (например, черезGetFileType) не добавлено. Комментарий помечен как "✅ Addressed in commit 081c943", но видимая реализацияisolate_inherited_standard_handles(строки 587-613) не отражает такое исправление — изменение в строках 602-603 затрагивает только SAFETY-комментарий.Независимое исследование подтверждает: Calling SetHandleInformation(dwMask=HANDLE_FLAG_INHERIT) fails on console handles, so the inheritability of an existing console handle cannot be changed. При этом сам подход (сбрасывать
HANDLE_FLAG_INHERITу собственных std-handle-ов раннера перед detached-запуском) — это признанный workaround: windows: when creating process with redirected stdio, make parent's std handles not inheritable.Если runner запущен интерактивно (stdout/stderr не перенаправлены и указывают на реальную консоль),
isolate_inherited_standard_handlesможет вернуть ошибку, отличную отERROR_INVALID_HANDLE, иspawn_commandзавершится какProcessError::SpawnFailedдля Detached/ManagedDetached — то есть обычный интерактивный запуск CLI-инструмента может перестать работать.Уточните код ошибки, который реально возвращает
SetHandleInformationдля консольных handle-ов на целевых версиях Windows, и при необходимости добавьте проверку типа handle-а (GetFileType) перед вызовомSetHandleInformation, пропуская ошибку для консольных handle-ов вместо толькоERROR_INVALID_HANDLE.Запустите скрипт для проверки полного тела
is_invalid_standard_handle_errorи наличия какой-либо консольной проверки, не попавшей в предоставленный фрагмент:#!/bin/bash set -euo pipefail ast-grep outline src/platform/process.rs --items all --match 'is_invalid_standard_handle_error|isolate_inherited_standard_handles' --view expanded rg -n -A 15 'fn is_invalid_standard_handle_error' src/platform/process.rs rg -n 'GetFileType|GetConsoleMode|FILE_TYPE_CHAR' src/platform/process.rsТакже подтвердите ожидаемый код ошибки
SetHandleInformationдля консольных handle-ов на актуальных версиях Windows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/process.rs` around lines 587 - 613, Update isolate_inherited_standard_handles to recognize real console standard handles before or during SetHandleInformation, using the appropriate Win32 handle-type/console detection and preserving the existing ERROR_INVALID_HANDLE handling. Treat the known console-handle failure as ignorable so interactive stdout/stderr do not cause the function to return an error, while still propagating unexpected failures; verify the relevant Windows error code and reuse or update is_invalid_standard_handle_error as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/platform/process.rs`:
- Around line 587-613: Update isolate_inherited_standard_handles to recognize
real console standard handles before or during SetHandleInformation, using the
appropriate Win32 handle-type/console detection and preserving the existing
ERROR_INVALID_HANDLE handling. Treat the known console-handle failure as
ignorable so interactive stdout/stderr do not cause the function to return an
error, while still propagating unexpected failures; verify the relevant Windows
error code and reuse or update is_invalid_standard_handle_error as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64446541-ad5f-4f46-a61e-869b32f678aa
📒 Files selected for processing (4)
scripts/test/README.mdscripts/test/ci-rust.shspec/acceptance/real-environment-validation.mdsrc/platform/process.rs
2 participants
Closes #31.
What changed
HANDLE_FLAG_INHERITfrom the runner's Windows stdin/stdout/stderr handles before detached and managed-detached process creationERROR_INVALID_HANDLEstandard handles as absent; other Win32 failures remain fail-closed throughProcessError::SpawnFailedtaskkillfailuresWhy this boundary
Stable Rust does not expose the Win32 process attribute handle allowlist. A custom
CreateProcessWpath would duplicate Rust command-line, environment, working-directory, child ownership, and Job Object semantics. Permanently clearing only the inherit flag is idempotent, does not close the handles, and avoids a temporary clear/restore race.Verification
cargo fmt --all -- --check,cargo check --all-targets, clippy, andgit diff --checkpassedbe558dbPlatform note
The local Homebrew Rust toolchain has no Windows target, so the cfg-gated EOF/liveness regression requires Windows CI/runtime confirmation. The
windows-syssignatures, features, handle sentinels, and error semantics were checked against the local generated bindings and Microsoft Win32 documentation.Summary by CodeRabbit
Исправления
Документация
Тестирование