From 0cf208fd87e8fc79e6e0c081d1999ba1785f0085 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 10:42:26 +0100 Subject: [PATCH 1/4] feat(tools): add tool factory wiring RootConfig.git_clone to GitCloneTool Create build_default_tools() factory that instantiates all 11 built-in workspace tools with config-driven parameters. The factory connects RootConfig.git_clone (GitCloneNetworkPolicy) to GitCloneTool's network_policy parameter, making YAML-configured hostname_allowlist functional. A convenience wrapper build_default_tools_from_config() extracts the policy from RootConfig automatically. --- src/synthorg/observability/events/tool.py | 3 + src/synthorg/tools/__init__.py | 3 + src/synthorg/tools/factory.py | 120 +++++++++++ .../tools/test_factory_integration.py | 66 ++++++ tests/unit/tools/test_factory.py | 190 ++++++++++++++++++ 5 files changed, 382 insertions(+) create mode 100644 src/synthorg/tools/factory.py create mode 100644 tests/integration/tools/test_factory_integration.py create mode 100644 tests/unit/tools/test_factory.py diff --git a/src/synthorg/observability/events/tool.py b/src/synthorg/observability/events/tool.py index aa980ce30c..2c3db1567e 100644 --- a/src/synthorg/observability/events/tool.py +++ b/src/synthorg/observability/events/tool.py @@ -23,6 +23,9 @@ TOOL_PERMISSION_CHECKER_CREATED: Final[str] = "tool.permission.checker_created" TOOL_PERMISSION_FILTERED: Final[str] = "tool.permission.filtered" +# ── Factory events ───────────────────────────────────────────── +TOOL_FACTORY_BUILT: Final[str] = "tool.factory.built" + # ── File system tool events ────────────────────────────────────── TOOL_FS_READ: Final[str] = "tool.fs.read" TOOL_FS_WRITE: Final[str] = "tool.fs.write" diff --git a/src/synthorg/tools/__init__.py b/src/synthorg/tools/__init__.py index 92a617a792..d64315cecb 100644 --- a/src/synthorg/tools/__init__.py +++ b/src/synthorg/tools/__init__.py @@ -11,6 +11,7 @@ ToolPermissionDeniedError, ) from .examples.echo import EchoTool +from .factory import build_default_tools, build_default_tools_from_config from .file_system import ( BaseFileSystemTool, DeleteFileTool, @@ -86,4 +87,6 @@ "ToolPermissionDeniedError", "ToolRegistry", "WriteFileTool", + "build_default_tools", + "build_default_tools_from_config", ] diff --git a/src/synthorg/tools/factory.py b/src/synthorg/tools/factory.py new file mode 100644 index 0000000000..c85c987a96 --- /dev/null +++ b/src/synthorg/tools/factory.py @@ -0,0 +1,120 @@ +"""Tool factory — instantiate built-in workspace tools with config-driven parameters. + +Provides ``build_default_tools`` (core factory) and +``build_default_tools_from_config`` (convenience wrapper that +extracts parameters from a ``RootConfig``). Both return +``tuple[BaseTool, ...]`` so callers can extend before wrapping +in a ``ToolRegistry``. +""" + +from typing import TYPE_CHECKING + +from synthorg.observability import get_logger +from synthorg.observability.events.tool import TOOL_FACTORY_BUILT +from synthorg.tools.file_system import ( + DeleteFileTool, + EditFileTool, + ListDirectoryTool, + ReadFileTool, + WriteFileTool, +) +from synthorg.tools.git_tools import ( + GitBranchTool, + GitCloneTool, + GitCommitTool, + GitDiffTool, + GitLogTool, + GitStatusTool, +) + +if TYPE_CHECKING: + from pathlib import Path + + from synthorg.config.schema import RootConfig + from synthorg.tools.base import BaseTool + from synthorg.tools.git_url_validator import GitCloneNetworkPolicy + from synthorg.tools.sandbox.protocol import SandboxBackend + +logger = get_logger(__name__) + + +def build_default_tools( + *, + workspace: Path, + git_clone_policy: GitCloneNetworkPolicy | None = None, + sandbox: SandboxBackend | None = None, +) -> tuple[BaseTool, ...]: + """Instantiate all built-in workspace tools. + + Returns a sorted tuple of tool instances ready for use or + extension before wrapping in a ``ToolRegistry``. + + Args: + workspace: Absolute path to the agent workspace root. + git_clone_policy: Network policy for git clone SSRF + prevention. ``None`` uses the default (block all + private IPs, empty hostname allowlist). + sandbox: Optional sandbox backend for git subprocess + isolation. + + Returns: + Tuple of ``BaseTool`` instances sorted by name. + """ + fs_tools: list[BaseTool] = [ + ReadFileTool(workspace_root=workspace), + WriteFileTool(workspace_root=workspace), + EditFileTool(workspace_root=workspace), + ListDirectoryTool(workspace_root=workspace), + DeleteFileTool(workspace_root=workspace), + ] + + git_tools: list[BaseTool] = [ + GitStatusTool(workspace=workspace, sandbox=sandbox), + GitLogTool(workspace=workspace, sandbox=sandbox), + GitDiffTool(workspace=workspace, sandbox=sandbox), + GitBranchTool(workspace=workspace, sandbox=sandbox), + GitCommitTool(workspace=workspace, sandbox=sandbox), + GitCloneTool( + workspace=workspace, + sandbox=sandbox, + network_policy=git_clone_policy, + ), + ] + + all_tools = sorted(fs_tools + git_tools, key=lambda t: t.name) + result = tuple(all_tools) + + logger.info( + TOOL_FACTORY_BUILT, + tool_count=len(result), + tools=tuple(t.name for t in result), + ) + + return result + + +def build_default_tools_from_config( + *, + workspace: Path, + config: RootConfig, + sandbox: SandboxBackend | None = None, +) -> tuple[BaseTool, ...]: + """Build default tools using parameters from a ``RootConfig``. + + Convenience wrapper that extracts ``config.git_clone`` and + delegates to :func:`build_default_tools`. + + Args: + workspace: Absolute path to the agent workspace root. + config: Validated root configuration. + sandbox: Optional sandbox backend for git subprocess + isolation. + + Returns: + Tuple of ``BaseTool`` instances sorted by name. + """ + return build_default_tools( + workspace=workspace, + git_clone_policy=config.git_clone, + sandbox=sandbox, + ) diff --git a/tests/integration/tools/test_factory_integration.py b/tests/integration/tools/test_factory_integration.py new file mode 100644 index 0000000000..affdbb20bf --- /dev/null +++ b/tests/integration/tools/test_factory_integration.py @@ -0,0 +1,66 @@ +"""Integration tests for tool factory + config loading pipeline.""" + +from pathlib import Path + +import pytest + +from synthorg.config.loader import load_config_from_string +from synthorg.tools.factory import ( + build_default_tools, + build_default_tools_from_config, +) +from synthorg.tools.git_tools import GitCloneTool +from synthorg.tools.registry import ToolRegistry + + +@pytest.mark.integration +class TestToolFactoryConfigIntegration: + """End-to-end: YAML config → RootConfig → factory → tool instances.""" + + def test_yaml_with_allowlist_wires_to_clone_tool( + self, + tmp_path: Path, + ) -> None: + """YAML hostname_allowlist propagates to GitCloneTool.""" + yaml = """\ +company_name: test-corp +git_clone: + hostname_allowlist: + - internal.example.com +""" + config = load_config_from_string(yaml) + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == ("internal.example.com",) + + def test_yaml_empty_git_clone_uses_defaults( + self, + tmp_path: Path, + ) -> None: + """Empty git_clone section yields default policy.""" + yaml = """\ +company_name: test-corp +git_clone: {} +""" + config = load_config_from_string(yaml) + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == () + assert clone._network_policy.block_private_ips is True + + def test_factory_tools_form_valid_registry( + self, + tmp_path: Path, + ) -> None: + """Factory output can be wrapped in ToolRegistry without errors.""" + tools = build_default_tools(workspace=tmp_path) + registry = ToolRegistry(tools) + assert len(list(registry.all_tools())) == 11 diff --git a/tests/unit/tools/test_factory.py b/tests/unit/tools/test_factory.py new file mode 100644 index 0000000000..3c19c7a21f --- /dev/null +++ b/tests/unit/tools/test_factory.py @@ -0,0 +1,190 @@ +"""Unit tests for the tool factory module.""" + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from synthorg.tools._git_base import _BaseGitTool +from synthorg.tools.base import BaseTool +from synthorg.tools.factory import ( + build_default_tools, + build_default_tools_from_config, +) +from synthorg.tools.file_system import BaseFileSystemTool +from synthorg.tools.git_tools import GitCloneTool +from synthorg.tools.git_url_validator import GitCloneNetworkPolicy + +_EXPECTED_TOOL_NAMES: tuple[str, ...] = ( + "delete_file", + "edit_file", + "git_branch", + "git_clone", + "git_commit", + "git_diff", + "git_log", + "git_status", + "list_directory", + "read_file", + "write_file", +) + + +@pytest.mark.unit +class TestBuildDefaultTools: + """Tests for build_default_tools().""" + + def test_returns_all_expected_tools( + self, + tmp_path: Path, + ) -> None: + """Factory returns all 11 built-in tools sorted by name.""" + tools = build_default_tools(workspace=tmp_path) + names = tuple(t.name for t in tools) + assert names == _EXPECTED_TOOL_NAMES + + def test_git_clone_receives_custom_policy( + self, + tmp_path: Path, + ) -> None: + """Custom GitCloneNetworkPolicy is wired to clone tool.""" + policy = GitCloneNetworkPolicy( + hostname_allowlist=("internal.example.com",), + ) + tools = build_default_tools( + workspace=tmp_path, + git_clone_policy=policy, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == ("internal.example.com",) + + def test_git_clone_default_policy_when_none( + self, + tmp_path: Path, + ) -> None: + """Without explicit policy, clone tool uses defaults.""" + tools = build_default_tools(workspace=tmp_path) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + default = GitCloneNetworkPolicy() + assert clone._network_policy.hostname_allowlist == default.hostname_allowlist + assert clone._network_policy.block_private_ips == default.block_private_ips + + def test_file_system_tools_receive_workspace( + self, + tmp_path: Path, + ) -> None: + """All file system tools have correct workspace_root.""" + tools = build_default_tools(workspace=tmp_path) + fs_names = { + "read_file", + "write_file", + "edit_file", + "list_directory", + "delete_file", + } + for tool in tools: + if tool.name in fs_names: + assert isinstance(tool, BaseFileSystemTool) + assert tool.workspace_root == tmp_path.resolve() + + def test_git_tools_receive_workspace( + self, + tmp_path: Path, + ) -> None: + """All git tools have correct workspace path.""" + tools = build_default_tools(workspace=tmp_path) + git_names = { + "git_status", + "git_log", + "git_diff", + "git_branch", + "git_commit", + "git_clone", + } + for tool in tools: + if tool.name in git_names: + assert isinstance(tool, _BaseGitTool) + assert tool._workspace == tmp_path.resolve() + + def test_sandbox_passed_to_git_tools( + self, + tmp_path: Path, + ) -> None: + """Sandbox backend is forwarded to all git tools.""" + mock_sandbox = MagicMock() + tools = build_default_tools( + workspace=tmp_path, + sandbox=mock_sandbox, + ) + git_names = { + "git_status", + "git_log", + "git_diff", + "git_branch", + "git_commit", + "git_clone", + } + for tool in tools: + if tool.name in git_names: + assert isinstance(tool, _BaseGitTool) + assert tool._sandbox is mock_sandbox + + def test_returns_tuple(self, tmp_path: Path) -> None: + """Factory returns a tuple, not a list or other sequence.""" + tools = build_default_tools(workspace=tmp_path) + assert isinstance(tools, tuple) + + def test_all_tools_are_base_tool_instances( + self, + tmp_path: Path, + ) -> None: + """Every returned tool is a BaseTool subclass instance.""" + tools = build_default_tools(workspace=tmp_path) + for tool in tools: + assert isinstance(tool, BaseTool) + + +@pytest.mark.unit +class TestBuildDefaultToolsFromConfig: + """Tests for build_default_tools_from_config().""" + + def test_extracts_policy_from_config( + self, + tmp_path: Path, + ) -> None: + """Policy from RootConfig.git_clone flows to clone tool.""" + from synthorg.config.schema import RootConfig + + policy = GitCloneNetworkPolicy( + hostname_allowlist=("git.corp.example.com",), + ) + config = RootConfig( + company_name="test-corp", + git_clone=policy, + ) + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == ("git.corp.example.com",) + + def test_default_config_uses_default_policy( + self, + tmp_path: Path, + ) -> None: + """Default RootConfig yields default network policy.""" + from synthorg.config.schema import RootConfig + + config = RootConfig(company_name="test-corp") + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == () + assert clone._network_policy.block_private_ips is True From e91a62bc3b884649ab68071441df3f294e4792dd Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 11:01:55 +0100 Subject: [PATCH 2/4] refactor(tools): address review findings from 11 agents Pre-reviewed by 11 agents, 10 findings addressed: - Add TOOL_FACTORY_BUILT to CLAUDE.md logging event catalogue - Add factory.py to CLAUDE.md Package Structure tools/ line - Use public .workspace property instead of ._workspace in tests - Add sandbox passthrough test for build_default_tools_from_config - Add block_private_ips=False edge case test - Replace magic number 11 with _EXPECTED_TOOL_COUNT constant - Assert literal values instead of comparing against fresh instance - Rename yaml var to yaml_str to avoid stdlib shadowing - Fix "End-to-end" -> "Integration:" in docstring - Move deferred RootConfig imports to module level - Simplify intermediate variable and normalize separator width --- CLAUDE.md | 4 +- src/synthorg/observability/events/tool.py | 2 +- src/synthorg/tools/factory.py | 3 +- .../tools/test_factory_integration.py | 14 ++++--- tests/unit/tools/test_factory.py | 42 +++++++++++++++---- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 00e567d606..ab68aafb69 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -132,7 +132,7 @@ src/synthorg/ definitions/ # Per-namespace setting definitions (company, providers, memory, budget, security, coordination, observability, backup) security/ # SecOps agent, rule engine (soft-allow/hard-deny, fail-closed), audit log, output scanner, output scan response policies (redact/withhold/log-only/autonomy-tiered), risk classifier, risk tier classifier, action type registry, ToolInvoker security integration, progressive trust (4 strategies: disabled/weighted/per-category/milestone), autonomy levels (presets, resolver, change strategy), timeout policies (park/resume) templates/ # Pre-built company templates, personality presets, and builder - tools/ # Tool registry, built-in tools (file_system/, git, sandbox/, code_runner), git clone SSRF prevention (git_url_validator), MCP bridge (mcp/), role-based access, approval tool (request_human_approval) + tools/ # Tool registry, built-in tools (file_system/, git, sandbox/, code_runner), git clone SSRF prevention (git_url_validator), MCP bridge (mcp/), role-based access, approval tool (request_human_approval), tool factory (build_default_tools, build_default_tools_from_config) web/ # Vue 3 + PrimeVue + Tailwind CSS dashboard src/ @@ -196,7 +196,7 @@ site/ # Astro landing page (synthorg.io) - **Every module** with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)` - **Never** use `import logging` / `logging.getLogger()` / `print()` in application code - **Variable name**: always `logger` (not `_logger`, not `log`) -- **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`, `CFO_ANOMALY_DETECTED` from `events.cfo`, `CONFLICT_DETECTED` from `events.conflict`, `MEETING_STARTED` from `events.meeting`, `MEETING_SCHEDULER_STARTED` from `events.meeting`, `MEETING_SCHEDULER_ERROR` from `events.meeting`, `MEETING_SCHEDULER_STOPPED` from `events.meeting`, `MEETING_PERIODIC_TRIGGERED` from `events.meeting`, `MEETING_EVENT_TRIGGERED` from `events.meeting`, `MEETING_PARTICIPANTS_RESOLVED` from `events.meeting`, `MEETING_NO_PARTICIPANTS` from `events.meeting`, `MEETING_NOT_FOUND` from `events.meeting`, `CLASSIFICATION_START` from `events.classification`, `CONSOLIDATION_START` from `events.consolidation`, `ORG_MEMORY_QUERY_START` from `events.org_memory`, `API_REQUEST_STARTED` from `events.api`, `API_REQUEST_COMPLETED` from `events.api`, `API_REQUEST_ERROR` from `events.api`, `API_ROUTE_NOT_FOUND` from `events.api`, `API_HEALTH_CHECK` from `events.api`, `API_COORDINATION_STARTED` from `events.api`, `API_COORDINATION_COMPLETED` from `events.api`, `API_COORDINATION_FAILED` from `events.api`, `API_COORDINATION_AGENT_RESOLVE_FAILED` from `events.api`, `API_CONTENT_NEGOTIATED` from `events.api`, `API_CORRELATION_FALLBACK` from `events.api`, `API_ACCEPT_PARSE_FAILED` from `events.api`, `API_WS_TICKET_ISSUED` from `events.api`, `API_WS_TICKET_CONSUMED` from `events.api`, `API_WS_TICKET_EXPIRED` from `events.api`, `API_WS_TICKET_INVALID` from `events.api`, `API_WS_TICKET_CLEANUP` from `events.api`, `CODE_RUNNER_EXECUTE_START` from `events.code_runner`, `DOCKER_EXECUTE_START` from `events.docker`, `MCP_INVOKE_START` from `events.mcp`, `SECURITY_EVALUATE_START` from `events.security`, `HR_HIRING_REQUEST_CREATED` from `events.hr`, `PERF_METRIC_RECORDED` from `events.performance`, `PERF_LLM_SAMPLE_STARTED` from `events.performance`, `PERF_LLM_SAMPLE_COMPLETED` from `events.performance`, `PERF_LLM_SAMPLE_FAILED` from `events.performance`, `PERF_OVERRIDE_SET` from `events.performance`, `PERF_OVERRIDE_CLEARED` from `events.performance`, `PERF_OVERRIDE_APPLIED` from `events.performance`, `PERF_OVERRIDE_EXPIRED` from `events.performance`, `TRUST_EVALUATE_START` from `events.trust`, `PROMOTION_EVALUATE_START` from `events.promotion`, `PROMPT_BUILD_START` from `events.prompt`, `MEMORY_RETRIEVAL_START` from `events.memory`, `MEMORY_BACKEND_CONNECTED` from `events.memory`, `MEMORY_ENTRY_STORED` from `events.memory`, `MEMORY_BACKEND_SYSTEM_ERROR` from `events.memory`, `MEMORY_RRF_FUSION_COMPLETE` from `events.memory`, `MEMORY_RRF_VALIDATION_FAILED` from `events.memory`, `AUTONOMY_ACTION_AUTO_APPROVED` from `events.autonomy`, `TIMEOUT_POLICY_EVALUATED` from `events.timeout`, `PERSISTENCE_AUDIT_ENTRY_SAVED` from `events.persistence`, `TASK_ENGINE_STARTED` from `events.task_engine`, `COORDINATION_STARTED` from `events.coordination`, `COORDINATION_FACTORY_BUILT` from `events.coordination`, `COMMUNICATION_DISPATCH_START` from `events.communication`, `COMPANY_STARTED` from `events.company`, `CONFIG_LOADED` from `events.config`, `CORRELATION_ID_CREATED` from `events.correlation`, `DECOMPOSITION_STARTED` from `events.decomposition`, `DELEGATION_STARTED` from `events.delegation`, `EXECUTION_LOOP_START` from `events.execution`, `CHECKPOINT_SAVED` from `events.checkpoint`, `PERSISTENCE_CHECKPOINT_SAVED` from `events.persistence`, `GIT_COMMAND_START` from `events.git`, `GIT_CLONE_URL_REJECTED` from `events.git`, `GIT_CLONE_SSRF_BLOCKED` from `events.git`, `GIT_CLONE_DNS_FAILED` from `events.git`, `GIT_CLONE_SSRF_DISABLED` from `events.git`, `PARALLEL_GROUP_START` from `events.parallel`, `PERSONALITY_LOADED` from `events.personality`, `QUOTA_CHECKED` from `events.quota`, `ROLE_ASSIGNED` from `events.role`, `ROUTING_STARTED` from `events.routing`, `SANDBOX_EXECUTE_START` from `events.sandbox`, `TASK_CREATED` from `events.task`, `TASK_ASSIGNMENT_STARTED` from `events.task_assignment`, `TASK_ROUTING_STARTED` from `events.task_routing`, `TEMPLATE_LOADED` from `events.template`, `TOOL_INVOKE_START` from `events.tool`, `TOOL_OUTPUT_WITHHELD` from `events.tool`, `WORKSPACE_CREATED` from `events.workspace`, `APPROVAL_GATE_ESCALATION_DETECTED` from `events.approval_gate`, `APPROVAL_GATE_ESCALATION_FAILED` from `events.approval_gate`, `APPROVAL_GATE_INITIALIZED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFIED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFY_FAILED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARKED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARK_FAILED` from `events.approval_gate`, `APPROVAL_GATE_PARK_TASKLESS` from `events.approval_gate`, `APPROVAL_GATE_RESUME_STARTED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_RESUMED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_DELETE_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_TRIGGERED` from `events.approval_gate`, `APPROVAL_GATE_NO_PARKED_CONTEXT` from `events.approval_gate`, `APPROVAL_GATE_LOOP_WIRING_WARNING` from `events.approval_gate`, `STAGNATION_CHECK_PERFORMED` from `events.stagnation`, `STAGNATION_DETECTED` from `events.stagnation`, `STAGNATION_CORRECTION_INJECTED` from `events.stagnation`, `STAGNATION_TERMINATED` from `events.stagnation`, `PERSISTENCE_AGENT_STATE_SAVED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_FETCHED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_ACTIVE_QUERIED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_DELETED` from `events.persistence`, `SETTINGS_VALUE_SET` from `events.settings`, `SETTINGS_VALUE_DELETED` from `events.settings`, `SETTINGS_VALUE_RESOLVED` from `events.settings`, `SETTINGS_CACHE_INVALIDATED` from `events.settings`, `SETTINGS_ENCRYPTION_ERROR` from `events.settings`, `SETTINGS_VALIDATION_FAILED` from `events.settings`, `SETTINGS_NOTIFICATION_PUBLISHED` from `events.settings`, `SETTINGS_NOTIFICATION_FAILED` from `events.settings`, `SETTINGS_FETCH_FAILED` from `events.settings`, `SETTINGS_SET_FAILED` from `events.settings`, `SETTINGS_DELETE_FAILED` from `events.settings`, `SETTINGS_NOT_FOUND` from `events.settings`, `SETTINGS_REGISTRY_DUPLICATE` from `events.settings`, `SETTINGS_CONFIG_PATH_MISS` from `events.settings`). Import directly: `from synthorg.observability.events. import EVENT_CONSTANT` +- **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`, `CFO_ANOMALY_DETECTED` from `events.cfo`, `CONFLICT_DETECTED` from `events.conflict`, `MEETING_STARTED` from `events.meeting`, `MEETING_SCHEDULER_STARTED` from `events.meeting`, `MEETING_SCHEDULER_ERROR` from `events.meeting`, `MEETING_SCHEDULER_STOPPED` from `events.meeting`, `MEETING_PERIODIC_TRIGGERED` from `events.meeting`, `MEETING_EVENT_TRIGGERED` from `events.meeting`, `MEETING_PARTICIPANTS_RESOLVED` from `events.meeting`, `MEETING_NO_PARTICIPANTS` from `events.meeting`, `MEETING_NOT_FOUND` from `events.meeting`, `CLASSIFICATION_START` from `events.classification`, `CONSOLIDATION_START` from `events.consolidation`, `ORG_MEMORY_QUERY_START` from `events.org_memory`, `API_REQUEST_STARTED` from `events.api`, `API_REQUEST_COMPLETED` from `events.api`, `API_REQUEST_ERROR` from `events.api`, `API_ROUTE_NOT_FOUND` from `events.api`, `API_HEALTH_CHECK` from `events.api`, `API_COORDINATION_STARTED` from `events.api`, `API_COORDINATION_COMPLETED` from `events.api`, `API_COORDINATION_FAILED` from `events.api`, `API_COORDINATION_AGENT_RESOLVE_FAILED` from `events.api`, `API_CONTENT_NEGOTIATED` from `events.api`, `API_CORRELATION_FALLBACK` from `events.api`, `API_ACCEPT_PARSE_FAILED` from `events.api`, `API_WS_TICKET_ISSUED` from `events.api`, `API_WS_TICKET_CONSUMED` from `events.api`, `API_WS_TICKET_EXPIRED` from `events.api`, `API_WS_TICKET_INVALID` from `events.api`, `API_WS_TICKET_CLEANUP` from `events.api`, `CODE_RUNNER_EXECUTE_START` from `events.code_runner`, `DOCKER_EXECUTE_START` from `events.docker`, `MCP_INVOKE_START` from `events.mcp`, `SECURITY_EVALUATE_START` from `events.security`, `HR_HIRING_REQUEST_CREATED` from `events.hr`, `PERF_METRIC_RECORDED` from `events.performance`, `PERF_LLM_SAMPLE_STARTED` from `events.performance`, `PERF_LLM_SAMPLE_COMPLETED` from `events.performance`, `PERF_LLM_SAMPLE_FAILED` from `events.performance`, `PERF_OVERRIDE_SET` from `events.performance`, `PERF_OVERRIDE_CLEARED` from `events.performance`, `PERF_OVERRIDE_APPLIED` from `events.performance`, `PERF_OVERRIDE_EXPIRED` from `events.performance`, `TRUST_EVALUATE_START` from `events.trust`, `PROMOTION_EVALUATE_START` from `events.promotion`, `PROMPT_BUILD_START` from `events.prompt`, `MEMORY_RETRIEVAL_START` from `events.memory`, `MEMORY_BACKEND_CONNECTED` from `events.memory`, `MEMORY_ENTRY_STORED` from `events.memory`, `MEMORY_BACKEND_SYSTEM_ERROR` from `events.memory`, `MEMORY_RRF_FUSION_COMPLETE` from `events.memory`, `MEMORY_RRF_VALIDATION_FAILED` from `events.memory`, `AUTONOMY_ACTION_AUTO_APPROVED` from `events.autonomy`, `TIMEOUT_POLICY_EVALUATED` from `events.timeout`, `PERSISTENCE_AUDIT_ENTRY_SAVED` from `events.persistence`, `TASK_ENGINE_STARTED` from `events.task_engine`, `COORDINATION_STARTED` from `events.coordination`, `COORDINATION_FACTORY_BUILT` from `events.coordination`, `COMMUNICATION_DISPATCH_START` from `events.communication`, `COMPANY_STARTED` from `events.company`, `CONFIG_LOADED` from `events.config`, `CORRELATION_ID_CREATED` from `events.correlation`, `DECOMPOSITION_STARTED` from `events.decomposition`, `DELEGATION_STARTED` from `events.delegation`, `EXECUTION_LOOP_START` from `events.execution`, `CHECKPOINT_SAVED` from `events.checkpoint`, `PERSISTENCE_CHECKPOINT_SAVED` from `events.persistence`, `GIT_COMMAND_START` from `events.git`, `GIT_CLONE_URL_REJECTED` from `events.git`, `GIT_CLONE_SSRF_BLOCKED` from `events.git`, `GIT_CLONE_DNS_FAILED` from `events.git`, `GIT_CLONE_SSRF_DISABLED` from `events.git`, `PARALLEL_GROUP_START` from `events.parallel`, `PERSONALITY_LOADED` from `events.personality`, `QUOTA_CHECKED` from `events.quota`, `ROLE_ASSIGNED` from `events.role`, `ROUTING_STARTED` from `events.routing`, `SANDBOX_EXECUTE_START` from `events.sandbox`, `TASK_CREATED` from `events.task`, `TASK_ASSIGNMENT_STARTED` from `events.task_assignment`, `TASK_ROUTING_STARTED` from `events.task_routing`, `TEMPLATE_LOADED` from `events.template`, `TOOL_INVOKE_START` from `events.tool`, `TOOL_OUTPUT_WITHHELD` from `events.tool`, `TOOL_FACTORY_BUILT` from `events.tool`, `WORKSPACE_CREATED` from `events.workspace`, `APPROVAL_GATE_ESCALATION_DETECTED` from `events.approval_gate`, `APPROVAL_GATE_ESCALATION_FAILED` from `events.approval_gate`, `APPROVAL_GATE_INITIALIZED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFIED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFY_FAILED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARKED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARK_FAILED` from `events.approval_gate`, `APPROVAL_GATE_PARK_TASKLESS` from `events.approval_gate`, `APPROVAL_GATE_RESUME_STARTED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_RESUMED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_DELETE_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_TRIGGERED` from `events.approval_gate`, `APPROVAL_GATE_NO_PARKED_CONTEXT` from `events.approval_gate`, `APPROVAL_GATE_LOOP_WIRING_WARNING` from `events.approval_gate`, `STAGNATION_CHECK_PERFORMED` from `events.stagnation`, `STAGNATION_DETECTED` from `events.stagnation`, `STAGNATION_CORRECTION_INJECTED` from `events.stagnation`, `STAGNATION_TERMINATED` from `events.stagnation`, `PERSISTENCE_AGENT_STATE_SAVED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_FETCHED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_ACTIVE_QUERIED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_DELETED` from `events.persistence`, `SETTINGS_VALUE_SET` from `events.settings`, `SETTINGS_VALUE_DELETED` from `events.settings`, `SETTINGS_VALUE_RESOLVED` from `events.settings`, `SETTINGS_CACHE_INVALIDATED` from `events.settings`, `SETTINGS_ENCRYPTION_ERROR` from `events.settings`, `SETTINGS_VALIDATION_FAILED` from `events.settings`, `SETTINGS_NOTIFICATION_PUBLISHED` from `events.settings`, `SETTINGS_NOTIFICATION_FAILED` from `events.settings`, `SETTINGS_FETCH_FAILED` from `events.settings`, `SETTINGS_SET_FAILED` from `events.settings`, `SETTINGS_DELETE_FAILED` from `events.settings`, `SETTINGS_NOT_FOUND` from `events.settings`, `SETTINGS_REGISTRY_DUPLICATE` from `events.settings`, `SETTINGS_CONFIG_PATH_MISS` from `events.settings`). Import directly: `from synthorg.observability.events. import EVENT_CONSTANT` - **Structured kwargs**: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)` - **All error paths** must log at WARNING or ERROR with context before raising - **All state transitions** must log at INFO diff --git a/src/synthorg/observability/events/tool.py b/src/synthorg/observability/events/tool.py index 2c3db1567e..bc50d6f468 100644 --- a/src/synthorg/observability/events/tool.py +++ b/src/synthorg/observability/events/tool.py @@ -23,7 +23,7 @@ TOOL_PERMISSION_CHECKER_CREATED: Final[str] = "tool.permission.checker_created" TOOL_PERMISSION_FILTERED: Final[str] = "tool.permission.filtered" -# ── Factory events ───────────────────────────────────────────── +# ── Factory events ────────────────────────────────────────────── TOOL_FACTORY_BUILT: Final[str] = "tool.factory.built" # ── File system tool events ────────────────────────────────────── diff --git a/src/synthorg/tools/factory.py b/src/synthorg/tools/factory.py index c85c987a96..11cbf82dc5 100644 --- a/src/synthorg/tools/factory.py +++ b/src/synthorg/tools/factory.py @@ -81,8 +81,7 @@ def build_default_tools( ), ] - all_tools = sorted(fs_tools + git_tools, key=lambda t: t.name) - result = tuple(all_tools) + result = tuple(sorted(fs_tools + git_tools, key=lambda t: t.name)) logger.info( TOOL_FACTORY_BUILT, diff --git a/tests/integration/tools/test_factory_integration.py b/tests/integration/tools/test_factory_integration.py index affdbb20bf..3ffac1047e 100644 --- a/tests/integration/tools/test_factory_integration.py +++ b/tests/integration/tools/test_factory_integration.py @@ -12,23 +12,25 @@ from synthorg.tools.git_tools import GitCloneTool from synthorg.tools.registry import ToolRegistry +_EXPECTED_TOOL_COUNT: int = 11 + @pytest.mark.integration class TestToolFactoryConfigIntegration: - """End-to-end: YAML config → RootConfig → factory → tool instances.""" + """Integration: YAML config -> RootConfig -> factory -> tool instances.""" def test_yaml_with_allowlist_wires_to_clone_tool( self, tmp_path: Path, ) -> None: """YAML hostname_allowlist propagates to GitCloneTool.""" - yaml = """\ + yaml_str = """\ company_name: test-corp git_clone: hostname_allowlist: - internal.example.com """ - config = load_config_from_string(yaml) + config = load_config_from_string(yaml_str) tools = build_default_tools_from_config( workspace=tmp_path, config=config, @@ -42,11 +44,11 @@ def test_yaml_empty_git_clone_uses_defaults( tmp_path: Path, ) -> None: """Empty git_clone section yields default policy.""" - yaml = """\ + yaml_str = """\ company_name: test-corp git_clone: {} """ - config = load_config_from_string(yaml) + config = load_config_from_string(yaml_str) tools = build_default_tools_from_config( workspace=tmp_path, config=config, @@ -63,4 +65,4 @@ def test_factory_tools_form_valid_registry( """Factory output can be wrapped in ToolRegistry without errors.""" tools = build_default_tools(workspace=tmp_path) registry = ToolRegistry(tools) - assert len(list(registry.all_tools())) == 11 + assert len(list(registry.all_tools())) == _EXPECTED_TOOL_COUNT diff --git a/tests/unit/tools/test_factory.py b/tests/unit/tools/test_factory.py index 3c19c7a21f..f0767cd2ed 100644 --- a/tests/unit/tools/test_factory.py +++ b/tests/unit/tools/test_factory.py @@ -5,6 +5,7 @@ import pytest +from synthorg.config.schema import RootConfig from synthorg.tools._git_base import _BaseGitTool from synthorg.tools.base import BaseTool from synthorg.tools.factory import ( @@ -67,9 +68,22 @@ def test_git_clone_default_policy_when_none( tools = build_default_tools(workspace=tmp_path) clone = next(t for t in tools if t.name == "git_clone") assert isinstance(clone, GitCloneTool) - default = GitCloneNetworkPolicy() - assert clone._network_policy.hostname_allowlist == default.hostname_allowlist - assert clone._network_policy.block_private_ips == default.block_private_ips + assert clone._network_policy.hostname_allowlist == () + assert clone._network_policy.block_private_ips is True + + def test_git_clone_permissive_policy( + self, + tmp_path: Path, + ) -> None: + """Policy with block_private_ips=False is wired correctly.""" + policy = GitCloneNetworkPolicy(block_private_ips=False) + tools = build_default_tools( + workspace=tmp_path, + git_clone_policy=policy, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.block_private_ips is False def test_file_system_tools_receive_workspace( self, @@ -106,7 +120,7 @@ def test_git_tools_receive_workspace( for tool in tools: if tool.name in git_names: assert isinstance(tool, _BaseGitTool) - assert tool._workspace == tmp_path.resolve() + assert tool.workspace == tmp_path.resolve() def test_sandbox_passed_to_git_tools( self, @@ -155,8 +169,6 @@ def test_extracts_policy_from_config( tmp_path: Path, ) -> None: """Policy from RootConfig.git_clone flows to clone tool.""" - from synthorg.config.schema import RootConfig - policy = GitCloneNetworkPolicy( hostname_allowlist=("git.corp.example.com",), ) @@ -177,8 +189,6 @@ def test_default_config_uses_default_policy( tmp_path: Path, ) -> None: """Default RootConfig yields default network policy.""" - from synthorg.config.schema import RootConfig - config = RootConfig(company_name="test-corp") tools = build_default_tools_from_config( workspace=tmp_path, @@ -188,3 +198,19 @@ def test_default_config_uses_default_policy( assert isinstance(clone, GitCloneTool) assert clone._network_policy.hostname_allowlist == () assert clone._network_policy.block_private_ips is True + + def test_sandbox_passed_through_config_wrapper( + self, + tmp_path: Path, + ) -> None: + """Sandbox arg is forwarded by build_default_tools_from_config.""" + mock_sandbox = MagicMock() + config = RootConfig(company_name="test-corp") + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + sandbox=mock_sandbox, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, _BaseGitTool) + assert clone._sandbox is mock_sandbox From 1706e1ee629f301b2483cb74308d3cc368ba1912 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 12:18:02 +0100 Subject: [PATCH 3/4] refactor(tools): address review findings from local agents, CodeRabbit, and Gemini - Split build_default_tools into private helpers (_build_file_system_tools, _build_git_tools) to satisfy the 50-line function limit - Add workspace absoluteness guard for fail-fast validation before partial tool construction - Add Raises section to both factory function docstrings - Log git_clone_block_private_ips and git_clone_allowlist_size in TOOL_FACTORY_BUILT event for security audit visibility - Add DEBUG-level entry log to build_default_tools_from_config - Add TOOL_FACTORY_ERROR event constant for validation failures - Parametrize three policy-variant unit tests per CLAUDE.md convention - Add workspace validation unit test for new guard - Add integration tests for YAML block_private_ips:false and absent git_clone key - Condense CLAUDE.md event constant mega-list to example + pointer --- CLAUDE.md | 2 +- src/synthorg/observability/events/tool.py | 1 + src/synthorg/tools/factory.py | 99 +++++++++++++------ .../tools/test_factory_integration.py | 37 +++++++ tests/unit/tools/test_factory.py | 66 +++++++------ 5 files changed, 145 insertions(+), 60 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ab68aafb69..d7126ff77e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -196,7 +196,7 @@ site/ # Astro landing page (synthorg.io) - **Every module** with business logic MUST have: `from synthorg.observability import get_logger` then `logger = get_logger(__name__)` - **Never** use `import logging` / `logging.getLogger()` / `print()` in application code - **Variable name**: always `logger` (not `_logger`, not `log`) -- **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `PROVIDER_CALL_START` from `events.provider`, `BUDGET_RECORD_ADDED` from `events.budget`, `CFO_ANOMALY_DETECTED` from `events.cfo`, `CONFLICT_DETECTED` from `events.conflict`, `MEETING_STARTED` from `events.meeting`, `MEETING_SCHEDULER_STARTED` from `events.meeting`, `MEETING_SCHEDULER_ERROR` from `events.meeting`, `MEETING_SCHEDULER_STOPPED` from `events.meeting`, `MEETING_PERIODIC_TRIGGERED` from `events.meeting`, `MEETING_EVENT_TRIGGERED` from `events.meeting`, `MEETING_PARTICIPANTS_RESOLVED` from `events.meeting`, `MEETING_NO_PARTICIPANTS` from `events.meeting`, `MEETING_NOT_FOUND` from `events.meeting`, `CLASSIFICATION_START` from `events.classification`, `CONSOLIDATION_START` from `events.consolidation`, `ORG_MEMORY_QUERY_START` from `events.org_memory`, `API_REQUEST_STARTED` from `events.api`, `API_REQUEST_COMPLETED` from `events.api`, `API_REQUEST_ERROR` from `events.api`, `API_ROUTE_NOT_FOUND` from `events.api`, `API_HEALTH_CHECK` from `events.api`, `API_COORDINATION_STARTED` from `events.api`, `API_COORDINATION_COMPLETED` from `events.api`, `API_COORDINATION_FAILED` from `events.api`, `API_COORDINATION_AGENT_RESOLVE_FAILED` from `events.api`, `API_CONTENT_NEGOTIATED` from `events.api`, `API_CORRELATION_FALLBACK` from `events.api`, `API_ACCEPT_PARSE_FAILED` from `events.api`, `API_WS_TICKET_ISSUED` from `events.api`, `API_WS_TICKET_CONSUMED` from `events.api`, `API_WS_TICKET_EXPIRED` from `events.api`, `API_WS_TICKET_INVALID` from `events.api`, `API_WS_TICKET_CLEANUP` from `events.api`, `CODE_RUNNER_EXECUTE_START` from `events.code_runner`, `DOCKER_EXECUTE_START` from `events.docker`, `MCP_INVOKE_START` from `events.mcp`, `SECURITY_EVALUATE_START` from `events.security`, `HR_HIRING_REQUEST_CREATED` from `events.hr`, `PERF_METRIC_RECORDED` from `events.performance`, `PERF_LLM_SAMPLE_STARTED` from `events.performance`, `PERF_LLM_SAMPLE_COMPLETED` from `events.performance`, `PERF_LLM_SAMPLE_FAILED` from `events.performance`, `PERF_OVERRIDE_SET` from `events.performance`, `PERF_OVERRIDE_CLEARED` from `events.performance`, `PERF_OVERRIDE_APPLIED` from `events.performance`, `PERF_OVERRIDE_EXPIRED` from `events.performance`, `TRUST_EVALUATE_START` from `events.trust`, `PROMOTION_EVALUATE_START` from `events.promotion`, `PROMPT_BUILD_START` from `events.prompt`, `MEMORY_RETRIEVAL_START` from `events.memory`, `MEMORY_BACKEND_CONNECTED` from `events.memory`, `MEMORY_ENTRY_STORED` from `events.memory`, `MEMORY_BACKEND_SYSTEM_ERROR` from `events.memory`, `MEMORY_RRF_FUSION_COMPLETE` from `events.memory`, `MEMORY_RRF_VALIDATION_FAILED` from `events.memory`, `AUTONOMY_ACTION_AUTO_APPROVED` from `events.autonomy`, `TIMEOUT_POLICY_EVALUATED` from `events.timeout`, `PERSISTENCE_AUDIT_ENTRY_SAVED` from `events.persistence`, `TASK_ENGINE_STARTED` from `events.task_engine`, `COORDINATION_STARTED` from `events.coordination`, `COORDINATION_FACTORY_BUILT` from `events.coordination`, `COMMUNICATION_DISPATCH_START` from `events.communication`, `COMPANY_STARTED` from `events.company`, `CONFIG_LOADED` from `events.config`, `CORRELATION_ID_CREATED` from `events.correlation`, `DECOMPOSITION_STARTED` from `events.decomposition`, `DELEGATION_STARTED` from `events.delegation`, `EXECUTION_LOOP_START` from `events.execution`, `CHECKPOINT_SAVED` from `events.checkpoint`, `PERSISTENCE_CHECKPOINT_SAVED` from `events.persistence`, `GIT_COMMAND_START` from `events.git`, `GIT_CLONE_URL_REJECTED` from `events.git`, `GIT_CLONE_SSRF_BLOCKED` from `events.git`, `GIT_CLONE_DNS_FAILED` from `events.git`, `GIT_CLONE_SSRF_DISABLED` from `events.git`, `PARALLEL_GROUP_START` from `events.parallel`, `PERSONALITY_LOADED` from `events.personality`, `QUOTA_CHECKED` from `events.quota`, `ROLE_ASSIGNED` from `events.role`, `ROUTING_STARTED` from `events.routing`, `SANDBOX_EXECUTE_START` from `events.sandbox`, `TASK_CREATED` from `events.task`, `TASK_ASSIGNMENT_STARTED` from `events.task_assignment`, `TASK_ROUTING_STARTED` from `events.task_routing`, `TEMPLATE_LOADED` from `events.template`, `TOOL_INVOKE_START` from `events.tool`, `TOOL_OUTPUT_WITHHELD` from `events.tool`, `TOOL_FACTORY_BUILT` from `events.tool`, `WORKSPACE_CREATED` from `events.workspace`, `APPROVAL_GATE_ESCALATION_DETECTED` from `events.approval_gate`, `APPROVAL_GATE_ESCALATION_FAILED` from `events.approval_gate`, `APPROVAL_GATE_INITIALIZED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFIED` from `events.approval_gate`, `APPROVAL_GATE_RISK_CLASSIFY_FAILED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARKED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_PARK_FAILED` from `events.approval_gate`, `APPROVAL_GATE_PARK_TASKLESS` from `events.approval_gate`, `APPROVAL_GATE_RESUME_STARTED` from `events.approval_gate`, `APPROVAL_GATE_CONTEXT_RESUMED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_DELETE_FAILED` from `events.approval_gate`, `APPROVAL_GATE_RESUME_TRIGGERED` from `events.approval_gate`, `APPROVAL_GATE_NO_PARKED_CONTEXT` from `events.approval_gate`, `APPROVAL_GATE_LOOP_WIRING_WARNING` from `events.approval_gate`, `STAGNATION_CHECK_PERFORMED` from `events.stagnation`, `STAGNATION_DETECTED` from `events.stagnation`, `STAGNATION_CORRECTION_INJECTED` from `events.stagnation`, `STAGNATION_TERMINATED` from `events.stagnation`, `PERSISTENCE_AGENT_STATE_SAVED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_FETCHED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_ACTIVE_QUERIED` from `events.persistence`, `PERSISTENCE_AGENT_STATE_DELETED` from `events.persistence`, `SETTINGS_VALUE_SET` from `events.settings`, `SETTINGS_VALUE_DELETED` from `events.settings`, `SETTINGS_VALUE_RESOLVED` from `events.settings`, `SETTINGS_CACHE_INVALIDATED` from `events.settings`, `SETTINGS_ENCRYPTION_ERROR` from `events.settings`, `SETTINGS_VALIDATION_FAILED` from `events.settings`, `SETTINGS_NOTIFICATION_PUBLISHED` from `events.settings`, `SETTINGS_NOTIFICATION_FAILED` from `events.settings`, `SETTINGS_FETCH_FAILED` from `events.settings`, `SETTINGS_SET_FAILED` from `events.settings`, `SETTINGS_DELETE_FAILED` from `events.settings`, `SETTINGS_NOT_FOUND` from `events.settings`, `SETTINGS_REGISTRY_DUPLICATE` from `events.settings`, `SETTINGS_CONFIG_PATH_MISS` from `events.settings`). Import directly: `from synthorg.observability.events. import EVENT_CONSTANT` +- **Event names**: always use constants from the domain-specific module under `synthorg.observability.events` (e.g., `API_REQUEST_STARTED` from `events.api`, `TOOL_INVOKE_START` from `events.tool`, `GIT_COMMAND_START` from `events.git`). Each domain has its own module — see `src/synthorg/observability/events/` for the full inventory of constants. Import directly: `from synthorg.observability.events. import EVENT_CONSTANT` - **Structured kwargs**: always `logger.info(EVENT, key=value)` — never `logger.info("msg %s", val)` - **All error paths** must log at WARNING or ERROR with context before raising - **All state transitions** must log at INFO diff --git a/src/synthorg/observability/events/tool.py b/src/synthorg/observability/events/tool.py index bc50d6f468..2a4748df94 100644 --- a/src/synthorg/observability/events/tool.py +++ b/src/synthorg/observability/events/tool.py @@ -25,6 +25,7 @@ # ── Factory events ────────────────────────────────────────────── TOOL_FACTORY_BUILT: Final[str] = "tool.factory.built" +TOOL_FACTORY_ERROR: Final[str] = "tool.factory.error" # ── File system tool events ────────────────────────────────────── TOOL_FS_READ: Final[str] = "tool.fs.read" diff --git a/src/synthorg/tools/factory.py b/src/synthorg/tools/factory.py index 11cbf82dc5..df44b1e478 100644 --- a/src/synthorg/tools/factory.py +++ b/src/synthorg/tools/factory.py @@ -10,7 +10,10 @@ from typing import TYPE_CHECKING from synthorg.observability import get_logger -from synthorg.observability.events.tool import TOOL_FACTORY_BUILT +from synthorg.observability.events.tool import ( + TOOL_FACTORY_BUILT, + TOOL_FACTORY_ERROR, +) from synthorg.tools.file_system import ( DeleteFileTool, EditFileTool, @@ -38,37 +41,28 @@ logger = get_logger(__name__) -def build_default_tools( +def _build_file_system_tools( *, workspace: Path, - git_clone_policy: GitCloneNetworkPolicy | None = None, - sandbox: SandboxBackend | None = None, ) -> tuple[BaseTool, ...]: - """Instantiate all built-in workspace tools. - - Returns a sorted tuple of tool instances ready for use or - extension before wrapping in a ``ToolRegistry``. - - Args: - workspace: Absolute path to the agent workspace root. - git_clone_policy: Network policy for git clone SSRF - prevention. ``None`` uses the default (block all - private IPs, empty hostname allowlist). - sandbox: Optional sandbox backend for git subprocess - isolation. - - Returns: - Tuple of ``BaseTool`` instances sorted by name. - """ - fs_tools: list[BaseTool] = [ + """Instantiate the five built-in file-system tools.""" + return ( ReadFileTool(workspace_root=workspace), WriteFileTool(workspace_root=workspace), EditFileTool(workspace_root=workspace), ListDirectoryTool(workspace_root=workspace), DeleteFileTool(workspace_root=workspace), - ] + ) + - git_tools: list[BaseTool] = [ +def _build_git_tools( + *, + workspace: Path, + git_clone_policy: GitCloneNetworkPolicy | None, + sandbox: SandboxBackend | None, +) -> tuple[BaseTool, ...]: + """Instantiate the six built-in git tools.""" + return ( GitStatusTool(workspace=workspace, sandbox=sandbox), GitLogTool(workspace=workspace, sandbox=sandbox), GitDiffTool(workspace=workspace, sandbox=sandbox), @@ -79,16 +73,56 @@ def build_default_tools( sandbox=sandbox, network_policy=git_clone_policy, ), - ] + ) - result = tuple(sorted(fs_tools + git_tools, key=lambda t: t.name)) +def build_default_tools( + *, + workspace: Path, + git_clone_policy: GitCloneNetworkPolicy | None = None, + sandbox: SandboxBackend | None = None, +) -> tuple[BaseTool, ...]: + """Instantiate all built-in workspace tools. + + Args: + workspace: Absolute path to the agent workspace root. + git_clone_policy: Network policy for git clone SSRF + prevention. ``None`` uses the default (block all + private IPs, empty hostname allowlist). + sandbox: Optional sandbox backend for subprocess + isolation (passed to git tools). + + Returns: + Sorted tuple of ``BaseTool`` instances. + + Raises: + ValueError: If *workspace* is not an absolute path. + """ + if not workspace.is_absolute(): + msg = f"workspace must be an absolute path, got: {workspace}" + logger.warning(TOOL_FACTORY_ERROR, error=msg) + raise ValueError(msg) + + all_tools = ( + *_build_file_system_tools(workspace=workspace), + *_build_git_tools( + workspace=workspace, + git_clone_policy=git_clone_policy, + sandbox=sandbox, + ), + ) + result = tuple(sorted(all_tools, key=lambda t: t.name)) + + policy = git_clone_policy + block_ips = policy.block_private_ips if policy is not None else True + allowlist_len = len(policy.hostname_allowlist) if policy is not None else 0 logger.info( TOOL_FACTORY_BUILT, tool_count=len(result), tools=tuple(t.name for t in result), + git_clone_block_private_ips=block_ips, + git_clone_allowlist_size=allowlist_len, ) - return result @@ -106,12 +140,19 @@ def build_default_tools_from_config( Args: workspace: Absolute path to the agent workspace root. config: Validated root configuration. - sandbox: Optional sandbox backend for git subprocess - isolation. + sandbox: Optional sandbox backend for subprocess + isolation (passed to git tools). Returns: - Tuple of ``BaseTool`` instances sorted by name. + Sorted tuple of ``BaseTool`` instances. + + Raises: + ValueError: If *workspace* is not an absolute path. """ + logger.debug( + TOOL_FACTORY_BUILT, + source="config", + ) return build_default_tools( workspace=workspace, git_clone_policy=config.git_clone, diff --git a/tests/integration/tools/test_factory_integration.py b/tests/integration/tools/test_factory_integration.py index 3ffac1047e..57bb132737 100644 --- a/tests/integration/tools/test_factory_integration.py +++ b/tests/integration/tools/test_factory_integration.py @@ -47,6 +47,43 @@ def test_yaml_empty_git_clone_uses_defaults( yaml_str = """\ company_name: test-corp git_clone: {} +""" + config = load_config_from_string(yaml_str) + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.hostname_allowlist == () + assert clone._network_policy.block_private_ips is True + + def test_yaml_block_private_ips_false_wires_to_clone_tool( + self, + tmp_path: Path, + ) -> None: + """YAML block_private_ips=false propagates to GitCloneTool.""" + yaml_str = """\ +company_name: test-corp +git_clone: + block_private_ips: false +""" + config = load_config_from_string(yaml_str) + tools = build_default_tools_from_config( + workspace=tmp_path, + config=config, + ) + clone = next(t for t in tools if t.name == "git_clone") + assert isinstance(clone, GitCloneTool) + assert clone._network_policy.block_private_ips is False + + def test_yaml_absent_git_clone_uses_defaults( + self, + tmp_path: Path, + ) -> None: + """YAML without git_clone key uses default policy.""" + yaml_str = """\ +company_name: test-corp """ config = load_config_from_string(yaml_str) tools = build_default_tools_from_config( diff --git a/tests/unit/tools/test_factory.py b/tests/unit/tools/test_factory.py index f0767cd2ed..6838b46d0e 100644 --- a/tests/unit/tools/test_factory.py +++ b/tests/unit/tools/test_factory.py @@ -44,46 +44,52 @@ def test_returns_all_expected_tools( names = tuple(t.name for t in tools) assert names == _EXPECTED_TOOL_NAMES - def test_git_clone_receives_custom_policy( + @pytest.mark.parametrize( + ("policy", "expected_allowlist", "expected_block_ips"), + [ + pytest.param( + GitCloneNetworkPolicy( + hostname_allowlist=("internal.example.com",), + ), + ("internal.example.com",), + True, + id="custom-allowlist", + ), + pytest.param( + None, + (), + True, + id="default-when-none", + ), + pytest.param( + GitCloneNetworkPolicy(block_private_ips=False), + (), + False, + id="permissive-policy", + ), + ], + ) + def test_git_clone_policy_wiring( self, tmp_path: Path, + policy: GitCloneNetworkPolicy | None, + expected_allowlist: tuple[str, ...], + expected_block_ips: bool, ) -> None: - """Custom GitCloneNetworkPolicy is wired to clone tool.""" - policy = GitCloneNetworkPolicy( - hostname_allowlist=("internal.example.com",), - ) + """Network policy is correctly wired to clone tool.""" tools = build_default_tools( workspace=tmp_path, git_clone_policy=policy, ) clone = next(t for t in tools if t.name == "git_clone") assert isinstance(clone, GitCloneTool) - assert clone._network_policy.hostname_allowlist == ("internal.example.com",) - - def test_git_clone_default_policy_when_none( - self, - tmp_path: Path, - ) -> None: - """Without explicit policy, clone tool uses defaults.""" - tools = build_default_tools(workspace=tmp_path) - clone = next(t for t in tools if t.name == "git_clone") - assert isinstance(clone, GitCloneTool) - assert clone._network_policy.hostname_allowlist == () - assert clone._network_policy.block_private_ips is True + assert clone._network_policy.hostname_allowlist == expected_allowlist + assert clone._network_policy.block_private_ips is expected_block_ips - def test_git_clone_permissive_policy( - self, - tmp_path: Path, - ) -> None: - """Policy with block_private_ips=False is wired correctly.""" - policy = GitCloneNetworkPolicy(block_private_ips=False) - tools = build_default_tools( - workspace=tmp_path, - git_clone_policy=policy, - ) - clone = next(t for t in tools if t.name == "git_clone") - assert isinstance(clone, GitCloneTool) - assert clone._network_policy.block_private_ips is False + def test_rejects_relative_workspace(self) -> None: + """Relative workspace path raises ValueError.""" + with pytest.raises(ValueError, match="absolute path"): + build_default_tools(workspace=Path("relative/path")) def test_file_system_tools_receive_workspace( self, From e7a7132029f62357fa314acafa9007d707eb5adc Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Tue, 17 Mar 2026 12:25:01 +0100 Subject: [PATCH 4/4] refactor(tools): use dedicated TOOL_FACTORY_CONFIG_ENTRY event for entry log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid reusing TOOL_FACTORY_BUILT for both the DEBUG entry log in build_default_tools_from_config and the INFO completion log in build_default_tools — distinct event constants prevent confusion in log aggregation and metrics. --- src/synthorg/observability/events/tool.py | 1 + src/synthorg/tools/factory.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/synthorg/observability/events/tool.py b/src/synthorg/observability/events/tool.py index 2a4748df94..64f671b45b 100644 --- a/src/synthorg/observability/events/tool.py +++ b/src/synthorg/observability/events/tool.py @@ -25,6 +25,7 @@ # ── Factory events ────────────────────────────────────────────── TOOL_FACTORY_BUILT: Final[str] = "tool.factory.built" +TOOL_FACTORY_CONFIG_ENTRY: Final[str] = "tool.factory.config_entry" TOOL_FACTORY_ERROR: Final[str] = "tool.factory.error" # ── File system tool events ────────────────────────────────────── diff --git a/src/synthorg/tools/factory.py b/src/synthorg/tools/factory.py index df44b1e478..98bb1dcca6 100644 --- a/src/synthorg/tools/factory.py +++ b/src/synthorg/tools/factory.py @@ -12,6 +12,7 @@ from synthorg.observability import get_logger from synthorg.observability.events.tool import ( TOOL_FACTORY_BUILT, + TOOL_FACTORY_CONFIG_ENTRY, TOOL_FACTORY_ERROR, ) from synthorg.tools.file_system import ( @@ -150,7 +151,7 @@ def build_default_tools_from_config( ValueError: If *workspace* is not an absolute path. """ logger.debug( - TOOL_FACTORY_BUILT, + TOOL_FACTORY_CONFIG_ENTRY, source="config", ) return build_default_tools(