Skip to content

fix(windows): resolve compatibility, path resolution, and encoding issues - #42775

Open
necoweb3 wants to merge 3 commits into
NousResearch:mainfrom
necoweb3:fix/windows-compatibility-issues
Open

fix(windows): resolve compatibility, path resolution, and encoding issues#42775
necoweb3 wants to merge 3 commits into
NousResearch:mainfrom
necoweb3:fix/windows-compatibility-issues

Conversation

@necoweb3

@necoweb3 necoweb3 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR addresses multiple Windows-compatibility issues in the test runner, static regression check scripts, and the ACP adapter that currently block developer and CI workflows on Windows hosts.

Changes Included:

  1. Static Regression Checking (scripts/check_subprocess_stdin.py - Unique)

    • Configured sys.stdout to force UTF-8 on launch to prevent UnicodeEncodeError when writing outputs on non-UTF-8 Windows consoles.
    • Replaced console-specific emojis ( / ) with standard tags ([FAIL] / [PASS]) to avoid terminal encoding crashes.
    • Added encoding="utf-8" to Path.read_text() calls to prevent UnicodeDecodeError when scanning files with Unicode comments or docstrings.
    • Normalized scanned paths using .as_posix() to resolve backslash separator mismatches (\\ vs /) on Windows, ensuring file exemptions match correctly.
  2. ACP Adapter (acp_adapter/server.py - Unique + Alignment)

    • Normalized carriage returns by converting \r\n to \n in _decode_text_bytes() to prevent line-ending mismatches during local test runs for attached text files.
    • Integrated platform-aware path conversion (os.name == "nt") in _path_from_file_uri() so that Windows host editors (e.g. Zed on Windows) do not have their local file URIs rewritten into nonexistent /mnt/<drive>/... paths (aligning with changes proposed in fix(acp): keep Windows file URIs native outside WSL #40649).
  3. Test Timeout Configuration (pyproject.toml - Alignment)

Verification

  • Tested locally on Windows.
  • Ran static regression checking successfully:
    python scripts/check_subprocess_stdin.py yields [PASS] All TUI-context subprocess calls have explicit stdin= without crashes.
  • Verified ACP image capability tests successfully:
    pytest tests/acp_adapter/test_acp_images.py passes cleanly on Windows using the fallback thread method automatically.

@necoweb3
necoweb3 requested a review from a team June 9, 2026 10:37
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/acp Agent Communication Protocol adapter labels Jun 9, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive Verification — Windows Compatibility Review

Reviewed the full diff across acp_adapter/server.py, pyproject.toml, and scripts/check_subprocess_stdin.py.

Changes verified:

  • _path_from_file_uri: correct Windows os.name == "nt" early return for both file:///C: and bare C: URI forms
  • _decode_text_bytes: CRLF → LF normalization applied after decode (binary check at top guards against binary content)
  • pytest timeout-method=signal → thread-based: necessary for Windows compatibility (signal-based timeout is POSIX-only)
  • check_subprocess_stdin.py: explicit UTF-8 encoding, POSIX path normalization, ASCII-safe output markers

No issues found. The pytest timeout-method change is the most impactful — signal-based timeout can hang on Windows. Thread-based timeout is the documented cross-platform alternative. The path normalization fixes are correct for WSL/Cygwin/mingw edge cases.

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hermes Agent Review — #42775 (Windows compatibility)

Reviewed locally against origin/main (real scope = the 3 files GitHub lists; ran the static checker + the cited ACP suites). Two of the three changes are correct and I verified them — but the pyproject.toml change directly conflicts with the canonical, already-reviewed #39881 and takes the inferior approach. Requesting changes to split that one hunk out; the rest is good and I'd happily approve it on its own.

Verified good ✅

1. scripts/check_subprocess_stdin.py — all four sub-changes correct.

  • sys.stdout.reconfigure(encoding='utf-8') is properly guarded (hasattr + try/except) — safe no-op where unsupported.
  • [FAIL]/[PASS] instead of emoji — avoids UnicodeEncodeError on cp1252 consoles.
  • read_text(encoding='utf-8') — correct; the default is locale-dependent (cp1252) on Windows.
  • .as_posix() for the KNOWN_SAFE lookup — this is a real bug fix, not cosmetic: KNOWN_SAFE entries use forward slashes (agent/shell_hooks.py), so on Windows the old str(relative_to(...)) produced agent\shell_hooks.py and the exemption silently never matched. .as_posix() fixes it and is a no-op on Unix.
  • Confirmed: python scripts/check_subprocess_stdin.py[PASS], exit 0.

2. acp_adapter/server.py — both changes correct.

  • os.name == 'nt' short-circuit in _path_from_file_uri — on native Windows, returns the real C:/... / C:\... path instead of the WSL-only /mnt/<drive>/... rewrite, which would point at a nonexistent path for a Windows-host editor (e.g. Zed on Windows). Non-nt path is unchanged. Correct, and aligns with #40649.
  • CRLF normalization in _decode_text_bytes — conservative (\r\n\n only, leaves lone \r), refactored cleanly to a single return. Harmless for text-resource content fed to the agent.
  • Confirmed: tests/acp_adapter/test_acp_images.py + tests/acp/test_events.py27 passed (with pytest-asyncio). No regression from these changes.

Blocking 🔴

3. pyproject.toml — conflicts with the canonical #39881; please drop this hunk.
This edits the exact same addopts line that #39881 owns, but differently and in a way that contradicts the approved direction:

  • #39881 (canonical, reviewed): --timeout-method=signal--timeout-method=thread — explicitly standardizes on thread on all platforms.
  • This PR: removes --timeout-method entirely, relying on pytest-timeout's auto-default (thread on Windows, signal on Unix).

The net effect is the opposite of what #39881 intends: your version re-introduces the signal method on Unix, which #39881 deliberately moved away from (signal-based timeouts can't interrupt blocking C calls and misbehave under threads). The two also can't both merge cleanly since they touch the same line.

The Windows symptom you describe (SIGALRM crash) is genuinely fixed by #39881's explicit thread just as well — so please remove the pyproject hunk from this PR and let #39881 carry it. That keeps this PR focused on its two unique, correct Windows fixes. (Same pattern as the stray timeout hunk we asked #41754 to split out — pyproject timeout config has one canonical home.)

Bottom line

Drop the pyproject.toml change (defer to #39881); keep the check_subprocess_stdin.py and acp_adapter/server.py fixes. With that split, this is an easy approve — both unique changes are verified correct.


Reviewed by Hermes Agent (verified locally against origin/main).

Comment thread pyproject.toml Outdated
@@ -305,7 +305,7 @@ markers = [
# scripts/run_tests_parallel.py). Per-file isolation gives every test
# file a fresh Python interpreter; pytest-timeout catches Python-level
# hangs within a file.
addopts = "-m 'not integration' --timeout=30 --timeout-method=signal"
addopts = "-m 'not integration' --timeout=30"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Conflicts with #39881 — please drop this hunk. #39881 (canonical, already reviewed) changes this same line to --timeout-method=thread (explicit thread on all platforms). Removing the flag instead falls back to pytest-timeout's auto-default, which is thread on Windows but signal on Unix — re-introducing the exact method #39881 deliberately moved away from, and the two edits can't both merge. Your Windows SIGALRM crash is fixed equally by #39881's explicit thread. Keep this PR's two unique Windows fixes and let #39881 own the timeout config.

@austinpickett

Copy link
Copy Markdown
Collaborator

Thanks for the update and @liuhao1024 for the verification — the two unique Windows fixes in acp_adapter/server.py and scripts/check_subprocess_stdin.py are correct and I'm happy to approve those.

The pyproject.toml hunk still needs to be dropped before this can merge. Here's why:

The current diff removes --timeout-method=signal entirely, which falls back to pytest-timeout's auto-default. On Linux/macOS the auto-default is signal — which re-introduces the POSIX-only hang risk on the exact platforms where this repo's CI runs. The canonical fix (in approved PR #39881, same line) is to change it to --timeout-method=thread explicitly, which is cross-platform and was the deliberate choice there.

So the conflict is:

Both can't merge to the same line. Please drop the pyproject.toml hunk from this PR — #39881 owns that line. The two .py file changes in this PR are genuinely valuable on their own and I'll approve immediately once that hunk is out.

@necoweb3 necoweb3 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've dropped the pyproject.toml changes and force-pushed the branch. The PR is now focused purely on the unique Windows fixes in check_subprocess_stdin.py and acp_adapter/server.py

@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 preserving the two Windows-specific fixes after dropping the pyproject.toml hunk discussed by the maintainer review. Current main still has the ACP and checker behavior this PR targets.

Problems

  • acp_adapter/server.py:165-174 rejects a raw drive path before the proposed native-Windows branch at diff line 182 can execute: generic non-file URI rejection occurs immediately after urlparse(raw). The stated bare C:\... case therefore remains unsupported.
  • No regression test covers native-Windows file URIs, raw drive paths, or CRLF decoding. Existing resource-link coverage uses tmp_path.as_uri() at tests/acp_adapter/test_acp_images.py:39-59.

Suggested changes

  • Detect drive-form paths before generic URI-scheme rejection, then preserve the os.name == "nt" behavior.
  • Add focused regression tests for both drive forms and CRLF normalization.

Automated hermes-sweeper review.

Comment thread acp_adapter/server.py
drive = path_text[1].lower()
rest = path_text[3:].lstrip("/\\").replace("\\", "/")
return Path("/mnt") / drive / rest
if len(path_text) >= 2 and path_text[1] == ":" and path_text[0].isalpha():
if os.name == "nt":
return Path(path_text)
drive = path_text[0].lower()

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.

This branch is bypassed for raw C:\... input: urlparse(raw) is followed by the generic non-file scheme rejection before path_text reaches this drive-form check. Recognize Windows drive paths before that rejection and add a regression test for the bare-path case.

@necoweb3

Copy link
Copy Markdown
Contributor Author

Updated the branch.

Changes:

  • Recognize bare Windows drive paths before the generic URI scheme rejection, so raw C:\... input no longer gets rejected as a non-file URI.
  • Preserved the existing native-Windows behavior and the non-Windows /mnt/<drive>/... mapping.
  • Added focused regressions for bare Windows drive paths, file:///C:/... drive URIs, and CRLF text normalization.

Validation:

  • Direct helper probe verified both Windows path forms and CRLF normalization locally.

@alt-glitch alt-glitch added platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 14, 2026
@necoweb3
necoweb3 force-pushed the fix/windows-compatibility-issues branch from 536804a to f4482fb Compare July 14, 2026 08:29
@necoweb3

Copy link
Copy Markdown
Contributor Author

Updated the branch again.

The latest CI failure was not from the ACP Windows path change; slice 7 was failing in tests/hermes_cli/test_model_validation.py because the branch was stale against current main and two User-Agent/client-context tests still patched urllib.request.urlopen while the live code now calls _urlopen_model_catalog_request.

Follow-up changes:

  • Rebased the branch onto current main.
  • Kept the ACP raw Windows path / file:///C:/... / CRLF regressions.
  • Updated the two model validation tests to patch the current catalog request wrapper.

Validation:

  • Selected ACP path/decode tests printed 4 passed.
  • Selected model validation class printed 4 passed.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants