Skip to content

test: isolate state-dir probes from user state#6305

Closed
pxxD1998 wants to merge 1 commit into
nesquena:masterfrom
pxxD1998:fix/isolate-state-dir-contract-probes
Closed

test: isolate state-dir probes from user state#6305
pxxD1998 wants to merge 1 commit into
nesquena:masterfrom
pxxD1998:fix/isolate-state-dir-contract-probes

Conversation

@pxxD1998

Copy link
Copy Markdown
Contributor

Thinking Path

  • The state-dir contract tests import api.config in fresh subprocesses.
  • Pytest injects state and workspace overrides into the parent environment for suite isolation.
  • The normal-install probe removed HERMES_HOME and HERMES_WEBUI_STATE_DIR, but still inherited the pytest workspace override while using the platform user home.
  • Import-time settings reconciliation could therefore write the pytest workspace into a production-like settings.json.
  • This PR keeps every probe inside fixture-owned platform paths and removes all parent state/config redirects before import.

Contract Routing

Task type: test-isolation bug fix; no intentional product-contract change.

Touched areas:

  • tests/test_issue4449_state_dir_contract.py
  • subprocess environment isolation for WebUI state/config probes

Relevant public docs:

  • AGENTS.md
  • CONTRIBUTING.md
  • docs/CONTRACTS.md
  • TESTING.md

Scope boundaries:

  • No runtime product code changes.
  • No production settings or existing sessions are repaired by this PR.

Evidence needed before claiming done:

  • A sentinel platform-default settings.json remains byte-for-byte unchanged.
  • All resolved state paths remain fixture-owned.
  • Existing explicit/unset state-dir precedence tests still pass.

What Changed

  • Require each subprocess probe to receive a fixture-owned platform home.
  • Set HOME, USERPROFILE, and LOCALAPPDATA inside the child environment.
  • Clear inherited Hermes home/base-home, WebUI state/test-state/default-workspace, and config-path overrides before applying the variables under test.
  • Add a regression test that preserves the active platform-default sentinel settings.json byte-for-byte, including %LOCALAPPDATA%\hermes\webui on native Windows.
  • Use json.dumps so the sentinel remains valid on Windows paths.

Why It Matters

A test that only intends to inspect STATE_DIR must never read or reconcile pytest defaults into real user state. The isolation now models a normal install without touching the operator's home on Linux, macOS, or Windows.

Verification

  • RED before the isolation fix: the new sentinel test failed and showed settings.json rewritten with the pytest workspace (1 failed, 3 passed).
  • ./scripts/test.sh tests/test_issue4449_state_dir_contract.py tests/test_pytest_state_isolation.py tests/test_issue2698_isolated_hermes_home.py tests/test_bootstrap_foreground.py -q
    • 78 passed
  • ruff check tests/test_issue4449_state_dir_contract.py
    • passed
  • ruff format --check tests/test_issue4449_state_dir_contract.py
    • passed
  • python scripts/ruff_lint.py --diff origin/master
    • 0 findings on added/modified lines
  • git diff --check
    • passed
  • Full ./scripts/test.sh
    • 13348 passed, 104 skipped, 1 xfailed, 2 xpassed, 34 subtests passed
    • one unrelated existing environment/order-dependent failure remained in test_issue4685_post_compression_context_metering.py: importing the external Hermes Agent agent.context_compressor saw an incomplete agent.auxiliary_client stub and could not import call_llm. The same failure occurred in both isolated PR worktrees and is outside this test-only diff.

Risks / Follow-ups

  • Risk is limited to test harness behavior; runtime code is unchanged.
  • This does not repair already-persisted settings or stale session workspaces. Those are intentionally separate concerns.
  • No docs update is needed because the product contract is unchanged; this enforces the repository's existing isolated-test-state rule.

Model Used

  • Provider: OpenAI Codex
  • Model: gpt-5.6-sol
  • Reasoning mode: medium
  • Notable tools: Hermes Agent repository inspection, isolated Git worktrees, official test runner, and independent read-only review agents.

@pxxD1998
pxxD1998 force-pushed the fix/isolate-state-dir-contract-probes branch from e0d6aa0 to 71c4778 Compare July 19, 2026 05:12
@greptile-apps

greptile-apps Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes test isolation in the #4449 state-dir contract suite by ensuring every subprocess probe runs inside a fixture-owned platform home rather than inheriting the real user home and pytest-injected env vars. A new regression test verifies that a sentinel settings.json is byte-for-byte unchanged after the probe runs.

  • _state_dir_for_env now requires a platform_home argument and explicitly sets HOME, USERPROFILE, and LOCALAPPDATA in the child env while clearing HERMES_BASE_HOME, HERMES_WEBUI_TEST_STATE_DIR, HERMES_WEBUI_DEFAULT_WORKSPACE, and HERMES_CONFIG_PATH before applying per-test overrides.
  • All four callers are updated to supply a tmp_path-rooted platform home, and the previously hardcoded /tmp/hermes-4449-elsewhere-base path is replaced with a portable tmp_path-relative directory.
  • The new sentinel mutation test sets HERMES_WEBUI_DEFAULT_WORKSPACE in the parent via monkeypatch, confirms the subprocess strips it before import, and then asserts the pre-written settings.json bytes are identical after _platform_default_state_dir returns.

Confidence Score: 5/5

Test-only change with no runtime code touched; all four probe call sites are correctly updated and the new regression test accurately models the isolation scenario it claims to prove.

The fix is surgical: one helper function gains a required argument, four call sites are updated consistently, and the new sentinel test creates a pre-written settings file, runs the probe with a stripped env, and asserts byte-for-byte preservation. The env-var clearing order is correct — inherited vars are removed before per-test overrides are applied, so callers can still opt back in by passing a var explicitly. The Windows path is covered by the platform-conditional settings path and the explicit LOCALAPPDATA injection. No production code is touched.

No files require special attention.

Important Files Changed

Filename Overview
tests/test_issue4449_state_dir_contract.py Strengthens subprocess environment isolation by adding a mandatory platform_home parameter, clearing additional inherited env vars (HERMES_BASE_HOME, HERMES_WEBUI_TEST_STATE_DIR, HERMES_WEBUI_DEFAULT_WORKSPACE, HERMES_CONFIG_PATH), and adding a sentinel mutation regression test; logic is correct and consistent across all four callers.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant T as Test (pytest process)
    participant H as _state_dir_for_env
    participant S as Subprocess (api.config)
    participant FS as Filesystem (tmp_path)

    T->>T: Create platform_home and sentinel settings.json
    T->>T: monkeypatch HERMES_WEBUI_DEFAULT_WORKSPACE

    T->>H: _platform_default_state_dir(platform_home)
    H->>H: "env = dict(os.environ)"
    H->>H: Override HOME, USERPROFILE, LOCALAPPDATA to platform_home
    H->>H: Pop HERMES_HOME, HERMES_BASE_HOME, HERMES_WEBUI_DEFAULT_WORKSPACE etc.

    H->>S: subprocess.run probe
    S->>FS: Read settings.json only
    S-->>H: Print resolved STATE_DIR

    H-->>T: Return Path(STATE_DIR)

    T->>FS: Read settings.json
    T->>T: Assert bytes unchanged
    T->>T: Assert STATE_DIR equals settings_file parent
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 T as Test (pytest process)
    participant H as _state_dir_for_env
    participant S as Subprocess (api.config)
    participant FS as Filesystem (tmp_path)

    T->>T: Create platform_home and sentinel settings.json
    T->>T: monkeypatch HERMES_WEBUI_DEFAULT_WORKSPACE

    T->>H: _platform_default_state_dir(platform_home)
    H->>H: "env = dict(os.environ)"
    H->>H: Override HOME, USERPROFILE, LOCALAPPDATA to platform_home
    H->>H: Pop HERMES_HOME, HERMES_BASE_HOME, HERMES_WEBUI_DEFAULT_WORKSPACE etc.

    H->>S: subprocess.run probe
    S->>FS: Read settings.json only
    S-->>H: Print resolved STATE_DIR

    H-->>T: Return Path(STATE_DIR)

    T->>FS: Read settings.json
    T->>T: Assert bytes unchanged
    T->>T: Assert STATE_DIR equals settings_file parent
Loading

Reviews (1): Last reviewed commit: "test: isolate state-dir probes from user..." | Re-trigger Greptile

@nesquena-hermes nesquena-hermes added the size:M Medium PR (≤10 files, ≤250 LOC) label Jul 19, 2026
nesquena-hermes added a commit that referenced this pull request Jul 19, 2026
…6180) + state-dir test isolation (#6305) (#6332)

* fix: catch unhandled exception in POST /api/updates/check (defensive hardening) (#6180)

* fix: generate non-empty approval_id when gateway approval.request omits it (#6008) (#6168)

* chore: mark update-check try/except as defensive-only guard, drop #6086 linkage

Per maintainer review, the try/except wrapper is defense-in-depth only —
it does NOT fix #6086 (root cause is signal/process-group reaping).
Updated log message and added inline comment to make this explicit.
Leave #6086 open.

* test: isolate state-dir probes from user state

* docs(changelog): stamp #6168 approval_id, #6180 update-check guard, #6305 test isolation

---------

Co-authored-by: webtecnica <75556242+webtecnica@users.noreply.github.com>
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
Co-authored-by: pxxD1998 <214340659+pxxD1998@users.noreply.github.com>
Co-authored-by: nesquena-hermes <agent@nesquena-hermes>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in exp-v0.52.117 (experimental channel) via release #6332. Thanks @pxxD1998! 🎉

The #4449 state-dir contract test's subprocess probes now pin HOME/USERPROFILE/LOCALAPPDATA to fixture-owned paths and clear inherited redirect vars, so an import-only api.config probe can no longer reconcile pytest workspace defaults into the real user settings.json. Includes byte-for-byte sentinel coverage, including native Windows paths. Test-infra only — no runtime behavior change.

Gate: full pytest suite (13443 passed / 0 failed), Codex regression gate SAFE TO SHIP, ruff clean.

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

Labels

size:M Medium PR (≤10 files, ≤250 LOC)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants