fix(windows): prevent Git Bash NUL redirect files - #65363
Conversation
|
This feels like a band-aid fix to me. Models writing >NUL in commands tells me that we're telling it "you're on windows" in a way that makes it unaware it's using Rather than this, can we look into where we tell the model its environment, and try to make it more specific about what exact kind of terminal execution it'll be running in? |
|
I dug through the persisted Hermes session history, and the model was already being told this. The offending session’s saved system prompt explicitly says: “your That hint has been present in
The preceding So I agree that the model should know it is using Git Bash, but that is already in the prompt and was ignored in the actual sessions that created the file. We could make the prompt even more explicit by mentioning |
|
I'll test some more explicit "use /dev/null, never NUL" type instructions in agent/prompt_builder.py and leave this PR here as an option for now while I explore the prompt guidance option. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing this to the Windows Git Bash execution boundary; live main still sends LocalEnvironment commands to Git Bash without a NUL normalization (tools/environments/local.py:1167-1182), while the existing prompt already states that contract (agent/prompt_builder.py:928-935).
Problems
tools/environments/local.py:61treats backticks like quotes and masks their entire contents. Backticks execute command substitutions in Bash, so a nestedgit show missing 2>NUL || trueremains unnormalized and can retain the reported failure mode. The added test attests/tools/test_local_env_windows_msys.py:66-73currently asserts that this class of executable text stays unchanged.- The global regex pass at
tools/environments/local.py:85-88has no heredoc handling. A literal heredoc payload line containing2>NULis rewritten even though it is data, not a redirect.
Suggested changes
- Handle executable backtick substitutions and add a regression that verifies their nested redirect is normalized.
- Make the scanner heredoc-aware, or narrow its scope to syntax it can distinguish safely; add quoted and unquoted heredoc payload regressions.
Automated hermes-sweeper review.
|
Closing this out. I'm doing some longer term guidance investigation. The prompt builder with more explicit instructions is probably the right direction. |
What does this PR do?
On native Windows,
LocalEnvironmentintentionally executes commands through Git Bash. Git Bash treats a CMD-styleNULredirect target as a relative filename rather than the null device, so model-authored commands such asgit show missing 2>NUL || truecan leave a literal Windows-reservedNULfile in the working directory.This PR normalizes common CMD-style
NULredirection targets to/dev/nullbefore the Windows Git Bash command is prepared. Literal quoted text, escaped characters, and comments are left untouched; executable backtick and$(...)substitutions remain eligible for normalization, including inside double quotes. Commands containing heredocs are conservatively left unchanged so redirect-looking payload data is never modified. Non-Windows command handling is unchanged.Related Issue
Related to #57081. #57212 guards the downstream update/autostash path when a reserved file already exists; this PR prevents the local terminal path from creating one through CMD-style null redirection.
Type of Change
Changes Made
NULredirect normalization intools/environments/local.py.LocalEnvironment._prepare_command()only on Windows.$(...)command substitutions visible to the matcher while protecting literal shell text.tests/tools/test_local_env_windows_msys.pyfor common redirect forms, executable command substitutions, protected quoted/escaped/commented text, quoted and unquoted heredocs, here-strings, non-redirect lookalikes, and the Windows-only environment path.How to Test
git show missing 2>NUL || truethroughLocalEnvironment.NULfile exists in the working directory.scripts/run_tests.sh -j 4 tests/tools/test_local_env_windows_msys.py -k WindowsBashNulRedirections -q.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passFull-suite coverage is left to GitHub CI per the repository workflow.
Documentation & Housekeeping
docs/, docstrings) — or N/A (docstrings updated; no user-facing documentation change needed)cli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
7 tests passed, 0 failedwith 4 workers.NULfile, and quoted heredoc payload output remained exactly2>NULwithout creating a file.git diff --checkpass.tests/tools/test_local_env_windows_msys.pyreached 42 passing tests and two pre-existing path-separator assertion failures outside this change; full-suite coverage is left to GitHub CI.