git:20260812.6377c5a to git:20260821.3dfe12f

# tests — suite conventions + CI/lint gotchas

> ↑ [root](../CLAUDE.md)

Conventions for the pytest suite under `tests/`: where the test seams sit, how to keep cross-platform defenses honest, and the CI/lint traps. Root holds the two principles these build on ("tests pin contracts, not implementation"; "tests prefer real code paths, stub only I/O"). Engine internals live in [core](../src/grove/core/CLAUDE.md).

## Test seams

**Patch the public module surface, never a private symbol.** `tests/conftest.py` monkey-patches `grove.core.tmux.create_session` and friends directly — no Protocol abstraction. Rename a public function and the test breaks loudly; that is the design. A test that patches a private symbol turns that path into an implicit contract: move it and the patch silently no-ops while the test still passes.

**Sandbox Grove paths by patching the Grove function, not the env var.** `monkeypatch.setattr("grove.core.paths.user_schema_path", lambda: tmp_path / "...")` is the right sandbox — deterministic on every OS, matching `tests/conftest.py::tmp_state_dir` (which already redirects `user_state_path` / `user_config_path` / `user_schema_path`). Do **not** `monkeypatch.setenv("LOCALAPPDATA", ...)`: recent `platformdirs` (≥4.x) resolves the user dir via `SHGetKnownFolderPath` through `ctypes` on Windows and only falls back to env vars when `ctypes` is unavailable, so `setenv` is a no-op there — the write lands outside `tmp_path` and `rglob` returns empty. Same rule for redirecting `~/.grove`, `~/.config/grove`, or any `platformdirs.user_*_dir` path.

**Every host path a test can write must be redirected by an AUTOUSE fixture, not an opt-in one.** `tmp_state_dir` is opt-in, so it only protects tests that already know to ask — and `init_log_path` was reachable without asking: `create()`/`resume()`/`respawn()` resolve it unconditionally whenever `init_script.enabled`, so the suite wrote its own fixture text into the developer's live `~/.local/state/grove/logs/`. **Test output masquerading as production diagnostics is the actual cost, not the disk space** — those files surface as the first "evidence" when someone debugs a real user-reported init failure. Hence the autouse `init_logs` (still requestable by name for the tests asserting on the log) alongside autouse `_isolated_agent_hook_paths`.

**`tmp_state_dir` does not cover every `platformdirs` path — it patches the FILE accessors, and some paths resolve `user_state_dir` themselves.** `paths.agent_workspace_config_dir` is one: `AgentSharePlan.seed()` creates it (and copies `.claude.json` into it) on every containerized provision, so any test driving a real provision wrote one directory per run into the developer's live `~/.local/state/grove/agent-config/`, opt-in isolation or not. It is now redirected by autouse `_isolated_agent_hook_paths` with the rest. The generalizable check when adding a path helper: does it go through an accessor a fixture already patches, or does it call `platformdirs` directly?

**`paths.ensure_dir` is a chokepoint for Grove's own writes but NOT for the fakes' — audit at the `pathlib` level.** Every host write in `src/grove` is preceded by `paths.ensure_dir(parent)`, so spying it finds engine leaks; it misses `FakeTmux.run_init_script`, which is handed a real path by the manager and `mkdir`s it itself. To prove isolation, patch `Path.mkdir`/`write_text`/`open`/`os.replace` session-wide and record any target under `$HOME` — that catches test-side writers too. The proof a redirect works is a **before/after file count on the real directory**, not a green suite: these writes never fail an assertion.

**Patching `<your_module>.subprocess.run` mutates the ONE shared `subprocess` module process-wide, not a copy scoped to your module.** `import subprocess; subprocess.run(...)` binds the name `subprocess` to the same global module object everywhere it's imported — `monkeypatch.setattr("grove.core.agents.onboarding.subprocess.run", fake)` therefore also intercepts `grove.core.git`'s own `subprocess.run` calls for the rest of that test (e.g. a CLI test that calls `detect_root` and then a subprocess-shelling engine call in the same invocation). Either (a) make the fake inspect `argv[0]` and pass through anything it doesn't care about to the real `subprocess.run` (captured before patching), or (b) patch the whole consuming function instead (`grove.core.agents.onboarding.register_mcp`) the same way `fake_tmux` replaces whole `grove.core.tmux` functions — see `tests/cli/test_onboarding_commands.py::fake_mcp_add` for the passthrough pattern. The passthrough form is mandatory whenever one test file must fake a subprocess boundary (e.g. `docker`) AND still drive real `git` in the same invocation: patching the whole consuming function isn't an option when the test wants the real git side effect.

### The false-green double — three shapes of the same bug

A double that is *incomplete* is worse than no double: the suite stays green while describing a program production does not run.

**A fake that SUBCLASSES a real boundary is only as offline as the methods it overrides.** `FakeCli` extends `DevcontainerCli` deliberately (so signature drift breaks loudly), but overriding only `probe`/`read_configuration`/`up` meant that the moment production called `exec` from the provisioner, every container test spawned a REAL `devcontainer exec` — silently, still green — and a real `docker build` cached a static tmux in the developer's live `~/.local/state/grove`. **Neither existing offline test could see it: one never creates a workspace and the other falls back to the host at arm 4, so both stop short of a real provision** — which needs the fake CLI + fake preflight injected, i.e. exactly the DI those tests deliberately avoid. `test_offline_guarantee.py` now drives the provision arm too, and asserts `state.container.provisioned` first so it cannot pass by never getting there. Two habits fall out: when adding a production call to an inherited boundary, check the fake overrides it; and when a fixture claims "no test needs X", the assertion of that claim must reach every arm that can call X, not just the arm that made the claim necessary.

**A double that never changes what the NEXT read reports is the same bug in its most invisible form.** `FakeCli.exec` scripted a fixed answer and the docker double answered a fixed session list — so a test could "start" an agent and every later read still described the world before it. That specifically hid the measured fact that `tmux new-session -d` exits 0 for a command that cannot run, so a failed start looked identical to a good one. The fix is for the start double to REGISTER what it was asked to start, with an explicit `starts=False` for the real failure. **When one double's action is another double's input, the two have to be wired together or neither can be wrong** — and the tell is that no assertion in the module reads a value the action produced.

**A monkeypatch stops at the process boundary, so a test that spawns a DAEMON must pin its config instead.** `tests/client/test_client_http.py` spawns a real `grove daemon serve` subprocess, which loads no conftest and honors no autouse fixture — so `_offline_container_runtime` never reached it, and once containers became the default runtime every workspace that file created was a REAL devcontainer (dozens of leaked `vsc-<title>-…` containers on the developer host; the file went 482s → 22.65s once pinned). **`test_offline_guarantee.py` is structurally unable to catch this**: its spy patches `subprocess` in the pytest process, so the argv it records is `grove daemon serve`, never the `devcontainer up` the grandchild runs. Config is the ONLY lever that crosses a process boundary — such a test pins `container.enabled=false` in the config the subprocess reads, which gates the create path and `ProjectInfra.registration_hook` together. It survives because **nothing fails when a unit test does real work — it just gets slower**, and a flipped default silently re-arms every test that never pinned it. The diagnostic to reuse: this surfaces as `TransportError` on `pause`/`kill`, because those ride the client's 30s `_DEFAULT_TIMEOUT_S` while `create`/`resume` get the 1260s `_LIFECYCLE_TIMEOUT_S` — **a verb-dependent timeout pattern means slowness, not a broken route**, and the fast verbs are the ones that report it.

**Hide EVERY system terminfo tree before testing a terminfo bundle, or the test passes while the bundle is broken.** ncurses searches `TERMINFO_DIRS` *before* its compiled-in fallbacks, not instead of them, so an image with its own database silently answers for a missing bundle entry — and images acquire one by accident (installing python3 on Alpine drops a real `/etc/terminfo`). Mask `/usr/share/terminfo`, `/etc/terminfo`, `/lib/terminfo`, `/usr/lib/terminfo` and `/usr/local/share/terminfo` (an empty read-only bind over each) and assert the mask took effect — a probe that only counts `ls` output lines reports "2" for two empty directories. Same false-green shape one level down: the double is the environment rather than a fake object.

**A redirected host path is about DETERMINISM as much as isolation.** The tmux payload cache had to be pointed into `tmp_path` not merely so tests stop writing to `~/.local/state/grove`, but because a developer who HAS built the bundle would otherwise get different answers from one who has not — a test that reads a real host artifact is a test whose result depends on the machine.

**No test hits the network — the release check is neutered by an autouse fixture.** `tests/conftest.py::_offline_release_check` patches `grove.core.release.fetch_latest_release_tag` → `None` for every test, so a default `ReleaseChecker` (the one the daemon/TUI build with no injected `fetcher`) reports "unknown" instead of GETting GitHub. Same discipline as the fake tmux/git seams. A test that wants a specific verdict injects a `fetcher` directly into `ReleaseChecker` (bypasses the seam) — see `test_release.py`, the daemon whoami tests, and the TUI worker test.

**Deterministic OTel ids do not prove replay idempotency.** Historical export

tests script Langfuse's trace-read boundary and prove completed checkpoints

skip, partial traces send only missing observation ids, ambiguous failures stay

uncheckpointed, and a changed payload under an accepted id conflicts rather than

updates. The ledger fixture is a separate temporary file from the disposable

usage cache, includes the destination in its key, and test manifests contain no

credentials. Pin a regression where the same trace id is completed for one

destination and remains pending for another.

Also pin asynchronous acceptance: a trace missing from the immediate read-back

is `submitted`, an immediate rerun emits nothing, and an expired-grace retry

sends only observation ids the remote still lacks.

**A DECLARED env var (`x-env-var`) is a config CASCADE LAYER, so the developer's own shell can fail tests that pass in CI, and the failure names the assertion rather than the environment.** `GROVE_GITEA_BASE_URL` exported in a shell beats the config a ticket test writes to disk (the declared layer sits above the project file — see [core](../src/grove/core/CLAUDE.md)), so `tests/cli/test_tickets_commands.py` and `tests/daemon/test_tickets.py` fail with *"no enabled ticket provider owns https://gitea.example.com/…"* on a host that actually uses Gitea, and pass everywhere else. Nothing is neutralized globally because the layer is a feature. Diagnose by re-running under `env -u GROVE_<…>`; the tell is that the DEBUG log line reads `ignoring non-config env var GROVE_GITEA_BASE_URL` — which is the *other* env mechanism disclaiming it, not evidence that nothing consumed it. Generalizable: when a test's config assertion loses to a value nobody in the test wrote, enumerate the schema's `x-env-var` names before suspecting the test.

**The mewbo adapter is a SECOND network reacher, and it is not neutralized globally.** Its `list_sessions`/`discover_all` are REST calls whose config default is `http://127.0.0.1:5125` — reachable on a developer box actually running Mewbo — and they sit inside `all_adapters()`, so *any* test touching `SessionExplorer.list` or `SessionCatalog.scan` reaches for a socket. Unlike the release check it has no autouse fixture, because the adapter's own tests inject an `httpx.MockTransport` and a class-level autouse patch would break them. Neutralize per test-module instead (patch `MewboAdapter.list_sessions`/`discover_all`), as `tests/daemon/test_sessions_endpoints.py` does. Same reasoning applies to `grove.core.process.list_agent_runtimes`: a real `/proc` walk makes a liveness assertion depend on what the developer happens to be running, so patch it and build the runtimes the test means to assert on.

## Cross-platform test gating

**CI is Linux-only (`ci.yml` runs `ubuntu-latest`, no OS matrix).** tmux is a hard runtime dependency (WSL2 on Windows) and native packaging was retired, so a `[ubuntu, macos, windows]` matrix burned minutes for no signal. The Windows/macOS defensive *code* stays (POSIX import guards, `os.replace`, `platformdirs` patching, the `bash` probe) — it is cheap correctness and dropping CI is not a license to write Linux-only code. But those defenses are **CI-unverified**: a Windows/macOS regression won't be caught automatically, so reason about path/subprocess/POSIX-module code by hand when you touch it.

**Compare resolved `Path` objects across tools, never raw strings.** Git CLI output emits `/`, Python `str(Path)` emits `\` on Windows; Linux makes the asymmetry invisible, so the bug only surfaces on Windows CI. Assert `Path(p).resolve() in {Path(q).resolve() for q in git_out}`. Same for any cross-tool path comparison (lazygit, gh, etc.).

**Gate shell-exec tests with an output probe, not `shutil.which("bash")`.** On Windows GitHub runners `bash.exe` resolves to the WSL launcher; with no installed distribution every `bash -c …` prints "Windows Subsystem for Linux has no installed distributions." and exits 1 — yet `which` still passes. Run a real probe (`bash -c "echo grove-shell-probe"`) and verify the output before skipping. (`git` escapes this only by accident: Windows runners ship real Git.)

**`uv sync --all-groups` fetches a group's direct-URL requirement even when you exclude that group, so name the group you want instead.** The `lint` and `test` jobs synced every group, which reached for the docs theme, pinned to a **GitHub release asset** with no index mirror to retry against. One `http2 ... refused stream` there failed the whole install over a package `make lint` never runs. **`--no-group docs` did NOT suppress the fetch** — that was the first fix and it failed identically. The evidence isolating the flag: the `Docs` workflow (`uv sync --group docs`) fetched the same wheel successfully in the same minutes, and the `test` job's install passed while `lint`'s failed on the identical command, so the fetch is flaky and `--all-groups` is only what makes it reachable. Both jobs now run `uv sync --frozen --group dev`: naming the group avoids the `--all-groups` path, and `--frozen` installs from `uv.lock` with no resolution step, so no URL metadata is ever needed. **Reproduce a CI install failure with the runner's own uv version** (`setup-uv` logs it) in a fresh clone with `UV_CACHE_DIR` pointed at an empty dir. A local checkout with a warm venv silently *removes* the excluded packages and exits 0, which reads as a pass and proves nothing. Pillow lives in `dev`, not `docs`, because `tests/tools/screenshots/` imports it: placement follows *which job needs the package*, not which concern the module reads as.

## Lint

**NEVER write a Python numeric separator inside a SQL string — `duration_ms=90_000` is a Python habit that SQL does not share, and the failure is environment-dependent.** SQLite accepts underscores in numeric literals only from **3.46.0**; this host ships 3.50.4 and CI's runner is older, so three tests passed locally and failed on CI with `sqlite3.OperationalError: unrecognized token: "90_000"`. **A local run is structurally unable to catch this**, which is the whole reason it is written down: the separator is invisible to `ruff`, to `mypy` and to review, and it reads as correct Python because inside the quotes it is not Python at all. Python-side comparisons (`... .fetchone()[0] == 9_000`) are fine — the rule is about the string.

**Run the full `make lint` before pushing, never just `ruff check`.** `make lint` runs `ruff format --check` too. A `ruff check --fix` that reflows code can still leave a format-check failure, so a green `ruff check` is not enough.

**A method named after a builtin shadows that builtin in every LATER annotation in the same class.** `WorkspaceManager.list` makes `-> tuple[bool, list[str]]` on any method defined after it fail mypy with *"Function grove.core.manager.WorkspaceManager.list is not valid as a type"* — class-body scope wins, and `from __future__ import annotations` does not change it. A local `x: list[str] = []` *inside* a method body is fine (function scope never consults the class), which is why the trap only ever appears in signatures. Reach for `collections.abc.Sequence` (or another container type) rather than renaming a published method.

**Keep `import-linter` honest with `include_external_packages = true`.** Without it, contracts that forbid third-party modules (textual, rich, typer, click) silently pass.

## Session lessons

- **CLI output tests must scan for their marker line, never index `splitlines()[0]`.** A command that "falls back loudly" (prints a warning line before its normal output) and a test that parses only the first line are incompatible conventions — the warning becomes line 0 and silently breaks the assertion. Scan the full output for the marker instead of assuming a fixed line position.

- **Absence assertions must match the template markers they own, not every occurrence of a character in expanded output.** Rendered environment values can legitimately contain syntax-like characters (for example, a package path containing `@`), so rejecting any such character turns a valid host environment into a false failure. Scope the assertion to the unresolved placeholder grammar (`@NAME@` here), and keep the production input visible in the test.

- Engine-side gotchas that *manifest* as test traps live with the engine: e.g. ROOT-placement tests must spy that `worktree_remove` / `branch_delete` are never *called*, not merely that the repo survives (git itself refuses both, so a survival-only assertion passes with the gate missing) — see [core](../src/grove/core/CLAUDE.md).

- **Live-driving a real `claude` in tmux (manual e2e verification) has three traps**, each of which costs a failed run: (1) the boot marker must be claude's own chrome (`"Claude Code v"` / `"? for shortcuts"`) — the zsh prompt glyph is ALSO `❯`, so a prompt-glyph check fires at the shell stage; (2) an Enter sent while the TUI is still painting is **swallowed** — the text stages in the composer unsubmitted, so verify submission by re-capturing the pane ~4s later and re-send Enter if the text is still staged (this race also exists for real `send_message` steering into a just-booted agent); (3) the trust-dialog copy is "Quick safety check: … Is this a project you created or one you trust?" / "Yes, I trust this folder" — match on `trust this folder`, not "trust the files". Run the engine **in-process from the worktree** (`uv run python`), never through the installed `grove` (it serves the last-installed checkout, not your branch).