Skip to content

fix: make the full suite reliable on macOS - #345

Merged
Kyzcreig merged 2 commits into
mainfrom
test-full-suite-macos-hermeticity
Jul 15, 2026
Merged

Kyzcreig merged 2 commits into
mainfrom
test-full-suite-macos-hermeticity

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Summary

  • replace PID-derived terminal snapshot temp names with atomically-created mktemp files that work under macOS bash 3.2 and modern bash
  • give every per-file pytest subprocess a private, cleaned --basetemp so parallel runs cannot delete another process's active tmp_path tree
  • make Linux/systemd, live-model, s6-mode, process-liveness, and /tmp path tests establish their real platform premises instead of inheriting the developer host

Why

The first complete local run at f98ca2fdc finished with 19 failures. Sixteen reproduced deterministically and were host-assumption failures; three were order-dependent temp-directory races. The terminal snapshot implementation also contained a real portability bug: macOS bash 3.2 has no BASHPID, while $$ is shared by concurrent subshells, so concurrent snapshot writers could collide.

Verification

  • full local suite, clean HOME, macOS, Python 3.11.15:
    • HOME=/Users/alexgierczyk/.hermes/tmp/full-suite-home scripts/run_tests.sh -j 12
    • 2,170 files; 42,522 passed; 0 failed; 699.6s
    • 10 optional-dependency files skipped all tests; 11 source files intentionally define no pytest tests; both are surfaced by the runner and are not failures
  • targeted regression files before the full rerun: 15 passed, 0 failed
  • Ruff on all 10 touched Python files: passed
  • git diff --check: passed

Failure-path coverage

  • behavioral concurrent snapshot test executes the emitted mktemp + atomic mv sequence
  • runner test proves two parallel files receive distinct explicit basetemps and that the runner removes both afterward
  • explicit caller --basetemp remains authoritative

Scope / deployment

This fixes test hermeticity plus the terminal snapshot portability defect. It does not change model routing, gateway configuration, or live service state. No gateway restart or deployment is required for review.

@Kyzcreig
Kyzcreig enabled auto-merge (squash) July 15, 2026 01:30
@Kyzcreig
Kyzcreig merged commit 4e4f719 into main Jul 15, 2026
35 checks passed
@Kyzcreig
Kyzcreig deleted the test-full-suite-macos-hermeticity branch July 15, 2026 01:36
@greptile-apps

greptile-apps Bot commented Jul 15, 2026 •

Copy link
Copy Markdown

Greptile Summary

This PR fixes test suite reliability on macOS by replacing PID-derived shell temp names with atomic mktemp-created files in tools/environments/base.py, giving each parallel pytest subprocess its own private --basetemp directory to prevent tmp_path races, and patching 16 tests to stop inheriting host assumptions (systemctl presence, Linux /proc, API network calls, and macOS-incompatible file permission modes).

  • Shell snapshot atomicity (tools/environments/base.py): Both the init_session bootstrap and _wrap_command now assign __hermes_snap_tmp=$(mktemp 'template.XXXXXX' 2>/dev/null) || __hermes_snap_tmp=, with a [ -n "$__hermes_snap_tmp" ] guard before the export/mv pipeline — correctly closing the $$/$BASHPID portability gap on macOS bash 3.2.
  • Parallel runner isolation (scripts/run_tests_parallel.py): _run_one_file creates a tempfile.mkdtemp(prefix="pytest-file-") basetemp for each subprocess when the caller did not supply one, and removes it with shutil.rmtree(ignore_errors=True) after the process group is reaped.
  • Test hermeticity patches: systemctl-dependent tests now carry a requires_systemctl skip mark; WSL/systemd tests mock shutil.which; model-switch tests stub fetch_api_models; file-tool tests resolve /tmp symlinks via Path.resolve() to match what _resolve_path_for_task actually returns on macOS.

Confidence Score: 4/5

Safe to merge; all production logic changes are confined to shell script generation and test runner isolation, with no impact on model routing, gateway config, or live service state.

The mktemp replacement in base.py is well-reasoned and correct — the prior BASHPID scheme had a real portability gap on macOS bash 3.2 and a documented pipeline-subshell bug on Linux CI. The private basetemp fix in run_tests_parallel.py addresses a genuine observed race. Two minor observations: the rm cleanup arm passes an empty argument when mktemp fails (harmless but slightly surprising), and the macOS-adapted permission assertion in test_service_manager.py means the setgid contract for s6 is only enforced in CI, not in local macOS runs.

tools/environments/base.py warrants a second look at the rm-on-failure arm when __hermes_snap_tmp is empty; tests/hermes_cli/test_service_manager.py is worth reviewing to confirm the Linux CI pipeline exercises the 0o3730 assertion path.

Important Files Changed

Filename Overview
tools/environments/base.py Core production change: replaces BASHPID-derived temp names with mktemp for portable atomic snapshot writes; logic and guards are correct, though the rm-on-failure path passes an empty argument to rm when mktemp fails (harmless due to 2>/dev/null
scripts/run_tests_parallel.py Adds private basetemp isolation per pytest subprocess using tempfile.mkdtemp; cleanup with shutil.rmtree(ignore_errors=True) occurs after process reap. Logic and placement are correct.
tests/test_run_tests_parallel.py New behavioral test validates distinct basetemps per parallel subprocess and confirms runner cleanup; assertions are correct.
tests/tools/test_base_environment.py Tests updated to assert mktemp-based temp assignment instead of BASHPID; static assertions and behavioral concurrency test both correctly reflect the new contract.
tests/hermes_cli/test_service_manager.py Accepts 0o1730 (no setgid) on macOS vs 0o3730 on Linux for s6 event/ directories; correct for macOS behavior but reduces Linux-only assertion coverage when run locally on macOS.
tests/test_live_system_guard_self_test.py Adds requires_systemctl skip marker so systemctl pass-through tests are skipped on macOS hosts without systemd; correct fix for host-assumption failure.
tests/hermes_cli/test_gateway_wsl.py Patches shutil.which to return a fixed systemctl path so WSL/systemd tests don't depend on host tooling; change is minimal and correct.
tests/hermes_cli/test_model_switch_custom_providers.py Stubs fetch_api_models to prevent live network calls; targets the correct module path for monkeypatching.
tests/hermes_cli/test_signal_handler_kanban_worker.py Removes inline reimplementation of _pid_alive in favour of importing the real function from hermes_cli.kanban_db; eliminates Linux-only /proc logic that failed on macOS.
tests/tools/test_file_tools.py Updates mock assertions to use Path.resolve() to match what _resolve_path_for_task actually returns, fixing /tmp → /private/tmp symlink mismatch on macOS.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Runner as run_tests_parallel.py
    participant OS as OS / tempfile
    participant Pytest as pytest subprocess
    participant SnapShell as bash (snapshot shell)

    Runner->>OS: "tempfile.mkdtemp(prefix="pytest-file-")"
    OS-->>Runner: /tmp/pytest-file-abc123/
    Runner->>Pytest: "launch with --basetemp=/tmp/pytest-file-abc123/"
    Pytest-->>Runner: exit (output captured)
    Runner->>OS: "shutil.rmtree(/tmp/pytest-file-abc123/, ignore_errors=True)"

    note over SnapShell: env snapshot atomic write (base.py)
    SnapShell->>OS: mktemp 'snap.tmp.XXXXXX'
    OS-->>SnapShell: unique temp path /snap.tmp.XYZ789
    SnapShell->>OS: "export -p > /snap.tmp.XYZ789"
    SnapShell->>OS: mv -f /snap.tmp.XYZ789 /snap (atomic)
    note over SnapShell: on mktemp failure: __hermes_snap_tmp="" guard skips write
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Runner as run_tests_parallel.py
    participant OS as OS / tempfile
    participant Pytest as pytest subprocess
    participant SnapShell as bash (snapshot shell)

    Runner->>OS: "tempfile.mkdtemp(prefix="pytest-file-")"
    OS-->>Runner: /tmp/pytest-file-abc123/
    Runner->>Pytest: "launch with --basetemp=/tmp/pytest-file-abc123/"
    Pytest-->>Runner: exit (output captured)
    Runner->>OS: "shutil.rmtree(/tmp/pytest-file-abc123/, ignore_errors=True)"

    note over SnapShell: env snapshot atomic write (base.py)
    SnapShell->>OS: mktemp 'snap.tmp.XXXXXX'
    OS-->>SnapShell: unique temp path /snap.tmp.XYZ789
    SnapShell->>OS: "export -p > /snap.tmp.XYZ789"
    SnapShell->>OS: mv -f /snap.tmp.XYZ789 /snap (atomic)
    note over SnapShell: on mktemp failure: __hermes_snap_tmp="" guard skips write
Loading

Comments Outside Diff (1)

  1. tools/environments/base.py, line 541-548 (link)

    P2 rm -f "" executed when mktemp fails

    When mktemp fails, __hermes_snap_tmp is set to the empty string. The [ -n "$__hermes_snap_tmp" ] guard correctly skips the export/mv block, but the || rm -f "$__hermes_snap_tmp" arm then runs with an empty argument — executing rm -f "". This is harmless on every platform because 2>/dev/null || true catches both the stderr noise and the non-zero exit, but it is slightly surprising. Consider guarding the cleanup arm with the same -n check to make the no-op explicit: || { [ -n "$__hermes_snap_tmp" ] && rm -f "$__hermes_snap_tmp"; } 2>/dev/null || true.

Reviews (1): Last reviewed commit: "fix(tests): make the full suite hermetic..." | Re-trigger Greptile

Comment thread tests/hermes_cli/test_service_manager.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant