Fix rex parent_environ case sensitivity on Windows#2098
Conversation
|
|
|
Closing+reopening to retrigger Read the Docs (initial build failed during clone, 2s duration, generic clone error — fork branch is public and accessible). |
On Windows env-var keys are case-insensitive natively. os.environ preserves that contract via os._Environ, but a plain dict copy of it (or any user-built dict) does not. Until now, ActionManager consumed whatever Mapping the caller passed as-is, so a package commands() that referenced env.SomeVariable against a parent_environ holding the same key under a different case raised RexUndefinedVariableError. Wrap the caller-supplied parent_environ in a small read-only case-insensitive proxy on the Windows path only. The proxy is module private; the wrapping is gated on platform_.name == "windows" and is idempotent so re-entering the gate is a no-op. Linux and macOS take the existing identity assignment unchanged. Also adds a Windows-only regression test that mirrors the issue reproducer and asserts both direct lookup and end-to-end rex code paths against a mixed-case parent_environ. Closes AcademySoftwareFoundation#2089 Reported by @nrusch. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
f3db9fb to
fa436d7
Compare
|
Status note for reviewers:
Local pre-push validation: |
maxnbk
left a comment
There was a problem hiding this comment.
Apologies for taking a long time to get to this review.
I've commented on a few specific items.
More generally, _CaseInsensitiveEnvironProxy could have a .copy() method added, which would be defensive against future code calling self.parent_environ.copy() to return a plain dict with uppercased keys.
ActionManager.__init__ docstring could document the Windows wrapping behavior.
And last, I feel it would be useful to consider a non-Windows identity passthrough test. i.e. assert ex.manager.parent_environ is env on non-Windows to guard against accidental wrapping. With that in mind, the test skip could be avoided and just assert differently for different host types, or be a different test skipping windows hosts, either way.
If you are using an LLM to produce this PR, please add a line to the PR description disclosing the usage, i.e. "Disclosure: used for (whichever things it was used for.)
549c434 to
1b6f0ce
Compare
- Add _CaseInsensitiveEnvironProxy.copy() returning a plain dict with upper-cased keys, for callers that expect a dict-like parent_environ. - Pass os.environ through unwrapped (it is already case-insensitive on Windows), preserving its identity and liveness. - Document the Windows wrapping behaviour in ActionManager.__init__ and clarify that the proxy is a construction-time snapshot, not a live view. - Drop the isinstance(key, str) guard in __contains__ so the proxy behaves like a normal mapping. - Tests: add a platform-independent unit test for the proxy, a non-Windows identity passthrough test, and switch the Windows regression test to @unittest.skipIf and the real env.<MixedCase> entrypoint via _test (execute_function + execute_code). Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The Windows fix normalizes lookups against a caller-supplied parent_environ only. ActionManager.environ (variables set within rex code) is stored verbatim and is not case-folded, which is a separate, pre-existing concern now tracked in AcademySoftwareFoundation#2164. Tighten the __init__ and proxy docstrings to say "lookups against parent_environ are case-insensitive" instead of the broader "match native Windows semantics", and note the boundary next to self.environ. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1b6f0ce to
c0b42f8
Compare
maxnbk
left a comment
There was a problem hiding this comment.
Tests failed, I believe it is because rex env is shadowing your variables. I am guessing something like this might fix it. GitHub UI is misbehaving so I wasn't able to apply it as a suggestion to click apply for:
"""Regression for #2089.
...
"""
- env = {"SomeVariable": "covfefe"}
- ex = self._create_executor(env)
+ parent_env = {"SomeVariable": "covfefe"}
+ ex = self._create_executor(parent_env)
# any casing of the key resolves
self.assertEqual(ex.manager.parent_environ["SomeVariable"], "covfefe")
...
# end-to-end via the real package-commands entrypoint
def _rex() -> None:
env.RESULT = env.SOMEVARIABLE # noqa: F821 - rex builtin
self._test(
func=_rex,
- env={"SomeVariable": "covfefe"},
+ env=parent_env,
expected_actions=[Setenv("RESULT", "covfefe")],
expected_output={"RESULT": "covfefe"},
)
(It may be wise to check the tests for any similar pattern and update those just on the off-chance someone introduces rex in any of those tests in the future and the same gotcha resurfaces too)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2098 +/- ##
==========================================
+ Coverage 60.03% 61.33% +1.29%
==========================================
Files 164 164
Lines 20557 20586 +29
Branches 3571 3578 +7
==========================================
+ Hits 12342 12627 +285
+ Misses 7347 7088 -259
- Partials 868 871 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The Windows regression test declared a local `env` dict, so the nested rex function closed over it (LOAD_DEREF) and shadowed the rex `env` builtin that execute_function injects into globals -- `env.SOMEVARIABLE` then hit a plain dict and failed on Windows CI. Rename the caller dict to `parent_env` in both new tests so the closure no longer shadows `env` (thanks @maxnbk for the diagnosis). Also address the Windows-only coverage gap codecov flagged: - test_parent_environ_os_environ_passthrough: covers the `parent_environ is os.environ` identity branch. - test_parent_environ_case_insensitive_windows_mocked: mocks platform_.name so the case-insensitive wrapping branch (and the full env.<MixedCase> end-to-end) is exercised on non-Windows CI too, giving the Windows path real coverage and off-Windows validation. Audited the rest of test_rex.py: no other test declares a local `env`; they all pass `env=` as a kwarg to _test, so none shadow the builtin. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
Thanks for the diagnosis — that was exactly it. Fixed in 1c3299e: renamed the caller dict to Per your future-proofing note, I audited the rest of I also closed the codecov gap on the Windows-only branch (it's skipped on the Linux/macOS jobs):
|
maxnbk
left a comment
There was a problem hiding this comment.
It LGTM now. Thank you for making the requested changes @thc1006 . I marked Approve (from me) and put this into "Next" milestone for merge in the next release. No further action needed unless some other reviewer requests changes.
| return self.interpreter.get_key_token(key) | ||
|
|
||
|
|
||
| class _CaseInsensitiveEnvironProxy(Mapping): |
There was a problem hiding this comment.
| class _CaseInsensitiveEnvironProxy(Mapping): | |
| class _CaseInsensitiveEnvironProxy(Mapping[str, str]): |
?
There was a problem hiding this comment.
tl;dr: happy to take the Mapping[str, str] change, but it should use typing.Mapping to stay 3.8-safe. Separately, the failing Ruff check looks like a pre-existing issue on main, not something in this PR.
On the base class: Mapping here is imported from collections.abc, and using one of those as a subscripted base (class X(Mapping[str, str])) only works on Python 3.9+. On 3.8 it raises TypeError at import, and we still support 3.8 (python_requires >= 3.8). The other parametrized bases in the repo, like AttrDictWrapper(MutableMapping[str, Any]) and PackageOrderList(List[...]), go through typing for that reason. I can switch this to from typing import Mapping and apply Mapping[str, str], or leave it bare, whichever you prefer.
On the Ruff failure: it's ISC004 at src/rez/utils/platform_.py:524 (the PowerShell command list), which this branch doesn't touch. It reproduces on a clean main (4946f9de) with ruff 0.16.0, the newest release matching >= 0.15.0 that ruff-action pulls in. So it reads as the new ISC004 rule catching an existing line rather than anything from here. I'm happy to fold the one-line fix into this PR if that unblocks things, otherwise it may fit better as its own change on main.
Thanks for the sphinx pass too, the :data:/:class: refs read a lot cleaner.
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com> Signed-off-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Fixes #2089
On Windows, environment variable keys are case-insensitive natively.
os.environpreserves that contract through Python'sos._Environwrapper, but a plaindictcopy of it (or any user-built dict) does not. Until now,ActionManagerconsumed whateverMappingthe caller passed as-is, so a package whosecommands()referencedenv.SomeVariableagainst aparent_environthat held the same variable under a different casing would raise:Issue #2089 has a clean reproducer that follows exactly this shape:
dict(os.environ)is passed toResolvedContext.get_environ, and a package looks upenv.SomeVariablewhile the dict only hasSOMEVARIABLE.The fix wraps a caller-supplied
parent_environin a small read-only, case-normalized snapshot (_CaseInsensitiveEnvironProxy), gated onplatform_.name == "windows".os.environandNoneare used as-is (they are already case-insensitive on Windows), so their identity and liveness are preserved. On Linux and macOS a caller-supplied mapping retains the existing identity passthrough. Keys are upper-cased once at construction, i.e. a point-in-time snapshot, which matches howparent_environis fixed for an executor's lifetime; iteration matches whatos.environyields on Windows, and when the input contains keys that differ only by case the last one encountered wins, mirroring howos.environcollapses duplicates. The proxy also exposes.copy()(returning a plain dict with upper-cased keys) for callers that expect a dict-likeparent_environ. Thenot isinstance(...)guard in the gate keeps wrapping idempotent.Tests:
test_case_insensitive_environ_proxyunit-tests the proxy directly and runs on every platform, so the core normalization logic has real CI coverage: get/contains across casings, missing-keyKeyError,.copy(), iteration/length, and last-write-wins collisions.test_parent_environ_case_insensitive_on_windows(Windows-only via@unittest.skipIf) drives the real package-commands entrypoint —env.<MixedCase>— through bothexecute_functionandexecute_codevia the_testhelper, alongside directparent_environ[...]lookups across casings.test_parent_environ_identity_passthrough_on_non_windows(non-Windows) assertsex.manager.parent_environ is envto guard against accidental wrapping.Verified locally on Linux:
python -m unittest rez.tests.test_rex→ 17 passed + 1 Windows-only skip. The fullrez-selftest,flake8, andmypymatrix runs in CI.Scope: this makes lookups against a caller-supplied
parent_environcase-insensitive on Windows — the exact #2089 reproducer. Variables set within rex code (ActionManager.environ) are stored verbatim and are not case-folded; that is a separate, pre-existing gap and intentionally out of scope here — tracked in #2164.Reported by @nrusch. Independent diagnosis previously posted in #2093 (closed because the author could not satisfy the EasyCLA/DCO requirement, not for technical reasons).
Disclosure: Claude Code (Opus 4.8) used for adversarial review, edge-case analysis, test design, and implementation of the review feedback.