fix: harden Windows portability tests and paths - #40891
eliaschofo wants to merge 16 commits into
Conversation
alt-glitch
added
type/test
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for pursuing the native Windows path cases. The ACP portion targets a real current-main defect: acp_adapter/server.py:176-184 always maps drive-letter URIs to /mnt/<drive>, while the PR’s acp_adapter/server.py:171-181 adds the needed native-Windows branch.
Problems
tests/acp/__init__.py:1-30should not be added.AGENTS.md:165-169explicitly documents test-tree__init__.pyrestoration as a load-bearing shadowing failure; the dynamic re-exec workaround still makes import behavior collection/path-order dependent.tools/terminal_tool.py:1725-1768and1920-1930globally reject broad raw pytest commands and redirect to a new wrapper. This is unrelated to the Windows fix and conflicts with the requiredscripts/run_tests.shrunner inAGENTS.md:1281-1285.scripts/hermes_safe_pytest.py:104,174,180hardcodes/tmpandvenv/bin, so the added wrapper is itself not native-Windows portable.tests/acp_adapter/test_acp_images.py:41changes file writing but does not cover_path_from_file_uri; the drive-letter regression needs direct tests.
Suggested changes
- Split out the narrow ACP/path fixes, remove
tests/acp/__init__.py, and add direct C:-drive URI tests for native Windows and WSL behavior. - Leave the pytest guardrail, terminal behavior, and unrelated dependency churn out of this salvage.
Automated hermes-sweeper review.
| @@ -0,0 +1,30 @@ | |||
| """ACP test package marker without shadowing ``agent-client-protocol``. | |||
There was a problem hiding this comment.
Please do not add this package marker. AGENTS.md:165-169 documents this exact test-tree __init__.py pattern as load-bearing because it can shadow the installed package; the dynamic re-exec workaround makes imports depend on collection name and sys.path order.
| return None | ||
|
|
||
|
|
||
| _RAW_PYTEST_RE = re.compile( |
There was a problem hiding this comment.
This introduces a global terminal-tool denial for broad pytest commands, which is unrelated to the Windows path fixes and conflicts with the repository’s required scripts/run_tests.sh runner (AGENTS.md:1281-1285). Please remove this guardrail from this PR.
| return usage.free / (1024**3) | ||
|
|
||
|
|
||
| def cleanup_known_pytest_artifacts(tmp_root: Path = Path("/tmp"), *, max_age_seconds: int = 0) -> int: |
There was a problem hiding this comment.
This new wrapper hardcodes POSIX /tmp here (and later venv/bin), so it is not native-Windows portable. It should not be introduced as part of a Windows portability fix.
teknium1
added
sweeper:risk-security-boundary
3 participants
Summary
Test Plan
python -m pytest tests\agent\test_external_skills.py tests\agent\test_skill_commands.py::TestInlineShellExpansion::test_inline_shell_runs_when_enabled -q -o addopts="" --tb=short→ 12 passed.python -m pytest tests\agent\test_skill_commands.py tests\agent\test_external_skills.py tests\agent\test_external_skills_dirs_cache.py -q -o addopts="" --tb=short→ 58 passed.python -m pytest tests\agent\test_subdirectory_hints.py tests\agent\test_skill_commands.py tests\agent\test_external_skills.py tests\agent\test_external_skills_dirs_cache.py -q -o addopts="" --tb=short→ 83 passed.python -m pytest tests\agent -q -o addopts= -m "not integration" --maxfail=1 --tb=short→ 3914 passed, 14 skipped.