fix(tools): preserve canonical Windows location env var names in _build_safe_env

[TheSmokeDev]

What

Preserve canonical mixed-case Windows location variable names (ProgramFiles, ProgramData, ProgramW6432, and the CommonProgram* family) in the filtered environment passed to stdio MCP subprocesses.

Why

The earlier #49486 fix allowlisted these variables, but current main still emits each source key verbatim. CPython normalizes os.environ keys to uppercase on Windows, so launcher-style MCP servers receive PROGRAMFILES instead of the documented ProgramFiles spelling they query.

This follow-up maps only those known location keys back to canonical spelling, and only when os.name == "nt". Secret filtering and explicit user-env precedence are unchanged. POSIX environments retain the exact source spelling.

Review feedback addressed

• Canonical-name rewriting is now gated to Windows.
• The Windows test uses uppercase source keys and forces the Windows branch, so it is platform-independent.
• A forced-POSIX regression verifies PROGRAMFILES remains PROGRAMFILES rather than being rewritten.

Current-main A/B proof

Fresh verification after rebasing onto main at 9f384783e:

• Current main, existing Windows-location test: fails with KeyError: 'ProgramFiles'.
• This branch (fdbe3ca81), TestBuildSafeEnv: 9 passed.
• Full tests/tools/test_mcp_tool.py: 217 passed.
• Canonical per-file runner with the pending fix(tests): make run_tests.sh work on native Windows (#67385) #67387 Windows wrapper environment fix: 217 passed, 0 failed.
• ruff check tools/mcp_tool.py tests/tools/test_mcp_tool.py: clean.
• git diff --check: clean.

Scope

Two files: the runtime mapping and focused behavioral regression tests. No dependency, lockfile, or unrelated MCP changes.

Follow-up to #49486.

[TheSmokeDev]

Quick bump on this one. Still green against main (no conflicts), single file (tools/mcp_tool.py, +22/-1), and it's a real runtime fix rather than a test-only change. On Windows, CPython upper-cases every os.environ key, so _build_safe_env() was emitting PROGRAMFILES instead of the canonical ProgramFiles that launcher-style MCP servers (for example Docker Desktop's plugin discovery) look up by exact case. Happy to rebase or split it smaller if that helps review. Thanks for taking a look.

[TheSmokeDev]

Rebased onto current upstream/main (fbf748b) and re-ran the focused verification on native Windows via uv / CPython 3.13.3:

\\ ext
uv run --python 3.13 --extra dev --extra acp python -m pytest tests/tools/test_mcp_tool.py::TestBuildSafeEnv -q

6 passed in 7.44s

uv run --python 3.13 --extra dev --extra acp python -m pytest tests/tools/test_mcp_tool.py -q

200 passed in 22.49s

uv run --python 3.13 --extra dev ruff check tools/mcp_tool.py

All checks passed

\\

Still a single-file runtime fix in \ ools/mcp_tool.py\ (+22/-1), no scope change.

[TheSmokeDev]

Hey, I’m trying to help with native Windows compatibility. I refreshed this PR onto current main; it’s still a single-file P2 bug fix for MCP env vars, and the full \ ests/tools/test_mcp_tool.py\ file passes on native Windows. Happy to adjust shape if there’s a preferred reviewer path.

[teknium1]

Thanks for the focused Windows compatibility fix. Current main still copies the source key verbatim in tools/mcp_tool.py:436-442, while the existing regression contract expects canonical names at tests/tools/test_mcp_tool.py:1711-1713.

Problems

• The new lookup at tools/mcp_tool.py:403 runs on every platform. A POSIX environment with PROGRAMFILES would now be emitted as ProgramFiles, despite the PR stating non-Windows behavior is unchanged.

Suggested changes

• Gate the canonical-name mapping on os.name == "nt"; retain the original key elsewhere.
• Add a platform-independent test using uppercase source keys, rather than relying only on Windows os.environ normalization.

This is an automated hermes-sweeper review.

[teknium1]

Please gate this mapping on os.name == "nt". As written, a POSIX environment that intentionally contains PROGRAMFILES is rewritten to ProgramFiles, so non-Windows behavior is not preserved as the PR description states.

[TheSmokeDev]

Rebased onto current main (bcea5371c) — clean, no conflicts, still +22/-1 in tools/mcp_tool.py. Re-verified on native Windows: full tests/tools/test_mcp_tool.py → 212 passed. The bug is still live on main: _build_safe_env() passes keys through as-is, so on Windows (where CPython upper-cases os.environ keys) launcher-style MCP servers receive PROGRAMFILES instead of the canonical ProgramFiles they look up. This layers on the _SAFE_ENV_KEYS_CASE_INSENSITIVE allowlist that landed in dca11b665 — that solved inclusion, this solves casing.

[TheSmokeDev]

Addressed the sweeper review and rebased onto current main (9f384783e) as fdbe3ca81:

• canonical-name mapping now runs only when os.name == "nt"
• Windows behavior is tested from uppercase source keys independent of host platform
• a forced-POSIX regression proves source spelling is preserved
• current main still fails the existing contract with KeyError: 'ProgramFiles'
• focused class: 9 passed
• full tests/tools/test_mcp_tool.py: 217 passed
• canonical per-file runner with fix(tests): make run_tests.sh work on native Windows (#67385) #67387's pending Windows wrapper environment fix: 217 passed, 0 failed
• ruff and diff checks: clean

Fork CI still requires maintainer dispatch. The explicit review blocker is now closed; ready for workflow approval and review, or salvage/cherry-pick with authorship preserved.

[TheSmokeDev]

The 2026-07-14 review's two requested changes are in, plus a rebase and a test receipt worth highlighting.

Changes:

1. The canonical-name mapping is gated on os.name == "nt" — POSIX environments retain the source spelling verbatim.
2. Added a platform-independent test (test_posix_preserves_uppercase_windows_location_var_spelling) that patches os.name to posix with an uppercase source key and asserts no rewrite happens.
3. Rebased onto current main (branch was 2,503 commits behind).

The receipt: in a clean venv (pytest + mcp), current main fails its own regression contract — TestBuildSafeEnv::test_windows_location_vars_passed_without_secrets expects canonical ProgramFiles spellings that _build_safe_env never produces. This branch makes that test pass. Failure-set diff between this branch and main on the full file: zero failures unique to this branch; the one pre-existing TestBuildSafeEnv failure (test_secret_source_injected_vars_are_passed) fails identically on main (env-dependent, unrelated).

[TheSmokeDev]

@teknium1 friendly bump — the fixes from your sweep were addressed (replies above) and CI is green. Want me to rebase onto current main before your next pass?