Skip to content

fix(tests): make isolated_home actually isolate, and apply it to every test - #64

Open
Hotragn wants to merge 2 commits into
AlmanacCode:mainfrom
Hotragn:fix/sandbox-user-home-everywhere
Open

fix(tests): make isolated_home actually isolate, and apply it to every test#64
Hotragn wants to merge 2 commits into
AlmanacCode:mainfrom
Hotragn:fix/sandbox-user-home-everywhere

Conversation

@Hotragn

@Hotragn Hotragn commented Aug 18, 2026

Copy link
Copy Markdown

Summary

tests/conftest.py's isolated_home fixture does not deliver the guarantee CONTRIBUTING.md makes for it. Two independent gaps, one commit each:

  1. It does not isolate on Windows. It patches $HOME, but ntpath.expanduser reads %USERPROFILE% and ignores $HOME, so all 390 call sites ran against the real ~/.codealmanac.
  2. It is not applied everywhere. 45 tests build an app or invoke the CLI without requesting it and without an explicit database_path, so they hit real global state on every platform, macOS and Linux included.

Net effect of (2): uv run pytest is not repeatable. It passes once, then fails.

Why

CONTRIBUTING.md: "Tests that touch user state should use the isolated_home fixture so they write under a temp ~/.codealmanac/, not the real registry."

Gap 1 — the fixture is a no-op on Windows. core.paths.home_dir() resolves the home through Path.home():

>>> os.environ["HOME"] = "C:/tmp/fake"
>>> pathlib.Path.home()
WindowsPath('C:/Users/hotra')          # real home, not the sandbox

After one uv run pytest on a clean Windows checkout:

$ sqlite3 ~/.codealmanac/codealmanac.db 'select repository_id, name, root_path from repositories'
repo_86e5aa4de43ad419|repo|C:/Users/hotra/AppData/Local/Temp/pytest-of-hotra/
pytest-391/test_cli_validate_returns_nonz0/repo

A real registry row pointing at a pytest temp directory, plus a real ~/.codealmanac/repos/<id>/index.db. Since "registry entries are never auto-dropped", it persists.

Gap 2 — the suite is not repeatable. repositories.name is NOT NULL UNIQUE, and RepositoryStore.remember upserts ON CONFLICT(repository_id) only — nothing handles a name conflict. So the second run registers a fresh temp path under the already-taken name "repo". Reproduced on main from a clean ~/.codealmanac:

uv run pytest tests/test_validate.py -q   # 7 passed
uv run pytest tests/test_validate.py -q   # 1 failed, 6 passed
tests/test_validate.py::test_cli_validate_returns_nonzero_for_issues
IntegrityError: UNIQUE constraint failed: repositories.name

CI never sees this because every job starts from a fresh container, so there is no second run. A developer running the suite twice hits it, and the only remedy is deleting ~/.codealmanac — which also destroys their real registry.

Verification

# Windows 11, Python 3.13 (uv-managed), uv 0.12.0

# before, on main
rm -rf ~/.codealmanac && uv run pytest -q    # 13 failed, 550 passed, 1 skipped
ls ~/.codealmanac                            # codealmanac.db  repos/   <-- real state written

# after
rm -rf ~/.codealmanac && uv run pytest -q    # 11 failed, 552 passed, 1 skipped
ls ~/.codealmanac                            # No such file or directory

# repeatability
rm -rf ~/.codealmanac
uv run pytest tests/test_validate.py -q      # 7 passed
uv run pytest tests/test_validate.py -q      # 7 passed

uv run ruff check .                          # All checks passed!
git diff --check                             # clean
uv build --out-dir dist                      # ok

Suite totals are identical across the autouse commit (11 failed / 552 passed both sides), so nothing depended on seeing the real home.

The two newly passing tests were fixed by the %USERPROFILE% commit: test_default_user_state_paths_are_product_specific and test_cli_setup_and_uninstall_codex_instructions. Both asserted correct behaviour and failed only because the fixture leaked.

The remaining 11 failures are pre-existing and unrelated — 5 macOS launchd tests plus non-portable fixtures (test_tagging.py writes \r\n through translating text mode; test_transcript_discovery.py interpolates a Windows path into JSON). Happy to send those separately; I kept them out to hold this diff to the isolation invariant.

Docs and wiki

  • Not applicable — test-harness only, no user-facing behaviour change.

Notes for reviewers

  • Two commits, deliberately reviewable apart. The %USERPROFILE% fix is Windows-only; the autouse change affects all platforms. They touch the same six lines, so splitting them into separate PRs would just create a conflict — but say the word and I will drop either commit.
  • No src/ change. Path.home() reading %USERPROFILE% is correct product behaviour on Windows; the bug is that the fixture never overrode it. I deliberately did not add a CODEALMANAC_HOME override, since that adds public env surface to fix a test-only problem.
  • The raise is the durable half of commit 1. Patching %USERPROFILE% fixes today's platforms; asserting Path.home() == home means any future platform where that env set is insufficient fails loudly at setup instead of silently using the real home. A silent no-op is what let this survive.
  • autouse over 45 signatures. Adding the fixture to each call site is a large mechanical diff that fixes today's tests and leaves the 46th to reintroduce the bug. autouse closes the class. A fixture can be autouse and still be requested by name, so the 390 explicit requests resolve to the same object and no assertion changes. Precedent: disable_external_telemetry_during_tests in this same file is already autouse for the same reason.
  • HOMEDRIVE/HOMEPATH left alone on purposentpath.expanduser consults %USERPROFILE% first, so they are unreachable here, and clearing them would affect unrelated subprocess behaviour.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…he real registry

`isolated_home` patched only $HOME. `core.paths.home_dir()` resolves the user
home through `Path.home()`, and on Windows `ntpath.expanduser` reads
%USERPROFILE% and ignores $HOME. Every test using the fixture therefore ran
against the developer's real `~/.codealmanac` on Windows.

`uv run pytest` on a clean Windows checkout created a real
`~/.codealmanac/codealmanac.db` holding a repositories row pointing at a pytest
temp directory, plus `~/.codealmanac/repos/<id>/index.db`. Because registry
entries are never auto-dropped, that row survives the run and every later one.

The fixture now patches %USERPROFILE% as well, and fails loudly if
`Path.home()` does not land inside the sandbox, so an insufficient patch set
can never silently degrade into using the real home again.

This also fixes two pre-existing Windows failures that were masked by the
escape: `test_default_user_state_paths_are_product_specific` and
`test_cli_setup_and_uninstall_codex_instructions`.
… ask for it

45 tests build an app or invoke the CLI without requesting `isolated_home` and
without passing an explicit `database_path`, so `AppConfig()` falls back to
`~/.codealmanac` and they operate on real global user state. This is not
Windows-specific: `Path.home()` is the developer's real home on every platform.

The visible consequence is that `uv run pytest` is not repeatable.
`repositories.name` is `NOT NULL UNIQUE`, and `RepositoryStore.remember` upserts
`ON CONFLICT(repository_id)` only, so a second run registers a fresh temp path
under the already-taken name "repo" and raises:

    tests/test_validate.py::test_cli_validate_returns_nonzero_for_issues
    IntegrityError: UNIQUE constraint failed: repositories.name

Reproduced on main, from a clean `~/.codealmanac`:

    uv run pytest tests/test_validate.py -q   # 7 passed
    uv run pytest tests/test_validate.py -q   # 1 failed, 6 passed

CI does not see it because each job starts from a fresh container, so the second
run never happens.

Rather than add the fixture to 45 call sites, `isolated_home` becomes `autouse`.
Every test now gets a sandboxed home under its own `tmp_path`, the 390 existing
explicit requests keep resolving to the same object and are unchanged, and no
future test can opt out of the invariant by forgetting an argument. This mirrors
the existing autouse `disable_external_telemetry_during_tests` fixture.

Full suite is unchanged at 11 failed / 552 passed, and the same two runs above
now pass twice with `~/.codealmanac` never created.

Stacked on the %USERPROFILE% fix, which this needs in order to sandbox anything
at all on Windows.
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