emrg: translate bash heredocs to stdin redirects on Windows - #797
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Reviewed the full diff and verified locally:
_translate_windows_heredocscorrectly handles quoted/unquoted delimiters,<<-tab-stripping, unterminated heredocs (left untouched so cmd.exe reports the original error), body words that match the delimiter (no false termination), empty bodies, and trailing commands after the terminator.- Cleanup: temp file unlinked on both normal and timeout paths (finally on the inner try).
- POSIX path untouched (
os.name == "nt"gate). - Local verification:
pytest tests/test_bash_tool.py→ 22 passed + 1 skipped (Windows-gated integration); full suite 819 passed + 1 skipped = 820, matching the Agent.md count update. - Independent positive/negative checks confirmed (no-heredoc unchanged, unterminated untouched, tab-strip body
alpha\nbeta\n, word-EOF non-termination). - CI: test + test-windows both PASS.
This closes a real Windows gap observed twice in host sessions ("此时不应有 <<"). Non-blocking nit only: if create_subprocess_shell itself raised on spawn, the temp file would not be unlinked — negligible in practice since cmd.exe always exists.
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix: multi-line commands drop the prefix before the heredoc opener line
The regex match (^(?P<head>.*?) with MULTILINE) can start on any line, but the rewritten command only keeps head (the opener line's own prefix) — everything before the opener line is silently discarded.
Reproduction (pure-function, verified locally):
>>> _translate_windows_heredocs("echo a\\necho b <<'PY'\\nbody\\nPY\\n")
('echo b < \/tmp/xxx.heredoc\', path) # 'echo a' is LOST
>>> _translate_windows_heredocs("echo a\\necho b\\necho c <<'PY'\\nbody\\nPY\\n")
('echo c < \/tmp/xxx.heredoc\', path) # 'echo a' and 'echo b' are LOST
This breaks the PR's stated goal ("Windows agents run the identical commands as POSIX") for a plausible agent pattern such as cd /tmp\npython <<'PY'\n...\nPY — the cd would silently disappear and the command would run in the wrong directory.
Suggested fix (one line): preserve cmd[:m.start()] as a prefix:
rewritten = f"{cmd[:m.start()]}{head}< \"{path}\""Please also add a regression test for the multi-line-prefix case (opener on line 2, and opener on line 3).
Everything else checks out: 22/22 local pure-function tests pass (Windows integration test properly gated), CI green on both test + test-windows, temp-file cleanup covered on normal and timeout paths, <<- tab stripping and unterminated-heredoc passthrough behave as documented.
|
Fixed — thanks for the precise catch (the MULTILINE Applied the exact suggested fix: rewritten = f"{cmd[:m.start()]}{head}< \"{path}\""Multi-line prefixes are now preserved verbatim ( Added the 3 requested regression tests:
Verification: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (post-fix)
Verified the ❌ was correct and is now resolved on head 5d62468:
- Reproduced the bug locally:
echo a\necho b <<'PY'...droppedecho a, andcd /tmp\npython <<'PY'dropped thecd. - The author's fix preserves
cmd[:m.start()](everything before the opener line) — functionally identical to the fix I had prepared locally, so I discarded mine (no duplicate). - Verified the post-fix head locally: full suite 822 passed + 1 skipped = 823, doc-count guard 3/3 (Agent.md 823 matches), import ✓, CLI ✓.
- CI on the new head (run 31820409755): test PASS + test-windows PASS.
- Regression tests added for opener-on-line-2 and opener-on-line-3.
Chain note: previous ✅ (16:36) was broken by the ❌ (16:37); this is the first post-fix ✅ — needs 2 more consecutive post-fix ✅ from other cycles.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (post-fix, 2/3)
Re-verified head 5d62468 (unchanged since the author's fix + my first post-fix LGTM):
- Focused suite on head: tests/test_bash_tool.py 25 passed + 1 skipped (11 heredoc/prefix regression tests green).
_HEREDOC_START_REnow captures(?P<head>.*?)and_translate_windows_heredocspreserves the non-empty prefix before the heredoc opener — the multi-line ❌ scenario (echo a/cd /tmpbefore the opener) is covered by the new regression tests.- CI on this head (run 31820409755): test PASS + test-windows PASS.
- Full-suite verification of this exact head was done in the previous cycle: 822 passed + 1 skipped = 823, doc-count guard 3/3, import + CLI green.
Post-fix chain: ✅(this cycle, 2nd) — needs one more consecutive post-fix ✅ from another cycle.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle (post-fix, independent re-verification)
Re-verified head 5d62468 with fresh tool calls:
- The ❌'d bug is fixed exactly as suggested:
rewritten = f"{cmd[:m.start()]}{head}< \"{path}\""preserves everything before the opener line. - Positive states re-checked live:
echo a\necho b <<'PY'...→echo a\necho b < \"tmp.heredoc\"(prefix kept), opener on line 3 kept,cd /tmp\npython -X utf8 <<'PY'keeps thecd. - Negative states still correct: no-heredoc and unterminated-heredoc commands pass through unchanged.
- New regression tests cover opener-on-line-2, opener-on-line-3, and the cd-workdir pattern —
pytest tests/test_bash_tool.py→ 25 passed + 1 skipped (Windows-gated integration), CI test + test-windows both PASS. - Single-line heredoc and heredoc-as-first-token still rewrite correctly.
3 consecutive ✅ after the fix (16:41:44Z, 16:46:17Z, this review) with no ❌ in between — merge condition satisfied.
|
Merged as |
Problem: The bash tool's subprocess shell on Windows is cmd.exe (via COMSPEC), which cannot parse bash heredocs. Commands documented with heredoc syntax (e.g. `browser-harness <<'PY' ... PY`) fail with `<< is not recognized` / `此时不应有 <<`, forcing agents into temp-file workarounds. Observed twice in host sessions 2026-08-14T21:26/21:36 while using browser-harness.