Skip to content

test(telegram): isolate HOME so the pid test can't clobber a live adapter - #2895

Draft
itomek wants to merge 2 commits into
mainfrom
tmi/fix-2892-telegram-pid-test-isolation
Draft

test(telegram): isolate HOME so the pid test can't clobber a live adapter#2895
itomek wants to merge 2 commits into
mainfrom
tmi/fix-2892-telegram-pid-test-isolation

Conversation

@itomek

@itomek itomek commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Running the unit suite used to silently destroy ~/.gaia/telegram.pid on the developer's own machine. test_background_writes_pid called the real adapter, let it write the real pid file, then os.remove()d it unconditionally in a finally block — so if a gaia telegram start adapter happened to be running, gaia telegram stop/status quietly stopped working and the only symptom was a green test run. The test now writes into a tmp_path sandbox, and a live adapter's pid file survives the suite untouched.

The assertion deliberately targets tmp_path/.gaia/telegram.pid rather than expanduser("~"): if the isolation is ever removed, the test fails loudly instead of passing while writing to the real home.

Closes #2892

Sibling of #2891 (same leak class, in the gaia init tests) — separate file, no dependency, so this is not stacked on it.

Evidence — the same test in three variants, each run with a sentinel value planted in a real ~/.gaia/telegram.pid:

variant pytest real ~/.gaia/telegram.pid
before (main) PASS destroyed
after (this PR) PASS survives, untouched
after, isolation removed FAIL guard fires

The middle row is the fix; the bottom row is why the fix can't silently rot.

Test plan

  • pytest tests/unit/test_telegram_background.py passes
  • With echo SENTINEL > ~/.gaia/telegram.pid planted first, the file still reads SENTINEL after that run
  • pytest tests/unit/ — 9186 passed; the 11 failures are pre-existing (identical set on main with this change stashed: CLI-binary-on-path, hub installer, wheel-packaging, and the known missing-API-key case)
  • python util/lint.py --all — no findings against this file; black/isort clean
Commands used for the evidence table
# per variant: plant sentinel, run, check survival
echo "SENTINEL" > ~/.gaia/telegram.pid
python -m pytest tests/unit/test_telegram_background.py -q
cat ~/.gaia/telegram.pid   # SENTINEL = survived, absent = destroyed

The "isolation removed" variant deletes the monkeypatch.setattr(os.path, "expanduser", ...) line and expects a FAILING test.

itomek added 2 commits August 10, 2026 15:54
…/telegram.pid

test_background_writes_pid wrote and unconditionally os.remove()'d the
developer's real ~/.gaia/telegram.pid. Running the unit suite while a
live 'gaia telegram start' adapter was up destroyed its pid file,
breaking 'gaia telegram stop'/'status'.

Redirect '~' at tmp_path via os.path.expanduser, matching the isolation
in test_mcp_init.py and test_cli_config.py, and drop the destructive
finally cleanup.

Fixes #2892
…ently regress

Asserting via expanduser("~") meant removing the monkeypatch would leave a
green test that writes to the developer's real home again. Assert against
tmp_path/.gaia/telegram.pid instead, so dropping the isolation fails the test.
@github-actions github-actions Bot added the tests Test changes label Aug 10, 2026
@itomek itomek added the ready_for_ci Run CI workflows on draft PR without requesting review label Aug 10, 2026
@itomek itomek closed this Aug 10, 2026
@itomek itomek reopened this Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

This is a clean, well-motivated test hardening. test_background_writes_pid used to write and delete the real ~/.gaia/telegram.pid, which could clobber the pid file of a live gaia telegram adapter on the developer's machine. The PR isolates HOME so the test writes into tmp_path instead, and — importantly — asserts on that sandbox path rather than expanduser("~"), so if the isolation is ever removed the test fails loudly instead of silently passing while stomping the real file. The comments explain the "why" precisely, and the assertions match how the adapter actually resolves the path.

Real-world evidence

N/A — test-only change (single file under tests/unit/), no user-visible surface. Verdict rests on static review, cross-checked against the adapter's actual pid-path logic.

🔍 Technical details

Scope: tests/unit/test_telegram_background.py only.

Correctness — verified against the adapter. The adapter builds its pid path as os.path.join(os.path.expanduser("~/.gaia"), "telegram.pid") (src/gaia/messaging/telegram.py:180-182). The test's fake_expanduser maps ~/.gaiatmp_path/.gaia, so the adapter writes tmp_path/.gaia/telegram.pid, and the assertion targets exactly that path. monkeypatch.setattr(os.path, "expanduser", ...) patches the shared os.path module object, so the adapter's direct os.path.expanduser call is covered. Correct.

Good call replacing the old cleanup. The previous try/finally: os.remove(pid_path) on the real path was the fragile bit — it both risked deleting a live adapter's pid and left the test green if the write silently landed somewhere unexpected. The new form (isolate + assert on the sandbox path) removes both problems.

Minor, non-blocking observations (no change requested):

  • fake_expanduser("~") returns os.path.join(tmp_path, ""), i.e. tmp_path with a trailing separator. Harmless and the bare ~ branch isn't exercised by this adapter path, but if you ever wanted it exact, path == "~" could return str(tmp_path). Not worth a change.
  • The adapter also writes telegram.log into the same dir (telegram.py:194); it now correctly lands in tmp_path/.gaia too, so no stray log file leaks either — a nice side benefit of the HOME isolation.

Strengths:

  • Fixes a genuine test-isolation bug (clobbering a real user's telegram.pid) rather than papering over it.
  • The "assert on the sandbox path so removing the isolation fails loudly" pattern is exactly the fail-loudly discipline GAIA asks for — the test can't silently regress.
  • Comments state the invariant (why HOME is isolated, why the assertion targets tmp_path) in one line each, matching the repo's comment style.

@itomek itomek self-assigned this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready_for_ci Run CI workflows on draft PR without requesting review tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: test_telegram_background writes and deletes the developer's real ~/.gaia/telegram.pid

1 participant