Private
Public Access
archive: test patrch fixes and sandbox hardening
This commit is contained in:
@@ -0,0 +1,373 @@
|
||||
# Track Specification: Test Sandbox Hardening (2026-06-19)
|
||||
|
||||
## Overview
|
||||
|
||||
This track adds a hard file-I/O sandbox for the test suite so that a misbehaving
|
||||
test (missing fixture, broken monkeypatch, direct `open()` to a hardcoded path)
|
||||
cannot corrupt user-owned files in the project root. The user has lost
|
||||
"important sample data" multiple times over the past month because tests have
|
||||
written to `manual_slop.toml`, `manual_slop_history.toml`, `personas.toml`,
|
||||
`presets.toml`, `tool_presets.toml`, or `credentials.toml` at the top of the
|
||||
repo.
|
||||
|
||||
The fix has 5 parts:
|
||||
|
||||
1. **Eliminate the silent `SLOP_CONFIG` env-var fallback** in `src/paths.py`. Replace
|
||||
it with a module-level override set explicitly by the CLI flag `--config`
|
||||
at the entry point (sloppy.py for production, conftest.py for tests). This
|
||||
is the root-cause fix — without it, every other defense is a band-aid.
|
||||
2. **Add a Python runtime file-I/O guard** (`sys.addaudithook` on `open` writes).
|
||||
Default-on for every pytest invocation.
|
||||
3. **Migrate the test workspace off `tmp_path_factory.mktemp`** (which lives in
|
||||
`%TEMP%`) onto `tests/artifacts/_isolation_workspace_<run_id>/` so the
|
||||
Layer 2 allowlist can be a single rule. Add pytest `--basetemp` to pyproject.toml
|
||||
so pytest's own tmp dirs also live under `./tests/`.
|
||||
4. **Add an OS-level restricted-token wrapper** (Windows-only, opt-in via
|
||||
`scripts/run_tests_sandboxed.ps1`) for users who want defense in depth on
|
||||
top of the Python guard.
|
||||
5. **Extend the static audit** to flag any test source code that could try
|
||||
to write to a top-level TOML file, plus `tempfile.mkdtemp()` /
|
||||
`tempfile.mkstemp()` calls without `dir=` pointing under `./tests/`.
|
||||
|
||||
**Out of scope (per user directive):** the OTHER `SLOP_*` env vars
|
||||
(`SLOP_GLOBAL_PRESETS`, `SLOP_GLOBAL_TOOL_PRESETS`, `SLOP_GLOBAL_PERSONAS`,
|
||||
`SLOP_GLOBAL_WORKSPACE_PROFILES`, `SLOP_CREDENTIALS`, `SLOP_MCP_ENV`,
|
||||
`SLOP_LOGS_DIR`, `SLOP_SCRIPTS_DIR`) remain env-var-driven for now. The user
|
||||
considers them a separate "mess" to be addressed in follow-up tracks. The
|
||||
test workspace still uses these env vars to redirect to per-run paths under
|
||||
`./tests/artifacts/`.
|
||||
|
||||
After this track, the rule is: **any `pytest` or `run_tests_batched.py`
|
||||
invocation cannot write a single byte outside `./tests/`, and the static audit
|
||||
flags any test source code that could try.**
|
||||
|
||||
## Current State Audit (as of 2026-06-19)
|
||||
|
||||
### Already Implemented (DO NOT re-implement)
|
||||
|
||||
1. **`isolate_workspace` autouse fixture** (`tests/conftest.py:259-281`)
|
||||
- Sets `SLOP_CONFIG`, `SLOP_GLOBAL_PRESETS`, `SLOP_GLOBAL_TOOL_PRESETS`,
|
||||
`SLOP_GLOBAL_PERSONAS`, `SLOP_GLOBAL_WORKSPACE_PROFILES` to a per-test
|
||||
path via `tmp_path_factory.mktemp("isolated_workspace")`.
|
||||
- Provides partial protection for tests that go through `src.paths.get_*_path()`.
|
||||
2. **`live_gui` fixture workspace** (`tests/conftest.py:484-525`)
|
||||
- Creates `tests/artifacts/live_gui_workspace_<TIMESTAMP>/` per pytest
|
||||
invocation with fresh `manual_slop.toml` + `config.toml` (per
|
||||
`workspace_path_finalize_20260609`).
|
||||
- Sets `SLOP_CREDENTIALS` + `SLOP_MCP_ENV` to the **real** project-root
|
||||
`credentials.toml` / `mcp_env.toml` (read-only intent).
|
||||
3. **`scripts/check_test_toml_paths.py`** (per `conductor/tracks.md:395`)
|
||||
- Static audit detects tests with hardcoded references to TOML basenames
|
||||
(`manual_slop.toml`, `config.toml`, `credentials.toml`, etc.) or to
|
||||
`Path("C:/projects/...")` and `Path("tests/artifacts/")` literals.
|
||||
- CI gate that exits 1 on violation.
|
||||
4. **`conductor/code_styleguides/workspace_paths.md`** (148 lines)
|
||||
- Hard rule: test workspaces must live under `tests/artifacts/`. Banned:
|
||||
`tmp_path_factory.mktemp` for test infrastructure workspaces, env vars
|
||||
for test paths, CLI args for test paths.
|
||||
5. **`scripts/audit_no_temp_writes.py`** (108 lines)
|
||||
- Audits `scripts/` for `%TEMP%` usage. Pattern reference for the new
|
||||
audit script in Layer 4.
|
||||
|
||||
### Gaps to Fill (This Track's Scope)
|
||||
|
||||
| # | Gap | Risk | Where |
|
||||
|---|-----|------|-------|
|
||||
| G1 | `isolate_workspace` uses `tmp_path_factory.mktemp` which lives in `%TEMP%`, violating the existing styleguide | Path allowlist for Layer 1 has to include `%TEMP%` = widened blast radius | `tests/conftest.py:265` |
|
||||
| G2 | `isolate_workspace` doesn't set `SLOP_CREDENTIALS` or `SLOP_MCP_ENV` | Non-live_gui tests that go through `src.paths.get_credentials_path()` read the real `credentials.toml` | `tests/conftest.py:259-281` |
|
||||
| G3 | No runtime file-I/O guard. Tests can `Path("manual_slop.toml").write_text(...)` with no consequence | **Direct cause of user's data loss** | New: `tests/conftest.py:_enforce_test_sandbox` |
|
||||
| G4 | Pytest's `tmp_path` / `tmp_path_factory` default to `%TEMP%\pytest-of-<user>\` | Same widening issue as G1 | New: `pyproject.toml` addopts + `tests/conftest.py:pytest_configure` |
|
||||
| G5 | `check_test_toml_paths.py` doesn't catch non-TOML writes (e.g., `Path("manualslop_layout.ini").write_text`, `Path("manualslop_history.toml").write_text`) | Hidden test paths slip through the static audit | New: `scripts/audit_test_sandbox_violations.py` (extends existing) |
|
||||
| G6 | No OS-level hard sandbox option for paranoid users (Tier 2 has `run_tier2_sandboxed.ps1`; tests have no equivalent) | Same risk for users running `pytest` interactively without the Python guard | New: `scripts/run_tests_sandboxed.ps1` (opt-in) |
|
||||
| G7 | `AppController()` is initialized at `tests/conftest.py:65` at module import (line 65-66: `_warmup_app_controller = AppController(); _warmup_app_controller.wait_for_warmup(timeout=60.0)`), BEFORE the autouse `isolate_workspace` fixture applies | **MOSTLY OK but no invariant.** Per the call chain audit: `AppController.__init__` (src/app_controller.py:787) only sets up state + starts warmup background thread; it does NOT call `init_state()` or `load_config()`. `init_state()` (which reads config) is called from `App.__init__()` in `src/gui_2.py`, AFTER fixtures apply. But there's no test asserting this invariant — a future refactor could accidentally move init_state() into __init__ and silently break the safety | New FR8 regression test: assert `AppController.__init__` does not call `init_state()` or `load_config()` |
|
||||
|
||||
## Goals
|
||||
|
||||
1. **Make it impossible for any test invocation to write outside `./tests/`** at the Python layer (Layer 1) and at the OS layer when the opt-in wrapper is used (Layer 3).
|
||||
2. **Make every test see a fully-sandboxed project root** (Layer 2): config, presets, personas, tool_presets, credentials, mcp_env, AND pytest's own tmp dirs all live under `./tests/`.
|
||||
3. **Catch sandbox violations statically** (Layer 4): a developer adding a bad path to a test source file gets a CI failure before the test ever runs.
|
||||
4. **No regression in test pass rate.** All 11 tiers must continue to pass clean after this track ships.
|
||||
5. **No new `@pytest.mark.skip` markers.** Per the user directive (per `conductor/workflow.md` Skip-Marker Policy), in-session fixes only.
|
||||
|
||||
## Functional Requirements
|
||||
|
||||
### FR1. Python runtime file-I/O sandbox (Layer 1 — DEFAULT ON)
|
||||
|
||||
**WHERE:** New `_enforce_test_sandbox` autouse fixture in `tests/conftest.py` (registered alongside `isolate_workspace` at line ~258).
|
||||
|
||||
**WHAT:** Install a `sys.addaudithook()` callable that intercepts the `open` audit event when the mode is `'w'`, `'a'`, `'x'`, or `'+'`, or when the call is to `os.makedirs` / `shutil.rmtree` / `tempfile.mkdtemp` / `tempfile.mkstemp`.
|
||||
|
||||
**HOW:**
|
||||
- Allowlist (writes ALLOWED):
|
||||
- Any path under `<project_root>/tests/` (resolved absolute; case-normalized on Windows).
|
||||
- Any path under `<project_root>/tests/artifacts/` (already covered by above; explicit for clarity).
|
||||
- Any path under the per-run `_RUN_WORKSPACE` (which lives inside `./tests/artifacts/`).
|
||||
- The pyproject.toml `pytest --basetemp` target (also inside `./tests/`).
|
||||
- Denylist (writes REJECTED):
|
||||
- Anything outside `./tests/`.
|
||||
- On violation: raise `RuntimeError("TEST_SANDBOX_VIOLATION: <test_name> attempted to write to <absolute_path> which is outside <project_root>/tests/. Use tmp_path or fixture-provided paths.")` and let pytest report the failure with the test name.
|
||||
- The hook is installed in `pytest_configure` (so it's in place before any test module imports), uninstalled in `pytest_unconfigure`.
|
||||
|
||||
**SAFETY:**
|
||||
- Reads are NOT blocked. Tests legitimately need to read the source tree (`src/`, `pyproject.toml`, `mcp_env.toml`, `credentials.toml`, etc.) for fixtures and mocks.
|
||||
- The hook must be thread-safe (pytest may run tests in xdist workers).
|
||||
- The hook must not break pytest's own internals (`.pytest_cache`, `_pytest_tmp_path_factory` cleanup). The basetemp migration (FR3) handles this.
|
||||
- Allowlist resolution must NOT block legitimate pytest cache writes (`<project_root>/.pytest_cache/`, `<project_root>/tests/.pytest_cache/`, `<project_root>/tests/artifacts/__pycache__/`). Add `.pytest_cache`, `__pycache__`, `.coverage`, `.slop_cache`, `.ruff_cache` to the allowlist.
|
||||
|
||||
### FR2. CLI flag `--config` replaces `SLOP_CONFIG` env var (ROOT-CAUSE FIX)
|
||||
|
||||
**WHERE:** `src/paths.py:42-46` (the silent fallback). `sloppy.py` (CLI parser). `tests/conftest.py` (CLI parser at module body BEFORE any src/ import).
|
||||
|
||||
**WHAT:** Remove the `os.environ.get("SLOP_CONFIG", ...)` fallback from `src/paths.py:get_config_path()`. Replace with a module-level `_CONFIG_OVERRIDE: Path | None` that is set ONLY by explicit CLI flag parsing at the entry point.
|
||||
|
||||
**HOW:**
|
||||
|
||||
```python
|
||||
# src/paths.py — REPLACE the existing get_config_path with:
|
||||
_CONFIG_OVERRIDE: Path | None = None
|
||||
|
||||
def set_config_override(path: Path | None) -> None:
|
||||
"""CLI flag is the ONLY way to override. Pass None to use default.
|
||||
[C: sloppy.py:main, tests/conftest.py:_setup_test_paths]"""
|
||||
global _CONFIG_OVERRIDE
|
||||
_CONFIG_OVERRIDE = path
|
||||
_RESOLVED.clear()
|
||||
|
||||
def get_config_path() -> Path:
|
||||
"""Returns the active config.toml. If override is set, returns it.
|
||||
Otherwise returns the default <project_root>/config.toml.
|
||||
[C: src/app_controller.py:AppController.load_config,
|
||||
src/app_controller.py:AppController.init_state,
|
||||
src/models.py:_load_config_from_disk]"""
|
||||
if _CONFIG_OVERRIDE is not None:
|
||||
return _CONFIG_OVERRIDE
|
||||
root_dir = Path(__file__).resolve().parent.parent
|
||||
return root_dir / "config.toml"
|
||||
```
|
||||
|
||||
```python
|
||||
# sloppy.py — ADD argparse flag (BEFORE any src/ import):
|
||||
parser.add_argument("--config", help="Path to config.toml (default: <project_root>/config.toml)")
|
||||
args = parser.parse_args()
|
||||
if args.config:
|
||||
from src import paths
|
||||
paths.set_config_override(Path(args.config).resolve())
|
||||
```
|
||||
|
||||
```python
|
||||
# tests/conftest.py — PARSE sys.argv at module body BEFORE any src/ import:
|
||||
import sys as _sys
|
||||
from pathlib import Path as _Path
|
||||
|
||||
def _parse_config_arg() -> _Path | None:
|
||||
"""Manual sys.argv parse for --config. Returns resolved Path or None."""
|
||||
args = _sys.argv[1:]
|
||||
for i, arg in enumerate(args):
|
||||
if arg == "--config" and i + 1 < len(args):
|
||||
return _Path(args[i + 1]).resolve()
|
||||
if arg.startswith("--config="):
|
||||
return _Path(arg.split("=", 1)[1]).resolve()
|
||||
return None
|
||||
|
||||
_config_arg = _parse_config_arg()
|
||||
if _config_arg is None:
|
||||
# Default for tests: sandboxed config_overrides.toml
|
||||
config_arg = _Path(f"tests/artifacts/_isolation_workspace_{_RUN_ID}/config_overrides.toml")
|
||||
else:
|
||||
config_arg = _config_arg
|
||||
|
||||
# Set override BEFORE any src/ import
|
||||
from src import paths # noqa: E402
|
||||
paths.set_config_override(config_arg)
|
||||
|
||||
# ALSO register with pytest so it doesn't warn about unknown flag:
|
||||
def pytest_addoption(parser):
|
||||
parser.addoption("--config", action="store", default=None,
|
||||
help="Manual Slop: override config.toml path for tests")
|
||||
```
|
||||
|
||||
**Test workspace contents** (auto-generated by `_setup_test_paths` helper in conftest):
|
||||
```
|
||||
tests/artifacts/_isolation_workspace_<RUN_ID>/
|
||||
├── config_overrides.toml # the AppController config (per user naming)
|
||||
├── credentials.toml # placeholder for SLOP_CREDENTIALS (env var stays)
|
||||
├── mcp_env.toml # placeholder for SLOP_MCP_ENV (env var stays)
|
||||
├── presets.toml # placeholder for SLOP_GLOBAL_PRESETS
|
||||
├── tool_presets.toml # placeholder for SLOP_GLOBAL_TOOL_PRESETS
|
||||
└── personas.toml # placeholder for SLOP_GLOBAL_PERSONAS
|
||||
```
|
||||
|
||||
**SAFETY:**
|
||||
- The new `get_config_path()` raises `KeyError`-like behavior if no override AND no default exists. This is intentional — better to fail fast than silently use a wrong path.
|
||||
- The desktop GUI (`sloppy.py` without `--config`) uses the default `<project_root>/config.toml`, which is unchanged behavior.
|
||||
- Tests ALWAYS use a path inside `./tests/` (either from `--config` or the auto-generated default), so the Layer 1 audit hook's allowlist catches any stray writes.
|
||||
- Conftest's sys.argv parse happens BEFORE pytest's own argparse (which is too late for src/ imports).
|
||||
- The `config_overrides.toml` naming is a convention; tests CAN pass `--config /some/other/path.toml` and it will work.
|
||||
|
||||
### FR3. Pytest tmp paths + `isolate_workspace` migration (Layer 2 — DEFAULT ON)
|
||||
|
||||
**WHERE:**
|
||||
1. `pyproject.toml` — add `addopts = "--basetemp=tests/artifacts/_pytest_tmp"` to `[tool.pytest.ini_options]`.
|
||||
2. `tests/conftest.py:isolate_workspace` (lines 259-281) — replace `tmp_path_factory.mktemp("isolated_workspace")` with `Path("tests/artifacts/_isolation_workspace") / _RUN_ID`.
|
||||
3. `tests/conftest.py:pytest_configure` — defensive normalization: if `config._tmp_path_factory._basetemp` resolves outside `./tests/`, override to `tests/artifacts/_pytest_tmp`.
|
||||
4. `conductor/tech-stack.md` — dated note explaining the `--basetemp` choice.
|
||||
|
||||
**WHAT:**
|
||||
- All pytest `tmp_path` / `tmp_path_factory` fixtures create temp dirs under `<project_root>/tests/artifacts/_pytest_tmp/`.
|
||||
- The `isolate_workspace` autouse fixture's workspace lives under `<project_root>/tests/artifacts/_isolation_workspace_<RUN_ID>/`.
|
||||
- Both the `--basetemp` path AND the `isolate_workspace` path are inside `./tests/`, so the Layer 1 audit hook's allowlist can be a single rule: "anything under `./tests/` is allowed."
|
||||
|
||||
**HOW:**
|
||||
- `pyproject.toml`: standard `addopts` entry.
|
||||
- `isolate_workspace`: replace `tmp_path_factory.mktemp("isolated_workspace")` with `Path("tests/artifacts/_isolation_workspace") / _RUN_ID`. Add `SLOP_CREDENTIALS`, `SLOP_MCP_ENV`, `SLOP_GLOBAL_PRESETS`, `SLOP_GLOBAL_TOOL_PRESETS`, `SLOP_GLOBAL_PERSONAS`, `SLOP_GLOBAL_WORKSPACE_PROFILES` env vars pointing inside the isolation workspace. (Note: these are the OTHER `SLOP_*` env vars that the user is punting; they stay env-var-driven for now.)
|
||||
- `conftest.py:pytest_configure`: defensive check for `_tmp_path_factory._basetemp`.
|
||||
- `tech-stack.md`: dated note.
|
||||
|
||||
**SAFETY:** Update `.gitignore` to ensure `tests/artifacts/_pytest_tmp/` and `tests/artifacts/_isolation_workspace/` are covered (already covered by `tests/artifacts/` pattern).
|
||||
|
||||
### FR4. Extended static audit (Layer 4 — DEFAULT ON as CI gate)
|
||||
|
||||
**WHERE:** New file `scripts/audit_test_sandbox_violations.py` (extends `scripts/check_test_toml_paths.py`).
|
||||
|
||||
**WHAT:** Detect test source files that contain hardcoded write operations targeting paths outside `./tests/`. Patterns:
|
||||
- `Path("manual_slop.toml")`, `Path("config.toml")`, `Path("credentials.toml")`, `Path("presets.toml")`, `Path("personas.toml")`, `Path("tool_presets.toml")`, `Path("workspace_profiles.toml")`, `Path("manualslop_layout.ini")`, `Path("manual_slop_history.toml")`
|
||||
- `Path("project.toml")`, `Path("manual_slop_history.toml")` (top-level TOMLs)
|
||||
- `open("manual_slop.toml", "w")` and similar
|
||||
- `Path("C:/projects/...")` and `Path("C:\\projects\\...")` (project root references)
|
||||
- `Path("tests/artifacts/...")` literal (violates workspace_paths.md; should use a fixture instead)
|
||||
- `Path(__file__).parent.parent / "config.toml"` (and similar `..` traversal)
|
||||
- `tempfile.mkdtemp()`, `tempfile.mkstemp()` (without a `dir=` kwarg pointing to `./tests/`)
|
||||
|
||||
**HOW:** Mirror the existing `check_test_toml_paths.py` structure: list of compiled regexes + `find_violations(root_dir)` + `main()` with `--strict` CI mode.
|
||||
|
||||
**SAFETY:** The audit is INFORMATIONAL by default (exits 0). `--strict` mode (CI) exits 1 on any violation. This matches the precedent set by `audit_no_temp_writes.py` and `check_test_toml_paths.py`.
|
||||
|
||||
### FR5. OS-level sandbox wrapper (Layer 3 — OPT IN)
|
||||
|
||||
**WHERE:** New file `scripts/run_tests_sandboxed.ps1` (analogous to `scripts/tier2/run_tier2_sandboxed.ps1`).
|
||||
|
||||
**WHAT:** A PowerShell launcher that:
|
||||
1. Acquires a Windows restricted token (drops `SeDebugPrivilege`, `SeBackupPrivilege`, `SeRestorePrivilege`, `SeTakeOwnershipPrivilege`, etc.) — same pattern as `run_tier2_sandboxed.ps1`.
|
||||
2. Sets the current directory to the project root.
|
||||
3. Wraps the pytest process tree in a Job Object so it cannot escape.
|
||||
4. Invokes `uv run python -m pytest` (or `uv run python scripts/run_tests_batched.py`) under the restricted token with `--basetemp=tests/artifacts/_pytest_tmp` (Layer 2 + FR3 ensure tmp dirs are inside the sandbox).
|
||||
5. Forwards `--config tests/artifacts/_isolation_workspace_<RUN_ID>/config_overrides.toml` so the test config is inside the sandbox.
|
||||
6. Reports the exit code.
|
||||
|
||||
**HOW:** Copy the structure of `scripts/tier2/run_tier2_sandboxed.ps1` (100 lines). Replace the OpenCode launch with a pytest launch. Keep the Job Object + restricted token machinery.
|
||||
|
||||
**SAFETY:** This is OPT-IN. Users who don't need OS-level enforcement continue to use `uv run pytest` or `uv run python scripts/run_tests_batched.py` directly and still get Layer 1 + Layer 2 + Layer 4 protection. The wrapper is for paranoid scenarios.
|
||||
|
||||
**Note on Windows ACLs:** The Tier 2 wrapper uses restricted-token; it does NOT use file ACLs. For tests, this is sufficient because (a) tests run as the same user, (b) the Python guard (Layer 1) is the primary defense, (c) restricted-token catches native code paths that bypass Python.
|
||||
|
||||
### FR6. Regression tests for the guard
|
||||
|
||||
**WHERE:** New file `tests/test_test_sandbox.py`.
|
||||
|
||||
**WHAT:** Tests that verify:
|
||||
1. `test_sandbox_blocks_writes_outside_tests_dir`: write to a hardcoded `Path("manual_slop.toml")` from a test → `RuntimeError` with the `TEST_SANDBOX_VIOLATION` prefix.
|
||||
2. `test_sandbox_allows_writes_inside_tests_dir`: write to `tmp_path / "foo.txt"` → succeeds.
|
||||
3. `test_sandbox_allows_writes_inside_tests_artifacts`: write to `Path("tests/artifacts/_pytest_tmp_xxx/foo.txt")` → succeeds.
|
||||
4. `test_sandbox_does_not_block_reads`: read from `Path("pyproject.toml")` → succeeds.
|
||||
5. `test_audit_test_sandbox_violations_flags_known_bad_pattern`: create a temp test file with `Path("manual_slop.toml").write_text(...)`, run the audit script as a subprocess, assert exit 1.
|
||||
6. `test_audit_test_sandbox_violations_passes_clean_test`: create a temp test file using only `tmp_path`, assert exit 0.
|
||||
7. `test_pyproject_toml_basetemp_is_under_tests`: parse `pyproject.toml`, assert `addopts` contains `--basetemp=tests/artifacts/_pytest_tmp`.
|
||||
8. `test_isolate_workspace_does_not_use_tmp_path_factory_for_infra`: parse `tests/conftest.py`, assert no `tmp_path_factory.mktemp` calls in `isolate_workspace`.
|
||||
9. `test_appcontroller_init_does_not_load_config`: parse `src/app_controller.py`, assert `AppController.__init__` does NOT call `init_state()` or `load_config()`. **Per the G7 audit: this guards the invariant that config reads happen AFTER fixtures apply.**
|
||||
10. `test_audit_flags_tempfile_mkdtemp_without_tests_dir`: create a test file with `tempfile.mkdtemp()`, run audit, assert exit 1. **Per user directive: tests should never need AppData temp.**
|
||||
11. `test_config_override_via_cli_flag`: invoke `python -c "..."` with `--config <path>` and verify `paths.get_config_path()` returns the override.
|
||||
12. `test_paths_get_config_path_no_env_fallback`: monkeypatch-delete `SLOP_CONFIG` env var, import `src.paths`, assert `get_config_path()` returns default (no env var lookup).
|
||||
13. `test_sloppy_py_parses_config_flag`: parse `sloppy.py` AST, assert `--config` argparse argument exists and triggers `paths.set_config_override()`.
|
||||
|
||||
**HOW:** Standard pytest. Use `subprocess.run` for the audit script invocations to test the CLI surface. Use `ast.parse` for static checks on `conftest.py`, `app_controller.py`, and `sloppy.py`.
|
||||
|
||||
**SAFETY:** The `test_sandbox_blocks_writes_outside_tests_dir` test will raise `RuntimeError` — pytest must catch it as a pass. Use `pytest.raises(RuntimeError, match="TEST_SANDBOX_VIOLATION")`.
|
||||
|
||||
### FR7. Documentation update
|
||||
|
||||
**WHERE:** New file `conductor/code_styleguides/test_sandbox.md`. Update `conductor/code_styleguides/workspace_paths.md`. Update `docs/guide_testing.md`.
|
||||
|
||||
**WHAT:** Document:
|
||||
- The `--config` CLI flag convention (replaces `SLOP_CONFIG` env var).
|
||||
- The `config_overrides.toml` naming convention for test workspace configs.
|
||||
- The 4-layer enforcement model (Python guard, conftest isolation, OS-level wrapper, static audit).
|
||||
- The `--basetemp` rule (why pytest tmp paths must live under `./tests/`).
|
||||
- The Layer 1 audit hook contract: writes outside `./tests/` raise `RuntimeError`.
|
||||
- The opt-in `scripts/run_tests_sandboxed.ps1` wrapper.
|
||||
- The audit script and CI gate.
|
||||
|
||||
## Non-Functional Requirements
|
||||
|
||||
- **NFR1. Performance:** The audit hook adds < 5% overhead to pytest run time (measured on the existing 11-tier suite). Conftest fixtures are unchanged in scope; only env-var setup is added.
|
||||
- **NFR2. Maintainability:** No new `src/` files (per `AGENTS.md` File Size and Naming Convention rule). The Python guard lives in `tests/conftest.py` (test infrastructure). The audit script lives in `scripts/` (project infrastructure). The PowerShell wrapper lives in `scripts/`.
|
||||
- **NFR3. Cross-platform:** The Python guard (Layer 1) and the static audit (Layer 4) work on Windows, macOS, and Linux. The PowerShell wrapper (Layer 3) is Windows-only; on non-Windows it's a documented no-op (`Write-Host "OS-level sandbox requires Windows"` and exit 0).
|
||||
- **NFR4. Discoverability:** The audit script's `--help` explains what it checks, how to fix violations, and how to run in `--strict` mode. The `RuntimeError` raised by Layer 1 includes a "How to fix" line pointing at `conductor/code_styleguides/test_sandbox.md`.
|
||||
|
||||
## Architecture Reference
|
||||
|
||||
- **`conductor/code_styleguides/workspace_paths.md`** — existing rule: test workspaces live under `tests/artifacts/`. This track extends it to ALL test infrastructure (including pytest's `tmp_path`).
|
||||
- **`conductor/code_styleguides/feature_flags.md`** — Layer 1 + Layer 2 + Layer 4 are file-presence-on = enabled (matches the project's "delete to turn off" convention for cross-cutting concerns). Layer 3 is opt-in via the explicit PowerShell wrapper.
|
||||
- **`scripts/audit_no_temp_writes.py`** — pattern reference for the new `scripts/audit_test_sandbox_violations.py`.
|
||||
- **`scripts/tier2/run_tier2_sandboxed.ps1`** — pattern reference for the new `scripts/run_tests_sandboxed.ps1`.
|
||||
- **`docs/guide_testing.md`** (existing) — test infrastructure deep-dive. Add a new "Sandbox Hardening" section that summarizes Layers 1-4 and links to the styleguide.
|
||||
- **`conductor/tracks/workspace_path_finalize_20260609/`** — prior track that established the `tests/artifacts/` workspace pattern. This track extends it.
|
||||
|
||||
## Out of Scope
|
||||
|
||||
1. **Reading protection.** Tests still need to read the source tree (`src/`, `pyproject.toml`, etc.) for fixtures. Reads are intentionally NOT blocked. If a future track wants read isolation, it's a separate scope.
|
||||
2. **Network sandboxing.** Tests that hit the live Gemini/Anthropic/etc. APIs continue to do so. The user's data loss is filesystem, not network.
|
||||
3. **Migrating existing tests to the new patterns.** The audit (Layer 4) catches new violations; existing tests that already pass continue to pass. If the audit catches a currently-passing test, that's a bug to fix in the test (separate, in-session fixes).
|
||||
4. **Converting the OTHER `SLOP_*` env vars to CLI flags** (`SLOP_GLOBAL_PRESETS`, `SLOP_GLOBAL_TOOL_PRESETS`, `SLOP_GLOBAL_PERSONAS`, `SLOP_GLOBAL_WORKSPACE_PROFILES`, `SLOP_CREDENTIALS`, `SLOP_MCP_ENV`, `SLOP_LOGS_DIR`, `SLOP_SCRIPTS_DIR`). Per user directive, this is the "mess" to address in follow-up tracks. This track only eliminates `SLOP_CONFIG`. The test workspace still uses the other env vars to redirect to per-run paths under `./tests/artifacts/`.
|
||||
5. **A cross-platform equivalent of `run_tests_sandboxed.ps1`.** macOS/Linux users get Layer 1 + Layer 2 + Layer 4. Adding a `run_tests_sandboxed.sh` would require `bwrap` or `unshare` setup; defer to a future track if needed.
|
||||
6. **Conftest fixture-level enforcement (e.g., `@pytest.fixture(sandbox_strict=True)` for tests that need full OS isolation).** The blanket autouse fixture is the v1. Per-fixture tuning is a v2 feature.
|
||||
|
||||
## Verification Criteria
|
||||
|
||||
For the track to be marked complete, ALL of the following must be true:
|
||||
|
||||
- [ ] **VC1.** `tests/test_test_sandbox.py` exists and all 13 tests pass.
|
||||
- [ ] **VC2.** `scripts/audit_test_sandbox_violations.py` runs in both modes:
|
||||
- Default (informational): exit 0, lists violations (or says "clean").
|
||||
- `--strict`: exit 1 on violation, exit 0 on clean.
|
||||
- [ ] **VC3.** `pyproject.toml` contains `addopts = "--basetemp=tests/artifacts/_pytest_tmp"` under `[tool.pytest.ini_options]`.
|
||||
- [ ] **VC4.** `tests/conftest.py:isolate_workspace` no longer calls `tmp_path_factory.mktemp` (per `workspace_paths.md`). All env-var redirects point to paths inside `./tests/artifacts/`.
|
||||
- [ ] **VC5.** `scripts/run_tests_sandboxed.ps1` exists, parses cleanly, and on Windows can be invoked (`-WhatIf` mode for dry-run).
|
||||
- [ ] **VC6.** `conductor/tech-stack.md` has a dated note explaining the `--basetemp` choice.
|
||||
- [ ] **VC7.** `conductor/code_styleguides/workspace_paths.md` (or new `test_sandbox.md`) documents the 3-layer model.
|
||||
- [ ] **VC8.** Full test suite: `uv run python scripts/run_tests_batched.py --tiers 1,2,3,4,5,6,7,8,9,10,11` runs to completion; no regression in pass rate vs. the pre-track baseline (1288 passed + 4 xdist-skipped per `result_migration_small_files_20260617`).
|
||||
- [ ] **VC9.** No new `@pytest.mark.skip` markers added (per `conductor/workflow.md` Skip-Marker Policy + user directive).
|
||||
- [ ] **VC10.** End-of-track report at `docs/reports/TRACK_COMPLETION_test_sandbox_hardening_20260619.md` (per Tier 2 conventions).
|
||||
|
||||
## Risk Assessment
|
||||
|
||||
| Risk | Likelihood | Mitigation |
|
||||
|---|---|---|
|
||||
| Layer 1 audit hook breaks a test that legitimately writes outside `./tests/` (e.g., a test that writes to a tempfile.mkdtemp default location) | medium | FR1 allowlist includes pytest's `--basetemp`; if a new path is needed, add it. The hook's `RuntimeError` includes the test name so the offending test is obvious. |
|
||||
| Layer 1 audit hook slows down the test suite | low | `sys.addaudithook` is a thin C-level callback; overhead measured in <2% per Python docs. |
|
||||
| Layer 4 audit flags a currently-passing test as a false positive | medium | The audit is INFORMATIONAL by default; `--strict` is opt-in for CI. If a real test is flagged, fix the test (don't suppress the audit). |
|
||||
| Layer 3 PowerShell wrapper breaks on a Windows version without the required privileges | low | Wrapper is opt-in; default invocation stays `uv run pytest`. Wrapper docs explain the privilege requirements. |
|
||||
| Existing tests that don't go through `isolate_workspace` still read real config files | high (known gap) | Reads are out of scope per the Out of Scope section. Layer 1 still blocks writes, which is the user's primary concern. |
|
||||
| `pytest_configure` setting `_tmp_path_factory._basetemp` uses a private API that changes between versions | medium | The `--basetemp` addopts is the primary mechanism. The `_basetemp` assignment is defensive only; if it breaks, addopts still works. |
|
||||
|
||||
## Execution Plan (high-level — see `plan.md` for worker-ready tasks)
|
||||
|
||||
- [ ] **Phase 1: Investigation + baseline** — capture current pass count, confirm `isolate_workspace` + audit script work as documented.
|
||||
- [ ] **Phase 2: Layer 4 (static audit) + tests** — write `audit_test_sandbox_violations.py`, write `test_test_sandbox.py` audit-tests (parts 5-8), commit.
|
||||
- [ ] **Phase 3: Layer 1 (Python guard) + tests** — implement `_enforce_test_sandbox` fixture, write guard-specific regression tests (parts 1-4), commit.
|
||||
- [ ] **Phase 4: Layer 2 (`isolate_workspace` migration + FR3 basetemp)** — refactor `isolate_workspace`, add `addopts` to `pyproject.toml`, update `tech-stack.md`, commit.
|
||||
- [ ] **Phase 5: Layer 3 (PowerShell wrapper)** — write `scripts/run_tests_sandboxed.ps1`, write a smoke test, commit.
|
||||
- [ ] **Phase 6: Documentation** — update `workspace_paths.md` (or write `test_sandbox.md`), update `docs/guide_testing.md`, commit.
|
||||
- [ ] **Phase 7: Full suite verification** — run all 11 tiers, verify no regression, commit.
|
||||
- [ ] **Phase 8: End-of-track report** — write `docs/reports/TRACK_COMPLETION_test_sandbox_hardening_20260619.md`, commit.
|
||||
|
||||
## See Also
|
||||
|
||||
- `conductor/tracks.md:395` — prior "Test Consolidation & TOML Sandboxing" track (added `check_test_toml_paths.py`).
|
||||
- `conductor/archive/workspace_path_finalize_20260609/` — prior track that established the `tests/artifacts/` workspace pattern.
|
||||
- `conductor/tracks/tier2_autonomous_sandbox_20260616/` — meta-tooling track that this sandbox mirrors.
|
||||
- `conductor/code_styleguides/workspace_paths.md` — existing test-workspace rule.
|
||||
- `conductor/code_styleguides/feature_flags.md` — file-presence = enabled convention.
|
||||
- `conductor/workflow.md` Skip-Marker Policy + Skip-Marker review checklist.
|
||||
- `docs/guide_testing.md` — existing test infrastructure deep-dive (add a "Sandbox Hardening" section).
|
||||
- `scripts/audit_no_temp_writes.py` — pattern reference for the new audit script.
|
||||
- `scripts/tier2/run_tier2_sandboxed.ps1` — pattern reference for the new PowerShell wrapper.
|
||||
Reference in New Issue
Block a user