fix(environments): Windows job-breakaway parity for LocalEnvironment foreground commands

[VerbalChainsaw]

fix(environments): Windows job-breakaway parity for LocalEnvironment foreground commands

What this changes

LocalEnvironment._run_bash (the foreground terminal spawn path) previously used
{"creationflags": windows_hide_flags()} on Windows (CREATE_NO_WINDOW only) alongside
start_new_session=True. On Windows that left the spawned bash inside the parent Hermes job object,
so it had no parity with the POSIX path, where start_new_session=True already puts the child in its
own session.

This change routes the spawn through windows_detach_popen_kwargs():

• POSIX: unchanged — the helper returns exactly start_new_session=True (the single keyword this
  call site already passed; os.setsid in the child). No preexec_fn.
• Windows: the full detach bundle — CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_NO_WINDOW | CREATE_BREAKAWAY_FROM_JOB — so a foreground child breaks out of the parent's job
  object where that job permits breakaway, giving best-effort process-survival parity with POSIX.

It adds a narrow fallback for an error consistent with a construction-time breakaway refusal
(WinError 5 plus confirmed job membership makes a breakaway refusal the plausible cause, not a
certainty — but the predicate is safe either way):

• retry only when the OSError has winerror == 5 (ERROR_ACCESS_DENIED) and an
  IsProcessInJob query confirms this process is actually in a job (a process in no job cannot
  receive a breakaway refusal, so winerror 5 there is unrelated);
• retry once, without CREATE_BREAKAWAY_FROM_JOB;
• fail closed: if the job-membership query fails or is unavailable, or winerror != 5, the
  original exception propagates with no retry;
• a successful Popen is never respawned — no post-construction fault can trigger a second spawn.

Correcting a prior claim

An earlier discussion held that DETACHED_PROCESS "severs stdio" and would break this call site's
stdout=PIPE. That is not correct. DETACHED_PROCESS only removes an inherited console;
explicitly redirected handles are unaffected. Native probes confirmed that explicitly
redirected stdin, stdout, and merged stderr remain functional with DETACHED_PROCESS. This call site
always redirects stdout/stderr/stdin explicitly, so there is no console to lose.

What this is and is not

This is best-effort process-survival parity with POSIX, nothing more:

• ✅ Where the job permits breakaway, the foreground child leaves the job and can outlive an abrupt
  parent death — the same property POSIX setsid already gives.
• ✅ Controlled Hermes ownership is fully retained: timeout, interrupt, graceful shutdown, and
  explicit process-tree termination all still kill the child (the detached, broken-away child stays
  explicitly killable — confirmed in native testing).
• ❌ Not universal job escape. On a restrictive job (no JOB_OBJECT_LIMIT_BREAKAWAY_OK), modern
  nested-job Windows can silently retain the child with no error raised — the fallback never fires,
  and the child dies when the job is torn down. No benefit is claimed there.
• ❌ Not durable execution and not durable output. After an abrupt Hermes death the surviving
  process has no result owner: its stdout pipe reader is gone, output is lost, the return code and
  CWD marker and session snapshot have no consumer, and a command may have completed side effects
  that are never recorded. This change does not attempt to fix that; durable execution belongs on a
  separately designed persistent surface, not on this foreground flag.

Scope

• Production: tools/environments/local.py only — the call-site change plus one private
  _process_in_job() helper (kept local; not exported).
• Test: tests/tools/test_local_windows_breakaway_parity.py (new).

Testing

10 new behavioral tests drive the real patched _run_bash with subprocess.Popen mocked, forcing
both platform branches so they run on any host: primary Windows flags, qualifying winerror 5 + in-job
single retry, winerror 5 not-in-job propagation, winerror != 5 propagation, real-boundary query
failure failing closed, second-failure propagation, POSIX single start_new_session, POSIX OSError
propagation, no preexec_fn, returned process killable, no respawn after success.

Focused adjacent suites (12 files: base environment, background-child hang, threaded handle, timeout,
exit semantics, init-session cwd, cwd recovery, Windows native support, terminal tool, Windows
tree-kill, two gateway-shutdown kill tests): 201 passed, 7 deselected, zero new failures on native
Windows vs the identical clean-baseline command (191 passed clean; +10 new). The 7 deselections are
pre-existing known-Windows skips, unchanged by this PR.

A native job-object experiment drove the exact patched call site through a sacrificial launcher under
an external controller:

• Permitting job: launcher assigned to and confirmed in the specific job; the patched
  bash runner and its descendant confirmed not in that job; after abrupt launcher+job teardown the
  descendant survived; a retained non-breakaway control process in the same job died on
  teardown (showing the harness distinguishes escape from retention); the survivor was then explicitly
  terminated and confirmed dead; exactly one side effect (no duplicate execution). Reproduced
  deterministically across 3 runs with distinct PIDs.
• Restrictive job: the resulting child remained in the restrictive job and died on teardown. The
  native harness did not instrument the number of real Popen attempts, so it does not distinguish
  silent primary retention from a constructor fallback; both preserve the disclosed restrictive-job
  limitation. (The mocked tests cover the fallback contract itself.)

Host: Windows 11 10.0.28120, CPython 3.11.9 (64-bit, AMD64). Full Win32 boundary detail (structure
sizes, argtypes/restype, API return values) is recorded in the evidence package.

Limitations

Live evidence covers the spawn/fallback contract, permitting-job escape, controlled killability, and
restrictive-job retention on one Windows build. The behavioral suite is mocked-Popen (by design, so
it runs cross-platform and asserts exact kwargs). The silent-retention behavior is
configuration-specific; other Windows builds may instead raise winerror 5, which the fallback
handles. See RISK_STATEMENT.md.

[alt-glitch]

This was generated by AI during triage.

Companion fix to #43252 — both replace the legacy windows_hide_flags() detach pattern with windows_detach_popen_kwargs() + OSError fallback. This one targets tools/environments/local.py; #43252 targets cron/scheduler.py. Both complete the same Windows job-object detach migration (parity with the merged hermes_cli/gateway.py pattern and #42993). Not duplicates — different call sites; both should land.

[tonydwb]

Code Review Summary

Verdict: Approved

✅ Looks Good

• Same Windows detach-flag fix as #43252 but for tools/environments/local.py: Replaces the legacy windows_hide_flags() (which only sets CREATE_NO_WINDOW) with windows_detach_popen_kwargs() which sets all 4 detach flags including CREATE_BREAKAWAY_FROM_JOB. On restrictive Windows job objects, this prevents terminal commands from being silently reaped when the parent process exits.
• OSError fallback with nested retry: Mirrors the canonical pattern in hermes_cli/gateway.py:716-742 — if the breakaway flag is denied by the job object, the fallback uses windows_detach_flags_without_breakaway() and retries. This is a significant improvement over the pre-fix behavior where the command would crash with PermissionError.
• Comprehensive static-analysis tests: Three AST-based tests verify (1) the new helper is imported and used, (2) subprocess.Popen is wrapped in try/except OSError, and (3) the fallback uses the no-breakaway helper. These prevent future maintainers from reverting to the broken pattern.
• Security consideration noted in code: The comment about not logging the full argv (because cron scripts may embed secrets like AWS_SECRET_ACCESS_KEY=...) is appropriate — no actual secrets appear in the diff.
• Well-scoped: Only 2 files, clear description referencing the sibling PR #43252.

---

Reviewed by Hermes Agent

[teknium1]

Thanks for tracing a real-looking Windows process-lifetime gap. tools/environments/local.py:1074 still uses only windows_hide_flags(), so this remains a relevant call site.

Problems

• The patch predates 515192c4b, which changed the same Popen to start_new_session=True at current tools/environments/local.py:1085 to avoid preexec_fn fork-time crashes. A current-main port must replace that argument with the helper, not retain both; on POSIX the helper itself supplies start_new_session=True.
• The added tests are AST/source checks (tests/tools/test_windows_native_support.py:871-1035), so they do not prove the first Popen failure actually retries with windows_detach_flags_without_breakaway().
• Current main later reverted broad Windows popup spawn changes through this call site in d3d621f7c; its maintainer rationale requires a focused native-Windows reproduction rather than another general spawn-layer migration.

Suggested changes

• Port only the current _run_bash spawn, preserving the no-preexec_fn guarantee from 515192c4b.
• Add behavioral mocked-Popen coverage for primary flags, OSError retry flags, and the POSIX single-session-kwarg path.

Automated hermes-sweeper review.

[teknium1]

Current main has start_new_session=True at this Popen from 515192c4b. When salvaging, replace that current argument with this helper rather than combining both: on POSIX the helper returns {"start_new_session": True}, so passing both would duplicate the Popen keyword.

[VerbalChainsaw]

Thanks — done. The call site now passes **windows_detach_popen_kwargs() and no longer
passes a literal start_new_session=True, so on POSIX there is exactly one start_new_session=True
(supplied by the helper) and no duplicate keyword. A dedicated test asserts this —
test_posix_single_start_new_session checks the POSIX branch receives start_new_session=True, no
creationflags, and no preexec_fn.

On Windows the helper supplies the full detach bundle; the narrow fallback (see the PR body) drops only
CREATE_BREAKAWAY_FROM_JOB on an error consistent with a breakaway refusal (WinError 5 plus confirmed
job membership). No preexec_fn is introduced on either platform (test_no_respawn_after_success /
test_windows_primary_full_detach_kwargs assert the exact kwargs).

[teknium1]

This only verifies AST structure. Add a mocked subprocess.Popen test that raises OSError on the primary call and asserts the retry receives windows_detach_flags_without_breakaway(); that exercises the claimed recovery contract.

[VerbalChainsaw]

Thanks — added. The new suite tests/tools/test_local_windows_breakaway_parity.py drives the real patched
_run_bash with subprocess.Popen mocked, exercising the recovery contract behaviorally rather than
by source inspection:

• test_windows_breakaway_refusal_retries_once — primary raises OSError(winerror=5) with the parent
  confirmed in a job; asserts exactly two Popen calls and that the second receives
  creationflags == windows_detach_flags_without_breakaway(), with argv/cwd/env/stdio/text/encoding/
  errors preserved across the retry.
• test_winerror_5_not_in_job_propagates and test_winerror_not_5_propagates — assert one attempt and
  propagation when the refusal conditions are not met.
• test_job_membership_query_failure_fails_closed — forces the real _process_in_job ctypes boundary
  to fail and asserts the original winerror 5 propagates with no retry (fail-closed).
• test_second_constructor_failure_propagates — asserts at most one retry.

Beyond the mocked contract, a native job-object experiment drove the exact patched call site and
demonstrated permitting-job escape (the child leaves the specific job, survives abrupt teardown while a
retained non-breakaway control dies, and stays explicitly killable) and restrictive-job retention
(the child stays in the job and dies on teardown). Evidence is in the package. No AST/source-text
assertions remain.

[VerbalChainsaw]

Heads up that I force-pushed a reworked version of this since the last review, so the diff and any "viewed" state will have reset. Sorry for the extra friction. Here is what actually changed.

The call site now routes through windows_detach_popen_kwargs() instead of the old windows_hide_flags() plus a literal start_new_session=True. On POSIX that means exactly one start_new_session=True and no duplicate keyword, which was your point on the earlier revision. Thanks for catching it.

The OSError fallback is much narrower now. It only retries without CREATE_BREAKAWAY_FROM_JOB when the error is winerror == 5 and an IsProcessInJob check confirms this process is in a job. Anything else propagates after a single attempt, it fails closed if the membership query fails, and a successful Popen is never respawned. The IsProcessInJob out parameter uses a correctly sized 4 byte BOOL buffer with explicit argtypes and restype.

The old AST only test is replaced by tests/tools/test_local_windows_breakaway_parity.py, which drives the real _run_bash with subprocess.Popen mocked and checks each branch of the fallback contract.

On scope, I want to be upfront: this is best effort process survival parity with POSIX, not durable execution. Where the parent job permits breakaway the child leaves the job and can outlive an abrupt parent death, but output and result ownership do not survive that, and on a restrictive job the child may be silently retained with no benefit at all. Controlled shutdown, timeout, interrupt, and explicit tree kill still terminate the child.

One correction to something said earlier in review: DETACHED_PROCESS does not break explicitly redirected stdio. It only drops an inherited console, so stdout=PIPE, merged stderr, and a redirected stdin pipe all keep working at this call site.

I ran a native job object experiment against the exact patched call site on one Windows build (Windows 11 10.0.28120, CPython 3.11.9). In a permitting job the child leaves the job and survives an abrupt teardown while a retained control process in the same job dies, and in a restrictive job the child stays in the job and dies on teardown. The fallback path itself is covered by the mocked tests rather than natively. I am happy to share the harness and the raw run output, or to test other job configurations if you have one in mind.

Thanks for the review.