Skip to content

fix(tools): preserve Windows process env#2382

Merged
graycyrus merged 1 commit into
tinyhumansai:mainfrom
aqilaziz:codex/2379-windows-tool-env
May 22, 2026
Merged

fix(tools): preserve Windows process env#2382
graycyrus merged 1 commit into
tinyhumansai:mainfrom
aqilaziz:codex/2379-windows-tool-env

Conversation

@aqilaziz
Copy link
Copy Markdown
Contributor

@aqilaziz aqilaziz commented May 21, 2026

Summary

  • forward Windows process essentials through the shell, node_exec, and npm_exec env allowlists after env_clear()
  • keep managed Node PATH rebuilding intact while preserving PATHEXT/COMSPEC/SystemRoot and user temp/profile paths
  • add regression coverage for the Windows env allowlist entries

Refs #2379

Testing

  • cargo fmt --all --check
  • git diff --check
  • cargo test windows_process_essentials --lib (fails before tests: whisper-rs-sys cannot find clang.dll/libclang.dll in this environment)

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Windows compatibility for system command execution by ensuring all essential Windows environment variables are properly configured for subprocess creation and lookup operations.
  • Tests

    • Added unit test coverage to verify that required Windows environment variables are present and properly configured in system execution contexts.

Review Change Stack

@aqilaziz aqilaziz requested a review from a team May 21, 2026 00:39
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b7e39544-2f4c-49c6-8322-45323e68321d

📥 Commits

Reviewing files that changed from the base of the PR and between 369a392 and 300935e.

📒 Files selected for processing (3)
  • src/openhuman/tools/impl/system/node_exec.rs
  • src/openhuman/tools/impl/system/npm_exec.rs
  • src/openhuman/tools/impl/system/shell.rs

📝 Walkthrough

Walkthrough

Three system execution modules update their environment variable allowlists to include Windows-specific variables required for child process creation. SAFE_ENV_VARS constants in node_exec.rs, npm_exec.rs, and shell.rs are expanded with platform essentials and validated by new unit tests.

Changes

Windows Process Creation Environment Variables

Layer / File(s) Summary
SAFE_ENV_VARS Windows environment expansion
src/openhuman/tools/impl/system/node_exec.rs, src/openhuman/tools/impl/system/npm_exec.rs, src/openhuman/tools/impl/system/shell.rs
Three system execution modules expand SAFE_ENV_VARS allowlists to include Windows-specific variables (SystemRoot, WINDIR, COMSPEC, PATHEXT, TEMP, TMP, USERPROFILE, APPDATA, LOCALAPPDATA, ProgramFiles variants) needed for reliable child process spawning after env_clear(). Each constant is reformatted into an explicit multiline list with documentation about Windows process requirements.
Windows environment variable validation tests
src/openhuman/tools/impl/system/node_exec.rs, src/openhuman/tools/impl/system/npm_exec.rs, src/openhuman/tools/impl/system/shell.rs
Unit tests verify that each module's SAFE_ENV_VARS includes key Windows environment variables (SystemRoot, COMSPEC, PATHEXT, TEMP, USERPROFILE) required for child process creation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Whiskers twitched with glee today,
Windows paths now find their way,
SAFE_ENV_VARS, three times blessed,
Child processes pass the test!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving Windows process environment variables across the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@YOMXXX
Copy link
Copy Markdown
Contributor

YOMXXX commented May 21, 2026

I split out the remaining default-allowlist facet from #2379 into #2399 so this PR can stay focused on env propagation.

Current split:

These are intentionally non-overlapping companion fixes rather than duplicate PRs.

Copy link
Copy Markdown
Contributor

@graycyrus graycyrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice work!

@graycyrus graycyrus merged commit 6b3a792 into tinyhumansai:main May 22, 2026
29 checks passed
senamakel pushed a commit to aqilaziz/openhuman that referenced this pull request May 23, 2026
@YellowSnnowmann YellowSnnowmann mentioned this pull request May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants