From 1d718582ac092db5a4eac70e551cfd1681be19d4 Mon Sep 17 00:00:00 2001 From: worldmozara Date: Fri, 24 Jul 2026 16:32:33 +0800 Subject: [PATCH] core/tools: drop process workspace/todo globals; require bound state --- src/plyngent/cli/app.py | 32 +++--- src/plyngent/cli/limits.py | 9 +- src/plyngent/cli/state.py | 17 +--- src/plyngent/tools/__init__.py | 5 - src/plyngent/tools/catalog.py | 8 +- src/plyngent/tools/context.py | 5 +- src/plyngent/tools/temp_workspace.py | 6 +- src/plyngent/tools/todo.py | 51 +--------- src/plyngent/tools/workspace.py | 105 +++++++------------- tests/test_cli/test_compact_cmd.py | 4 - tests/test_cli/test_limits.py | 4 - tests/test_cli/test_repl_commands.py | 3 - tests/test_cli/test_workspace_resume.py | 2 - tests/test_tools/conftest.py | 15 +-- tests/test_tools/test_catalog.py | 15 ++- tests/test_tools/test_danger.py | 10 +- tests/test_tools/test_todo_session.py | 29 ++---- tests/test_tools/test_workspace.py | 11 +- tests/test_tools/test_workspace_instance.py | 30 ++---- 19 files changed, 126 insertions(+), 235 deletions(-) diff --git a/src/plyngent/cli/app.py b/src/plyngent/cli/app.py index a666c6a..1c71745 100644 --- a/src/plyngent/cli/app.py +++ b/src/plyngent/cli/app.py @@ -26,7 +26,6 @@ from plyngent.config.models import DatabaseConfig from plyngent.memory import MemoryStore from plyngent.prompting import NonInteractiveBackend, configure_prompting from plyngent.runtime import ProviderNotSupportedError, create_client -from plyngent.tools import set_workspace_root if TYPE_CHECKING: from collections.abc import Mapping @@ -115,16 +114,8 @@ def _read_prompt_text(prompt: str | None, *, stdin_isatty: bool) -> str | None: return text or None -def _setup_workspace_and_hooks( - store: ConfigStore, - workspace: Path, - *, - interactive: bool, -) -> None: - _ = set_workspace_root(workspace) - from plyngent.tools import set_path_denylist - - set_path_denylist(store.agent_config.path_denylist or None) +def _setup_hooks(*, interactive: bool) -> None: + """Install interactive limit hooks (workspace policy is set on ReplState).""" if interactive: install_cli_limit_hooks() else: @@ -217,7 +208,7 @@ async def _run_chat( # noqa: C901, PLR0912, PLR0915 — chat orchestration if store.recoverable_providers: _warn_recoverable_providers(store.recoverable_providers) - _setup_workspace_and_hooks(store, workspace, interactive=interactive) + _setup_hooks(interactive=interactive) # --yes forces sticky YOLO; else derive from config.confirm_destructive. from plyngent.cli.state import YoloMode @@ -297,6 +288,12 @@ async def _run_chat( # noqa: C901, PLR0912, PLR0915 — chat orchestration interactive_limits=interactive, yolo=yolo, ) + # Path denylist and policy confirm live on instance.workspace (no process bag). + state.instance_state.workspace.path_denylist = tuple(store.agent_config.path_denylist or ()) + if interactive: + from plyngent.cli.limits import prompt_policy_command_confirm + + state.instance_state.workspace.policy_confirm_hook = prompt_policy_command_confirm # Seed cache if we already fetched; else warm in background for Tab. if remote_ids is not None: state.seed_remote_models(remote_ids) @@ -323,20 +320,17 @@ async def _run_chat( # noqa: C901, PLR0912, PLR0915 — chat orchestration return EXIT_OK finally: await memory.close() - from plyngent.tools.workspace import clear_policy_allowed_commands, set_policy_confirm_hook - - set_policy_confirm_hook(None) - clear_policy_allowed_commands() # PTY + temp workspace cleanup via instance shutdown when state exists. state_obj = locals().get("state") if isinstance(state_obj, ReplState): + state_obj.instance_state.workspace.policy_confirm_hook = None + state_obj.instance_state.workspace.policy_allowed_commands.clear() await state_obj.instance_state.shutdown() else: + # No ReplState: only PTY class cleanup (temps require instance allowlist). from plyngent.tools.process.pty_session import PtyManager - from plyngent.tools.temp_workspace import cleanup_temporary_workspaces PtyManager.close_all() - _ = cleanup_temporary_workspaces() def _configure_logging(level: str) -> None: @@ -384,7 +378,7 @@ def main(ctx: click.Context, log_level: str) -> None: ) @click.option("--provider", "provider_name", default=None, help="Provider name from config.") @click.option("--model", default=None, help="Model id.") -@click.option("--tools/--no-tools", default=True, show_default=True, help="Enable DEFAULT_TOOLS.") +@click.option("--tools/--no-tools", default=True, show_default=True, help="Enable catalog tools (local surface).") @click.option( "--workspace", type=click.Path(path_type=Path, file_okay=False, exists=True), diff --git a/src/plyngent/cli/limits.py b/src/plyngent/cli/limits.py index b20fa63..553d01e 100644 --- a/src/plyngent/cli/limits.py +++ b/src/plyngent/cli/limits.py @@ -350,9 +350,10 @@ async def prompt_workspace_mismatch_async( def install_cli_limit_hooks() -> None: - """Register interactive continue hooks, policy confirm, and prompt cancel-pause.""" + """Register interactive continue hooks and prompt cancel-pause. + + Command denylist policy confirm is set on :class:`~plyngent.tools.context.InstanceState` + when the REPL starts (no process-global hook). + """ configure_prompting(pause_factory=pause_task_cancel_for_prompt) PtyManager.set_limit_continue_hook(prompt_continue_limit) - from plyngent.tools.workspace import set_policy_confirm_hook - - set_policy_confirm_hook(prompt_policy_command_confirm) diff --git a/src/plyngent/cli/state.py b/src/plyngent/cli/state.py index ba5d0eb..9bb0e4a 100644 --- a/src/plyngent/cli/state.py +++ b/src/plyngent/cli/state.py @@ -18,11 +18,7 @@ from plyngent.cli.models_source import ( ) from plyngent.memory.database.store import normalize_workspace from plyngent.runtime import create_client -from plyngent.tools import ( - InstanceState, - SessionState, - set_workspace_root, -) +from plyngent.tools import InstanceState, SessionState from plyngent.tools.view import MemoryViewStore, session_data_view if TYPE_CHECKING: @@ -80,8 +76,6 @@ class ReplState: self.workspace = Path(self.workspace).expanduser().resolve() self.instance_state.workspace_root = self.workspace self.instance_state.workspace.root = self.workspace - # Process-global workspace remains for tools/tests that run without instance bind. - _ = set_workspace_root(self.workspace) self.session_state = self._session_data_for_todo() self.agent = self._make_agent() self.sync_display_flags() @@ -165,11 +159,10 @@ class ReplState: ) def _bind_todo_tools(self) -> None: - """Bind session/instance state for tools (prefer context over process globals). + """Bind session/instance state for tools. Seeds ``session.data["todo"]`` from the live stack and attaches - :meth:`_todo_on_change` so memory persist no longer depends on - process-global ``set_todo_stack(on_change=...)``. + :meth:`_todo_on_change` for memory persist. """ self.session_state.session_id = self.session_id self.session_state.todo = self.todo_stack @@ -397,10 +390,9 @@ class ReplState: from plyngent.cli.selection import select_model, select_provider from plyngent.runtime import ProviderNotSupportedError - from plyngent.tools import set_path_denylist self.config.reload() - set_path_denylist(self.config.agent_config.path_denylist or None) + self.instance_state.workspace.path_denylist = tuple(self.config.agent_config.path_denylist or ()) selectable = self.config.selectable_providers() preferred_provider = self.provider_name if self.provider_name in selectable else None @@ -438,7 +430,6 @@ class ReplState: self.workspace = resolved self.instance_state.workspace_root = resolved self.instance_state.workspace.root = resolved - _ = set_workspace_root(resolved) if hasattr(self, "agent") and self.agent.tools is not None: self.agent.tools.set_instance_state(self.instance_state) diff --git a/src/plyngent/tools/__init__.py b/src/plyngent/tools/__init__.py index df63995..29d9448 100644 --- a/src/plyngent/tools/__init__.py +++ b/src/plyngent/tools/__init__.py @@ -41,8 +41,6 @@ from .process import write_pty_keys as write_pty_keys from .temp_workspace import cleanup_temporary_workspaces as cleanup_temporary_workspaces from .temp_workspace import new_temporary_workspace as new_temporary_workspace from .todo import TODO_TOOLS as TODO_TOOLS -from .todo import get_todo_stack as get_todo_stack -from .todo import set_todo_stack as set_todo_stack from .todo import todo_clear as todo_clear from .todo import todo_list as todo_list from .todo import todo_pop as todo_pop @@ -82,6 +80,3 @@ from .workspace import set_path_denylist as set_path_denylist from .workspace import set_policy_confirm_hook as set_policy_confirm_hook from .workspace import set_policy_confirm_timeout as set_policy_confirm_timeout from .workspace import set_workspace_root as set_workspace_root - -# Deprecated alias: prefer default_tool_definitions() (catalog select). -DEFAULT_TOOLS = [*FILE_TOOLS, *PROCESS_TOOLS, *VCS_TOOLS, *CHAT_TOOLS, *TODO_TOOLS] diff --git a/src/plyngent/tools/catalog.py b/src/plyngent/tools/catalog.py index 19ed9e2..186f0fe 100644 --- a/src/plyngent/tools/catalog.py +++ b/src/plyngent/tools/catalog.py @@ -211,9 +211,13 @@ def _ensure_builtin_definitions_registered(catalog: ToolCatalog) -> None: import time (usually the process catalog). Test ``catalog_scope(empty=True)`` overrides must still see builtins for ``default_tool_definitions``. """ - from plyngent.tools import DEFAULT_TOOLS + from plyngent.tools.chat import CHAT_TOOLS + from plyngent.tools.file import FILE_TOOLS + from plyngent.tools.process import PROCESS_TOOLS + from plyngent.tools.todo import TODO_TOOLS + from plyngent.tools.vcs import VCS_TOOLS - for definition in DEFAULT_TOOLS: + for definition in (*FILE_TOOLS, *PROCESS_TOOLS, *VCS_TOOLS, *CHAT_TOOLS, *TODO_TOOLS): if catalog.get(definition.name) is None: catalog.register(definition, source=_BUILTIN_SOURCE) diff --git a/src/plyngent/tools/context.py b/src/plyngent/tools/context.py index bf88b19..571bd38 100644 --- a/src/plyngent/tools/context.py +++ b/src/plyngent/tools/context.py @@ -70,10 +70,9 @@ class SessionState: session_id: int | str | None = None data: PersistentDataView[Any] = field(default_factory=_default_session_data) - # Live domain object during migration (also under data["todo"] when views bind). + # Live domain object (also under data["todo"] when views bind). todo: TodoStack | None = None - # Host hook after todo tools mutate (e.g. CLI memory persist). Prefer this over - # process-global set_todo_stack(on_change=...) when a session is bound. + # Host hook after todo tools mutate (e.g. CLI memory persist). on_todo_change: Callable[[], None] | None = None # Soft-confirm grants live map: tool_name → True (Phase 1 key is tool name). # Durable copy lives under data["grants"] (see plyngent.tools.grants). diff --git a/src/plyngent/tools/temp_workspace.py b/src/plyngent/tools/temp_workspace.py index 8c0f72f..7d6fd47 100644 --- a/src/plyngent/tools/temp_workspace.py +++ b/src/plyngent/tools/temp_workspace.py @@ -47,8 +47,12 @@ def cleanup_temporary_workspaces() -> int: """Remove directories created by :func:`new_temporary_workspace` (chat exit). Only deletes paths still under the system temp dir. Returns the number of - directories removed. + directories removed. No-op when no instance is bound. """ + from plyngent.tools.context import get_instance + + if get_instance() is None: + return 0 removed = 0 for path in pop_owned_temporary_workspaces(): if not _is_under_system_temp(path): diff --git a/src/plyngent/tools/todo.py b/src/plyngent/tools/todo.py index 7adb0f8..af416c5 100644 --- a/src/plyngent/tools/todo.py +++ b/src/plyngent/tools/todo.py @@ -9,84 +9,41 @@ if TYPE_CHECKING: from plyngent.agent.todo_stack import TodoStack, TodoStatus -_stack: TodoStack | None = None -_on_change: Callable[[], None] | None = None - _VALID_STATUS = frozenset({"pending", "in_progress", "done", "cancelled"}) -def set_todo_stack(stack: TodoStack | None, *, on_change: Callable[[], None] | None = None) -> None: - """Bind the session todo stack for tool handlers (and optional persist hook). - - Process-global bind remains for CLI/agent hosts that keep a live - :class:`~plyngent.agent.todo_stack.TodoStack` outside the view store. - Prefer session ``data["todo"]`` transactions when available. - """ - global _stack, _on_change # noqa: PLW0603 — intentional process bind - _stack = stack - _on_change = on_change - - -def get_todo_stack() -> TodoStack | None: - return _stack - - def _notify() -> None: - """Fire host persist hooks: prefer session.on_todo_change, else process bind.""" + """Fire host persist hook on the bound session (if any).""" from plyngent.tools.context import get_session session = get_session() if session is not None and session.on_todo_change is not None: session.on_todo_change() - return - if _on_change is not None: - _on_change() - - -def _process_stack() -> TodoStack: - if _stack is None: - msg = "todo stack is not available in this session" - raise RuntimeError(msg) - return _stack async def _with_todo_stack(mutator: Callable[[TodoStack], str]) -> str: """Run *mutator* against the session todo stack and publish changes. - Prefer:: + Requires a bound :class:`~plyngent.tools.context.SessionState`:: async with session.data: stack = session.data["todo"].typed(TodoStack) ... - - When a host already bound ``session.todo`` / process ``set_todo_stack``, - mutate that live object and still refresh the view buffer when a session - is bound so commits stay consistent. """ from plyngent.agent.todo_stack import TodoStack - from plyngent.tools.context import get_session - - session = get_session() - if session is None: - result = mutator(_process_stack()) - _notify() - return result + from plyngent.tools.context import require_session + session = require_session() result = "" async with session.data as data: - # Prefer host-bound live stack (CLI keeps TodoStack for nags / memory). if session.todo is not None: stack = session.todo - elif _stack is not None: - stack = _stack - session.todo = stack else: stack = data["todo"].typed(TodoStack) session.todo = stack # Keep view domain + buffer in sync for durable commit. data["todo"].store(stack) result = mutator(stack) - # View commit serialized to_raw; host on_todo_change may persist CLI memory. _notify() return result diff --git a/src/plyngent/tools/workspace.py b/src/plyngent/tools/workspace.py index 1774b44..6dfa469 100644 --- a/src/plyngent/tools/workspace.py +++ b/src/plyngent/tools/workspace.py @@ -51,9 +51,8 @@ class WorkspaceError(ValueError): class WorkspacePolicy: """Workspace path/command policy for one agent host / instance. - Prefer hanging this on :class:`~plyngent.tools.context.InstanceState.workspace`. - Module-level APIs still fall back to a process-global policy when no instance - is bound (tests and early boot). + Lives on :class:`~plyngent.tools.context.InstanceState.workspace`. There is + no process-global policy bag — hosts must bind instance state around tool use. """ root: Path | None = None @@ -66,79 +65,54 @@ class WorkspacePolicy: policy_confirm_timeout_seconds: float = DEFAULT_POLICY_CONFIRM_TIMEOUT_SECONDS -# Process fallback when no InstanceState is bound. -_process_policy = WorkspacePolicy() - - def _bound_instance() -> InstanceState | None: from plyngent.tools.context import get_instance return get_instance() -def active_workspace_policy() -> WorkspacePolicy: - """Return the policy bag for the bound instance, else the process fallback.""" +def require_bound_instance() -> InstanceState: + """Return the bound instance or raise :class:`WorkspaceError`.""" instance = _bound_instance() - if instance is not None: - return instance.workspace - return _process_policy + if instance is None: + msg = "instance state is not bound; host must set InstanceState around tool execution" + raise WorkspaceError(msg) + return instance + + +def active_workspace_policy() -> WorkspacePolicy: + """Return the policy bag for the bound instance (required).""" + return require_bound_instance().workspace def set_workspace_root(root: Path | str) -> Path: - """Set the workspace root used by tools; returns the resolved root. - - Writes the active policy bag (instance when bound, else process) and keeps - ``instance.workspace_root`` in sync for callers that read the facet. - """ + """Set the workspace root on the bound instance; returns the resolved root.""" path = Path(root).expanduser().resolve() if not path.is_dir(): msg = f"workspace root is not a directory: {path}" raise WorkspaceError(msg) - policy = active_workspace_policy() - policy.root = path - instance = _bound_instance() - if instance is not None: - instance.workspace_root = path - # When bound, also keep process root as a bootstrapped default for tools - # that briefly run without context (compat). - if _process_policy.root is None: - _process_policy.root = path - else: - _process_policy.root = path + instance = require_bound_instance() + instance.workspace.root = path + instance.workspace_root = path return path def get_workspace_root() -> Path: - """Return the configured workspace root. - - Prefers a bound instance's ``workspace_root`` / policy root; otherwise the - process-global root from :func:`set_workspace_root`. - """ - instance = _bound_instance() - if instance is not None: - if instance.workspace_root is not None: - return instance.workspace_root - if instance.workspace.root is not None: - return instance.workspace.root - if _process_policy.root is None: - msg = "workspace root is not set; call set_workspace_root() first" - raise WorkspaceError(msg) - return _process_policy.root + """Return the bound instance workspace root.""" + instance = require_bound_instance() + if instance.workspace_root is not None: + return instance.workspace_root + if instance.workspace.root is not None: + return instance.workspace.root + msg = "workspace root is not set on the bound instance" + raise WorkspaceError(msg) def clear_workspace_root() -> None: - """Clear workspace root (mainly for tests). Does not clear allowlist. - - When an instance is bound, clears both the instance facet and the process - fallback so ``get_workspace_root`` cannot revive the old process root. - """ - policy = active_workspace_policy() - policy.root = None - instance = _bound_instance() - if instance is not None: - instance.workspace_root = None - instance.workspace.root = None - _process_policy.root = None + """Clear workspace root on the bound instance (mainly for tests).""" + instance = require_bound_instance() + instance.workspace.root = None + instance.workspace_root = None def set_path_denylist(patterns: list[str] | tuple[str, ...] | None) -> None: @@ -155,7 +129,6 @@ def set_command_denylist(names: list[str] | tuple[str, ...] | frozenset[str] | N """Set denied command basenames (None restores defaults).""" policy = active_workspace_policy() policy.command_denylist = DEFAULT_COMMAND_DENYLIST if names is None else frozenset(names) - # Session overrides only make sense against the active denylist. policy.policy_allowed_commands &= policy.command_denylist @@ -190,7 +163,7 @@ def clear_policy_allowed_commands() -> None: def grant_policy_command(basename: str) -> None: - """Allow *basename* for the rest of this process despite the denylist.""" + """Allow *basename* for this instance despite the denylist.""" name = basename.strip() if name: active_workspace_policy().policy_allowed_commands.add(name) @@ -249,22 +222,17 @@ def remove_workspace_allowlist(root: Path | str) -> None: policy.allowlist.remove(path) -def _primary_roots(policy: WorkspacePolicy) -> list[Path]: - """Primary workspace roots: bound instance facet then policy root.""" +def _primary_roots(instance: InstanceState, policy: WorkspacePolicy) -> list[Path]: roots: list[Path] = [] - instance = _bound_instance() - if instance is not None and instance.workspace_root is not None: + if instance.workspace_root is not None: roots.append(instance.workspace_root) if policy.root is not None and policy.root not in roots: roots.append(policy.root) - # Process root as last resort when instance policy has no root yet. - if _process_policy.root is not None and _process_policy.root not in roots: - roots.append(_process_policy.root) return roots -def _under_any_root(resolved: Path, policy: WorkspacePolicy) -> bool: - roots = _primary_roots(policy) +def _under_any_root(resolved: Path, instance: InstanceState, policy: WorkspacePolicy) -> bool: + roots = _primary_roots(instance, policy) roots.extend(policy.allowlist) for root in roots: try: @@ -281,13 +249,14 @@ def resolve_path(path: str | Path) -> Path: Relative paths resolve against the **primary** workspace root. Absolute paths may also land under a temporary workspace allowlist entry. """ - policy = active_workspace_policy() + instance = require_bound_instance() + policy = instance.workspace root = get_workspace_root() candidate = Path(path) if not candidate.is_absolute(): candidate = root / candidate resolved = candidate.expanduser().resolve() - if not _under_any_root(resolved, policy): + if not _under_any_root(resolved, instance, policy): msg = f"path escapes workspace root ({root}): {path}" raise WorkspaceError(msg) # Normalize separators so denylist entries like ``/secrets/`` match on Windows. diff --git a/tests/test_cli/test_compact_cmd.py b/tests/test_cli/test_compact_cmd.py index 20a6d6f..8ecc0be 100644 --- a/tests/test_cli/test_compact_cmd.py +++ b/tests/test_cli/test_compact_cmd.py @@ -18,7 +18,6 @@ from plyngent.lmproto.openai_compatible.model import ( UserChatMessage, ) from plyngent.memory import MemoryStore -from plyngent.tools import set_workspace_root if TYPE_CHECKING: from collections.abc import AsyncIterator @@ -59,7 +58,6 @@ class SummaryClient: async def test_compact_to_new_session(tmp_path: Path) -> None: - _ = set_workspace_root(tmp_path) memory = await MemoryStore.open(DatabaseConfig()) try: provider = OpenAIProvider(access_key_or_token="sk-test") @@ -131,7 +129,6 @@ async def test_compact_to_new_session(tmp_path: Path) -> None: async def test_rebuild_client_preserves_persist_cursor(tmp_path: Path) -> None: - _ = set_workspace_root(tmp_path) memory = await MemoryStore.open(DatabaseConfig()) try: provider = OpenAIProvider(access_key_or_token="sk-test") @@ -166,7 +163,6 @@ async def test_rebuild_client_preserves_persist_cursor(tmp_path: Path) -> None: async def test_compact_empty_fails(tmp_path: Path) -> None: - _ = set_workspace_root(tmp_path) memory = await MemoryStore.open(DatabaseConfig()) try: provider = OpenAIProvider(access_key_or_token="sk-test") diff --git a/tests/test_cli/test_limits.py b/tests/test_cli/test_limits.py index a51ed19..9058706 100644 --- a/tests/test_cli/test_limits.py +++ b/tests/test_cli/test_limits.py @@ -41,13 +41,9 @@ def test_format_tool_confirm_box_multiline() -> None: def test_install_cli_limit_hooks() -> None: - from plyngent.tools.workspace import get_policy_confirm_hook, set_policy_confirm_hook - install_cli_limit_hooks() assert callable(getattr(PtyManager, "_limit_continue", None)) - assert get_policy_confirm_hook() is not None PtyManager.set_limit_continue_hook(None) - set_policy_confirm_hook(None) # Backend remains usable after install. assert get_prompt_backend().is_interactive() or True diff --git a/tests/test_cli/test_repl_commands.py b/tests/test_cli/test_repl_commands.py index 9de4175..228523b 100644 --- a/tests/test_cli/test_repl_commands.py +++ b/tests/test_cli/test_repl_commands.py @@ -18,7 +18,6 @@ from plyngent.lmproto.openai_compatible.model import ( ChatCompletionsParam, ) from plyngent.memory import MemoryStore -from plyngent.tools import set_workspace_root if TYPE_CHECKING: from collections.abc import AsyncIterator @@ -68,7 +67,6 @@ class DummyClient: @pytest.fixture async def state(tmp_path: Path) -> AsyncIterator[ReplState]: - _ = set_workspace_root(tmp_path) memory = await MemoryStore.open(DatabaseConfig()) provider = OpenAIProvider(access_key_or_token="sk-test") config = ConfigStore(path=tmp_path / "plyngent.toml", document=tomlkit.document()) @@ -433,7 +431,6 @@ async def test_yolo_toggle(state: ReplState, capsys: pytest.CaptureFixture[str]) async def test_yolo_rebuilds_tool_registry(tmp_path: Path) -> None: - _ = set_workspace_root(tmp_path) memory = await MemoryStore.open(DatabaseConfig()) provider = OpenAIProvider(access_key_or_token="sk-test") config = ConfigStore(path=tmp_path / "plyngent.toml", document=tomlkit.document()) diff --git a/tests/test_cli/test_workspace_resume.py b/tests/test_cli/test_workspace_resume.py index a48c4dd..5875845 100644 --- a/tests/test_cli/test_workspace_resume.py +++ b/tests/test_cli/test_workspace_resume.py @@ -19,7 +19,6 @@ from plyngent.lmproto.openai_compatible.model import ( ) from plyngent.memory import MemoryStore from plyngent.memory.database.store import normalize_workspace -from plyngent.tools import set_workspace_root if TYPE_CHECKING: from collections.abc import AsyncIterator @@ -70,7 +69,6 @@ class DummyClient: def _make_state(memory: MemoryStore, workspace: Path) -> ReplState: - _ = set_workspace_root(workspace) provider = OpenAIProvider(access_key_or_token="sk-test") config = ConfigStore(path=workspace / "plyngent.toml", document=tomlkit.document()) config.providers = {"local": provider} diff --git a/tests/test_tools/conftest.py b/tests/test_tools/conftest.py index 6c92e27..60ded72 100644 --- a/tests/test_tools/conftest.py +++ b/tests/test_tools/conftest.py @@ -4,6 +4,7 @@ from typing import TYPE_CHECKING import pytest +from plyngent.tools.context import InstanceState, bind_instance from plyngent.tools.workspace import ( clear_workspace_allowlist, clear_workspace_root, @@ -17,9 +18,11 @@ if TYPE_CHECKING: @pytest.fixture def workspace(tmp_path: Path) -> Iterator[Path]: - clear_workspace_root() - clear_workspace_allowlist() - root = set_workspace_root(tmp_path) - yield root - clear_workspace_allowlist() - clear_workspace_root() + """Bind an InstanceState workspace for tool tests (no process globals).""" + instance = InstanceState(workspace_root=tmp_path.resolve()) + with bind_instance(instance): + clear_workspace_allowlist() + root = set_workspace_root(tmp_path) + yield root + clear_workspace_allowlist() + clear_workspace_root() diff --git a/tests/test_tools/test_catalog.py b/tests/test_tools/test_catalog.py index 6ed6e99..00c8309 100644 --- a/tests/test_tools/test_catalog.py +++ b/tests/test_tools/test_catalog.py @@ -5,16 +5,21 @@ from __future__ import annotations import inspect from plyngent.agent import ToolTag, tool -from plyngent.tools import DEFAULT_TOOLS, default_tool_definitions, register_builtin_tools +from plyngent.tools import default_tool_definitions, register_builtin_tools from plyngent.tools.catalog import ToolCatalog, ToolSource, catalog_scope, get_catalog, registration_source +from plyngent.tools.chat import CHAT_TOOLS +from plyngent.tools.file import FILE_TOOLS +from plyngent.tools.process import PROCESS_TOOLS +from plyngent.tools.todo import TODO_TOOLS +from plyngent.tools.vcs import VCS_TOOLS -def test_default_tool_names_match_legacy_lists() -> None: +def test_default_tool_names_match_group_lists() -> None: register_builtin_tools() selected = default_tool_definitions(surface="local") - legacy = [t.name for t in DEFAULT_TOOLS] - assert sorted(t.name for t in selected) == sorted(legacy) - assert len(selected) == len(legacy) + groups = [*FILE_TOOLS, *PROCESS_TOOLS, *VCS_TOOLS, *CHAT_TOOLS, *TODO_TOOLS] + assert sorted(t.name for t in selected) == sorted(t.name for t in groups) + assert len(selected) == len(groups) def test_all_builtins_are_async_and_tagged() -> None: diff --git a/tests/test_tools/test_danger.py b/tests/test_tools/test_danger.py index 2778270..9b10e44 100644 --- a/tests/test_tools/test_danger.py +++ b/tests/test_tools/test_danger.py @@ -18,10 +18,12 @@ def test_classify_copy() -> None: def test_classify_write_overwrite_only(tmp_path: Path) -> None: - from plyngent.tools.workspace import clear_workspace_root, set_workspace_root + from plyngent.tools.context import InstanceState, bind_instance + from plyngent.tools.workspace import set_workspace_root - set_workspace_root(tmp_path) - try: + instance = InstanceState(workspace_root=tmp_path.resolve()) + with bind_instance(instance): + _ = set_workspace_root(tmp_path) # New file: no soft-confirm assert classify_danger("write_file", {"path": "new.txt"}) is None # Partial edits: never soft-confirm @@ -36,8 +38,6 @@ def test_classify_write_overwrite_only(tmp_path: Path) -> None: assert classify_danger("copy_path", {"src": "src.txt", "dst": "dst.txt", "overwrite": False}) is None creason = classify_danger("copy_path", {"src": "src.txt", "dst": "dst.txt", "overwrite": True}) assert creason is not None and "overwrite" in creason - finally: - clear_workspace_root() def test_classify_safe_tools() -> None: diff --git a/tests/test_tools/test_todo_session.py b/tests/test_tools/test_todo_session.py index c3e59f5..f9e1c01 100644 --- a/tests/test_tools/test_todo_session.py +++ b/tests/test_tools/test_todo_session.py @@ -1,18 +1,15 @@ -"""Todo tools via session.data PersistentDataView (no process global).""" +"""Todo tools via session.data PersistentDataView (session-bound only).""" from __future__ import annotations from plyngent.agent import ToolRegistry from plyngent.agent.todo_stack import TodoStack from plyngent.tools.context import SessionState, bind_tool_context -from plyngent.tools.todo import TODO_TOOLS, get_todo_stack, set_todo_stack +from plyngent.tools.todo import TODO_TOOLS from plyngent.tools.view import MemoryViewStore, session_data_view -async def test_todo_tools_session_data_without_process_bind() -> None: - set_todo_stack(None) - assert get_todo_stack() is None - +async def test_todo_tools_session_data() -> None: store = MemoryViewStore({}) session = SessionState(session_id="s1", data=session_data_view(store=store)) registry = ToolRegistry(list(TODO_TOOLS), session_state=session) @@ -36,7 +33,6 @@ async def test_todo_tools_session_data_without_process_bind() -> None: async def test_todo_tools_prefer_session_todo_facet() -> None: stack = TodoStack() - set_todo_stack(stack) # process bind present store = MemoryViewStore({}) session = SessionState(session_id="s2", data=session_data_view(store=store), todo=stack) registry = ToolRegistry(list(TODO_TOOLS), session_state=session) @@ -48,11 +44,9 @@ async def test_todo_tools_prefer_session_todo_facet() -> None: loaded = await store.load() assert isinstance(loaded, dict) assert isinstance(loaded.get("todo"), dict) - set_todo_stack(None) async def test_todo_tools_view_isolation_two_sessions() -> None: - set_todo_stack(None) store_a = MemoryViewStore({}) store_b = MemoryViewStore({}) session_a = SessionState(session_id="a", data=session_data_view(store=store_a)) @@ -72,13 +66,11 @@ async def test_todo_tools_view_isolation_two_sessions() -> None: raw_b = TodoStack.from_raw(loaded_b.get("todo")) assert [i.title for i in raw_a.all_items()] == ["only-a"] assert [i.title for i in raw_b.all_items()] == ["only-b"] - # Live facets must not alias across sessions. assert session_a.todo is not session_b.todo async def test_with_bound_context_without_registry_session() -> None: """Handlers honor contextvars when registry does not hold session_state.""" - set_todo_stack(None) store = MemoryViewStore({}) session = SessionState(session_id="ctx", data=session_data_view(store=store)) registry = ToolRegistry(list(TODO_TOOLS), auto_bind_state=False) @@ -90,19 +82,13 @@ async def test_with_bound_context_without_registry_session() -> None: assert isinstance(loaded.get("todo"), dict) -async def test_session_on_todo_change_preferred_over_process() -> None: - """Session.on_todo_change fires instead of process set_todo_stack on_change.""" - set_todo_stack(None) +async def test_session_on_todo_change_fires() -> None: hits: list[str] = [] - def process_hook() -> None: - hits.append("process") - def session_hook() -> None: hits.append("session") stack = TodoStack() - set_todo_stack(stack, on_change=process_hook) store = MemoryViewStore({}) session = SessionState( session_id="hook", @@ -113,4 +99,9 @@ async def test_session_on_todo_change_preferred_over_process() -> None: registry = ToolRegistry(list(TODO_TOOLS), session_state=session) _ = await registry.execute("todo_push", '{"titles": ["H"]}') assert hits == ["session"] - set_todo_stack(None) + + +async def test_todo_without_session_errors() -> None: + registry = ToolRegistry(list(TODO_TOOLS)) + out = await registry.execute("todo_list", "{}") + assert "error" in out.lower() diff --git a/tests/test_tools/test_workspace.py b/tests/test_tools/test_workspace.py index 84aa409..a0a6851 100644 --- a/tests/test_tools/test_workspace.py +++ b/tests/test_tools/test_workspace.py @@ -5,7 +5,6 @@ import pytest from plyngent.tools import ( WorkspaceError, check_command_allowed, - clear_workspace_root, get_workspace_root, resolve_path, set_command_denylist, @@ -129,6 +128,12 @@ def test_command_denylist_policy_confirm_timeout_value(workspace: object) -> Non def test_root_required() -> None: - clear_workspace_root() - with pytest.raises(WorkspaceError, match="not set"): + from plyngent.tools.context import InstanceState, bind_instance + + with bind_instance(InstanceState()), pytest.raises(WorkspaceError, match="workspace root is not set"): + _ = get_workspace_root() + + +def test_unbound_instance_errors() -> None: + with pytest.raises(WorkspaceError, match="instance state is not bound"): _ = get_workspace_root() diff --git a/tests/test_tools/test_workspace_instance.py b/tests/test_tools/test_workspace_instance.py index 0326e93..a871c78 100644 --- a/tests/test_tools/test_workspace_instance.py +++ b/tests/test_tools/test_workspace_instance.py @@ -1,4 +1,4 @@ -"""Workspace root prefers bound InstanceState when set.""" +"""Workspace policy requires bound InstanceState.""" from __future__ import annotations @@ -16,26 +16,18 @@ from plyngent.tools.workspace import ( ) -def test_get_workspace_prefers_instance(tmp_path: Path) -> None: - clear_workspace_root() - other = tmp_path / "other" - other.mkdir() - _ = set_workspace_root(tmp_path) - instance = InstanceState(workspace_root=other) - with bind_instance(instance): - assert get_workspace_root() == other.resolve() - assert get_workspace_root() == tmp_path.resolve() - clear_workspace_root() +def test_unbound_get_workspace_errors() -> None: + with pytest.raises(WorkspaceError, match="instance state is not bound"): + _ = get_workspace_root() -def test_set_workspace_mirrors_to_bound_instance(tmp_path: Path) -> None: - clear_workspace_root() +def test_set_workspace_on_bound_instance(tmp_path: Path) -> None: instance = InstanceState() with bind_instance(instance): path = set_workspace_root(tmp_path) assert instance.workspace_root == path + assert instance.workspace.root == path assert get_workspace_root() == path - clear_workspace_root() def test_clear_clears_bound_instance(tmp_path: Path) -> None: @@ -44,25 +36,20 @@ def test_clear_clears_bound_instance(tmp_path: Path) -> None: _ = set_workspace_root(tmp_path) clear_workspace_root() assert instance.workspace_root is None - with pytest.raises(WorkspaceError): + with pytest.raises(WorkspaceError, match="workspace root is not set"): _ = get_workspace_root() -def test_resolve_path_accepts_instance_root_without_process_global(tmp_path: Path) -> None: - """Instance-only workspace must be a valid resolve root (not only process global).""" - clear_workspace_root() +def test_resolve_path_uses_instance_root(tmp_path: Path) -> None: target = tmp_path / "note.txt" _ = target.write_text("hi", encoding="utf-8") instance = InstanceState(workspace_root=tmp_path.resolve()) with bind_instance(instance): resolved = resolve_path("note.txt") assert resolved == target.resolve() - clear_workspace_root() def test_path_denylist_uses_instance_policy(tmp_path: Path) -> None: - """Path denylist is read from the bound instance policy bag.""" - clear_workspace_root() secret = tmp_path / "secrets" secret.mkdir() _ = (secret / "x.txt").write_text("no", encoding="utf-8") @@ -70,4 +57,3 @@ def test_path_denylist_uses_instance_policy(tmp_path: Path) -> None: instance.workspace.path_denylist = ("/secrets/",) with bind_instance(instance), pytest.raises(WorkspaceError, match="denied by policy"): _ = resolve_path("secrets/x.txt") - clear_workspace_root()