Skip to content

test(tools): shell-quote Windows sentinel paths passed to Bash - #63641

Open
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:win-approved-sentinel-posix
Open

test(tools): shell-quote Windows sentinel paths passed to Bash#63641
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:win-approved-sentinel-posix

Conversation

@LceAn

@LceAn LceAn commented Jul 13, 2026

Copy link
Copy Markdown

What & why

These two regression tests interpolate a pathlib.Path into a Bash command. On native Windows, the terminal uses Git Bash: backslashes are consumed as escapes, and an unquoted path with spaces can be split into multiple arguments. The original as_posix() fix addressed separators but not shell metacharacters or spaces.

Change

  • Pass both sentinels through shlex.quote(sentinel.as_posix()).
  • Keep the tests as real post-start interrupt checks; sentinel filenames now include spaces so the quoting contract is exercised.

Validation

  • Native Windows: the three focused approval/interrupt tests pass.
  • ruff and py_compile pass.

This remains a focused test-only fix. Related direct Bash path interpolations in test_file_tools_live.py are separate follow-up work.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure tool/terminal Terminal execution and process management platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 13, 2026
@LceAn
LceAn force-pushed the win-approved-sentinel-posix branch from cf090c9 to 9efa4dd Compare July 13, 2026 08:46
@LceAn

LceAn commented Jul 13, 2026

Copy link
Copy Markdown
Author

Gentle ping for review. (Rebased onto current main — the branch is now mergeable again.)

Two things worth flagging on this one beyond the diff itself:

  1. It fixes a double fault on Windows: the two tests fail (AssertionError: command did not start) AND each run leaks a junk file into the repo root, because Git Bash consumes the backslashes in the interpolated C:\Users\A\… path and touch writes a mangled, separator-less filename there. With as_posix() both tests pass and the leak stops (stray-file count before/after = 0).

  2. The as_posix() pattern recurs elsewhere — tests/tools/test_file_tools_live.py has the same class of path-interpolation-into-bash bug (reported on Windows-native pytest suite has baseline failures in ACP, path, CRLF, and file-handle tests #48986 alongside an orthogonal CRLF issue). If the maintainers like this shape, I can extend it there in a follow-up.

No production code touched; no-op on POSIX where Path.as_posix() returns the native string unchanged.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows regression fix. Current main still formats the two tmp_path sentinels directly into Bash command strings at tests/tools/test_approved_command_clean_slate.py:97 and :128; this diff updates exactly those sites. The existing Windows shell-path contract independently documents that Git Bash consumes backslashes in embedded paths at tools/environments/local.py:69-88.

No blocking issue found in this two-line test-only change. The related direct shell-path interpolations in tests/tools/test_file_tools_live.py:141, :413, :420, :428, and :435 remain a separate follow-up, consistent with the linked #48986 discussion.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 16, 2026
@LceAn
LceAn force-pushed the win-approved-sentinel-posix branch from 9efa4dd to d0e27df Compare July 20, 2026 02:59
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Maintenance update: rebased onto current main. The reviewed two-line as_posix() fix is unchanged and the branch remains a focused one-file diff. Ready for review.

@LceAn
LceAn force-pushed the win-approved-sentinel-posix branch from d0e27df to 74b24c6 Compare July 20, 2026 05:13
@LceAn LceAn changed the title test(tools): use as_posix() for sentinel paths passed to bash on Windows test(tools): shell-quote Windows sentinel paths passed to Bash Jul 20, 2026
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Second-pass maintenance: the original separator fix was not sufficient for spaces or shell metacharacters. The two sentinels now use shlex.quote(as_posix()), with space-containing filenames to prove the full contract; focused Windows tests pass.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Three PRs are associated with this complex. #52000 and #52001 propagate session_id through execute_code and its local and remote nested-tool RPC paths, while #63641 only shell-quotes Windows sentinel paths in an unrelated test file.

Related pull requests

Duplicates

#52000 and #52001 implement substantially the same session_id propagation; #52001 is the closed duplicate of #52000. #63641 is not part of that duplicate set.

Suggested consolidation

Keep #52000 open with a salvage path: preserve its explicit registry contract, local and remote RPC propagation, rpc_token handling, and focused regressions; this follows the recorded best-fix designation and the maintainer-bot keep_open review without making a merge recommendation. Keep #52001 closed as duplicate of #52000, and keep #63641 open for separate triage of its reviewed Windows path-quoting fix rather than associating it with #51931.

Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 24 kB of PR diffs, 19 kB of issue/PR text, 8 kB of discussion (13 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/terminal Terminal execution and process management type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants