Skip to content

[codex] fix(tests): stabilize local interrupt cleanup check - #39496

Open
sweetcornna wants to merge 1 commit into
NousResearch:mainfrom
sweetcornna:fix/local-interrupt-cleanup-timeout
Open

[codex] fix(tests): stabilize local interrupt cleanup check#39496
sweetcornna wants to merge 1 commit into
NousResearch:mainfrom
sweetcornna:fix/local-interrupt-cleanup-timeout

Conversation

@sweetcornna

@sweetcornna sweetcornna commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • skip only the two POSIX process-group cleanup regressions on platforms where os.getpgid / os.killpg do not exist
  • keep the tests fully active on POSIX and preserve current main's test-runner timeout policy
  • mark the gated os.killpg probe as an intentional Windows-footgun scanner exception

Root cause

tests/tools/test_local_interrupt_cleanup.py directly uses POSIX-only process-group APIs. On native Windows, current main fails both tests before reaching the behavior under test because os.getpgid and os.killpg are absent.

The earlier branch also added a per-test pytest-timeout marker. Current main removed pytest-timeout in 2f9d18711 and now bounds each test file through scripts/run_tests_parallel.py (300 seconds), so the rebased patch follows that current policy instead of restoring a stale marker.

Current-main integration

  • rebased onto 11089899fbb3ec1c043427c680e8fd8f4cab06c9
  • skip markers are scoped to the two POSIX-dependent tests rather than the whole module
  • the Windows tree-kill regression that previously existed in this file was removed from current main by 39975613b; this patch does not remove or skip any current Windows-specific test

Validation

  • .venv\Scripts\python.exe -m pytest tests\tools\test_local_interrupt_cleanup.py -q (Windows 11: 2 skipped)
  • wsl -e sh -lc "cd /mnt/d/project/hermes/hermes-agent-work && UV_PROJECT_ENVIRONMENT=/tmp/hermes-pr39496-uv-env uv run --extra dev --extra all python -m pytest tests/tools/test_local_interrupt_cleanup.py -q" (Linux/WSL: 2 passed)
  • scripts/run_tests.sh tests/tools/test_local_interrupt_cleanup.py -q (canonical runner: file passed)
  • .venv\Scripts\ruff.exe check tests\tools\test_local_interrupt_cleanup.py
  • .venv\Scripts\python.exe -m py_compile tests\tools\test_local_interrupt_cleanup.py
  • .venv\Scripts\python.exe scripts\check-windows-footguns.py --diff upstream/main
  • git diff --check upstream/main...HEAD

Independent review found no high-confidence issues.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management backend/local Local shell execution labels Jun 5, 2026
@sweetcornna
sweetcornna force-pushed the fix/local-interrupt-cleanup-timeout branch from 87e5068 to acb454c Compare June 5, 2026 05:41
@sweetcornna
sweetcornna marked this pull request as ready for review June 5, 2026 05:52

@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 addressing the cross-platform and CI-stability concerns. The timeout increase targets a real 30-second wait in tests/tools/test_local_interrupt_cleanup.py:52.

Problems

  • The module-wide pytestmark in this PR would skip the newer Windows regression test at tests/tools/test_local_interrupt_cleanup.py:100-123 on native Windows. That test was added by e5253d852 to verify Windows process-tree cleanup, so it should remain runnable there.

Suggested changes

  • Resolve the file conflict and scope the POSIX skip markers to only the POSIX-dependent tests at lines 68 and 126. Keep test_kill_process_uses_windows_tree_kill active, and keep the 75-second timeout on the live POSIX test.

Automated hermes-sweeper review.

@@ -22,6 +22,11 @@

from tools.environments.local import LocalEnvironment

pytestmark = pytest.mark.skipif(

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 module-wide skip would also skip current main's test_kill_process_uses_windows_tree_kill (tests/tools/test_local_interrupt_cleanup.py:100-123, added by e5253d852) on Windows, removing the regression coverage that platform needs. Please scope this skip to the two POSIX-dependent tests instead.

@teknium1 teknium1 added sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
@sweetcornna
sweetcornna force-pushed the fix/local-interrupt-cleanup-timeout branch from acb454c to ae5dff3 Compare July 30, 2026 09:44
@sweetcornna

Copy link
Copy Markdown
Contributor Author

Rebased and reworked this onto current main (11089899f). The stale module-wide skip and pytest-timeout marker are gone: the skip is now scoped to the two POSIX process-group tests, and current main's 300-second per-file runner remains authoritative. The Windows-specific test mentioned in the sweeper review was later pruned from this file by 39975613b, so the updated diff does not suppress any current Windows regression.

Validation: Windows 2 skipped; Linux/WSL 2 passed; canonical targeted runner, Ruff, py_compile, Windows footgun scan, and git diff --check all passed.

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

Labels

backend/local Local shell execution P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation 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.

3 participants