[PR #3639] [CLOSED] fix(setup): preserve overridden runner variables and test isolation on Windows #5861

Closed
opened 2026-10-31 01:10:00 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3639
Author: @Swatantra-66
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/windows-runner-env-isolation


📄 Description

Summary

Fixes package runner identity resolution when ambient uppercase environment variables exist on Windows, isolates USERPROFILE in CLI setup tests, and hardens PTY JSON extraction against Windows ConPTY escape sequences.

Fixes #3635

Problem and expected behavior

  1. When callers construct child environments like { ...process.env, npm_command: "exec" } on Windows, npm's ambient NPM_COMMAND="run" precedes the lowercase entry in Object.keys(). environmentValue picked the first match, ignoring the override.
  2. In tests/boundary/cli/cliSetup.test.ts, runJourney only set HOME: isolatedHome, leaving USERPROFILE pointed at the host home directory.
  3. Windows ConPTY emits terminal restore sequences (\x1b[?25h) trailing stdout lines, breaking .split(/\r?\n/u, 1)[0] JSON parsing.

Change and scope

  • In src/application/ClientRegistrationIdentity.ts, updated environmentValue on Windows to use findLast() with case-normalized target comparison (candidate.toLowerCase() === target).
  • In src/setupEnvironment.test.ts, added unit regression coverage for Windows ambient case collisions.
  • In tests/boundary/cli/cliSetup.test.ts, set USERPROFILE: isolatedHome, bounded JSON parsing between { and }, sequenced multiselect prompt steps deterministically, and filtered ambient runner variables case-insensitively.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/application/ClientRegistrationIdentity.ts (invokingPackageRunner)

  • Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing contracts)

  • CLI and MCP/tool-catalog contract: none

  • Provider, bridge, target-format, or platform compatibility: Windows win32

  • Evidence, artifact, provenance, or reconstruction contract: none

  • Target selection, authority, upstream behavior, and cleanup details: none

  • Generated metadata: none

  • Read existing files or captured data.

  • Launch or attach to a process.

  • Inject instrumentation or run scripts/code.

  • Change process memory, registers, files, or configuration.

  • Contact network services.

  • Recover protected code or work around protections.

  • Documentation or usability only; no target interaction.

  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated:

    • src/setupEnvironment.test.ts ("prioritizes overridden package runner environment variables over ambient keys with differing case on Windows")
    • tests/boundary/cli/cliSetup.test.ts (14 tests passed on Windows ConPTY)
  • Base reproduction or other evidence: On Windows, invokingPackageRunner({ NPM_COMMAND: "run", npm_command: "exec", NPM_CONFIG_USER_AGENT: "npm/..." }, "win32") returned undefined.

  • Remaining proof gaps: none for Windows environment variable resolution and ConPTY journey parsing.

  • Observed, derived, and inferred claims remain distinguishable.

  • Artifact identity, source provenance, and failed attempts remain preserved.

  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npm run test:local -- src/setupEnvironment.test.ts — 7 tests passed
  • npm run test:focused -- tests/boundary/cli/cliSetup.test.ts — 14 tests passed
  • npm run check:fast — passed (turbo typecheck and lint)
  • npm run docs:check — passed (documentation and product catalog consistency)

Compatibility, safety, and release

  • Breaking changes or migration steps: none
  • Real Hopper/Ghidra, browser, or OS coverage: Windows 11 x64 verified
  • Package or release metadata impact: none
  • Security, privacy, process, or containment review: Ensures test processes do not leak into host USERPROFILE.

Author readiness

Human owner: @Swatantra-66

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3639 **Author:** [@Swatantra-66](https://github.com/Swatantra-66) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/windows-runner-env-isolation` --- ### 📄 Description ## Summary Fixes package runner identity resolution when ambient uppercase environment variables exist on Windows, isolates `USERPROFILE` in CLI setup tests, and hardens PTY JSON extraction against Windows ConPTY escape sequences. Fixes #3635 ## Problem and expected behavior 1. When callers construct child environments like `{ ...process.env, npm_command: "exec" }` on Windows, npm's ambient `NPM_COMMAND="run"` precedes the lowercase entry in `Object.keys()`. `environmentValue` picked the first match, ignoring the override. 2. In `tests/boundary/cli/cliSetup.test.ts`, `runJourney` only set `HOME: isolatedHome`, leaving `USERPROFILE` pointed at the host home directory. 3. Windows ConPTY emits terminal restore sequences (`\x1b[?25h`) trailing stdout lines, breaking `.split(/\r?\n/u, 1)[0]` JSON parsing. ## Change and scope - In `src/application/ClientRegistrationIdentity.ts`, updated `environmentValue` on Windows to use `findLast()` with case-normalized target comparison (`candidate.toLowerCase() === target`). - In `src/setupEnvironment.test.ts`, added unit regression coverage for Windows ambient case collisions. - In `tests/boundary/cli/cliSetup.test.ts`, set `USERPROFILE: isolatedHome`, bounded JSON parsing between `{` and `}`, sequenced multiselect prompt steps deterministically, and filtered ambient runner variables case-insensitively. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/application/ClientRegistrationIdentity.ts` (`invokingPackageRunner`) - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing contracts) - CLI and MCP/tool-catalog contract: none - Provider, bridge, target-format, or platform compatibility: Windows `win32` - Evidence, artifact, provenance, or reconstruction contract: none - Target selection, authority, upstream behavior, and cleanup details: none - Generated metadata: none - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [x] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: - `src/setupEnvironment.test.ts` ("prioritizes overridden package runner environment variables over ambient keys with differing case on Windows") - `tests/boundary/cli/cliSetup.test.ts` (14 tests passed on Windows ConPTY) - Base reproduction or other evidence: On Windows, `invokingPackageRunner({ NPM_COMMAND: "run", npm_command: "exec", NPM_CONFIG_USER_AGENT: "npm/..." }, "win32")` returned `undefined`. - Remaining proof gaps: none for Windows environment variable resolution and ConPTY journey parsing. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:local -- src/setupEnvironment.test.ts` — 7 tests passed - `npm run test:focused -- tests/boundary/cli/cliSetup.test.ts` — 14 tests passed - `npm run check:fast` — passed (turbo typecheck and lint) - `npm run docs:check` — passed (documentation and product catalog consistency) ## Compatibility, safety, and release - Breaking changes or migration steps: none - Real Hopper/Ghidra, browser, or OS coverage: Windows 11 x64 verified - Package or release metadata impact: none - Security, privacy, process, or containment review: Ensures test processes do not leak into host `USERPROFILE`. ## Author readiness Human owner: @Swatantra-66 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:10:00 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#5861
No description provided.