Skip to content

fix(windows): centralize subprocess compat, add safe_split_command, secure_file_chmod, master_subprocess_run - #37241

Open
dellastreet53-dev wants to merge 1 commit into
NousResearch:mainfrom
dellastreet53-dev:fix/windows-portability
Open

fix(windows): centralize subprocess compat, add safe_split_command, secure_file_chmod, master_subprocess_run#37241
dellastreet53-dev wants to merge 1 commit into
NousResearch:mainfrom
dellastreet53-dev:fix/windows-portability

Conversation

@dellastreet53-dev

Copy link
Copy Markdown

Adds a comprehensive Windows subprocess compatibility module with:

  • safe_split_command: shlex.split replacement that preserves backslashes on Windows
  • secure_file_chmod: cross-platform chmod (ICACLS on Windows, os.chmod on POSIX)
  • master_subprocess_run/master_subprocess_popen: centralized .cmd shim resolution
  • safe_subprocess_run: convenience wrapper for string commands
  • windows_detach_popen_kwargs: correct detached-process creation flags

Migrates 31 files from raw shlex.split (backslash destruction on Windows),
os.chmod (ineffective on Windows), and Path.home()/.hermes (breaks profiles)
to the centralized wrapper.

…ecure_file_chmod, master_subprocess_run

Adds a comprehensive Windows subprocess compatibility module with:
- safe_split_command: shlex.split replacement that preserves backslashes on Windows
- secure_file_chmod: cross-platform chmod (ICACLS on Windows, os.chmod on POSIX)
- master_subprocess_run/master_subprocess_popen: centralized .cmd shim resolution
- safe_subprocess_run: convenience wrapper for string commands
- windows_detach_popen_kwargs: correct detached-process creation flags

Migrates 31 files from raw shlex.split (backslash destruction on Windows),
os.chmod (ineffective on Windows), and Path.home()/.hermes (breaks profiles)
to the centralized wrapper.
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels Jun 2, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the broad Windows portability audit. The underlying problem is still real on current main: agent/shell_hooks.py:451 parses commands before the shell=False spawn at agent/shell_hooks.py:462-469, and agent/copilot_acp_client.py:70-74 constructs ACP argv from shlex.split.

Problems

  • In a2cbfbda48e3, safe_split_command() returns [command] on Windows, but the migrated shell-hook caller still uses shell=False. A command with arguments therefore becomes one executable name instead of an argv vector.
  • plugins/disk-cleanup/__init__.py calls safe_split_command(cmd, posix=True), while the new helper accepts no posix keyword; this raises TypeError.
  • master_subprocess_run / master_subprocess_popen pass the entire remainder of a string Node command as one argument, so npm install --save x is not reconstructed as separate argv entries.
  • The fallback get_hermes_home edits in plugins/disk-cleanup/disk_cleanup.py and plugins/hermes-achievements/dashboard/plugin_api.py recurse when the constants import fails.

Suggested changes

  • Separate parser, shell=False argv, and intentional-shell command-string use cases; preserve a real argv for the first two rather than returning a raw one-item list.
  • Add regression tests for Windows tokenization, migrated shell hooks/ACP args, disk cleanup, Node arguments, and fallback imports.

This is an automated hermes-sweeper review.

# Tokenise the command — catches `touch /tmp/hermes-x/test_foo.py`
try:
for tok in shlex.split(cmd, posix=True):
for tok in safe_split_command(cmd, posix=True):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

safe_split_command is defined in this PR with only a command parameter, so this existing posix=True call becomes a TypeError. Either preserve the needed keyword contract or remove/rework this call with a tokenizer appropriate for this parser.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants