-
Notifications
You must be signed in to change notification settings - Fork 1
feat(tools): wire per-category sandbox backend selection #534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
58a6d77
64600a7
f6fe155
c3d335e
6b09681
5aecbfd
a31ff5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| """Sandbox backend factory -- build and resolve backends from config. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Provides ``build_sandbox_backends`` to instantiate only the backends | ||||||||||||||||||||||||||||||||||||||||||||
| referenced by a ``SandboxingConfig``, ``resolve_sandbox_for_category`` | ||||||||||||||||||||||||||||||||||||||||||||
| to look up the correct backend for a tool category, and | ||||||||||||||||||||||||||||||||||||||||||||
| ``cleanup_sandbox_backends`` to release resources. | ||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import asyncio | ||||||||||||||||||||||||||||||||||||||||||||
| from types import MappingProxyType | ||||||||||||||||||||||||||||||||||||||||||||
| from typing import TYPE_CHECKING | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.observability import get_logger | ||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.observability.events.sandbox import ( | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_BUILT, | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_CLEANUP, | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_RESOLVE, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.tools.sandbox.docker_sandbox import DockerSandbox | ||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.tools.sandbox.subprocess_sandbox import SubprocessSandbox | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if TYPE_CHECKING: | ||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import Mapping | ||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.core.enums import ToolCategory | ||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.tools.sandbox.protocol import SandboxBackend | ||||||||||||||||||||||||||||||||||||||||||||
| from synthorg.tools.sandbox.sandboxing_config import SandboxingConfig | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| logger = get_logger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| def build_sandbox_backends( | ||||||||||||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||||||||||||
| config: SandboxingConfig, | ||||||||||||||||||||||||||||||||||||||||||||
| workspace: Path, | ||||||||||||||||||||||||||||||||||||||||||||
| ) -> MappingProxyType[str, SandboxBackend]: | ||||||||||||||||||||||||||||||||||||||||||||
| """Build only the backend instances actually referenced by *config*. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Collects which backend names are needed (the default plus all | ||||||||||||||||||||||||||||||||||||||||||||
| override values), then instantiates ``SubprocessSandbox`` and/or | ||||||||||||||||||||||||||||||||||||||||||||
| ``DockerSandbox`` with their respective sub-configs. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||
| config: Top-level sandboxing configuration. | ||||||||||||||||||||||||||||||||||||||||||||
| workspace: Absolute path to the agent workspace root. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||
| A read-only mapping of backend name to backend instance. | ||||||||||||||||||||||||||||||||||||||||||||
| Only contains keys for backends that are actually referenced. | ||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||
| needed: set[str] = {config.default_backend} | ||||||||||||||||||||||||||||||||||||||||||||
| needed.update(config.overrides.values()) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| backends: dict[str, SandboxBackend] = {} | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if "subprocess" in needed: | ||||||||||||||||||||||||||||||||||||||||||||
| backends["subprocess"] = SubprocessSandbox( | ||||||||||||||||||||||||||||||||||||||||||||
| config=config.subprocess, | ||||||||||||||||||||||||||||||||||||||||||||
| workspace=workspace, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if "docker" in needed: | ||||||||||||||||||||||||||||||||||||||||||||
| backends["docker"] = DockerSandbox( | ||||||||||||||||||||||||||||||||||||||||||||
| config=config.docker, | ||||||||||||||||||||||||||||||||||||||||||||
| workspace=workspace, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation uses a series of
Suggested change
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| logger.info( | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_BUILT, | ||||||||||||||||||||||||||||||||||||||||||||
| backends=sorted(backends.keys()), | ||||||||||||||||||||||||||||||||||||||||||||
| default=config.default_backend, | ||||||||||||||||||||||||||||||||||||||||||||
| override_count=len(config.overrides), | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| return MappingProxyType(backends) | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+71
to
+116
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Refactor This function is currently over the project’s 50-line limit; split validation/instantiation into helpers to keep the public function focused. ♻️ Proposed refactor split+def _collect_needed_backends(config: SandboxingConfig) -> set[str]:
+ needed: set[str] = {config.default_backend}
+ needed.update(config.overrides.values())
+ return needed
+
+def _validate_needed_backends(needed: set[str]) -> None:
+ unknown = needed - _KNOWN_BACKENDS
+ if unknown:
+ msg = (
+ f"Unrecognized sandbox backend name(s): {sorted(unknown)}; "
+ f"known backends: {sorted(_KNOWN_BACKENDS)}"
+ )
+ logger.error(SANDBOX_FACTORY_BUILD_FAILED, error=msg)
+ raise ValueError(msg)
+
def build_sandbox_backends(
@@
- needed: set[str] = {config.default_backend}
- needed.update(config.overrides.values())
-
- unknown = needed - _KNOWN_BACKENDS
- if unknown:
- msg = (
- f"Unrecognized sandbox backend name(s): {sorted(unknown)}; "
- f"known backends: {sorted(_KNOWN_BACKENDS)}"
- )
- logger.error(SANDBOX_FACTORY_BUILD_FAILED, error=msg)
- raise ValueError(msg)
+ needed = _collect_needed_backends(config)
+ _validate_needed_backends(needed)🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| def resolve_sandbox_for_category( | ||||||||||||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||||||||||||
| config: SandboxingConfig, | ||||||||||||||||||||||||||||||||||||||||||||
| backends: Mapping[str, SandboxBackend], | ||||||||||||||||||||||||||||||||||||||||||||
| category: ToolCategory, | ||||||||||||||||||||||||||||||||||||||||||||
| ) -> SandboxBackend: | ||||||||||||||||||||||||||||||||||||||||||||
| """Look up the correct backend for a tool category. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Uses ``config.backend_for_category()`` to determine the backend | ||||||||||||||||||||||||||||||||||||||||||||
| name, then returns the corresponding instance from *backends*. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||
| config: Top-level sandboxing configuration. | ||||||||||||||||||||||||||||||||||||||||||||
| backends: Mapping of backend name to backend instance. | ||||||||||||||||||||||||||||||||||||||||||||
| category: The tool category to resolve. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||
| The ``SandboxBackend`` instance for the given category. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||||||||||||||
| KeyError: If the resolved backend name is not present in | ||||||||||||||||||||||||||||||||||||||||||||
| *backends*. | ||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||
| backend_name = config.backend_for_category(category.value) | ||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||
| backend = backends[backend_name] | ||||||||||||||||||||||||||||||||||||||||||||
| except KeyError: | ||||||||||||||||||||||||||||||||||||||||||||
| msg = ( | ||||||||||||||||||||||||||||||||||||||||||||
| f"Backend {backend_name!r} resolved for category " | ||||||||||||||||||||||||||||||||||||||||||||
| f"{category.value!r} not found in backends mapping " | ||||||||||||||||||||||||||||||||||||||||||||
| f"(available: {sorted(backends.keys())})" | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_RESOLVE, | ||||||||||||||||||||||||||||||||||||||||||||
| category=category.value, | ||||||||||||||||||||||||||||||||||||||||||||
| backend=backend_name, | ||||||||||||||||||||||||||||||||||||||||||||
| error=msg, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| raise KeyError(msg) from None | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| logger.debug( | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_RESOLVE, | ||||||||||||||||||||||||||||||||||||||||||||
| category=category.value, | ||||||||||||||||||||||||||||||||||||||||||||
| backend=backend_name, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
| return backend | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| async def cleanup_sandbox_backends( | ||||||||||||||||||||||||||||||||||||||||||||
| backends: Mapping[str, SandboxBackend], | ||||||||||||||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||||||||||||||
| """Clean up all backends by calling ``cleanup()`` on each. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Errors from individual backends are logged but do not prevent | ||||||||||||||||||||||||||||||||||||||||||||
| cleanup of remaining backends. Uses ``asyncio.TaskGroup`` for | ||||||||||||||||||||||||||||||||||||||||||||
| parallel cleanup. | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||
| backends: Mapping of backend name to backend instance. | ||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| async def _cleanup_one(name: str, backend: SandboxBackend) -> None: | ||||||||||||||||||||||||||||||||||||||||||||
| logger.debug(SANDBOX_FACTORY_CLEANUP, backend=name) | ||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||
| await backend.cleanup() | ||||||||||||||||||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||||||||||||
| SANDBOX_FACTORY_CLEANUP, | ||||||||||||||||||||||||||||||||||||||||||||
| backend=name, | ||||||||||||||||||||||||||||||||||||||||||||
| error=f"cleanup failed for backend {name!r}", | ||||||||||||||||||||||||||||||||||||||||||||
| exc_info=True, | ||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| async with asyncio.TaskGroup() as tg: | ||||||||||||||||||||||||||||||||||||||||||||
| for name, backend in backends.items(): | ||||||||||||||||||||||||||||||||||||||||||||
| tg.create_task(_cleanup_one(name, backend)) | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: Aureliolo/synthorg
Length of output: 9129
Return a plain string instead of calling
NotBlankStras a constructor.NotBlankStris a type alias (Annotated[str, ...]), not a callable type. WhileNotBlankStr("docker")won't fail at runtime, it's incorrect usage. Instead, return the plain string:Or use
castfor clarity:🤖 Prompt for AI Agents