fix(mcp): prevent detached Windows venv workers from opening a console window - #2523
zittenyetten wants to merge 2 commits into
Conversation
Replace DETACHED_PROCESS with CREATE_NO_WINDOW while retaining the configured interpreter and process-group flag. Add calibrated real-worker regressions and targeted Windows CI coverage. Refs Q00#2522
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|---|---|
| PR | #2523 |
| HEAD checked | 7937aaf1062b1a91251ca8a0f61e3f382047d213 |
| Request ID | req_1791050741_67 |
| Review record | b1e5af9c-9535-443e-b7b2-3b2d1bbe70d1 |
What Improved
- Replaces
DETACHED_PROCESSwithCREATE_NO_WINDOWfor Windows durable workers while retaining the venv interpreter, process-group flag, redirected streams, reaper, and POSIX branch. Adds real-worker Windows console and visibility checks with a legacy positive control.
Issue Requirements
| Requirement | Status |
|---|---|
| #2522: reproduce the original console and verify its absence on an affected Windows venv | Covered by legacy-control and candidate tests; PR reports a passing local run |
| Preserve worker startup, accepting-parent exit survival, nested handoff, cancellation, and worker exit | Covered by changed integration tests; PR reports a passing targeted run |
| Include Windows regressions in CI and distinguish unsupported visibility | Implemented in the Windows workflow and reporter |
| Preserve the non-Windows launch branch and assess unresolved failures without claiming no regression | POSIX branch is unchanged; failures and evidence limits are disclosed |
Prior Findings Status
No prior ouroboros-agent review rounds or human review comments were supplied. No prior blocker required reconciliation.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | PR body: Verification | Medium | High | MCP and test-infrastructure maintainers should disposition the nine disclosed failures and review the required remote gates before merge. The single historical comparison does not establish whether every failure is unrelated to this change. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the changed unit, lifecycle, console, visibility, and Windows CI tests. The six changed Python files compiled successfully. Local pytest could not start because the snapshot environment lacks pytest. The PR reports 13 passing targeted Windows cases; remote CI and repository-wide gates remain unverified.
Design Notes
The change stays within the existing MCP spawn and test boundaries. No blocking root-cause class or new subsystem is established by current source.
Design / Roadmap Gate
The declared problem is an empty console during Windows CPython 3.12 venv background-job acceptance. The flag change targets that spawn path without changing persistence or the POSIX launch branch. Tests exercise genuine workers and separate console association from visible-window observation. The disclosed broad failures and pending remote gates limit merge readiness but do not establish a current, in-boundary contract violation.
Directional Notes
Maintainer-memory cues prompted inspection of process identity, cleanup, and incomplete preflight evidence. The verdict rests on the current source and declared #2522 contract, not those cues.
Convergence Gate
-
Decision:
CONTINUE -
Review round: 1
-
Subsystem budget: 4 —
github/workflows,src/ouroboros/mcp,tests/integration/mcp,tests/unit/mcp -
Blocking findings: 0 (0 repeated)
-
Root-cause hypothesis: No blocking root cause
-
Minimal reproduction: N/A
-
Subsystem budget: MCP detached spawning, MCP test infrastructure, Windows CI
-
Scope expansion: none
-
Blocker classification: none
-
Stagnation decision: CONTINUE
-
Reset success condition: N/A
-
Reset prohibition: N/A
-
Reset verification command: N/A
-
Effect owner: MCP detached spawning
Test Coverage
- Reviewed the changed unit, lifecycle, console, visibility, and Windows CI tests. The six changed Python files compiled successfully. Local pytest could not start because the snapshot environment lacks pytest. The PR reports 13 passing targeted Windows cases; remote CI and repository-wide gates remain unverified.
Merge Recommendation
Approve this review: no in-boundary violation is proven from the current snapshot. Resolve the disclosed failures and required CI gates before merge.
Review-Metadata:
verdict: APPROVE
head_sha: 7937aaf
request_id: req_1791050741_67
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Updated in Bounded local verification: the same six-file expanded MyPy check changed from 1 reproduced error on For this new head, the 22:33 UTC CI snapshot shows 10 success, 2 in progress, 1 conditional skip. Windows and Python 3.12 tests were still running; the earlier head's green results are not substituted for them. The body now separates this head's checks from This PR is ready for formal review. Please re-review the latest head; the earlier approval covered |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|---|---|
| PR | #2523 |
| HEAD checked | ddfc17c0905a522b1ef19adab8472232e4f22ec0 |
| Request ID | req_1791066925_69 |
| Review record | f59427ea-76fd-4d4c-bff5-b7f1d0ddd0d5 |
What Improved
- Replaces the Windows detached-worker launch flag with
CREATE_NO_WINDOWwhile retaining the configured venv interpreter, process-group flag, and existing launch arguments. - Adds real-worker checks for console association, visible windows, parent exit, cancellation, and nested handoff.
Issue Requirements
| Requirement | Status |
|---|---|
| Suppress the Windows CPython 3.12 venv worker console on the existing MCP spawn path | Implemented; calibrated evidence is reported for the earlier head |
| Preserve interpreter, arguments, cwd/env, redirected streams, process group, reaper, and POSIX branch | Preserved in the current source and covered by launch-contract tests |
| Preserve worker startup, parent-exit survival, nested handoff, and cooperative cancellation | Covered by targeted real-worker tests; current-head Windows CI result was pending in the supplied body |
| Add targeted Windows CI observations for console association and visibility | Implemented; visibility remains conditional on detecting its legacy positive control |
| Linked issue #2522 requirements beyond those stated in the PR body | Not independently available in the supplied review inputs |
Prior Findings Status
The prior approval’s classification is maintained. The current snapshot shows the same production flag change; the later annotation change alters no runtime operation or assertion. No previously discussed concern is independently proven as an in-boundary blocker at this head.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | PR body:122 | Medium | High | MCP and test-infrastructure maintainers should disposition the nine disclosed Windows comparison failures separately. The supplied evidence does not establish that this flag change caused or resolved them. |
Non-blocking Suggestions
None.
Test Coverage Notes
- Reviewed the selected test paths and Windows CI step. The PR body reports 13 passing targeted Windows cases on the earlier head and a passing expanded MyPy check plus two focused cases after the annotation change. Those historical results are not current-head CI results.
- Local execution was unavailable:
uv run --python 3.12 --no-sync pytest ...could not start becausepytestis absent from this source snapshot’s environment.
Design Notes
The change stays within MCP worker spawning and its existing test and CI surfaces. No new subsystem or ownership boundary is needed.
Design / Roadmap Gate
The declared contract is limited to the Windows venv launch path under the stated conditions. Current source retains the POSIX branch and existing worker launch inputs. The new tests distinguish console association from desktop visibility and mark an undetected legacy control as unverified. Disclosed broader failures and pending merge gates require maintainer disposition, but do not demonstrate that this PR breaks its promised behavior.
Directional Notes
Prior review reasoning and maintainer memory guided inspection of process identity, cleanup, and verification scope. The verdict rests on the current source and diff. The PR body separately identifies a pending protected-workflow approval decision.
Convergence Gate
-
Decision:
CONTINUE -
Review round: 2
-
Subsystem budget: 4 —
github/workflows,src/ouroboros/mcp,tests/integration/mcp,tests/unit/mcp -
Blocking findings: 0 (0 repeated)
-
Root-cause hypothesis: No blocking root cause
-
Minimal reproduction: N/A
-
Subsystem budget: MCP worker spawning, MCP tests, Windows test workflow
-
Scope expansion: none
-
Blocker classification: none
-
Stagnation decision: CONTINUE
-
Reset success condition: N/A
-
Reset prohibition: N/A
-
Reset verification command: N/A
-
Effect owner: MCP worker spawning
Test Coverage
- Reviewed the selected test paths and Windows CI step. The PR body reports 13 passing targeted Windows cases on the earlier head and a passing expanded MyPy check plus two focused cases after the annotation change. Those historical results are not current-head CI results.
- Local execution was unavailable:
uv run --python 3.12 --no-sync pytest ...could not start becausepytestis absent from this source snapshot’s environment.
Merge Recommendation
Approve this code review: no current-head, in-boundary violation is proven. Maintainers still need to disposition the disclosed Windows failures, protected-workflow approval, and applicable current-head merge checks.
Review-Metadata:
verdict: APPROVE
head_sha: ddfc17c
request_id: req_1791066925_69
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1 participant
Summary
Background work launched through
ooo runcan open an empty Windows console when_spawn_worker()uses a CPython venv redirector. ReplaceDETACHED_PROCESSwithCREATE_NO_WINDOW, retainingCREATE_NEW_PROCESS_GROUPand the configured venv interpreter.The product change remains the same as the reviewed six-file patch. The seven-file submission adds calibrated console/visible-window checks and a targeted Windows CI step. The latest follow-up changes one test-local type annotation to resolve its expanded MyPy diagnostic; it makes no further product or CI change. This PR is ready for formal review, with the unresolved failures and approval decisions disclosed below; it is not a merge-ready or all-tests-passing claim. The earlier matched comparison remains original: 274 passed / 11 failed; modified: 276 passed / 9 failed.
Review boundary
_spawn_worker()path; locally verified on Windows build 26200 with CPython 3.12.12 venv, redirected streams and model-free durable probe jobs. Other runner/session/Python combinations require their own capability result.sys.executable, arguments, cwd/env, DEVNULL streams, process-group flag and reaper. Preserve actual worker startup, work after normal/forced accepting-parent exit, nested handoff, and cooperative cancellation followed by launcher/worker exit. Preserve the POSIX launch branch. Evidence and unverified portions are stated below.Cause and minimal change
The affected chain includes a Windows venv redirector and the native Python worker. Under the original flags, the real worker acquires a console despite stream redirection. The earlier comparison recorded nonzero versus zero
GetConsoleWindow()handles while both workers completed the model-free job.The current real-process positive control restores only the original creation flags inside the accepting test child; it still invokes the genuine worker entrypoint. It reproduced both console association and a visible, uniquely titled window. The proposed flags produced a completed venv worker with no console association and no matching visible window observed.
CREATE_NO_WINDOWis ignored when combined withDETACHED_PROCESSorCREATE_NEW_CONSOLE, so the flag is replaced, not combined. The POSIX product code is unchanged. Microsoft process-creation flags.Changes
src/ouroboros/mcp/detached_jobs.py: existing one-flag fix and explanatory comment; no further product edits in this revision.tests/unit/mcp/test_detached_jobs.py: preserve Windows/POSIX Popen contracts.tests/integration/mcp/detached_probe_process.py: real accepting-parent handshake and identity/handle-based cleanup; add an opt-in legacy-flag positive control. The default candidate launch is unmodified by the control.tests/integration/mcp/test_detached_probe_process.py: five helper safety/error-handling cases. Explicitcreationflagspreserves the existing default while resolving the helper's typing ambiguity.tests/integration/mcp/test_detached_job_worker.py: normal/forced parent, nested handoff, cancellation and isolation improvements; the latest follow-up additionally narrows one test-local state annotation fromdict[str, object]todict[str, int | bool]. Operations, waiting conditions and assertions are unchanged.tests/integration/mcp/test_detached_worker_window.py: separate console-association and visible-window tests, each requiring its own legacy positive control. Unique title/visible-window polling begins before launch and continues through exit. Evidence is saved to JSON and JUnit properties..github/workflows/test.yml: after the existing Windows checks, install the frozen dev/MCP test profile and run the same targeted 13 cases serially with isolated HOME/USERPROFILE/AppData/temp. Keep existing tests, permissions, runner and timeout. Save JUnit, scope summary and probe observations.Reproduction and upstream status
During the earlier 2026-10-04 KST validation, main and latest release v0.55.4 both resolved to
5fc67bb2e3131cca56d8cdc9a1b7534252aa5301. At publication preflight, main had advanced one commit todfc5307through #2517; its twelve changed files do not overlap this seven-file patch. Latest release v0.55.4 still resolves to the validated baseline. The branch retains the validated base without rebase. The earlier local runs were on that base; subsequent CI for head7937aafused PR merge commit63d4c4e, including maindfc5307, as detailed below. The latest-release reproduction requirement is supported by that matching revision. Relevant issue/PR searches and a refreshed open-PR check found no exact duplicate within the recorded scope.With the supplied regression tests in an isolated Windows venv, after installing the dev/MCP test dependencies:
This reviewer command selects the two window tests included in the recorded 13-case run; it was not executed as an additional standalone run. They run a legacy-flag control followed by the candidate using actual workers and the built-in model-free probe. The older 285-case comparison used identical tests/diagnostics and ordered IDs with only the product flag difference; its results are below.
GetConsoleWindow()establishes console association, not desktop visibility. A pseudoconsole can have a non-displayed handle. Visibility usesEnumWindows,IsWindowVisibleand a unique worker-set title, sampled every 50 ms. It does not prove the absence of shorter transients, unrelated windows or windows on inaccessible desktops. Microsoft GetConsoleWindow.If a legacy control is not detected, the relevant test is UNSUPPORTED / NOT VERIFIED, not a zero-window pass. Missing observations/assertion failures fail the tests. CI reports console association and visibility separately; a generally green job with a visibility skip does not establish visibility coverage on that runner. A calibrated interactive Windows run remains the separate visibility evidence when hosted CI cannot observe it.
Latest follow-up: test-local MyPy correction
Current head:
ddfc17c0905a522b1ef19adab8472232e4f22ec0. Compared with7937aaf1062b1a91251ca8a0f61e3f382047d213, this follow-up changes one line intests/integration/mcp/test_detached_job_worker.py:state: dict[str, object]becomesstate: dict[str, int | bool]. The dictionary stores a runner-call count and Boolean state;objectwas too broad for the existingint(state["runner_calls"])expression. No cast, ignore, assertion, wait condition, runtime operation, product file or workflow is changed in this follow-up.Bounded local validation used CPython 3.12.12, MyPy 2.3.1 and the same dependency versions in isolated source/profile/cache copies:
7937aafreproduced 1call-overloaderror / 6 checked files attest_detached_job_worker.py:432(exit 1, 13.48 s). The one-line candidate passed 6 checked files with no issues (exit 0, 12.32 s). This is an actual test-inclusive before/after result, separate from the earlier source-only CI gate.test_worker_preserves_live_job_after_postaccept_receipt_failure[exception-1]and[structured_error-0]both passed in the focused native-Windows diagnostic (pytest 9.1.1; 2 passed, 0 failed, 0 skipped, 4.53 s pytest time; natural process exit in 6.31 s).Before this correction, a separate 2026-10-04 isolated check reproduced the same
int(object)diagnostic once on each of maindfc530775fa0c1a3cd560708ee5e0a0f9cb2f5ceand release v0.55.4 / original PR base5fc67bb2e3131cca56d8cdc9a1b7534252aa5301: 1 error in 1 checked file at line 473, using Python 3.12.12 and MyPy 2.3.1 with--follow-imports=silent. Those runs establish a pre-patch reproduction under that toolchain, not a historical six-file baseline run. They reused the installed dependency set and one generated version-metadata file; source copies, profiles and caches were isolated, rather than claiming a wholly new dependency installation.New-head remote CI snapshot (2026-10-03 22:33 UTC): 10 checks succeeded, 2 were in progress, 1 conditional check was skipped, all associated with
ddfc17c0905a522b1ef19adab8472232e4f22ec0. The pending checks were Runtime sandbox (Windows) and Test Python 3.12. Source-only MyPy, Ruff and Bridge TypeScript had succeeded;Performance budget (advisory)was skipped. This is a time-bounded snapshot, not a claim that all current-head CI or Windows visibility checks passed; subsequent status is available in the PR Checks tab.The broader Windows comparison was not rerun for this annotation-only change. Its nine candidate failures remain disclosed below; a successful bounded MyPy/test check or source-only CI does not resolve them. The earlier bot approval covered
7937aaf, not this new head, so updated review is requested.Historical local verification: seven-file head 7937aaf
Base:
5fc67bb2e3131cca56d8cdc9a1b7534252aa5301. Earlier submitted head:7937aaf1062b1a91251ca8a0f61e3f382047d213. These results predate the latest annotation correction. Patch SHA-256:9c570a2ea94bfde9817278ee2edd0799b7ab02a985c5d23b9075d1a3662a88a4. The working copy and isolated test source were matched by all seven file hashes before/after verification.7937aaf:mypy --follow-imports=silenton those six Python files reported 1 error intest_detached_job_worker.py:432,int(state["runner_calls"])(objectargument). Two helperPopentyping ambiguities had already been corrected without changing behavior. A baseline run had not been performed at submission; later isolated main/release reproduction and the latest before/after correction are reported separately above. This historical six-file result is a failure, not the successful source-only CI gate.Historical remote verification: head 7937aaf
Snapshot: 2026-10-04 03:27 KST. All seven workflow runs associated with
7937aaf1062b1a91251ca8a0f61e3f382047d213succeeded on attempt 1; check runs were 12 success, 0 failure/pending, 2 skipped. The conditional skips werePerformance budget (advisory)andUpload coverage. Actions checked out PR merge commit63d4c4eb126ba9fcf6a5aacfa73edd49e356caad, combining that head with basedfc530775fa0c1a3cd560708ee5e0a0f9cb2f5ce. These are results for the earlier head, not proof of the latest head's CI or review status.Windows job · uploaded JUnit and observations
Runner: Windows Server 2025 build 26100,
windows-2025-vs2026image, CPython 3.12.10 venv.The lifecycle cases cover normal/forced accepting-parent exit, cancellation followed by worker/launcher exit, and nested handoff. Evidence is finite sampling on one runner session, not a guarantee for every desktop, sub-50-ms transient or arbitrary descendant. The same job separately recorded 65 plugin-hook passes and 67 sandbox passes / 19 sandbox skips; the latter skips are not skips in the selected worker cases.
Ubuntu Python 3.12 recorded 25,631 passed / 191 skipped / 1 xfailed, 8 warnings, with Python 3.12.14 and four workers on the non-performance
tests/selection. Source-only MyPy ranuv run mypy src/ouroborosand passed 686 source files; it did not cover the separate expanded test-file diagnostic. Ruff, Bridge TypeScript, Native TUI, MCP 1 SDK, issue-link and associated policy checks also succeeded for that earlier head.None of the nine failing IDs from the historical Windows comparison is in this Windows 13-case selection. Neither this result nor Ubuntu's different platform/selection establishes that those failures were fixed, unrelated or unchanged in frequency.
Historical six-file evidence — not rerun for either later revision
Previous patch SHA-256:
09fe9ea372fc2064882c3006ac04d52e39319a10e4adbb9b260213171074c347.All nine modified-version failures below are existing tests. The two 9-failure totals differ in membership: in-flight-pause failed only in the candidate; transient-resume failed only in the original without an event snapshot. No frequency or no-regression conclusion follows from one pair. Clocks, UUIDs, scheduling and sequential cache effects were uncontrolled.
The frozen six-file selected Windows run was 12 passed. Its selected Linux run was 11 passed / 1 Windows-only skip, WSL2 Linux 6.6.87.2, glibc 2.35, CPython 3.12.12; all six file hashes match that revision. An earlier Linux broad run was 284 passed / 1 Windows-only skip on eight related modules, before the final helper review; it lacks a complete source fingerprint and is not verification of either latest revision. Its historical MyPy 686-file result is also not a current-head CI result.
Remaining modified-version failures from the earlier pair
All names below are existing tests. Repeated symptoms are not sufficient to establish repeated causes.
test_cancel_job_preserves_signal_when_runner_starts_during_canceltest_cancel_job_stops_task_when_linked_session_inspection_failstest_spawn_initializes_concrete_event_store_before_emittingtest_replay_during_in_flight_pause_append_uses_last_durable_statustest_replay_after_completed_returns_completed_snapshottest_replay_after_cancel_returns_cancelled_snapshottest_agent_process_snapshot_ignores_other_processes_and_malformed_rowstest_cancelled_durable_resume_preserves_pause_when_append_fails_pre_committest_production_detached_evolve_rejects_malformed_seed_before_job_acceptanceThe two directly observed ordering failures use identical
agent_process.pyand test code and do not call_spawn_worker(). They are proposed for a separate lifecycle-ordering issue. That does not classify every other failure as pre-existing or unrelated; the malformed-seed case traverses the actual changed launch path and remains a disclosed follow-up.Review status and remaining decisions
The Windows workflow explicitly selects the new option, real-worker, console-association and visibility checks; existing Ubuntu and Windows checks remain. The results for the current head are reported in the latest-follow-up section. Earlier results above are historical evidence and do not become current-head results automatically.
The bot approval of
7937aaffound no demonstrated blocker within the declared review boundary and requested disposition of the nine failures before merge. It is not approval of the annotation follow-up. Its review environment lacked pytest; the executed Actions results are identified separately.The existing protected-workflow approval request in #2522 remains outstanding. The issue's
needs-approvalstatus concerns the protected CI change; successful Actions runs and the previous bot's code-review approval do not independently resolve it. Formal review is requested without waiving that decision or the existing test contracts.Related issues
Refs #2522
The lifecycle event-ordering issue remains a separate follow-up draft and is not fixed or closed by this PR. It has not been published as part of this submission.