Private
Public Access
refactor(paths): v3 design - explicit initialize_paths + frozen PathsConfig singleton
This commit is contained in:
+10
-9
@@ -34,7 +34,7 @@ if _config_override_arg is None:
|
||||
_config_override_arg = _ISOLATION_WORKSPACE / "config_overrides.toml"
|
||||
|
||||
from src import paths as _paths # noqa: E402
|
||||
_paths.set_config_override(_config_override_arg)
|
||||
_paths.initialize_paths(_config_override_arg)
|
||||
|
||||
thirdparty_dir = os.path.join(os.path.dirname(__file__), "..", "thirdparty")
|
||||
if thirdparty_dir not in sys.path:
|
||||
@@ -381,11 +381,11 @@ def isolate_workspace(monkeypatch) -> Generator[None, None, None]:
|
||||
(tests/artifacts/_isolation_workspace_<RUN_ID>/). Also writes placeholder
|
||||
TOML files for the redirected paths. NO SLOP_* env vars are set;
|
||||
src/paths.py reads the overrides from config.toml [paths] (with env var
|
||||
as fallback if needed).
|
||||
[C: tests/conftest.py:_ISOLATION_WORKSPACE, src/paths.py:_resolve_path]
|
||||
as fallback if needed). Also re-initializes the paths singleton so every
|
||||
getter sees the test-workspace overrides.
|
||||
[C: tests/conftest.py:_ISOLATION_WORKSPACE, src/paths.py:initialize_paths]
|
||||
"""
|
||||
from src import paths as _paths
|
||||
_paths.reset_resolved()
|
||||
|
||||
test_workspace = _ISOLATION_WORKSPACE
|
||||
|
||||
@@ -421,14 +421,15 @@ def isolate_workspace(monkeypatch) -> Generator[None, None, None]:
|
||||
@pytest.fixture(autouse=True)
|
||||
def reset_paths() -> Generator[None, None, None]:
|
||||
"""
|
||||
|
||||
|
||||
Autouse fixture that resets the paths global state before each test.
|
||||
Autouse fixture that resets the paths singleton before each test.
|
||||
The isolate_workspace fixture re-initializes paths before the test runs,
|
||||
so reset_paths is a no-op here (PathsConfig is frozen at init time,
|
||||
the per-getter cache is gone). Kept as a marker for backward compatibility
|
||||
with test code that asserts the singleton is cleared at test boundaries.
|
||||
"""
|
||||
from src import paths
|
||||
paths.reset_resolved()
|
||||
yield
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def reset_ai_client() -> Generator[None, None, None]:
|
||||
|
||||
@@ -18,7 +18,7 @@ def tmp_session_dir(tmp_path, monkeypatch):
|
||||
|
||||
monkeypatch.setenv("SLOP_LOGS_DIR", str(logs_dir))
|
||||
monkeypatch.setenv("SLOP_SCRIPTS_DIR", str(scripts_dir))
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
|
||||
# Ensure session_logger is clean
|
||||
with patch("src.session_logger._comms_fh", None):
|
||||
|
||||
@@ -26,7 +26,7 @@ def test_save_paths():
|
||||
|
||||
with patch('shutil.copy') as mock_copy, \
|
||||
patch('src.paths.get_config_path') as mock_get_cfg, \
|
||||
patch('src.paths.reset_resolved') as mock_reset, \
|
||||
patch('src.paths.reset_paths') as mock_reset, \
|
||||
patch.object(MockApp, 'init_state') as mock_init:
|
||||
|
||||
mock_get_cfg.return_value = MagicMock()
|
||||
|
||||
@@ -50,7 +50,7 @@ def test_conductor_setup_scan(app_instance, tmp_path, monkeypatch):
|
||||
(cond_dir / "tracks" / "track1").mkdir(exist_ok=True)
|
||||
|
||||
monkeypatch.setenv('SLOP_CONDUCTOR_DIR', str((tmp_path / 'conductor').resolve()))
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
|
||||
app_instance._cb_run_conductor_setup()
|
||||
|
||||
|
||||
+2
-2
@@ -5,9 +5,9 @@ from src import paths
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def reset_paths():
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
yield
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
|
||||
def test_default_paths(tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("SLOP_CONFIG", str(tmp_path / "non_existent.toml"))
|
||||
|
||||
@@ -7,13 +7,13 @@ from src import paths
|
||||
from src import project_manager
|
||||
|
||||
def test_get_conductor_dir_default():
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
# Should return absolute path to "conductor" in project root
|
||||
expected = Path(__file__).resolve().parent.parent / "conductor"
|
||||
assert paths.get_conductor_dir() == expected
|
||||
|
||||
def test_get_conductor_dir_project_specific_with_toml(tmp_path):
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
project_root = tmp_path / "my_project"
|
||||
project_root.mkdir()
|
||||
|
||||
@@ -31,7 +31,7 @@ def test_get_conductor_dir_project_specific_with_toml(tmp_path):
|
||||
assert res == project_root / "custom_tracks"
|
||||
|
||||
def test_get_all_tracks_project_specific(tmp_path):
|
||||
paths.reset_resolved()
|
||||
paths.reset_paths()
|
||||
project_root = tmp_path / "my_project"
|
||||
project_root.mkdir()
|
||||
|
||||
|
||||
+110
-37
@@ -149,47 +149,64 @@ def test_sandbox_allows_pytest_cache_write() -> None:
|
||||
|
||||
|
||||
def test_config_override_via_cli_flag(tmp_path) -> None:
|
||||
"""paths.set_config_override(path) makes get_config_path() return that path."""
|
||||
"""paths.initialize_paths(config_path) makes all path getters return paths
|
||||
rooted at config_path's [paths] section (or default).
|
||||
[C: src/paths.py:initialize_paths, sloppy.py:main]"""
|
||||
from src import paths
|
||||
config_path = tmp_path / "my_config.toml"
|
||||
config_path.write_text("[ai]\nprovider='gemini'\n", encoding="utf-8")
|
||||
original = paths._CONFIG_OVERRIDE
|
||||
paths.initialize_paths(config_path)
|
||||
try:
|
||||
paths.set_config_override(config_path)
|
||||
assert paths.get_config_path() == config_path
|
||||
assert paths.get_global_presets_path().name == "presets.toml"
|
||||
assert paths.get_logs_dir().name == "sessions"
|
||||
finally:
|
||||
paths.set_config_override(original)
|
||||
paths.reset_paths()
|
||||
|
||||
|
||||
def test_paths_get_config_path_no_env_fallback(monkeypatch) -> None:
|
||||
"""Without an override AND without SLOP_CONFIG, get_config_path returns default."""
|
||||
monkeypatch.delenv("SLOP_CONFIG", raising=False)
|
||||
def test_paths_runtime_refresh_atomic_swap(tmp_path) -> None:
|
||||
"""Calling initialize_paths() a second time atomically swaps the singleton.
|
||||
Reader threads see the new config immediately. The PathsConfig is frozen.
|
||||
[C: src/paths.py:initialize_paths, src/paths.py:PathsConfig]"""
|
||||
from src import paths
|
||||
original = paths._CONFIG_OVERRIDE
|
||||
try:
|
||||
paths.set_config_override(None)
|
||||
expected = Path(__file__).resolve().parent.parent / "config.toml"
|
||||
assert paths.get_config_path() == expected
|
||||
finally:
|
||||
paths.set_config_override(original)
|
||||
cfg_a = tmp_path / "a.toml"
|
||||
cfg_b = tmp_path / "b.toml"
|
||||
cfg_a.write_text("[ai]\nprovider='gemini-a'\n", encoding="utf-8")
|
||||
cfg_b.write_text("[ai]\nprovider='gemini-b'\n", encoding="utf-8")
|
||||
paths.initialize_paths(cfg_a)
|
||||
assert paths.get_config_path() == cfg_a
|
||||
paths.initialize_paths(cfg_b)
|
||||
assert paths.get_config_path() == cfg_b
|
||||
paths.reset_paths()
|
||||
|
||||
|
||||
def test_paths_uninitialized_raises(tmp_path) -> None:
|
||||
"""Calling a path getter before initialize_paths() raises RuntimeError
|
||||
(not silent fallback). This is the contract that enforces explicit init.
|
||||
[C: src/paths.py:_cfg]"""
|
||||
from src import paths
|
||||
paths.reset_paths()
|
||||
with pytest.raises(RuntimeError, match="not initialized"):
|
||||
paths.get_logs_dir()
|
||||
paths.reset_paths()
|
||||
|
||||
|
||||
def test_sloppy_py_parses_config_flag() -> None:
|
||||
"""sloppy.py has a --config argparse argument that calls set_config_override."""
|
||||
"""sloppy.py has a --config argparse argument that calls initialize_paths."""
|
||||
import ast
|
||||
sloppy = Path(__file__).resolve().parent.parent / "sloppy.py"
|
||||
tree = ast.parse(sloppy.read_text(encoding="utf-8"))
|
||||
found_config_arg = False
|
||||
found_set_override_call = False
|
||||
found_init_call = False
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Constant) and node.value == "--config":
|
||||
found_config_arg = True
|
||||
if isinstance(node, ast.Call):
|
||||
func = node.func
|
||||
if isinstance(func, ast.Name) and func.id == "set_config_override":
|
||||
found_set_override_call = True
|
||||
if isinstance(func, ast.Name) and func.id == "initialize_paths":
|
||||
found_init_call = True
|
||||
assert found_config_arg, "sloppy.py must have a --config argparse argument"
|
||||
assert found_set_override_call, "sloppy.py must call paths.set_config_override(args.config)"
|
||||
assert found_init_call, "sloppy.py must call paths.initialize_paths(args.config)"
|
||||
|
||||
|
||||
def test_pyproject_toml_basetemp_is_under_tests() -> None:
|
||||
@@ -276,30 +293,86 @@ def test_config_overrides_toml_has_paths_section() -> None:
|
||||
)
|
||||
|
||||
|
||||
def test_path_getters_read_from_config_paths_section() -> None:
|
||||
"""Every global path getter in src/paths.py reads from the [paths] section
|
||||
of the active config when no env var is set. Verifies via AST that each
|
||||
get_*_path() function calls _resolve_path (not raw os.environ.get)."""
|
||||
def test_path_getters_are_trivial_field_access() -> None:
|
||||
"""Every global path getter in src/paths.py is a trivial field access on the
|
||||
PathsConfig singleton (return _cfg().<field>). They must NOT do file I/O
|
||||
or call _resolve_path() (which is internal-only, called from initialize_paths).
|
||||
This enforces the v3 design: explicit init at startup, trivial getters."""
|
||||
import ast
|
||||
paths_py = Path(__file__).resolve().parent.parent / "src" / "paths.py"
|
||||
tree = ast.parse(paths_py.read_text(encoding="utf-8"))
|
||||
getters = {
|
||||
"get_global_presets_path": "presets",
|
||||
"get_global_tool_presets_path": "tool_presets",
|
||||
"get_global_personas_path": "personas",
|
||||
"get_global_themes_path": "themes",
|
||||
"get_global_workspace_profiles_path": "workspace_profiles",
|
||||
"get_credentials_path": "credentials",
|
||||
"get_logs_dir": "logs_dir",
|
||||
"get_scripts_dir": "scripts_dir",
|
||||
}
|
||||
trivial_getters = [
|
||||
"get_global_presets_path", "get_global_tool_presets_path",
|
||||
"get_global_personas_path", "get_global_themes_path",
|
||||
"get_global_workspace_profiles_path", "get_credentials_path",
|
||||
"get_logs_dir", "get_scripts_dir",
|
||||
]
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.FunctionDef) and node.name in getters:
|
||||
if isinstance(node, ast.FunctionDef) and node.name in trivial_getters:
|
||||
src = ast.unparse(node)
|
||||
assert "_resolve_path(" in src, (
|
||||
f"{node.name} must call _resolve_path() to read from config.toml [paths]; "
|
||||
f"got direct os.environ.get() instead."
|
||||
assert "_cfg()" in src, (
|
||||
f"{node.name} must call _cfg() to read from the PathsConfig singleton; "
|
||||
f"got direct file I/O or env var lookup instead."
|
||||
)
|
||||
assert "_resolve_path(" not in src, (
|
||||
f"{node.name} must NOT call _resolve_path(); that's internal-only "
|
||||
f"(called once from initialize_paths)."
|
||||
)
|
||||
assert "os.environ" not in src, (
|
||||
f"{node.name} must NOT use os.environ directly; env vars are read once "
|
||||
f"during initialize_paths()."
|
||||
)
|
||||
|
||||
|
||||
def test_initialize_paths_thread_safe_atomic_swap(tmp_path) -> None:
|
||||
"""initialize_paths() uses an RLock; concurrent swaps don't corrupt the
|
||||
singleton. Reader threads always see a consistent PathsConfig snapshot
|
||||
(frozen dataclass). Per the user's "bad programmers take shortcuts" rule:
|
||||
no torn writes, no partial reads.
|
||||
[C: src/paths.py:_PATHS_LOCK, src/paths.py:PathsConfig]"""
|
||||
import threading
|
||||
import tomllib
|
||||
from src import paths
|
||||
cfg_a = tmp_path / "a.toml"
|
||||
cfg_b = tmp_path / "b.toml"
|
||||
cfg_a.write_bytes(b"[paths]\nlogs_dir = 'C:/tmp/thread_a'\n")
|
||||
cfg_b.write_bytes(b"[paths]\nlogs_dir = 'C:/tmp/thread_b'\n")
|
||||
errors = []
|
||||
def swap(cfg, n):
|
||||
for _ in range(n):
|
||||
try:
|
||||
paths.initialize_paths(cfg)
|
||||
except Exception as e:
|
||||
errors.append(e)
|
||||
t1 = threading.Thread(target=swap, args=(cfg_a, 100))
|
||||
t2 = threading.Thread(target=swap, args=(cfg_b, 100))
|
||||
t1.start(); t2.start()
|
||||
t1.join(); t2.join()
|
||||
assert not errors, f"thread-safety violation: {errors}"
|
||||
paths.reset_paths()
|
||||
|
||||
|
||||
def test_pathsconfig_is_frozen_dataclass() -> None:
|
||||
"""PathsConfig uses @dataclass(frozen=True) so individual field reads are
|
||||
atomic and cannot be mutated by readers. Per user directive: gated
|
||||
transactions, no data race over config.
|
||||
[C: src/paths.py:PathsConfig]"""
|
||||
import dataclasses
|
||||
import tempfile, tomli_w
|
||||
from src import paths
|
||||
params = getattr(paths.PathsConfig, "__dataclass_params__", None)
|
||||
assert params is not None, "PathsConfig must be a dataclass"
|
||||
assert params.frozen is True, "PathsConfig must be @dataclass(frozen=True)"
|
||||
with tempfile.NamedTemporaryFile(suffix=".toml", delete=False, mode="wb") as f:
|
||||
tomli_w.dump({"paths": {"logs_dir": "C:/tmp/frozen_test"}}, f)
|
||||
cfg = Path(f.name)
|
||||
try:
|
||||
paths.initialize_paths(cfg)
|
||||
with pytest.raises(dataclasses.FrozenInstanceError):
|
||||
paths._PATHS_CONFIG.presets = Path("/tmp/should_fail")
|
||||
finally:
|
||||
paths.reset_paths()
|
||||
cfg.unlink()
|
||||
|
||||
|
||||
@pytest.mark.skipif(os.name != "nt", reason="Windows-only sandbox wrapper")
|
||||
|
||||
Reference in New Issue
Block a user