Skip to content

tests: isolate persistence directories to prevent writes to user home#699

Open
cbagwell wants to merge 3 commits intoOpenHands:mainfrom
cbagwell:fix/test-home-config-writes
Open

tests: isolate persistence directories to prevent writes to user home#699
cbagwell wants to merge 3 commits intoOpenHands:mainfrom
cbagwell:fix/test-home-config-writes

Conversation

@cbagwell
Copy link
Copy Markdown
Contributor

Tests were writing to ~/.openhands/agent_settings.json and other files, which pollutes the user's home directory and causes test failures when running in environments with different configurations.

Tests that did not explicitly use the mock_locations fixture would call location functions that default to ~/.openhands/. This is especially problematic when running tests via 'uv run pytest' instead of 'make test', as agents often do, since uv may use a different Python environment that does not have the same setup as the Makefile.

Add an autouse fixture in conftest.py that runs before every test and:

  • Sets OPENHANDS_PERSISTENCE_DIR to an isolated temp directory
  • Sets OPENHANDS_CONVERSATIONS_DIR to an isolated temp directory
  • Sets PERSISTENCE_DIR (legacy) to an isolated temp directory
  • Mocks os.path.expanduser('~') to return the isolated temp home

This ensures ALL tests are isolated from the user's home directory.

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

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

🔴 Needs improvement - Critical scope issue with test isolation

[CRITICAL ISSUES]
The autouse fixture in tests/conftest.py only applies to tests within the tests/ directory hierarchy. It does NOT apply to tui_e2e/ tests because pytest's conftest discovery is directory-scoped. Removing the HOME override from the test-binary Makefile target leaves tui_e2e/ tests completely unprotected - they will write to the user's real ~/.openhands/ directory.

[RISK ASSESSMENT]
⚠️ Risk Assessment: 🔴 HIGH

This PR introduces a critical regression: tui_e2e/ tests lose their isolation and will pollute the user's home directory, which is exactly what this PR claims to fix. The implementation is only halfway complete.

Recommendation: Create a root-level conftest.py with the autouse fixture so it applies to ALL test directories (both tests/ and tui_e2e/), or create a separate tui_e2e/conftest.py with the same fixture.

VERDICT:
Needs rework - Critical scope issue must be fixed before merging

KEY INSIGHT:
Pytest's conftest.py files are directory-scoped; a fixture in tests/conftest.py does not magically apply to sibling directories like tui_e2e/.

Comment thread Makefile
Comment thread tests/conftest.py Outdated
Tests were writing to ~/.openhands/agent_settings.json and other files,
which pollutes the user's home directory and causes test failures when
running in environments with different configurations.

Tests that did not explicitly use the mock_locations fixture would call
location functions that default to ~/.openhands/. This is especially
problematic when running tests via 'uv run pytest' instead of 'make test',
as agents often do, since uv may use a different Python environment that
does not have the same setup as the Makefile.

Add an autouse fixture in conftest.py that runs before every test and:
- Sets OPENHANDS_PERSISTENCE_DIR to an isolated temp directory
- Sets OPENHANDS_CONVERSATIONS_DIR to an isolated temp directory
- Sets PERSISTENCE_DIR (legacy) to an isolated temp directory
- Mocks os.path.expanduser('~') to return the isolated temp home

This ensures ALL tests are isolated from the user's home directory.
@cbagwell cbagwell force-pushed the fix/test-home-config-writes branch from 3e49b19 to 04fd940 Compare April 26, 2026 03:15
@malhotra5 malhotra5 requested a review from all-hands-bot April 29, 2026 15:04
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

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

🟡 Acceptable - Core implementation is sound. The autouse fixture now correctly applies to all test directories including tui_e2e/, addressing the critical scope issue from the previous review.

[RISK ASSESSMENT]
⚠️ Risk Assessment: 🟡 MEDIUM

This autouse fixture affects every test in the repository. While the implementation is sound and the previous review cycle addressed the critical scope issue, any bugs in the mocking logic could cause widespread test failures or silently allow tests to write to the real home directory. The logic is straightforward and uses standard pytest patterns, keeping risk at medium rather than high.

VERDICT:
Worth merging: Core logic is sound, minor simplification suggested

KEY INSIGHT:
Moving the autouse fixture to root-level conftest.py ensures complete test isolation across all test directories, solving the critical gap where tui_e2e/ tests were previously unprotected.


Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
  2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Comment thread conftest.py Outdated
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.

3 participants