From 93ec28097c66db7af209e4ff3a7666b7ad7a8dad Mon Sep 17 00:00:00 2001 From: Ed_ Date: Tue, 9 Jun 2026 20:36:41 -0400 Subject: [PATCH] =?UTF-8?q?docs(styleguide):=20add=20workspace=5Fpaths.md?= =?UTF-8?q?=20=E2=80=94=20hard=20rule=20for=20test=20workspace=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- conductor/code_styleguides/workspace_paths.md | 148 ++++++++++++++++++ conductor/workflow.md | 2 +- 2 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 conductor/code_styleguides/workspace_paths.md diff --git a/conductor/code_styleguides/workspace_paths.md b/conductor/code_styleguides/workspace_paths.md new file mode 100644 index 00000000..99e4e9e7 --- /dev/null +++ b/conductor/code_styleguides/workspace_paths.md @@ -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\\AppData\Local\Temp\pytest-of-\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-/...` 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-/...` — 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_/` — 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 diff --git a/conductor/workflow.md b/conductor/workflow.md index 6243b0be..0b8a6771 100644 --- a/conductor/workflow.md +++ b/conductor/workflow.md @@ -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/_")` 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