From e6df36e3088628bf1e2766f835777b53898701de Mon Sep 17 00:00:00 2001 From: Fernando Celmer Date: Sat, 15 Aug 2026 14:09:55 -0300 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=AA=B2=20BUG-#21:=20Replace=20process?= =?UTF-8?q?-wide=20workspace-jail=20global=20with=20per-tool-instance=20st?= =?UTF-8?q?ate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pycodeloop/core/config.py | 17 +++--- pycodeloop/tools/__init__.py | 100 +++++++++++++++++++-------------- pycodeloop/tools/_workspace.py | 33 ++++------- pycodeloop/tools/filesystem.py | 49 +++++++++++----- pycodeloop/tools/search.py | 10 +++- 5 files changed, 124 insertions(+), 85 deletions(-) diff --git a/pycodeloop/core/config.py b/pycodeloop/core/config.py index d032562..d038b76 100644 --- a/pycodeloop/core/config.py +++ b/pycodeloop/core/config.py @@ -16,8 +16,7 @@ render_skills_index, ) from pycodeloop.store.sqlite_sessions import SqliteSessions -from pycodeloop.tools import DEFAULT_TOOLS, READ_ONLY_TOOLS, DelegateTool -from pycodeloop.tools._workspace import set_workspace_enabled +from pycodeloop.tools import DelegateTool, build_tools def _default_provider() -> Provider: @@ -141,21 +140,23 @@ def __init__( self.provider = ( provider if provider is not None else _default_provider() ) - self.tools = list(tools) if tools is not None else list(DEFAULT_TOOLS) + self.workspace = workspace + if tools is not None: + self.tools = list(tools) + _, read_only_tools = build_tools(workspace) + else: + default_tools, read_only_tools = build_tools(workspace) + self.tools = default_tools self.system_prompt = system_prompt self.max_turns = max_turns self.max_history_turns = max_history_turns - self.workspace = workspace - set_workspace_enabled(workspace) self.skills = self._discover_skills( skills, skill_sources, skills_refresh ) if delegation: self.tools = [ *self.tools, - DelegateTool( - provider=self.provider, tools=list(READ_ONLY_TOOLS) - ), + DelegateTool(provider=self.provider, tools=read_only_tools), ] if memory: self._load_memory() diff --git a/pycodeloop/tools/__init__.py b/pycodeloop/tools/__init__.py index cb9df02..6bd10ff 100644 --- a/pycodeloop/tools/__init__.py +++ b/pycodeloop/tools/__init__.py @@ -18,49 +18,66 @@ from .sql import SqlQueryTool, SqlSchemaTool from .web import WebFetchTool -_read_file = ReadFileTool() -_list_dir = ListDirTool() -_glob = GlobTool() -_grep = GrepTool() -_web_fetch = WebFetchTool() -_git_status = GitStatusTool() -_git_diff = GitDiffTool() -_git_log = GitLogTool() -_sql_schema = SqlSchemaTool() -_sql_query = SqlQueryTool() -DEFAULT_TOOLS: list[Tool] = [ - _read_file, - WriteFileTool(), - EditFileTool(), - DeleteFileTool(), - _list_dir, - _glob, - _grep, - BashTool(), - _web_fetch, - HttpRequestTool(), - _git_status, - _git_diff, - _git_log, - GitCommitTool(), - EnvTool(), - _sql_schema, - _sql_query, -] +def build_tools(workspace: bool = True) -> tuple[list[Tool], list[Tool]]: + """Fresh `(default_tools, read_only_tools)` for one `Config` — never + shared across `Config`/`Agent` instances, so each one's `workspace` + jail setting stays scoped to itself instead of racing through a + process-wide global. Tools common to both lists (read_file, list_dir, + glob, grep, web_fetch, the read-only git/sql tools) are still + instantiated once per call and shared between the two returned + lists, matching the previous single-instantiation behavior — just + scoped per call instead of per process.""" + read_file = ReadFileTool(workspace=workspace) + list_dir = ListDirTool(workspace=workspace) + glob_tool = GlobTool(workspace=workspace) + grep = GrepTool(workspace=workspace) + web_fetch = WebFetchTool() + git_status = GitStatusTool() + git_diff = GitDiffTool() + git_log = GitLogTool() + sql_schema = SqlSchemaTool() + sql_query = SqlQueryTool() -READ_ONLY_TOOLS: list[Tool] = [ - _read_file, - _list_dir, - _glob, - _grep, - _web_fetch, - _git_status, - _git_diff, - _git_log, - _sql_schema, - _sql_query, -] + default_tools: list[Tool] = [ + read_file, + WriteFileTool(workspace=workspace), + EditFileTool(workspace=workspace), + DeleteFileTool(workspace=workspace), + list_dir, + glob_tool, + grep, + BashTool(), + web_fetch, + HttpRequestTool(), + git_status, + git_diff, + git_log, + GitCommitTool(), + EnvTool(), + sql_schema, + sql_query, + ] + + read_only_tools: list[Tool] = [ + read_file, + list_dir, + glob_tool, + grep, + web_fetch, + git_status, + git_diff, + git_log, + sql_schema, + sql_query, + ] + + return default_tools, read_only_tools + + +DEFAULT_TOOLS: list[Tool] +READ_ONLY_TOOLS: list[Tool] +DEFAULT_TOOLS, READ_ONLY_TOOLS = build_tools() __all__ = [ "Tool", @@ -85,4 +102,5 @@ "WebFetchTool", "DEFAULT_TOOLS", "READ_ONLY_TOOLS", + "build_tools", ] diff --git a/pycodeloop/tools/_workspace.py b/pycodeloop/tools/_workspace.py index 2de5260..79a4798 100644 --- a/pycodeloop/tools/_workspace.py +++ b/pycodeloop/tools/_workspace.py @@ -13,8 +13,6 @@ from pathlib import Path -_enabled = True - class OutsideWorkspaceError(ValueError): def __init__(self, path: str, root: Path) -> None: @@ -25,39 +23,32 @@ def __init__(self, path: str, root: Path) -> None: ) -def set_workspace_enabled(enabled: bool) -> None: - """Toggle the jail process-wide. Callers doing this mid-run (rather - than once at startup via `Config(workspace=...)`) should know it - affects every tool call from that point on, not just their own.""" - global _enabled - _enabled = enabled - - -def is_workspace_enabled() -> bool: - return _enabled - - def workspace_root() -> Path: return Path.cwd().resolve() -def resolve_in_workspace(path: str, root: Path | None = None) -> Path: +def resolve_in_workspace( + path: str, root: Path | None = None, enabled: bool = True +) -> Path: """Resolve `path` under `root` (default: cwd). Raises `OutsideWorkspaceError` if the resolved path escapes the root via - `..` or an absolute path outside it — unless the jail was disabled - via `set_workspace_enabled(False)`, in which case `path` resolves - as-is with no restriction.""" + `..` or an absolute path outside it — unless `enabled` is False, in + which case `path` resolves as-is with no restriction. + + `enabled` is a plain parameter, not process-wide state — each tool + instance decides for itself so two `Config`s with different + `workspace=` settings (or tests running in the same process) can't + interfere with each other.""" target = Path(path).expanduser() + base = (root or workspace_root()).resolve() - if not _enabled: - base = (root or workspace_root()).resolve() + if not enabled: return ( target.resolve() if target.is_absolute() else (base / target).resolve() ) - base = (root or workspace_root()).resolve() if not target.is_absolute(): target = base / target resolved = target.resolve() diff --git a/pycodeloop/tools/filesystem.py b/pycodeloop/tools/filesystem.py index 3e11374..5635550 100644 --- a/pycodeloop/tools/filesystem.py +++ b/pycodeloop/tools/filesystem.py @@ -33,9 +33,9 @@ def _looks_like_diff(text: str) -> bool: return bool(_HUNK_HEADER.search(text) or _DIFF_PREAMBLE.search(text)) -def _resolve_path(path: str) -> Path | ToolResult: +def _resolve_path(path: str, *, workspace: bool = True) -> Path | ToolResult: try: - return resolve_in_workspace(path) + return resolve_in_workspace(path, enabled=workspace) except OutsideWorkspaceError as exc: return ToolResult(output=str(exc), is_error=True) @@ -68,8 +68,13 @@ class ReadFileTool(Tool): "required": ["path"], } - def __init__(self, access_log: FileAccessLog | None = None) -> None: + def __init__( + self, + access_log: FileAccessLog | None = None, + workspace: bool = True, + ) -> None: self._log = access_log or default_log + self._workspace = workspace def run( self, @@ -78,7 +83,7 @@ def run( limit: int | None = None, force: bool = False, ) -> ToolResult: - resolved = _resolve_path(path) + resolved = _resolve_path(path, workspace=self._workspace) if isinstance(resolved, ToolResult): return resolved target = resolved @@ -141,8 +146,13 @@ class WriteFileTool(Tool): } dangerous = True - def __init__(self, access_log: FileAccessLog | None = None) -> None: + def __init__( + self, + access_log: FileAccessLog | None = None, + workspace: bool = True, + ) -> None: self._log = access_log or default_log + self._workspace = workspace def preview(self, path: str, content: str, **_) -> str: if _looks_like_diff(content): @@ -153,7 +163,7 @@ def preview(self, path: str, content: str, **_) -> str: ) try: - target = resolve_in_workspace(path) + target = resolve_in_workspace(path, enabled=self._workspace) except OutsideWorkspaceError as exc: return str(exc) @@ -174,7 +184,7 @@ def run(self, path: str, content: str) -> ToolResult: is_error=True, ) - resolved = _resolve_path(path) + resolved = _resolve_path(path, workspace=self._workspace) if isinstance(resolved, ToolResult): return resolved target = resolved @@ -212,8 +222,13 @@ class EditFileTool(Tool): } dangerous = True - def __init__(self, access_log: FileAccessLog | None = None) -> None: + def __init__( + self, + access_log: FileAccessLog | None = None, + workspace: bool = True, + ) -> None: self._log = access_log or default_log + self._workspace = workspace def _apply( self, path: str, old_string: str, new_string: str, replace_all: bool @@ -229,7 +244,7 @@ def _apply( is_error=True, ) - resolved = _resolve_path(path) + resolved = _resolve_path(path, workspace=self._workspace) if isinstance(resolved, ToolResult): return resolved target = resolved @@ -318,12 +333,17 @@ class DeleteFileTool(Tool): } dangerous = True - def __init__(self, access_log: FileAccessLog | None = None) -> None: + def __init__( + self, + access_log: FileAccessLog | None = None, + workspace: bool = True, + ) -> None: self._log = access_log or default_log + self._workspace = workspace def preview(self, path: str, **_) -> str: try: - target = resolve_in_workspace(path) + target = resolve_in_workspace(path, enabled=self._workspace) except OutsideWorkspaceError as exc: return str(exc) @@ -334,7 +354,7 @@ def preview(self, path: str, **_) -> str: return _diff(path, before, "") def run(self, path: str) -> ToolResult: - resolved = _resolve_path(path) + resolved = _resolve_path(path, workspace=self._workspace) if isinstance(resolved, ToolResult): return resolved target = resolved @@ -359,8 +379,11 @@ class ListDirTool(Tool): "properties": {"path": {"type": "string", "default": "."}}, } + def __init__(self, workspace: bool = True) -> None: + self._workspace = workspace + def run(self, path: str = ".") -> ToolResult: - resolved = _resolve_path(path) + resolved = _resolve_path(path, workspace=self._workspace) if isinstance(resolved, ToolResult): return resolved target = resolved diff --git a/pycodeloop/tools/search.py b/pycodeloop/tools/search.py index 3a02af5..efc892b 100644 --- a/pycodeloop/tools/search.py +++ b/pycodeloop/tools/search.py @@ -44,11 +44,14 @@ class GrepTool(Tool): "required": ["pattern"], } + def __init__(self, workspace: bool = True) -> None: + self._workspace = workspace + def run( self, pattern: str, path: str = ".", max_results: int = 100 ) -> ToolResult: try: - root = resolve_in_workspace(path) + root = resolve_in_workspace(path, enabled=self._workspace) except OutsideWorkspaceError as exc: return ToolResult(output=str(exc), is_error=True) @@ -91,11 +94,14 @@ class GlobTool(Tool): "required": ["pattern"], } + def __init__(self, workspace: bool = True) -> None: + self._workspace = workspace + def run( self, pattern: str, path: str = ".", max_results: int = 100 ) -> ToolResult: try: - root = resolve_in_workspace(path) + root = resolve_in_workspace(path, enabled=self._workspace) except OutsideWorkspaceError as exc: return ToolResult(output=str(exc), is_error=True) From a89acc88922282ab1d6176de32b391364cd39085 Mon Sep 17 00:00:00 2001 From: Fernando Celmer Date: Sat, 15 Aug 2026 14:09:55 -0300 Subject: [PATCH 2/3] =?UTF-8?q?=E2=9D=A4=EF=B8=8F=20TEST-#21:=20Verify=20c?= =?UTF-8?q?oncurrent=20Configs/tool=20instances=20don't=20interfere=20on?= =?UTF-8?q?=20workspace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/core/test_config.py | 32 ++++++++++++++++++++---------- tests/tools/test_workspace.py | 37 ++++++++++++++++++++++------------- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/tests/core/test_config.py b/tests/core/test_config.py index ce8abdc..d48413a 100644 --- a/tests/core/test_config.py +++ b/tests/core/test_config.py @@ -4,10 +4,6 @@ from pycodeloop.core.config import Config from pycodeloop.providers import GenericProvider -from pycodeloop.tools._workspace import ( - is_workspace_enabled, - set_workspace_enabled, -) class TestConfigDelegation(unittest.TestCase): @@ -43,18 +39,34 @@ def _provider(self) -> GenericProvider: url="http://fake/v1/chat/completions", model="fake-model" ) - def setUp(self): - self.addCleanup(set_workspace_enabled, True) + def _read_file_tool(self, config: Config): + return next(t for t in config.tools if t.name == "read_file") def test_workspace_on_by_default(self): - Config(provider=self._provider(), storage=False) + config = Config(provider=self._provider(), storage=False) - self.assertTrue(is_workspace_enabled()) + self.assertTrue(self._read_file_tool(config)._workspace) def test_workspace_false_disables_the_jail(self): - Config(provider=self._provider(), storage=False, workspace=False) + config = Config( + provider=self._provider(), storage=False, workspace=False + ) + + self.assertFalse(self._read_file_tool(config)._workspace) + + def test_two_configs_with_different_workspace_settings_dont_interfere( + self, + ): + """Regression: workspace used to be a process-wide global — the + Config built last would silently win for every Config's tools, + not just its own.""" + jailed = Config(provider=self._provider(), storage=False) + unjailed = Config( + provider=self._provider(), storage=False, workspace=False + ) - self.assertFalse(is_workspace_enabled()) + self.assertTrue(self._read_file_tool(jailed)._workspace) + self.assertFalse(self._read_file_tool(unjailed)._workspace) if __name__ == "__main__": diff --git a/tests/tools/test_workspace.py b/tests/tools/test_workspace.py index 04d5f17..e256fec 100644 --- a/tests/tools/test_workspace.py +++ b/tests/tools/test_workspace.py @@ -9,9 +9,7 @@ from pycodeloop.store.file_access_log import FileAccessLog from pycodeloop.tools._workspace import ( OutsideWorkspaceError, - is_workspace_enabled, resolve_in_workspace, - set_workspace_enabled, ) from pycodeloop.tools.filesystem import ReadFileTool, WriteFileTool from pycodeloop.tools.http_request import HttpRequestTool @@ -68,7 +66,6 @@ def setUp(self): self._cwd = Path.cwd() os.chdir(self.root) self.addCleanup(os.chdir, self._cwd) - self.addCleanup(set_workspace_enabled, True) # A second, unrelated tmp dir standing in for "outside the # workspace" — freshly random per test run, unlike a fixed name @@ -86,33 +83,45 @@ def setUp(self): self.addCleanup(log_patcher.stop) def test_enabled_by_default(self): - self.assertTrue(is_workspace_enabled()) + outside = self.outside_dir / "default-on" + + with self.assertRaises(OutsideWorkspaceError): + resolve_in_workspace(str(outside)) def test_disabled_lets_paths_outside_root_resolve(self): outside = self.outside_dir / "toggle-test" - set_workspace_enabled(False) - resolved = resolve_in_workspace(str(outside)) + resolved = resolve_in_workspace(str(outside), enabled=False) self.assertEqual(resolved, outside) def test_disabled_lets_read_file_read_outside_workspace(self): outside = self.outside_dir / "toggle-read" outside.write_text("secret\n") - set_workspace_enabled(False) - result = ReadFileTool().run(path=str(outside)) + result = ReadFileTool(workspace=False).run(path=str(outside)) self.assertFalse(result.is_error) self.assertIn("secret", result.output) - def test_re_enabling_restores_the_jail(self): - outside = self.outside_dir / "toggle-back" - set_workspace_enabled(False) - set_workspace_enabled(True) + def test_two_tool_instances_with_different_settings_dont_interfere(self): + """Regression: the jail used to be a process-wide global — + constructing a workspace=False tool anywhere would silently + disable the jail for every other tool/instance in the same + process, including ones built with workspace=True earlier.""" + outside = self.outside_dir / "concurrent" + outside.write_text("secret\n") - with self.assertRaises(OutsideWorkspaceError): - resolve_in_workspace(str(outside)) + jailed = ReadFileTool(workspace=True) + unjailed = ReadFileTool(workspace=False) + + jailed_result = jailed.run(path=str(outside)) + unjailed_result = unjailed.run(path=str(outside), force=True) + still_jailed_result = jailed.run(path=str(outside), force=True) + + self.assertTrue(jailed_result.is_error) + self.assertFalse(unjailed_result.is_error) + self.assertTrue(still_jailed_result.is_error) class TestUrlSchemes(unittest.TestCase): From d1a7523cb2b9df81df2d5ed18f27a09f445451d6 Mon Sep 17 00:00:00 2001 From: Fernando Celmer Date: Sat, 15 Aug 2026 14:50:34 -0300 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=AA=B2=20BUG-#21:=20Only=20build=20re?= =?UTF-8?q?ad-only=20tools=20when=20delegation=20is=20enabled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config.__init__ called build_tools(workspace) unconditionally to get read_only_tools for DelegateTool, even when a caller passed a custom tools list and delegation was off — instantiating every built-in tool just to discard the result. Now build_tools() for the read-only subset only runs when delegation=True actually needs it. --- pycodeloop/core/config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pycodeloop/core/config.py b/pycodeloop/core/config.py index d038b76..b99a0b5 100644 --- a/pycodeloop/core/config.py +++ b/pycodeloop/core/config.py @@ -143,9 +143,8 @@ def __init__( self.workspace = workspace if tools is not None: self.tools = list(tools) - _, read_only_tools = build_tools(workspace) else: - default_tools, read_only_tools = build_tools(workspace) + default_tools, _ = build_tools(workspace) self.tools = default_tools self.system_prompt = system_prompt self.max_turns = max_turns @@ -154,6 +153,7 @@ def __init__( skills, skill_sources, skills_refresh ) if delegation: + _, read_only_tools = build_tools(workspace) self.tools = [ *self.tools, DelegateTool(provider=self.provider, tools=read_only_tools),