2dc1e227eb
* fix(langgraph-dev): rotate langgraph_dev.log when it exceeds 50MB ``_LOG_FILE`` (``~/.config/evoscientist/langgraph_dev.log``) was opened in ``start_langgraph_dev`` with plain ``"ab"`` and never rotated, so it grew unbounded over weeks/months of heavy use — especially when chatty MCP servers spawned by langgraph dev filled it, or when failure paths produced stack traces. Implement the recommended option 1 from #209: filesize-based rollover. When the active log exceeds 50MB on the next ``start_langgraph_dev`` invocation, rename it to ``langgraph_dev.log.1`` (overwriting any existing backup) via ``os.replace`` and start fresh. Single-backup policy keeps the disk footprint bounded at roughly 2x threshold. Rotation is best-effort: ``_rotate_log_if_needed`` logs and swallows OSError so a permission error or racing rename can't block langgraph dev from starting. The next ``start`` invocation will try again — worst case the log grows for one more session. Options 2 (timestamped per-session + 7-day sweep) and 3 (``RotatingFileHandler`` + pipe) are explicitly NOT done — option 1 is simplest, no async machinery, matches the issue's recommendation. Closes #209 * test(langgraph-dev): redirect _PID_DIR in rotate integration test Address CodeRabbit review comment on #270: the ``TestStartLanggraphDevRotatesLog::test_rotate_called_before_open`` test patched only ``_LOG_FILE`` to a tmp path, but ``start_langgraph_dev`` also calls ``_PID_DIR.mkdir(...)`` as part of its prelude, which would create a real directory under ``~/.config/evoscientist/`` on a dev machine. Redirect ``_PID_DIR`` to ``tmp_path / "pids"`` too so the test stays fully isolated. Add a final assertion that ``pid_dir.is_dir()`` holds, proving the function reached past the mkdir call. * refactor(langgraph-dev): bundle runtime paths into LanggraphRuntimePaths @din0s review follow-up on #270: the previous test isolation patched only ``_LOG_FILE`` (and after a second round, ``_PID_DIR``), but ``start_langgraph_dev`` still touches 5 distinct on-disk paths. Patching any subset of those still leaves the others pointing at the user's real ``~/.config/evoscientist/`` — exactly the case that produced the "Port 6174 cannot be bound after waiting 60s" symptom on the reviewer's machine. Replace the five free-floating module-level constants (``_PID_DIR`` / ``_PID_FILE`` / ``_LOG_FILE`` / ``_WORKSPACE_SIDECAR`` / ``_FILE_LOCK_PATH``) with a single ``LanggraphRuntimePaths`` frozen dataclass exposed as a module-level ``RUNTIME`` instance. Production code accesses ``RUNTIME.pid_file`` etc.; tests can now substitute the *whole* bundle in one assignment: monkeypatch.setattr( manager, "RUNTIME", manager.LanggraphRuntimePaths.for_directory(tmp_path / "runtime"), ) The classmethod ``for_directory(pid_dir)`` builds an isolated bundle rooted at a single dir, so the test author doesn't spell out every path field. Tests that only care about one field (e.g. pid_file during the stale-process kill path) use ``dataclasses.replace(manager.RUNTIME, pid_file=X)`` — frozen dataclass-friendly, no need to enumerate the other four fields. The dataclass's docstring records the migration rationale (the old five-name layout invited inconsistent patches). External callers of the old constants updated: - ``EvoScientist/deploy/server.py`` and ``webui.py`` now import ``RUNTIME`` and use ``RUNTIME.log_file`` for the on-screen log path hint. The other imports they had (``_DEFAULT_PORT``, ``_is_port_occupied``, ``_read_workspace_sidecar``) are still module-level functions/values, untouched. Test updates: - ``tests/test_langgraph_manager.py``: ``patch.object(manager, "_XXX", X)`` patterns now go through ``dataclasses.replace(manager.RUNTIME, xxx=X)``; the ``TestStartLanggraphDevRotatesLog::test_rotate_called_before_open`` test (from the previous #270 review iteration) uses ``for_directory`` for one-shot isolation. - ``tests/test_langgraph_dev_workspace_sidecar.py``: each test now goes through a tiny ``_isolated_runtime(monkeypatch, tmp_path)`` helper that calls ``for_directory``. - ``tests/test_langgraph_dev_deploy_mode.py``: same ``for_directory`` swap. No production behavior change. All ``langgraph_dev``-side tests (``test_langgraph_manager.py`` 26/26, ``test_langgraph_dev_workspace_sidecar.py`` 14/14, ``test_langgraph_dev_deploy_mode.py`` 14/14, ``test_cli_deploy.py`` 18/18 — which indirectly exercises deploy/server.py and deploy/webui.py imports) pass. Full-project test count unchanged from baseline; the remaining 22 Windows-only pre-existing failures (test_background ``os.killpg``, test_file_mentions tilde, mcp_client ``shutil.which``, test_sessions 8.3 short path) are documented as out-of-scope for #207. * style: apply ruff format to langgraph_dev test + module files CI lint check on #270 failed: Run ruff format --check . Would reformat: EvoScientist/langgraph_dev/manager.py Would reformat: tests/test_langgraph_manager.py Plus two test files touched by the prior consolidation commit that ``ruff format`` hadn't seen yet: tests/test_langgraph_dev_deploy_mode.py tests/test_langgraph_dev_workspace_sidecar.py Just formatting. No logic change. All 75 refactor-related tests pass. * fix(test): use for_directory for full path isolation + patch _can_bind_port to skip real socket ops Two fixes for TestStartLanggraphDevRotatesLog: 1. Replace dataclasses.replace(manager.RUNTIME, ...) with LanggraphRuntimePaths.for_directory(pid_dir) so pid_file, workspace_sidecar, and lock_file are also temp-rooted (prevents leak to ~/.config/evoscientist/). 2. Monkeypatch _can_bind_port to always return True so the bind-poll loop in _wait_for_port_bindable passes immediately without touching real sockets (fixes 60s timeout on machines where port 6174 is already in use). * fix: cross-platform compatibility for Windows CI runners - background.py: replace POSIX-only os.killpg/os.getpgid with cross-platform _kill_process_tree() helper. On Windows falls back to Popen.terminate()/Popen.kill() (TerminateProcess); on POSIX keeps existing os.killpg logic. - test_backends.py: replace mkdir -p shell execution in test_literal_workspace_path_replaced with preprocessing-boundary assertion (patch LocalShellBackend.execute, capture command, assert workspace path was rewritten to ./). Avoids POSIX-only mkdir -p on Windows runners. - test_file_mentions.py: monkeypatch USERPROFILE on Windows so ntpath.expanduser() resolves ~ to tmp_path even when HOME is unset on CI runners. * refactor(test): add runtime_paths fixture to isolate manager.RUNTIME Adds a reusable fixture that monkeypatches manager.RUNTIME to a temp-rooted LanggraphRuntimePaths.for_directory(). Tests that need specific fields can still dataclasses.replace(runtime_paths, ...) but the baseline is always temp-isolated, preventing leaks to ~/.config/evoscientist/. Updated test_langgraph_dev_deploy_mode.py, test_langgraph_dev_workspace_sidecar.py, and test_langgraph_manager.py to use the fixture, consolidating sequential lock_file + pid_dir patches into single dataclasses.replace calls. * Revert "fix: cross-platform compatibility for Windows CI runners" This reverts commit eb025d24af32e195a982cd40f6d70dba885c4019. * style: ruff format conftest.py * fix: address review issues in log-rotation + runtime paths - Use for_directory(tmp_path/pids) as base in ensure_langgraph_dev tests so pid_file/log_file are co-located with pid_dir, not split across paths - Remove unused runtime_paths param from test_no_existing_file_is_noop - Replace manager.RUNTIME with runtime_paths in two sidecar tests - Use for_directory(DEFAULT_PID_DIR) instead of explicit construction - Fix stale _LOG_FILE reference in TestRotateLogIfNeeded docstring * style: ruff format test files --------- Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com>
239 lines
8.7 KiB
Python
239 lines
8.7 KiB
Python
"""Tests for the workspace-fingerprint sidecar protocol.
|
|
|
|
When langgraph dev is reused across processes (e.g., TUI / serve detects
|
|
a deploy-started instance on the configured port), the workspace recorded
|
|
in the sidecar JSON must match the workspace requested by the caller. On
|
|
mismatch we raise ``WorkspaceMismatchError`` so callers can surface a
|
|
clear refuse-with-hint error rather than silently operating on the wrong
|
|
project's files.
|
|
|
|
Background: ``EvoSci deploy --workdir /A`` running + ``EvoSci`` (TUI) in
|
|
/B previously took the "reuse externally-managed langgraph dev" branch
|
|
in ``ensure_langgraph_dev`` and only logged a warning. The deployed
|
|
sub-agents stayed pinned to /A while the TUI's main agent ran in /B,
|
|
breaking ``task()`` delegations.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import dataclasses
|
|
import json
|
|
|
|
import pytest
|
|
|
|
from EvoScientist.langgraph_dev import manager
|
|
|
|
|
|
def test_sidecar_path_is_next_to_pid_file():
|
|
"""Sidecar JSON lives at ``pid_dir / 'langgraph_dev.workspace.json'``."""
|
|
assert (
|
|
manager.RUNTIME.workspace_sidecar
|
|
== manager.RUNTIME.pid_dir / "langgraph_dev.workspace.json"
|
|
)
|
|
|
|
|
|
def test_write_workspace_sidecar_records_workspace_and_pid(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""``_write_workspace_sidecar`` writes JSON with workspace + pid."""
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"),
|
|
)
|
|
workspace = tmp_path / "some" / "ws"
|
|
manager._write_workspace_sidecar(workspace_dir=workspace, pid=12345)
|
|
data = json.loads((tmp_path / "ws.json").read_text())
|
|
assert data["workspace"] == str(workspace)
|
|
assert data["pid"] == 12345
|
|
|
|
|
|
def test_read_workspace_sidecar_returns_none_when_missing(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "absent.json"),
|
|
)
|
|
assert manager._read_workspace_sidecar() is None
|
|
|
|
|
|
def test_read_workspace_sidecar_returns_none_on_corrupt_json(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
sidecar = tmp_path / "bad.json"
|
|
sidecar.write_text("not json at all")
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=sidecar),
|
|
)
|
|
assert manager._read_workspace_sidecar() is None
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"payload",
|
|
[
|
|
"[]", # valid JSON, wrong top-level type
|
|
"{}", # valid JSON dict but missing "workspace"
|
|
'{"pid": 12345}', # valid JSON dict but missing "workspace"
|
|
'"just a string"', # valid JSON scalar
|
|
"null", # valid JSON null
|
|
'{"workspace": null}', # workspace present but null → Path(None) TypeError
|
|
'{"workspace": []}', # workspace present but list → Path([]) TypeError
|
|
'{"workspace": 12345}', # workspace present but int → Path(int) TypeError
|
|
'{"workspace": ""}', # workspace present but empty string → resolves to cwd
|
|
],
|
|
)
|
|
def test_read_workspace_sidecar_returns_none_on_wrong_schema(
|
|
payload, tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""JSON that parses but doesn't match the expected schema must degrade
|
|
to None — otherwise the reuse branch's ``Path(sidecar["workspace"]).resolve()``
|
|
would raise KeyError/TypeError or silently resolve to cwd, surfacing as
|
|
an unhandled exception or producing a misleading match check."""
|
|
sidecar = tmp_path / "schema.json"
|
|
sidecar.write_text(payload)
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=sidecar),
|
|
)
|
|
assert manager._read_workspace_sidecar() is None
|
|
|
|
|
|
def test_read_workspace_sidecar_round_trip(tmp_path, monkeypatch, runtime_paths):
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "rt.json"),
|
|
)
|
|
workspace = tmp_path / "x" / "y"
|
|
manager._write_workspace_sidecar(workspace_dir=workspace, pid=42)
|
|
data = manager._read_workspace_sidecar()
|
|
assert data == {"workspace": str(workspace), "pid": 42}
|
|
|
|
|
|
def test_workspace_mismatch_error_is_runtime_error_subclass():
|
|
assert issubclass(manager.WorkspaceMismatchError, RuntimeError)
|
|
|
|
|
|
def test_ensure_langgraph_dev_refuses_on_workspace_mismatch(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""Cross-process reuse with sidecar workspace ≠ requested → raises."""
|
|
ws_a = tmp_path / "A"
|
|
ws_b = tmp_path / "B"
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"),
|
|
)
|
|
manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999)
|
|
|
|
monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True)
|
|
monkeypatch.setattr(manager, "_PROCESS", None)
|
|
monkeypatch.setattr(manager, "_PROCESS_WORKSPACE", None)
|
|
|
|
cfg = manager.EvoScientistConfig()
|
|
cfg.enable_async_subagents = True
|
|
with pytest.raises(manager.WorkspaceMismatchError) as exc:
|
|
manager.ensure_langgraph_dev(cfg, workspace_dir=ws_b)
|
|
assert str(ws_a.resolve()) in str(exc.value)
|
|
assert str(ws_b) in str(exc.value)
|
|
|
|
|
|
def test_ensure_langgraph_dev_refuses_on_mismatch_with_stale_process(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""A non-None but dead ``_PROCESS`` handle must NOT short-circuit the
|
|
sidecar check. Regression for the case where our subprocess exited and a
|
|
different langgraph dev rebound the port."""
|
|
ws_a = tmp_path / "A"
|
|
ws_b = tmp_path / "B"
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"),
|
|
)
|
|
manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999)
|
|
|
|
class _DeadProc:
|
|
def poll(self):
|
|
return 1 # non-None → process has exited
|
|
|
|
monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True)
|
|
monkeypatch.setattr(manager, "_PROCESS", _DeadProc())
|
|
# _PROCESS_WORKSPACE matches ws_b so the earlier owned-restart branch (which
|
|
# also gates on _PROCESS.poll() is None) doesn't fire on this dead handle.
|
|
monkeypatch.setattr(manager, "_PROCESS_WORKSPACE", ws_b)
|
|
|
|
cfg = manager.EvoScientistConfig()
|
|
cfg.enable_async_subagents = True
|
|
with pytest.raises(manager.WorkspaceMismatchError):
|
|
manager.ensure_langgraph_dev(cfg, workspace_dir=ws_b)
|
|
|
|
|
|
def test_ensure_langgraph_dev_reuses_when_workspace_matches(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""Cross-process reuse with matching sidecar workspace → no raise."""
|
|
ws_a = tmp_path / "A"
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"),
|
|
)
|
|
manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999)
|
|
|
|
monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True)
|
|
monkeypatch.setattr(manager, "_PROCESS", None)
|
|
monkeypatch.setattr(manager, "_PROCESS_WORKSPACE", None)
|
|
|
|
cfg = manager.EvoScientistConfig()
|
|
cfg.enable_async_subagents = True
|
|
# Should NOT raise.
|
|
manager.ensure_langgraph_dev(cfg, workspace_dir=ws_a)
|
|
|
|
|
|
def test_ensure_langgraph_dev_reuses_when_sidecar_missing(
|
|
tmp_path, monkeypatch, runtime_paths
|
|
):
|
|
"""Backward compat: pre-feature langgraph dev (no sidecar) falls back to
|
|
the existing log-warning behavior rather than refusing."""
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "absent.json"),
|
|
)
|
|
monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True)
|
|
monkeypatch.setattr(manager, "_PROCESS", None)
|
|
monkeypatch.setattr(manager, "_PROCESS_WORKSPACE", None)
|
|
|
|
cfg = manager.EvoScientistConfig()
|
|
cfg.enable_async_subagents = True
|
|
# Should NOT raise — degrades to the prior reuse-with-warning branch.
|
|
manager.ensure_langgraph_dev(cfg, workspace_dir=tmp_path / "B")
|
|
|
|
|
|
def test_stop_langgraph_dev_removes_sidecar(tmp_path, monkeypatch, runtime_paths):
|
|
"""``stop_langgraph_dev`` should unlink the sidecar alongside the PID file."""
|
|
sidecar = tmp_path / "ws.json"
|
|
pid_file = tmp_path / "pid.txt"
|
|
monkeypatch.setattr(
|
|
manager,
|
|
"RUNTIME",
|
|
dataclasses.replace(
|
|
runtime_paths, workspace_sidecar=sidecar, pid_file=pid_file
|
|
),
|
|
)
|
|
manager._write_workspace_sidecar(workspace_dir=tmp_path / "x", pid=42)
|
|
assert sidecar.exists()
|
|
|
|
# _PROCESS is None so stop_langgraph_dev shouldn't try to kill anything;
|
|
# we're only verifying the sidecar cleanup path here.
|
|
monkeypatch.setattr(manager, "_PROCESS", None)
|
|
manager.stop_langgraph_dev()
|
|
assert not sidecar.exists()
|