docs(styleguide): add workspace_paths.md — hard rule for test workspace paths
This commit is contained in:
@@ -0,0 +1,148 @@
|
||||
# Test Workspace Paths — Hard Rule
|
||||
|
||||
## TL;DR
|
||||
|
||||
Test workspaces live in the project tree under `tests/artifacts/`. Conftest creates them. No env vars. No CLI args. No `tmp_path_factory`. No `%TEMP%`. No runner changes. **The user must be able to find every test workspace by looking in `tests/artifacts/`.**
|
||||
|
||||
## The Rule
|
||||
|
||||
When creating a test workspace, fixture, or scratch directory for any test infrastructure:
|
||||
|
||||
```python
|
||||
# CORRECT — conftest creates the path
|
||||
from datetime import datetime
|
||||
_RUN_ID = datetime.now().strftime("%Y%m%d_%H%M%S")
|
||||
_RUN_WORKSPACE = Path(f"tests/artifacts/live_gui_workspace_{_RUN_ID}")
|
||||
|
||||
@pytest.fixture(scope="session")
|
||||
def live_gui(request):
|
||||
temp_workspace = _RUN_WORKSPACE
|
||||
...
|
||||
```
|
||||
|
||||
```python
|
||||
# WRONG — env vars
|
||||
import os
|
||||
WORKSPACE = os.environ.get("LIVE_GUI_WORKSPACE", "tests/artifacts/live_gui_workspace")
|
||||
|
||||
# WRONG — CLI args
|
||||
def pytest_addoption(parser):
|
||||
parser.addoption("--workspace", action="store", default="tests/artifacts/live_gui_workspace")
|
||||
|
||||
# WRONG — tmp_path_factory (lives in %TEMP%, not in project tree)
|
||||
def live_gui(request, tmp_path_factory):
|
||||
temp_workspace = tmp_path_factory.mktemp("live_gui_workspace")
|
||||
# Creates: C:\Users\<user>\AppData\Local\Temp\pytest-of-<user>\pytest-N\live_gui_workspace0
|
||||
# User CANNOT FIND THIS from the project tree.
|
||||
```
|
||||
|
||||
## Why This Rule Exists
|
||||
|
||||
This rule was added 2026-06-09 after a 4-day agent churn on workspace paths. The chain of decisions:
|
||||
|
||||
1. Original conftest: `temp_workspace = Path("tests/artifacts/live_gui_workspace")`. Sims worked. User could find the workspace. **This was correct.**
|
||||
|
||||
2. Phase 3 of test_infrastructure_hardening_20260609: agent changed it to `tmp_path_factory.mktemp("live_gui_workspace")`. The user did not catch this for 2 days. It moved the workspace to `%TEMP%/pytest-of-<user>/...` which:
|
||||
- The user cannot find from the project tree
|
||||
- The sims (which compute `os.path.abspath("tests/artifacts/...")` from the project root) could not find the workspace either
|
||||
- Caused `test_extended_sims.py::test_context_sim_live` to fail with "stale ui - ops disabled" because the sim's project path didn't match the controller's active_project_path
|
||||
- The agent then spent 2 more days trying to fix the sim timing, the MMA state, the RAG state, the watchdog — none of which were the actual cause
|
||||
|
||||
3. The user caught the regression. Their feedback: "we should be using a folder in `./tests/`" — i.e., the project tree, not the system temp dir.
|
||||
|
||||
4. The agent tried `Path("tests/artifacts/live_gui_workspace")` (no timestamp). That solved the sim issue but was per-session, not per-run. Per-test pollution is desirable (it exposes fragility), so per-run isolation is what we want.
|
||||
|
||||
5. The user pushed back on adding CLI args: "have conftest make it, conftest is the right place." The agent then tried env vars as an indirection layer.
|
||||
|
||||
6. The user rejected env vars: "env vars are hidden global state, pass it to conftest directly." Conftest is the source of truth.
|
||||
|
||||
7. Final solution: conftest creates a per-run timestamped folder under `tests/artifacts/`. One source of truth. No indirection. The user must be able to find every test workspace by looking in `tests/artifacts/`.
|
||||
|
||||
## Forbidden Patterns (Hard Bans)
|
||||
|
||||
### 1. `tmp_path_factory` for test infrastructure workspaces
|
||||
|
||||
`tmp_path_factory` is for pytest's own test isolation (e.g., when a unit test needs a temp dir to write a file). It is **NOT** for test infrastructure workspaces (e.g., the `live_gui` subprocess's CWD). Why:
|
||||
|
||||
- `tmp_path_factory` lives in `%TEMP%/pytest-of-<user>/...` — outside the project tree
|
||||
- The user cannot find the workspace by looking in the project tree
|
||||
- Any code that uses `os.path.abspath("tests/artifacts/...")` from the project root cannot find the workspace
|
||||
- The 4 sim tests in `simulation/sim_base.py` are exactly such code
|
||||
|
||||
**Use `tmp_path` or `tmp_path_factory` ONLY for:**
|
||||
- Unit tests that need a temp file/dir
|
||||
- Test data fixtures that don't outlive the test
|
||||
- Any case where the path is consumed only by the test itself, not by a subprocess
|
||||
|
||||
**Do NOT use for:**
|
||||
- The `live_gui` subprocess CWD
|
||||
- Any workspace that a long-running subprocess (GUI, server) operates on
|
||||
- Any path that other code computes via `os.path.abspath("tests/...")` from the project root
|
||||
|
||||
### 2. Environment variables for test paths
|
||||
|
||||
Env vars are hidden global state. The user has explicitly banned them. They are also a host for the "I'll just check the env var" anti-pattern, which is what bad coders do.
|
||||
|
||||
**Do NOT use `os.environ` for:**
|
||||
- Test workspace paths
|
||||
- Test configuration that could be a conftest constant
|
||||
- Anything that the conftest can compute itself
|
||||
|
||||
### 3. CLI args for test paths
|
||||
|
||||
The conftest is the right place. CLI args add a layer of indirection between the runner and the test, and they require the runner to be modified to pass them. The user has explicitly rejected this.
|
||||
|
||||
**Do NOT add `--workspace=PATH` or similar CLI args.** If you need a path, compute it in conftest.
|
||||
|
||||
## The Correct Pattern
|
||||
|
||||
```python
|
||||
# tests/conftest.py
|
||||
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
|
||||
# Module-level constants, computed once at conftest import time.
|
||||
# Per-pytest-invocation isolation: each `uv run pytest` gets a new folder.
|
||||
# Per-test pollution is INTENTIONAL (exposes fragility).
|
||||
_RUN_ID = datetime.now().strftime("%Y%m%d_%H%M%S")
|
||||
_RUN_WORKSPACE = Path(f"tests/artifacts/live_gui_workspace_{_RUN_ID}")
|
||||
|
||||
@pytest.fixture(scope="session")
|
||||
def live_gui(request) -> Generator["_LiveGuiHandle", None, None]:
|
||||
temp_workspace = _RUN_WORKSPACE
|
||||
# ... use temp_workspace
|
||||
```
|
||||
|
||||
## What Lives in `tests/artifacts/`
|
||||
|
||||
Everything test-related that needs to be on disk:
|
||||
|
||||
- `tests/artifacts/live_gui_workspace_<timestamp>/` — per-run live_gui workspace (this rule)
|
||||
- `tests/artifacts/manualslop_layout_default.ini` — read-only default layout
|
||||
- `tests/artifacts/*.log` — test logs
|
||||
- `tests/artifacts/post_*_batch_*.log` — batch run logs
|
||||
|
||||
All of these are gitignored via the existing `tests/artifacts/` entry in `.gitignore`.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
# The workspace must be in the project tree:
|
||||
$ ls tests/artifacts/ | grep live_gui_workspace
|
||||
live_gui_workspace_20260609_201530
|
||||
|
||||
# It must be gitignored:
|
||||
$ git check-ignore tests/artifacts/live_gui_workspace_20260609_201530
|
||||
tests/artifacts/live_gui_workspace_20260609_201530
|
||||
```
|
||||
|
||||
## Audit
|
||||
|
||||
`scripts/check_test_toml_paths.py` already flags `Path("C:/projects/")` and other hardcoded paths. Add a check for `tmp_path_factory.mktemp` and `os.environ.get.*WORKSPACE` in production-style conftest changes. (This is a follow-up task, not a hard requirement.)
|
||||
|
||||
## See Also
|
||||
|
||||
- `conductor/workflow.md` §"Process Anti-Patterns" #9 (this rule, added 2026-06-09)
|
||||
- `conductor/tracks/workspace_path_finalize_20260609/` — the track that established this rule
|
||||
- `docs/reports/rag_test_batch_failure_status_20260609_pm3.md` — the audit findings that led to the rule
|
||||
@@ -464,7 +464,7 @@ For the full rationale on each, see `AGENTS.md` "Process Anti-Patterns." The sum
|
||||
6. **The "I Am Not Going To Attempt Another Fix" Surrender (kill it).** This is correct ONLY if you have already done: read the source, predicted the failure, instrumented state, run once, captured full output. Otherwise you are surrendering too early.
|
||||
7. **The Verbose-Commit-Message Pattern (kill it).** A commit message is 1-3 sentences. If it's longer than 15 lines, it's a report, not a commit message. Save the report for `docs/reports/`.
|
||||
8. **The Isolated-Pass Verification Fallacy (kill it).** A test that passes in isolation but fails in batch is failing. Verify in batch, not isolation, for any test that touches shared subprocess state.
|
||||
|
||||
9. **The Workspace-Path Drift Pattern (kill it, added 2026-06-09).** Test workspaces live in the project tree under `tests/artifacts/`. Conftest creates them. **Never** use `tmp_path_factory.mktemp` for test infrastructure workspaces (it lives in `%TEMP%` and the user cannot find it). **Never** use env vars for test paths (hidden global state). **Never** add CLI args for test paths (conftest is the right place). The pattern: module-level constant in conftest that computes `Path(f"tests/artifacts/<workspace>_<timestamp>")` at import time. See `conductor/code_styleguides/workspace_paths.md` for the full rule and the 4-day agent churn that led to it.
|
||||
---
|
||||
|
||||
## Planning Session Workflow
|
||||
|
||||
Reference in New Issue
Block a user