fix: set packaged Windows runtime build env for pip native builds

[zouyonghe]

Summary

• normalize packaged Windows runtime paths before native-extension builds
• inject packaged runtime include/lib directories into the in-process pip environment on Windows desktop builds
• add a regression test covering packaged desktop runtime env injection

Verification

• uv run pytest tests/test_pip_installer.py

Summary by Sourcery

Adjust in-process pip installation to respect packaged Windows desktop runtimes and ensure native builds see the correct include/lib paths while keeping the process environment consistent.

Bug Fixes:

• Normalize Windows runtime and UNC paths before deriving native build include/lib directories for pip installations.
• Inject packaged Windows runtime include and lib directories into the in-process pip environment on Windows desktop builds only when those directories exist, without affecting non-Windows or non-packaged runs.
• Ensure temporary modification of INCLUDE and LIB during in-process pip runs is thread-safe and restores original environment values afterward.

Tests:

• Add comprehensive tests covering Windows path normalization, temporary environment handling, runtime build env construction, and pip in-process env injection behavior across packaged/non-packaged and Windows/non-Windows scenarios.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue where native Python extension builds fail within a packaged Windows runtime environment due to improperly configured INCLUDE and LIB environment variables. By introducing robust path normalization, dynamic identification of runtime directories, and a safe mechanism for environment variable injection, the changes ensure that pip can successfully compile native components. This enhancement significantly improves the reliability of installing Python packages with native dependencies in desktop applications.

Highlights

• Windows Path Normalization: Implemented a utility function _normalize_windows_native_build_path to correctly handle and normalize various Windows path formats, including UNC and device paths, ensuring compatibility for native builds.
• Packaged Runtime Environment Injection: Introduced _build_packaged_windows_runtime_build_env to identify and prepare the include and libs directories from the packaged Python runtime, which are then injected into the INCLUDE and LIB environment variables for native extension compilation.
• Temporary Environment Management: Added a _temporary_environ context manager to safely apply and revert environment variable modifications, ensuring that changes made for pip builds do not persist beyond the build process.
• Regression Test for Windows Builds: Included a new asynchronous test case test_run_pip_in_process_injects_windows_runtime_build_env to validate that the INCLUDE and LIB environment variables are correctly set and restored when pip executes within a packaged Windows runtime.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[sourcery-ai]

Hey - I've found 3 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_pip_installer.py" line_range="229-230" />
<code_context>
     ]


+@pytest.mark.asyncio
+async def test_run_pip_in_process_injects_windows_runtime_build_env(monkeypatch):
+    runtime_executable = (
+        r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe"
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for cases where INCLUDE/LIB are not set in the environment before injection

The current test only covers the case where INCLUDE/LIB already exist. To fully cover `_prepend_windows_env_path`, please add a test where INCLUDE/LIB are not set initially, and assert that:

- Inside `_run_pip_in_process`, INCLUDE/LIB are set only to the packaged paths (no extra semicolons).
- After the call, both variables are removed from `os.environ` again (i.e., `_temporary_environ` deletes keys that were not originally present).

Suggested implementation:

```python
    observed_env = {}

    def fake_pip_main(args):
        del args
        observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")

```

To cover the case where `INCLUDE`/`LIB` are not set before injection, please **add** the following new test (e.g. directly below `test_run_pip_in_process_injects_windows_runtime_build_env`):

```python
@pytest.mark.asyncio
async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
    monkeypatch,
):
    # Simulate the packaged runtime python (same pattern as the other test)
    runtime_executable = (
        r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe"
    )
    include_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include"
    libs_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs"

    # Make sure INCLUDE / LIB are *not* present before the call
    monkeypatch.delenv("INCLUDE", raising=False)
    monkeypatch.delenv("LIB", raising=False)

    observed_env: dict[str, str | None] = {}

    def fake_pip_main(args: list[str]) -> None:
        # Capture the environment as seen inside _run_pip_in_process
        del args
        observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
        observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")

    # Make _run_pip_in_process call our fake instead of the real pip main
    monkeypatch.setattr(pip_installer_module, "_pip_main", fake_pip_main, raising=True)

    # Run with a Windows runtime executable so that _prepend_windows_env_path is used
    await pip_installer_module._run_pip_in_process(
        ["install", "dummy-package"],
        python_executable=runtime_executable,
    )

    # Inside the pip process, INCLUDE / LIB should be set only to the packaged paths
    assert observed_env["INCLUDE"] == include_dir
    assert observed_env["LIB"] == libs_dir
    # No extra semicolons when there were no original values
    assert ";" not in observed_env["INCLUDE"]
    assert ";" not in observed_env["LIB"]

    # After the call, the temporary environment should be cleaned up:
    # keys that did not exist originally must be removed again.
    assert "INCLUDE" not in pip_installer_module.os.environ
    assert "LIB" not in pip_installer_module.os.environ
```

You may need to adjust the following details to match the existing test suite:

1. **Name of the internal pip entry point**:
   * If the existing test monkeypatches a different symbol than `_pip_main` (for example `pip_installer_module.pip_main` or `pip_installer_module._run_pip`), update the `monkeypatch.setattr(...)` call to match.

2. **Signature of `_run_pip_in_process`**:
   * If the current test calls `_run_pip_in_process` positionally (e.g. `await _run_pip_in_process(["install"], runtime_executable)`) instead of using `python_executable=...`, mirror that calling convention.

3. **Environment access**:
   * If the other tests use `os.environ` directly instead of `pip_installer_module.os.environ`, you can replace `pip_installer_module.os.environ` with `os.environ` in both the capturing and the final assertions to stay consistent.

Once wired to the correct symbols, this test will verify that:
- `INCLUDE`/`LIB` are set to *only* the packaged paths inside `_run_pip_in_process` when they were previously absent.
- `_temporary_environ` removes these keys from the environment after the call.
</issue_to_address>

### Comment 2
<location path="tests/test_pip_installer.py" line_range="234-235" />
<code_context>
+    runtime_executable = (
+        r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe"
+    )
+    include_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include"
+    libs_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs"
+    existing_include = r"C:\VS\include"
+    existing_lib = r"C:\VS\lib"
</code_context>
<issue_to_address>
**suggestion (testing):** Cover scenarios where only include or only libs directories exist

Right now `os.path.isdir` is mocked to return `True` for both include and libs, so we only cover the happy path where both are present. Please add tests (or parametrize this one) to also cover:

- Only include exists: INCLUDE updated, LIB unchanged.
- Only libs exists: LIB updated, INCLUDE unchanged.

This will verify `_build_packaged_windows_runtime_build_env` handles partially present runtimes correctly.

Suggested implementation:

```python
@pytest.mark.asyncio
@pytest.mark.parametrize(
    "include_exists, libs_exists",
    [
        (True, True),   # both include and libs exist
        (True, False),  # only include exists
        (False, True),  # only libs exist
    ],
)
async def test_run_pip_in_process_injects_windows_runtime_build_env(
    monkeypatch, include_exists, libs_exists
):
    runtime_executable = (
        r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe"
    )
    include_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include"
    libs_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs"
    existing_include = r"C:\VS\include"
    existing_lib = r"C:\VS\lib"
    observed_env = {}

    def fake_pip_main(args):
        del args
        observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
        observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
        return 0

    # Only report the runtime include / libs directories as existing according to the
    # current parametrized scenario; delegate all other paths to the real isdir.
    real_isdir = pip_installer_module.os.path.isdir

    def fake_isdir(path: str) -> bool:
        if path == include_dir:
            return include_exists
        if path == libs_dir:
            return libs_exists
        return real_isdir(path)

    monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
    monkeypatch.setenv("INCLUDE", existing_include)
    monkeypatch.setenv("LIB", existing_lib)

```

To fully implement the scenarios, adjust the remainder of this test (the part not shown in the snippet) as follows:

1. **Apply the monkeypatch for `os.path.isdir` and the fake pip main:**
   - After the `monkeypatch.setenv(...)` calls in this test, add:
   ```python
   monkeypatch.setattr(pip_installer_module.os.path, "isdir", fake_isdir)
   monkeypatch.setattr(pip_installer_module, "pip_main", fake_pip_main)
   ```
   (or whatever attribute name the test previously patched; preserve the existing naming convention and target.)

2. **Trigger the code under test as before:**
   - Keep the existing call that was invoking `_build_packaged_windows_runtime_build_env` indirectly (likely via `pip_installer_module.run_pip_in_process(...)`) so the behavior is tested exactly as before.

3. **Update the assertions to be scenario‑aware:**
   - After invoking the function under test, compute the expected values:
   ```python
   expected_include = existing_include
   expected_lib = existing_lib

   if include_exists:
       expected_include = existing_include + pip_installer_module.os.pathsep + include_dir
   if libs_exists:
       expected_lib = existing_lib + pip_installer_module.os.pathsep + libs_dir
   ```
   - Then assert:
   ```python
   assert observed_env["INCLUDE"] == expected_include
   assert observed_env["LIB"] == expected_lib
   ```
   This will validate:
   - when both exist: both INCLUDE and LIB are extended,
   - when only include exists: INCLUDE is extended, LIB unchanged,
   - when only libs exists: LIB is extended, INCLUDE unchanged.
</issue_to_address>

### Comment 3
<location path="tests/test_pip_installer.py" line_range="246-250" />
<code_context>
+        observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
+        return 0
+
+    monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
+    monkeypatch.setenv("INCLUDE", existing_include)
+    monkeypatch.setenv("LIB", existing_lib)
+    monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
+    monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
+    monkeypatch.setattr(
+        pip_installer_module.os.path,
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for non-packaged or non-Windows runtimes to ensure no env injection occurs

Specifically, consider tests where:
- `sys.platform != 'win32'` (e.g., `'linux'`) so `_run_pip_in_process` leaves `INCLUDE`/`LIB` untouched.
- `ASTRBOT_DESKTOP_CLIENT` is unset or set to a non-packaged value so no `INCLUDE`/`LIB` updates occur.

In those cases, assert that the env passed to `fake_pip_main` matches the original values (or remains absent).

Suggested implementation:

```python
    existing_include = r"C:\VS\include"
    existing_lib = r"C:\VS\lib"
    observed_env = {}

    def fake_pip_main(args):
        del args
        observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
        observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
        return 0

    monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
    monkeypatch.setenv("INCLUDE", existing_include)
    monkeypatch.setenv("LIB", existing_lib)
    monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
    monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
    monkeypatch.setattr(
        pip_installer_module.os.path,
        "isdir",
        lambda path: path in {include_dir, libs_dir},
    )
    monkeypatch.setattr(
        "astrbot.core.utils.pip_installer._get_pip_main",
        lambda: fake_pip_main,
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    # New tests: ensure no env injection on non-Windows and non-packaged runtimes.

    async def test_run_pip_in_process_does_not_modify_env_on_non_windows(
        monkeypatch, pip_installer_module, runtime_executable
    ):
        existing_include = "/usr/local/include"
        existing_lib = "/usr/local/lib"
        observed_env = {}

        def fake_pip_main(args):
            del args
            observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
            observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
            return 0

        # Even if the desktop flag is set, a non-Windows platform should prevent injection
        monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
        monkeypatch.setenv("INCLUDE", existing_include)
        monkeypatch.setenv("LIB", existing_lib)
        monkeypatch.setattr(pip_installer_module.sys, "platform", "linux")
        monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
        monkeypatch.setattr(
            "astrbot.core.utils.pip_installer._get_pip_main",
            lambda: fake_pip_main,
        )

        installer = PipInstaller("")
        await installer._run_pip_in_process(["install", "demo-package"])

        # INCLUDE/LIB should be untouched for non-Windows platforms
        assert observed_env["INCLUDE"] == existing_include
        assert observed_env["LIB"] == existing_lib

    async def test_run_pip_in_process_does_not_inject_env_when_not_packaged(
        monkeypatch, pip_installer_module, runtime_executable
    ):
        observed_env = {}

        def fake_pip_main(args):
            del args
            observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
            observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
            return 0

        # Simulate non-packaged runtime: no ASTRBOT_DESKTOP_CLIENT flag
        monkeypatch.delenv("ASTRBOT_DESKTOP_CLIENT", raising=False)
        monkeypatch.delenv("INCLUDE", raising=False)
        monkeypatch.delenv("LIB", raising=False)
        monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
        monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
        monkeypatch.setattr(
            "astrbot.core.utils.pip_installer._get_pip_main",
            lambda: fake_pip_main,
        )

        installer = PipInstaller("")
        await installer._run_pip_in_process(["install", "demo-package"])

        # For non-packaged runtimes, INCLUDE/LIB should not be injected at all
        assert observed_env["INCLUDE"] is None
        assert observed_env["LIB"] is None

```

The new tests I added assume:
1. `pip_installer_module` and `runtime_executable` are existing fixtures already used by the surrounding tests.
2. `PipInstaller` is imported at module level in `tests/test_pip_installer.py`.

If fixture or import names differ in your codebase, adjust the parameter names of the new tests and the imports accordingly. Also ensure the new async test functions are at module scope (no extra indentation) alongside the existing `_run_pip_in_process` tests.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Add tests for non-packaged or non-Windows runtimes to ensure no env injection occurs

Specifically, consider tests where:

• sys.platform != 'win32' (e.g., 'linux') so _run_pip_in_process leaves INCLUDE/LIB untouched.
• ASTRBOT_DESKTOP_CLIENT is unset or set to a non-packaged value so no INCLUDE/LIB updates occur.

In those cases, assert that the env passed to fake_pip_main matches the original values (or remains absent).

Suggested implementation:

    existing_include = r"C:\VS\include"
    existing_lib = r"C:\VS\lib"
    observed_env = {}

    def fake_pip_main(args):
        del args
        observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
        observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
        return 0

    monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
    monkeypatch.setenv("INCLUDE", existing_include)
    monkeypatch.setenv("LIB", existing_lib)
    monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
    monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
    monkeypatch.setattr(
        pip_installer_module.os.path,
        "isdir",
        lambda path: path in {include_dir, libs_dir},
    )
    monkeypatch.setattr(
        "astrbot.core.utils.pip_installer._get_pip_main",
        lambda: fake_pip_main,
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    # New tests: ensure no env injection on non-Windows and non-packaged runtimes.

    async def test_run_pip_in_process_does_not_modify_env_on_non_windows(
        monkeypatch, pip_installer_module, runtime_executable
    ):
        existing_include = "/usr/local/include"
        existing_lib = "/usr/local/lib"
        observed_env = {}

        def fake_pip_main(args):
            del args
            observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
            observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
            return 0

        # Even if the desktop flag is set, a non-Windows platform should prevent injection
        monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
        monkeypatch.setenv("INCLUDE", existing_include)
        monkeypatch.setenv("LIB", existing_lib)
        monkeypatch.setattr(pip_installer_module.sys, "platform", "linux")
        monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
        monkeypatch.setattr(
            "astrbot.core.utils.pip_installer._get_pip_main",
            lambda: fake_pip_main,
        )

        installer = PipInstaller("")
        await installer._run_pip_in_process(["install", "demo-package"])

        # INCLUDE/LIB should be untouched for non-Windows platforms
        assert observed_env["INCLUDE"] == existing_include
        assert observed_env["LIB"] == existing_lib

    async def test_run_pip_in_process_does_not_inject_env_when_not_packaged(
        monkeypatch, pip_installer_module, runtime_executable
    ):
        observed_env = {}

        def fake_pip_main(args):
            del args
            observed_env["INCLUDE"] = pip_installer_module.os.environ.get("INCLUDE")
            observed_env["LIB"] = pip_installer_module.os.environ.get("LIB")
            return 0

        # Simulate non-packaged runtime: no ASTRBOT_DESKTOP_CLIENT flag
        monkeypatch.delenv("ASTRBOT_DESKTOP_CLIENT", raising=False)
        monkeypatch.delenv("INCLUDE", raising=False)
        monkeypatch.delenv("LIB", raising=False)
        monkeypatch.setattr(pip_installer_module.sys, "platform", "win32")
        monkeypatch.setattr(pip_installer_module.sys, "executable", runtime_executable)
        monkeypatch.setattr(
            "astrbot.core.utils.pip_installer._get_pip_main",
            lambda: fake_pip_main,
        )

        installer = PipInstaller("")
        await installer._run_pip_in_process(["install", "demo-package"])

        # For non-packaged runtimes, INCLUDE/LIB should not be injected at all
        assert observed_env["INCLUDE"] is None
        assert observed_env["LIB"] is None

The new tests I added assume:

1. pip_installer_module and runtime_executable are existing fixtures already used by the surrounding tests.
2. PipInstaller is imported at module level in tests/test_pip_installer.py.

If fixture or import names differ in your codebase, adjust the parameter names of the new tests and the imports accordingly. Also ensure the new async test functions are at module scope (no extra indentation) alongside the existing _run_pip_in_process tests.

[gemini-code-assist]

Code Review

This pull request introduces a fix for setting up the build environment for pip native builds within a packaged Windows runtime. It adds helper functions to normalize Windows paths and to temporarily modify environment variables. These are then used in _run_pip_in_process to inject the INCLUDE and LIB directories from the packaged runtime into pip's environment.

The changes are accompanied by a solid regression test that verifies the environment variables are correctly set during the pip execution and restored afterward.

My main concern, detailed in a specific comment, is a potential race condition due to the modification of the global os.environ in an asynchronous context, especially if the critical section spans an await point. I've recommended using a lock to serialize access and prevent issues with concurrent pip installations.

[gemini-code-assist]

Modifying the global os.environ within an async function across an await point can lead to race conditions if _run_pip_in_process is called concurrently. One coroutine could overwrite the environment variables set by another, leading to incorrect build environments for native extensions.

To make this operation safe, you should serialize the execution of this critical section. I recommend adding an asyncio.Lock to the PipInstaller class and acquiring it before modifying the environment.

For example:

# In PipInstaller.__init__:
self._in_process_lock = asyncio.Lock()

# In _run_pip_in_process:
async def _run_pip_in_process(self, args: list[str]) -> int:
    async with self._in_process_lock:
        # ... the rest of the method's logic

This will ensure that concurrent calls do not interfere with each other's environment.

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The hard-coded absolute Windows paths in the tests (e.g., C:\Users\buding\...) make the intent a bit less clear; consider using more generic placeholder paths or constructing them via ntpath.join to better express that only the structure (not the specific user/profile) matters.
• The four new async tests around _run_pip_in_process share quite a bit of setup (fake pip_main, runtime_executable, monkeypatching sys and _get_pip_main); extracting a small helper or fixture for this common scaffolding would reduce duplication and make each test’s behavioral differences easier to see.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The hard-coded absolute Windows paths in the tests (e.g., `C:\Users\buding\...`) make the intent a bit less clear; consider using more generic placeholder paths or constructing them via `ntpath.join` to better express that only the structure (not the specific user/profile) matters.
- The four new async tests around `_run_pip_in_process` share quite a bit of setup (fake `pip_main`, `runtime_executable`, monkeypatching `sys` and `_get_pip_main`); extracting a small helper or fixture for this common scaffolding would reduce duplication and make each test’s behavioral differences easier to see.

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="285-294" />
<code_context>
+
+
+@contextlib.contextmanager
+def _temporary_environ(updates: dict[str, str]):
+    if not updates:
+        yield
+        return
+
+    missing = object()
+    previous_values = {key: os.environ.get(key, missing) for key in updates}
+
+    try:
+        os.environ.update(updates)
+        yield
+    finally:
+        for key, previous_value in previous_values.items():
+            if previous_value is missing:
+                os.environ.pop(key, None)
</code_context>
<issue_to_address>
**issue (bug_risk):** Process-wide os.environ mutation may impact other concurrent tasks

Because this context manager mutates `os.environ` while `_run_pip_main_streaming` runs in a background thread, other threads/async tasks may briefly see these INCLUDE/LIB changes. To avoid cross-task interference, consider running pip in a subprocess with an explicit `env`, or otherwise confine the environment changes strictly to the pip invocation.
</issue_to_address>

### Comment 2
<location path="tests/test_pip_installer.py" line_range="238-245" />
<code_context>
+        (False, True),
+    ],
+)
+async def test_run_pip_in_process_injects_windows_runtime_build_env(
+    monkeypatch, include_exists, libs_exists
+):
+    runtime_executable = (
+        r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\python.exe"
+    )
+    include_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include"
+    libs_dir = r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs"
+    existing_include = r"C:\VS\include"
+    existing_lib = r"C:\VS\lib"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for UNC and forward-slash paths to cover `_normalize_windows_native_build_path` behavior

These tests only cover the `\?\C:\...` executable style and the derived include/libs dirs. `_normalize_windows_native_build_path` also has special handling for `\?\UNC\...` / `\??\UNC\...` and forward slashes, which aren’t exercised. Please either parametrize with UNC and `/`-separator paths and assert INCLUDE/LIB are still normalized correctly, or add unit tests that call `_normalize_windows_native_build_path` directly with those cases.

Suggested implementation:

```python
@pytest.mark.parametrize(
    ("path", "expected"),
    [
        (
            r"\\?\C:\Users\buding\AppData\Local\AstrBot\backend\python\include",
            r"C:\Users\buding\AppData\Local\AstrBot\backend\python\include",
        ),
        (
            r"\\?\UNC\server\share\include",
            r"\\server\share\include",
        ),
        (
            r"\\??\UNC\server\share\libs",
            r"\\server\share\libs",
        ),
        (
            r"C:/Users/buding/AppData/Local/AstrBot/backend/python/libs",
            r"C:\Users\buding\AppData\Local\AstrBot\backend\python\libs",
        ),
    ],
)
def test_normalize_windows_native_build_path_variants(path, expected):
    assert (
        pip_installer_module._normalize_windows_native_build_path(path) == expected
    )


@pytest.mark.asyncio
@pytest.mark.parametrize(
    ("include_exists", "libs_exists"),

```

If `_normalize_windows_native_build_path`’s behavior differs from these expectations (e.g. it preserves certain prefixes or normalizes UNC paths differently), adjust the `expected` values to match its actual, intended normalization rules. Also ensure `pip_installer_module` is imported in this test file (it appears to be already, as it’s used in `test_run_pip_in_process_injects_windows_runtime_build_env`).
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_pip_installer.py" line_range="87-89" />
<code_context>
+    return observed_env
+
+
 @pytest.mark.asyncio
 async def test_install_targets_site_packages_for_desktop_client(monkeypatch, tmp_path):
     monkeypatch.setenv("ASTRBOT_DESKTOP_CLIENT", "1")
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the case where neither include nor libs directories exist in the packaged runtime.

The parametrized `test_run_pip_in_process_injects_windows_runtime_build_env` only exercises cases where at least one of `WINDOWS_RUNTIME_INCLUDE_DIR` or `WINDOWS_RUNTIME_LIBS_DIR` exists. We should also assert behavior when `_build_packaged_windows_runtime_build_env()` returns `{}` so we know the build env is not mutated and existing `INCLUDE`/`LIB` values are preserved.

For example:

```python
@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_inject_when_runtime_dirs_missing(monkeypatch):
    observed_env = _configure_run_pip_in_process_capture(
        monkeypatch,
        platform="win32",
        packaged_runtime=True,
        include_value=EXISTING_WINDOWS_INCLUDE_DIR,
        lib_value=EXISTING_WINDOWS_LIB_DIR,
        existing_runtime_dirs=set(),
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0
    assert observed_env == {
        "INCLUDE": EXISTING_WINDOWS_INCLUDE_DIR,
        "LIB": EXISTING_WINDOWS_LIB_DIR,
    }
    assert pip_installer_module.os.environ["INCLUDE"] == EXISTING_WINDOWS_INCLUDE_DIR
    assert pip_installer_module.os.environ["LIB"] == EXISTING_WINDOWS_LIB_DIR
```

This ensures the temporary env injection is a no-op when the runtime provides no build directories.

Suggested implementation:

```python
    monkeypatch.setattr(
        "astrbot.core.utils.pip_installer._get_pip_main",
        lambda: fake_pip_main,
    )
    return observed_env


@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_inject_when_runtime_dirs_missing(monkeypatch):
    observed_env = _configure_run_pip_in_process_capture(
        monkeypatch,
        platform="win32",
        packaged_runtime=True,
        include_value=EXISTING_WINDOWS_INCLUDE_DIR,
        lib_value=EXISTING_WINDOWS_LIB_DIR,
        existing_runtime_dirs=set(),
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0
    assert observed_env == {
        "INCLUDE": EXISTING_WINDOWS_INCLUDE_DIR,
        "LIB": EXISTING_WINDOWS_LIB_DIR,
    }
    assert pip_installer_module.os.environ["INCLUDE"] == EXISTING_WINDOWS_INCLUDE_DIR
    assert pip_installer_module.os.environ["LIB"] == EXISTING_WINDOWS_LIB_DIR


    else:

```

1. Ensure `EXISTING_WINDOWS_INCLUDE_DIR` and `EXISTING_WINDOWS_LIB_DIR` are defined constants in this test module (they are likely already used by `test_run_pip_in_process_injects_windows_runtime_build_env`; if not, define them near the top of the file).
2. Ensure `pip_installer_module` and `PipInstaller` are already imported/available in this file as used in the other tests.
3. If the `else:` shown in the snippet is actually part of another function or fixture, please confirm that this new test is placed outside of that function’s body (i.e., at top-level indentation) to remain consistent with the rest of the test layout.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've left some high level feedback:

• There is a subtle race in how build_env is constructed: _build_packaged_windows_runtime_build_env reads os.environ outside _PIP_IN_PROCESS_ENV_LOCK, so another thread could modify INCLUDE/LIB between computing build_env and calling _run_pip_main_with_temporary_environ; consider either building build_env inside the locked section or basing the prepend logic on the environment snapshot within _temporary_environ to keep the mutation atomic.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- There is a subtle race in how `build_env` is constructed: `_build_packaged_windows_runtime_build_env` reads `os.environ` outside `_PIP_IN_PROCESS_ENV_LOCK`, so another thread could modify `INCLUDE`/`LIB` between computing `build_env` and calling `_run_pip_main_with_temporary_environ`; consider either building `build_env` inside the locked section or basing the prepend logic on the environment snapshot within `_temporary_environ` to keep the mutation atomic.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_pip_installer.py" line_range="289-298" />
<code_context>
     ]


+@pytest.mark.parametrize(
+    ("path", "expected"),
+    [
+        (
+            WINDOWS_PACKAGED_RUNTIME_EXECUTABLE,
+            WINDOWS_RUNTIME_EXECUTABLE,
+        ),
+        (
+            r"\\?\UNC\server\share\include",
+            r"\\server\share\include",
+        ),
+        (
+            r"\??\UNC\server\share\libs",
+            r"\\server\share\libs",
+        ),
+        (
+            "C:/astrbot-test/backend/python/libs",
+            WINDOWS_RUNTIME_LIBS_DIR,
+        ),
+    ],
+)
+def test_normalize_windows_native_build_path_variants(path, expected):
+    assert pip_installer_module._normalize_windows_native_build_path(path) == expected
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding normalization tests for already-normalized and `\\?\C:\...` style non-UNC paths.

These tests already cover UNC and forward‑slash paths. To better exercise `_normalize_windows_native_build_path`, please also add:

- An input that is already normalized (e.g. `WINDOWS_RUNTIME_EXECUTABLE`) to verify it’s returned unchanged.
- A non‑UNC extended path like `\\?\C:\...` or `\??\C:\...` to cover the branch that strips extended‑length prefixes without going through the UNC logic.

This will ensure both prefix‑handling branches and the no-op path are properly tested and guard against regressions in Windows extended path handling.
</issue_to_address>

### Comment 2
<location path="tests/test_pip_installer.py" line_range="358-383" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
+    monkeypatch,
+):
+    observed_env = _configure_run_pip_in_process_capture(
+        monkeypatch,
+        platform="win32",
+        packaged_runtime=True,
+        existing_runtime_dirs={
+            WINDOWS_RUNTIME_INCLUDE_DIR,
+            WINDOWS_RUNTIME_LIBS_DIR,
+        },
+    )
+
+    installer = PipInstaller("")
+    result = await installer._run_pip_in_process(["install", "demo-package"])
+
+    assert result == 0
+    assert observed_env == {
+        "INCLUDE": WINDOWS_RUNTIME_INCLUDE_DIR,
+        "LIB": WINDOWS_RUNTIME_LIBS_DIR,
+    }
+    assert ";" not in observed_env["INCLUDE"]
+    assert ";" not in observed_env["LIB"]
+    assert "INCLUDE" not in pip_installer_module.os.environ
+    assert "LIB" not in pip_installer_module.os.environ
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test where only one of the runtime include/lib directories exists and no existing INCLUDE/LIB env vars are set.

The existing tests cover: (1) both runtime dirs present with no pre-existing INCLUDE/LIB, (2) combinations of `include_exists`/`libs_exists` when INCLUDE/LIB are already set, and (3) the case where both runtime dirs are missing. What’s missing is when only one of `include`/`libs` exists and both env vars are unset. Extending this test parametrically (or adding a small new one) to assert that only the existing dir is injected and that no trailing `;` is added would close this gap and better guard against subtle string-formatting issues in `_build_packaged_windows_runtime_build_env`.

```suggestion
@pytest.mark.asyncio
@pytest.mark.parametrize(
    "include_exists, libs_exists",
    [
        (True, True),   # both runtime dirs present
        (True, False),  # only include dir present
        (False, True),  # only libs dir present
    ],
)
async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
    monkeypatch,
    include_exists,
    libs_exists,
):
    existing_runtime_dirs = set()
    if include_exists:
        existing_runtime_dirs.add(WINDOWS_RUNTIME_INCLUDE_DIR)
    if libs_exists:
        existing_runtime_dirs.add(WINDOWS_RUNTIME_LIBS_DIR)

    observed_env = _configure_run_pip_in_process_capture(
        monkeypatch,
        platform="win32",
        packaged_runtime=True,
        existing_runtime_dirs=existing_runtime_dirs,
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0

    expected_env = {}
    if include_exists:
        expected_env["INCLUDE"] = WINDOWS_RUNTIME_INCLUDE_DIR
    if libs_exists:
        expected_env["LIB"] = WINDOWS_RUNTIME_LIBS_DIR

    assert observed_env == expected_env

    if include_exists:
        assert ";" not in observed_env["INCLUDE"]
    if libs_exists:
        assert ";" not in observed_env["LIB"]

    # Ensure process env is not polluted
    assert "INCLUDE" not in pip_installer_module.os.environ
    assert "LIB" not in pip_installer_module.os.environ
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Consider adding normalization tests for already-normalized and \\?\C:\... style non-UNC paths.

These tests already cover UNC and forward‑slash paths. To better exercise _normalize_windows_native_build_path, please also add:

• An input that is already normalized (e.g. WINDOWS_RUNTIME_EXECUTABLE) to verify it’s returned unchanged.
• A non‑UNC extended path like \\?\C:\... or \??\C:\... to cover the branch that strips extended‑length prefixes without going through the UNC logic.

This will ensure both prefix‑handling branches and the no-op path are properly tested and guard against regressions in Windows extended path handling.

[sourcery-ai]

suggestion (testing): Consider a test where only one of the runtime include/lib directories exists and no existing INCLUDE/LIB env vars are set.

The existing tests cover: (1) both runtime dirs present with no pre-existing INCLUDE/LIB, (2) combinations of include_exists/libs_exists when INCLUDE/LIB are already set, and (3) the case where both runtime dirs are missing. What’s missing is when only one of include/libs exists and both env vars are unset. Extending this test parametrically (or adding a small new one) to assert that only the existing dir is injected and that no trailing ; is added would close this gap and better guard against subtle string-formatting issues in _build_packaged_windows_runtime_build_env.

Suggested change

	@pytest.mark.asyncio
	async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
	monkeypatch,
	):
	observed_env = _configure_run_pip_in_process_capture(
	monkeypatch,
	platform="win32",
	packaged_runtime=True,
	existing_runtime_dirs={
	WINDOWS_RUNTIME_INCLUDE_DIR,
	WINDOWS_RUNTIME_LIBS_DIR,
	},
	)
	installer = PipInstaller("")
	result = await installer._run_pip_in_process(["install", "demo-package"])
	assert result == 0
	assert observed_env == {
	"INCLUDE": WINDOWS_RUNTIME_INCLUDE_DIR,
	"LIB": WINDOWS_RUNTIME_LIBS_DIR,
	}
	assert ";" not in observed_env["INCLUDE"]
	assert ";" not in observed_env["LIB"]
	assert "INCLUDE" not in pip_installer_module.os.environ
	assert "LIB" not in pip_installer_module.os.environ
	@pytest.mark.asyncio
	@pytest.mark.parametrize(
	"include_exists, libs_exists",
	[
	(True, True), # both runtime dirs present
	(True, False), # only include dir present
	(False, True), # only libs dir present
	],
	)
	async def test_run_pip_in_process_injects_windows_runtime_build_env_without_existing_paths(
	monkeypatch,
	include_exists,
	libs_exists,
	):
	existing_runtime_dirs = set()
	if include_exists:
	existing_runtime_dirs.add(WINDOWS_RUNTIME_INCLUDE_DIR)
	if libs_exists:
	existing_runtime_dirs.add(WINDOWS_RUNTIME_LIBS_DIR)
	observed_env = _configure_run_pip_in_process_capture(
	monkeypatch,
	platform="win32",
	packaged_runtime=True,
	existing_runtime_dirs=existing_runtime_dirs,
	)
	installer = PipInstaller("")
	result = await installer._run_pip_in_process(["install", "demo-package"])
	assert result == 0
	expected_env = {}
	if include_exists:
	expected_env["INCLUDE"] = WINDOWS_RUNTIME_INCLUDE_DIR
	if libs_exists:
	expected_env["LIB"] = WINDOWS_RUNTIME_LIBS_DIR
	assert observed_env == expected_env
	if include_exists:
	assert ";" not in observed_env["INCLUDE"]
	if libs_exists:
	assert ";" not in observed_env["LIB"]
	# Ensure process env is not polluted
	assert "INCLUDE" not in pip_installer_module.os.environ
	assert "LIB" not in pip_installer_module.os.environ

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've left some high level feedback:

• The _build_packaged_windows_runtime_build_env helper currently reads from os.environ via _prepend_windows_env_path, which makes it tightly coupled to process state; consider passing in a base env mapping (e.g. Mapping[str, str]) so it can build upon a snapshot and be more reusable/testable outside of process-global mutations.
• The Windows path normalization logic in _normalize_windows_native_build_path handles several prefix variants inline; extracting the prefix sets (e.g. extended-path vs UNC) into named constants or a small lookup structure would make the intent clearer and reduce the chance of inconsistencies if more cases are added later.
• The _configure_run_pip_in_process_capture test helper is fairly involved and reused across multiple tests; you might consider turning parts of it into pytest fixtures (e.g. a fixture that sets up a packaged Windows runtime env) to make individual test cases more focused on behavior rather than setup details.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `_build_packaged_windows_runtime_build_env` helper currently reads from `os.environ` via `_prepend_windows_env_path`, which makes it tightly coupled to process state; consider passing in a base env mapping (e.g. `Mapping[str, str]`) so it can build upon a snapshot and be more reusable/testable outside of process-global mutations.
- The Windows path normalization logic in `_normalize_windows_native_build_path` handles several prefix variants inline; extracting the prefix sets (e.g. extended-path vs UNC) into named constants or a small lookup structure would make the intent clearer and reduce the chance of inconsistencies if more cases are added later.
- The `_configure_run_pip_in_process_capture` test helper is fairly involved and reused across multiple tests; you might consider turning parts of it into pytest fixtures (e.g. a fixture that sets up a packaged Windows runtime env) to make individual test cases more focused on behavior rather than setup details.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="254-255" />
<code_context>
+    # the worker.
+    with _PIP_IN_PROCESS_ENV_LOCK:
+        if env_updates is None:
+            env_updates = _build_packaged_windows_runtime_build_env(
+                base_env=dict(os.environ)
+            )
+        with _temporary_environ(env_updates):
</code_context>
<issue_to_address>
**issue (bug_risk):** Using dict(os.environ) loses Windows case-insensitive behavior for env vars and may skip existing INCLUDE/LIB entries.

On Windows, `os.environ` is case-insensitive, but `dict(os.environ)` is not. If the environment uses a different casing (e.g., `include` instead of `INCLUDE`), `_prepend_windows_env_path` will miss the existing value and overwrite rather than prepend. Please pass `os.environ` directly (or use a copy that preserves its mapping type) and only copy if you need to mutate it independently.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_pip_installer.py" line_range="316-323" />
<code_context>
+        ),
+    ],
+)
+def test_normalize_windows_native_build_path_variants(path, expected):
+    assert pip_installer_module._normalize_windows_native_build_path(path) == expected
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a case for already-normal UNC paths in `_normalize_windows_native_build_path`

Since this test already covers extended prefixes and forward-slash normalization, consider adding a case with an already-normal UNC path (e.g. `r"\\server\share\include"`) to verify `_normalize_windows_native_build_path` is a no-op in that scenario and to guard against future double-normalization or incorrect prefixing.

```suggestion
        (
            r"\??\UNC\server\share\libs",
            r"\\server\share\libs",
        ),
        # Already-normal UNC path should be a no-op
        (
            r"\\server\share\include",
            r"\\server\share\include",
        ),
        (
            "C:/astrbot-test/backend/python/libs",
            WINDOWS_RUNTIME_LIBS_DIR,
        ),
```
</issue_to_address>

### Comment 2
<location path="astrbot/core/utils/pip_installer.py" line_range="243" />
<code_context>
     return result_code, stream.lines


+def _run_pip_main_with_temporary_environ(
+    pip_main,
+    args: list[str],
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the new environment-setup helpers into the main functions to keep all pip in-process env mutation and Windows build-path logic localized and easier to follow.

The added helpers around the pip env setup can be collapsed without changing behavior, which should make the flow easier to follow.

You can keep all current semantics but:

1. Inline the case-insensitive env lookup + prepend logic into `_build_packaged_windows_runtime_build_env`, and remove `_get_windows_env_value` / `_prepend_windows_env_path`.

```python
def _build_packaged_windows_runtime_build_env(
    *, base_env: Mapping[str, str] | None = None,
) -> dict[str, str]:
    if sys.platform != "win32" or not is_packaged_desktop_runtime():
        return {}

    if base_env is None:
        base_env = os.environ

    runtime_executable = _normalize_windows_native_build_path(sys.executable)
    runtime_dir = ntpath.dirname(runtime_executable)
    if not runtime_dir:
        return {}

    include_dir = _normalize_windows_native_build_path(
        ntpath.join(runtime_dir, "include")
    )
    libs_dir = _normalize_windows_native_build_path(
        ntpath.join(runtime_dir, "libs")
    )

    if not (os.path.isdir(include_dir) or os.path.isdir(libs_dir)):
        return {}

    # Case-insensitive view of the existing env
    upper_to_key: dict[str, str] = {k.upper(): k for k in base_env.keys()}

    def _prepend(name: str, path: str) -> str:
        existing_key = upper_to_key.get(name)
        existing = base_env.get(existing_key) if existing_key is not None else None
        return f"{path};{existing}" if existing else path

    env_updates: dict[str, str] = {}
    if os.path.isdir(include_dir):
        env_updates["INCLUDE"] = _prepend("INCLUDE", include_dir)
    if os.path.isdir(libs_dir):
        env_updates["LIB"] = _prepend("LIB", libs_dir)
    return env_updates
```

This removes two global helpers and concentrates the Windows build-path logic in one place.

2. Inline `_temporary_environ` into `_run_pip_main_with_temporary_environ` so that the lock, env mutation, and restore logic live in a single function. This keeps the behavior but reduces indirection and makes the mutation window explicit:

```python
def _run_pip_main_with_temporary_environ(
    pip_main,
    args: list[str],
    env_updates: dict[str, str] | None = None,
) -> tuple[int, list[str]]:
    with _PIP_IN_PROCESS_ENV_LOCK:
        if env_updates is None:
            env_updates = _build_packaged_windows_runtime_build_env(base_env=os.environ)

        if not env_updates:
            return _run_pip_main_streaming(pip_main, args)

        missing = object()
        previous_values = {k: os.environ.get(k, missing) for k in env_updates}

        try:
            os.environ.update(env_updates)
            return _run_pip_main_streaming(pip_main, args)
        finally:
            for key, previous in previous_values.items():
                if previous is missing:
                    os.environ.pop(key, None)
                else:
                    os.environ[key] = previous
```

With this, `_temporary_environ` can be removed entirely, and the only remaining “pip in-process + env” entry point is `_run_pip_main_with_temporary_environ`.

The `_normalize_windows_native_build_path` function is already self-contained and reasonably clear. Unless you have concrete scenarios requiring the separate UNC vs extended-prefix handling, you might later consider simplifying the prefix handling, but the main complexity win comes from reducing the number of small helpers around the env setup as above.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Consider adding a case for already-normal UNC paths in _normalize_windows_native_build_path

Since this test already covers extended prefixes and forward-slash normalization, consider adding a case with an already-normal UNC path (e.g. r"\\server\share\include") to verify _normalize_windows_native_build_path is a no-op in that scenario and to guard against future double-normalization or incorrect prefixing.

Suggested change

	(
	r"\??\UNC\server\share\libs",
	r"\\server\share\libs",
	),
	(
	"C:/astrbot-test/backend/python/libs",
	WINDOWS_RUNTIME_LIBS_DIR,
	),
	(
	r"\??\UNC\server\share\libs",
	r"\\server\share\libs",
	),
	# Already-normal UNC path should be a no-op
	(
	r"\\server\share\include",
	r"\\server\share\include",
	),
	(
	"C:/astrbot-test/backend/python/libs",
	WINDOWS_RUNTIME_LIBS_DIR,
	),

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/utils/pip_installer.py" line_range="243" />
<code_context>
     return result_code, stream.lines


+def _run_pip_main_with_temporary_environ(
+    pip_main,
+    args: list[str],
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a reusable context manager for environment mutation, simplifying the env-building helpers, and documenting path normalization to keep the new behavior while reducing surface-area complexity.

You can reduce the added complexity by (1) encapsulating the env mutation in a small context manager and (2) simplifying the env‑building helpers. This keeps behavior but shrinks surface area.

### 1. Inline env computation and use a context manager

`_run_pip_main_with_temporary_environ` doesn’t need an `env_updates` parameter yet, and the save/restore loop can be pushed into a reusable helper:

```python
@contextlib.contextmanager
def _temporary_environ(updates: Mapping[str, str]):
    # process-wide: serialize env mutations
    with _PIP_IN_PROCESS_ENV_LOCK:
        if not updates:
            # no-op context
            yield
            return

        missing = object()
        previous_values = {k: os.environ.get(k, missing) for k in updates}
        try:
            os.environ.update(updates)
            yield
        finally:
            for key, previous_value in previous_values.items():
                if previous_value is missing:
                    os.environ.pop(key, None)
                else:
                    os.environ[key] = previous_value


def _run_pip_main_with_temporary_environ(
    pip_main,
    args: list[str],
) -> tuple[int, list[str]]:
    env_updates = _build_packaged_windows_runtime_build_env(base_env=os.environ)
    # Fast path: nothing to do
    if not env_updates:
        return _run_pip_main_streaming(pip_main, args)

    with _temporary_environ(env_updates):
        return _run_pip_main_streaming(pip_main, args)
```

This keeps the lock, preserves the save/restore semantics, but makes the call site trivial and removes the unused `env_updates` abstraction.

### 2. Simplify env building by extracting a case‑insensitive helper

You can keep the `upper_to_key` optimization but move the lookup into a focused helper. That flattens `_build_packaged_windows_runtime_build_env` and makes the intent explicit:

```python
def _get_case_insensitive(env: Mapping[str, str], upper_map: dict[str, str], name: str) -> str | None:
    direct = env.get(name)
    if direct is not None:
        return direct
    existing_key = upper_map.get(name.upper())
    if existing_key is not None:
        return env.get(existing_key)
    return None


def _build_packaged_windows_runtime_build_env(
    *,
    base_env: Mapping[str, str] | None = None,
) -> dict[str, str]:
    if sys.platform != "win32" or not is_packaged_desktop_runtime():
        return {}

    base_env = os.environ if base_env is None else base_env

    runtime_executable = _normalize_windows_native_build_path(sys.executable)
    runtime_dir = ntpath.dirname(runtime_executable)
    if not runtime_dir:
        return {}

    include_dir = _normalize_windows_native_build_path(ntpath.join(runtime_dir, "include"))
    libs_dir = _normalize_windows_native_build_path(ntpath.join(runtime_dir, "libs"))
    include_exists = os.path.isdir(include_dir)
    libs_exists = os.path.isdir(libs_dir)
    if not (include_exists or libs_exists):
        return {}

    upper_to_key = {key.upper(): key for key in base_env}
    env_updates: dict[str, str] = {}

    if include_exists:
        existing = _get_case_insensitive(base_env, upper_to_key, "INCLUDE")
        env_updates["INCLUDE"] = f"{include_dir};{existing}" if existing else include_dir

    if libs_exists:
        existing = _get_case_insensitive(base_env, upper_to_key, "LIB")
        env_updates["LIB"] = f"{libs_dir};{existing}" if existing else libs_dir

    return env_updates
```

This removes the inline `_prepend` closure and the multi‑step lookup, while keeping the case‑insensitive behavior and early‑return conditions intact.

### 3. Make `_normalize_windows_native_build_path` self‑documenting

A minimal docstring and comments make the mixed prefix/normalization logic easier to follow without changing behavior:

```python
def _normalize_windows_native_build_path(path: str) -> str:
    """Normalize a Windows path coming from native APIs or sys.executable.

    Handles:
    - UNC extended prefixes (\\?\UNC\, \??\UNC\) -> standard UNC form (\\server\share\...)
    - Other extended prefixes (\\?\ , \??\ ) by stripping the prefix
    - Then normalizes separators and dot components.
    """
    normalized = path.replace("/", "\\")

    # Extended UNC: \\?\UNC\server\share\... -> \\server\share\...
    for prefix in _WINDOWS_UNC_PATH_PREFIXES:
        if normalized.startswith(prefix):
            return ntpath.normpath(f"\\\\{normalized[len(prefix):]}")

    # Other extended prefixes: strip and then normalize
    for prefix in _WINDOWS_EXTENDED_PATH_PREFIXES:
        if normalized.startswith(prefix):
            normalized = normalized[len(prefix):]
            break

    return ntpath.normpath(normalized)
```

These changes keep all existing behavior but reduce branching, remove an unused parameter, and localize the “tricky” parts into small, well‑named helpers.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[SourceryAI]

Hey - I've reviewed your changes and they look great!

---

Hi @zouyonghe! 👋

Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀

Install the sourcery-ai bot to get automatic code reviews on every pull request ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_pip_installer.py" line_range="503-459" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_run_pip_in_process_does_not_inject_when_runtime_dirs_missing(
+    configure_run_pip_in_process_capture,
+):
+    observed_env = configure_run_pip_in_process_capture(
+        platform="win32",
+        packaged_runtime=True,
+        include_value=EXISTING_WINDOWS_INCLUDE_DIR,
+        lib_value=EXISTING_WINDOWS_LIB_DIR,
+        existing_runtime_dirs=set(),
+    )
+
+    installer = PipInstaller("")
+    result = await installer._run_pip_in_process(["install", "demo-package"])
+
+    assert result == 0
+    assert observed_env == {
+        "INCLUDE": EXISTING_WINDOWS_INCLUDE_DIR,
+        "LIB": EXISTING_WINDOWS_LIB_DIR,
+    }
+    assert pip_installer_module.os.environ["INCLUDE"] == EXISTING_WINDOWS_INCLUDE_DIR
+    assert pip_installer_module.os.environ["LIB"] == EXISTING_WINDOWS_LIB_DIR
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a variant where INCLUDE/LIB are initially unset and runtime dirs are missing

This already covers the case where runtime dirs are missing but existing `INCLUDE`/`LIB` values are present. Please also add a variant where `INCLUDE` and `LIB` are absent from `os.environ` and `existing_runtime_dirs` is an empty set, and assert that the in-process pip run does not create these keys. That will exercise the "unset env + missing runtime dirs" scenario.

Suggested implementation:

```python
WINDOWS_RUNTIME_LIBS_DIR = ntpath.join(WINDOWS_RUNTIME_ROOT, "libs")
EXISTING_WINDOWS_INCLUDE_DIR = ntpath.join(r"C:\toolchain", "include")
EXISTING_WINDOWS_LIB_DIR = ntpath.join(r"C:\toolchain", "lib")


@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_create_env_when_unset_and_runtime_dirs_missing(
    configure_run_pip_in_process_capture,
):
    observed_env = configure_run_pip_in_process_capture(
        platform="win32",
        packaged_runtime=True,
        include_value=None,
        lib_value=None,
        existing_runtime_dirs=set(),
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0
    assert "INCLUDE" not in observed_env
    assert "LIB" not in observed_env
    assert "INCLUDE" not in pip_installer_module.os.environ
    assert "LIB" not in pip_installer_module.os.environ

```

This assumes `configure_run_pip_in_process_capture` treats `include_value=None` and `lib_value=None` as “do not set these env vars initially”. If in your fixture the absence is represented differently (e.g. by omitting the arguments or using a sentinel), adjust the call accordingly. Also, this test relies on `pip_installer_module` already being imported/available as used in the existing tests.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Add a variant where INCLUDE/LIB are initially unset and runtime dirs are missing

This already covers the case where runtime dirs are missing but existing INCLUDE/LIB values are present. Please also add a variant where INCLUDE and LIB are absent from os.environ and existing_runtime_dirs is an empty set, and assert that the in-process pip run does not create these keys. That will exercise the "unset env + missing runtime dirs" scenario.

Suggested implementation:

WINDOWS_RUNTIME_LIBS_DIR = ntpath.join(WINDOWS_RUNTIME_ROOT, "libs")
EXISTING_WINDOWS_INCLUDE_DIR = ntpath.join(r"C:\toolchain", "include")
EXISTING_WINDOWS_LIB_DIR = ntpath.join(r"C:\toolchain", "lib")


@pytest.mark.asyncio
async def test_run_pip_in_process_does_not_create_env_when_unset_and_runtime_dirs_missing(
    configure_run_pip_in_process_capture,
):
    observed_env = configure_run_pip_in_process_capture(
        platform="win32",
        packaged_runtime=True,
        include_value=None,
        lib_value=None,
        existing_runtime_dirs=set(),
    )

    installer = PipInstaller("")
    result = await installer._run_pip_in_process(["install", "demo-package"])

    assert result == 0
    assert "INCLUDE" not in observed_env
    assert "LIB" not in observed_env
    assert "INCLUDE" not in pip_installer_module.os.environ
    assert "LIB" not in pip_installer_module.os.environ

This assumes configure_run_pip_in_process_capture treats include_value=None and lib_value=None as “do not set these env vars initially”. If in your fixture the absence is represented differently (e.g. by omitting the arguments or using a sentinel), adjust the call accordingly. Also, this test relies on pip_installer_module already being imported/available as used in the existing tests.