From a3364f067b9237ffc50fcb9dfe48ae4c7d127c9a Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Wed, 1 Jul 2026 15:19:09 -0700 Subject: [PATCH 1/8] feat: isolate and attest OpenShell jobs Signed-off-by: Kyle Zheng --- configs/config_openshell.yml | 29 +- configs/openshell/aiq-research-policy.yaml | 9 +- docs/source/architecture/agents/sandbox.md | 22 +- frontends/aiq_api/src/aiq_api/jobs/runner.py | 9 + scripts/README.md | 20 +- scripts/setup_openshell.sh | 43 +- src/aiq_agent/agents/deep_researcher/agent.py | 4 + .../deep_researcher/deepagents_runtime.py | 116 ++++- .../agents/deep_researcher/register.py | 12 +- .../agents/deep_researcher/sandbox/README.md | 82 ++-- .../agents/deep_researcher/sandbox/base.py | 21 + .../agents/deep_researcher/sandbox/config.py | 50 ++- .../sandbox/providers/openshell.py | 252 +++++++++-- .../sandbox/test_openshell_provider.py | 413 +++++++++++++++++- .../test_deepagents_runtime.py | 74 ++++ tests/aiq_agent/jobs/test_runner.py | 10 + 16 files changed, 1029 insertions(+), 137 deletions(-) diff --git a/configs/config_openshell.yml b/configs/config_openshell.yml index 1d6537d18..ca7ec33c6 100644 --- a/configs/config_openshell.yml +++ b/configs/config_openshell.yml @@ -1,7 +1,5 @@ -# EXPERIMENTAL: AI-Q deep research over a local, single-operator OpenShell sandbox. -# Run ./scripts/setup_openshell.sh first to provision the gateway and named sandbox. -# Jobs attach to that shared sandbox; per-job directories prevent filename collisions -# but are not a security boundary. Do not run mutually untrusted jobs concurrently. +# EXPERIMENTAL: AI-Q deep research with one policy-bound OpenShell sandbox per job. +# Run ./scripts/setup_openshell.sh first to provision the gateway, image, and policy. general: use_uvloop: true @@ -141,13 +139,30 @@ functions: deep_research_sandbox: _type: deep_research_sandbox provider: openshell - sandbox_name: ${AIQ_OPENSHELL_SANDBOX_NAME:-aiq-openshell-demo} + openshell_image: ${AIQ_OPENSHELL_IMAGE:-aiq-openshell-demo:latest} policy: ${AIQ_OPENSHELL_POLICY_FILE:-configs/openshell/generated/aiq-openshell-policy.yaml} workdir: /sandbox - network: blocked + # The OpenShell policy is authoritative. This declaration is an upper bound: + # startup fails if the policy grants a hostname not listed here. + network: allowlist + network_allow: + - api.github.com + - github.com + - integrate.api.nvidia.com + - api.tavily.com + - google.serper.dev + - pypi.org + - files.pythonhosted.org + - huggingface.co + - cdn-lfs.huggingface.co + - export.arxiv.org + - api.semanticscholar.org + - registry.npmjs.org timeout: 1200 idle_timeout: 1800 - delete_on_exit: false + delete_on_exit: true + attest: true + require_hard_landlock: true artifact_capture: enabled: true max_file_bytes: 50000000 diff --git a/configs/openshell/aiq-research-policy.yaml b/configs/openshell/aiq-research-policy.yaml index 8bb9775fa..ef3017ab4 100644 --- a/configs/openshell/aiq-research-policy.yaml +++ b/configs/openshell/aiq-research-policy.yaml @@ -21,11 +21,12 @@ filesystem_policy: - /tmp - /dev/null -# best_effort lets the sandbox start on hosts without Landlock (e.g. Docker Desktop on -# macOS), but there filesystem confinement is silently dropped. Acceptable only for the -# local single-operator demo; production must use `hard_requirement` (fail closed). +# Fail closed when the host cannot enforce Landlock filesystem confinement. Local demo +# environments that intentionally accept weaker isolation must generate a separate policy +# with `scripts/setup_openshell.sh --landlock-compatibility best_effort` and configure +# `require_hard_landlock: false` explicitly. landlock: - compatibility: best_effort + compatibility: hard_requirement process: run_as_user: sandbox diff --git a/docs/source/architecture/agents/sandbox.md b/docs/source/architecture/agents/sandbox.md index 5b92b8c88..4d77a9c15 100644 --- a/docs/source/architecture/agents/sandbox.md +++ b/docs/source/architecture/agents/sandbox.md @@ -6,9 +6,9 @@ SPDX-License-Identifier: Apache-2.0 # Deep Research Sandbox Notes Deep research can optionally run DeepAgents `execute` calls through a sandbox provider -(Modal, OpenShell, or any registered provider). Modal creates a sandbox per job. The -experimental OpenShell path attaches to a pre-created named sandbox shared by its jobs; -job-scoped directories prevent filename collisions but are not a security boundary. +(Modal, OpenShell, or any registered provider). Modal and OpenShell create a fresh physical +sandbox per job. OpenShell binds the configured policy at creation and attests readiness plus +the loaded policy revision before exposing the execution backend. The sandbox is an internal execution detail. There are no sandbox-specific API endpoints, and job-level auth remains responsible for submit, stream, status, @@ -21,12 +21,13 @@ runtime (`.../job/{job_id}/artifacts`), which is also auth-scoped to the job. ## Current Behavior -- Modal uses one sandbox per deep research job. OpenShell currently attaches jobs to - the configured shared sandbox name and is intended for local, single-operator testing. +- Modal and OpenShell use one physical sandbox per deep research job. OpenShell shared + attachment is available only through an explicit debug-only opt-in and is not job-isolated. - Synchronous sandbox-enabled runs use an internal per-agent runtime ID. - Providers are selected by config (`sandbox.provider` + `providers.`); the provider is validated against the registry and gated by its declared capabilities. - OpenShell policy is provisioned externally and is not verified when AI-Q attaches. + OpenShell policy YAML is parsed strictly against the installed SDK schema, applied in the + job's creation spec, checked against the declared network upper bound, and attested before use. - Job IDs must satisfy each provider's object-name rules (Modal: 64 chars or fewer, alphanumeric plus dash/period/underscore). - `timeout` bounds individual execution. Other lifecycle controls are provider-dependent. @@ -36,13 +37,13 @@ runtime (`.../job/{job_id}/artifacts`), which is also auth-scoped to the job. ## Operational Notes -- High-concurrency Modal runs create one sandbox per job. OpenShell runs share the named - sandbox and must not be used concurrently for mutually untrusted jobs. Optional submit-path +- High-concurrency Modal and OpenShell runs create one sandbox per job. Optional submit-path caps (`AIQ_MAX_SANDBOXES_PER_PRINCIPAL` / `AIQ_MAX_SANDBOXES_GLOBAL`, default-off) bound - concurrency/cost but do not provide filesystem isolation. + concurrency and cost. - Custom client-supplied job IDs must not be reused for a new job. - The runtime closes provider sessions on success, failure, cancellation, and timeout. - A named OpenShell sandbox persists when `delete_on_exit` is disabled. +- Per-job OpenShell mode requires `delete_on_exit: true`. A persistent shared sandbox is + possible only through the explicit debug attachment settings. ## Current Safeguards @@ -54,3 +55,4 @@ The following safeguards are in place: with MIME-from-bytes spoof rejection, SVG sanitization, and an inline-render allowlist. - Sandbox quota and concurrency controls, and artifact retention via job-expiry cleanup. - Structured lifecycle logging for sandbox create, reuse, failure, and cleanup. +- Structured `sandbox.attestation` and `sandbox.cleanup` events. diff --git a/frontends/aiq_api/src/aiq_api/jobs/runner.py b/frontends/aiq_api/src/aiq_api/jobs/runner.py index 1df9069ae..b68b2c5cc 100644 --- a/frontends/aiq_api/src/aiq_api/jobs/runner.py +++ b/frontends/aiq_api/src/aiq_api/jobs/runner.py @@ -646,6 +646,8 @@ async def run_agent_job( # worker. The single artifact harvest already ran in agent.run() before this point, so # teardown only closes/terminates; interrupted jobs terminate() to preempt a live execute. await asyncio.to_thread(_teardown_sandbox, sandbox_runtime, job_id=job_id, interrupted=interrupted) + if event_store is not None and hasattr(event_store, "flush"): + event_store.flush() # Clean up job-scoped auth token if _auth_token_reset is not None: from ._auth_context import job_auth_token @@ -662,6 +664,13 @@ def _teardown_sandbox(sandbox_runtime: Any | None, *, job_id: str, interrupted: """ if sandbox_runtime is None: return + finalize = getattr(sandbox_runtime, "finalize", None) + if finalize is not None: + try: + finalize(interrupted=interrupted) + except Exception: # noqa: BLE001 - cleanup must never replace the job result + logger.warning("Sandbox cleanup failed for job %s", job_id, exc_info=True) + return teardown = getattr(sandbox_runtime, "terminate", None) if interrupted else None if teardown is None: teardown = getattr(sandbox_runtime, "close", None) diff --git a/scripts/README.md b/scripts/README.md index 12a7dbbef..70afbc972 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -51,18 +51,18 @@ Starts the agent in CLI mode with browser-based authentication. ### `setup_openshell.sh` - OpenShell Sandbox Setup -Sets up the experimental, local single-operator NVIDIA OpenShell path for AI-Q. Run this once before using +Sets up the experimental NVIDIA OpenShell path for AI-Q. Run this once before using `configs/config_openshell.yml` with `start_cli.sh` or `start_e2e.sh`. It installs the `openshell` SDK and the `langchain-nvidia-openshell` adapter, starts/verifies -the local OpenShell gateway, builds the sandbox image, generates a network policy, -and creates the named sandbox `aiq-openshell-demo`. Inference is unaffected (it -stays host-side, routed to NVIDIA Build); only generated code runs in the -network-blocked sandbox. +the local OpenShell gateway, builds the reusable sandbox image, and generates a network +policy. AI-Q then creates, attests, and deletes a policy-bound physical sandbox for every +job. Inference is unaffected (it stays host-side, routed to NVIDIA Build); only generated +code runs in the sandbox. -The generated configuration attaches all jobs to one named sandbox. Per-job directories -avoid filename collisions but do not isolate mutually untrusted jobs, and AI-Q does not -verify the provisioned policy when attaching. Do not treat this setup as a multi-tenant -security boundary. +Production setup defaults to `landlock.compatibility: hard_requirement`. For a local demo +host that cannot enforce Landlock, pass `--landlock-compatibility best_effort` and explicitly +set `require_hard_landlock: false` in the AI-Q config. The legacy named shared sandbox is +created only with `--create-shared-debug-sandbox` and remains a debug-only, non-isolated mode. ```bash ./scripts/setup_openshell.sh --policy offline @@ -187,7 +187,7 @@ Starts both backend and frontend for full WebSocket support and HITL workflows. | `configs/config_web_frag.yml` | Server/E2E mode with Foundational RAG | | `configs/config_web_default_llamaindex.yml` | Server/E2E mode with LlamaIndex | | `configs/config_skills.yml` | Deep research with DeepAgents skills + Modal sandbox | -| `configs/config_openshell.yml` | Experimental local single-operator OpenShell sandbox + artifact capture (run `setup_openshell.sh` first) | +| `configs/config_openshell.yml` | Experimental per-job OpenShell sandbox + artifact capture (run `setup_openshell.sh` first) | ## Development Workflow diff --git a/scripts/setup_openshell.sh b/scripts/setup_openshell.sh index 9a5a62493..7bf3ebd7c 100755 --- a/scripts/setup_openshell.sh +++ b/scripts/setup_openshell.sh @@ -14,7 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. # -# Set up NVIDIA OpenShell for AI-Q with a named, policy-backed sandbox. +# Set up NVIDIA OpenShell for AI-Q per-job policy-backed sandboxes. set -euo pipefail @@ -48,6 +48,7 @@ SANDBOX_LOG_LEVEL="${AIQ_OPENSHELL_SANDBOX_LOG_LEVEL:-warn}" POLICY_PRESET="${AIQ_OPENSHELL_POLICY:-}" POLICY_ALLOWLIST="${AIQ_OPENSHELL_POLICY_ALLOWLIST:-${AIQ_OPENSHELL_POLICY_SERVICES:-}}" POLICY_FILE="${AIQ_OPENSHELL_POLICY_FILE:-$REPO_ROOT/configs/openshell/generated/aiq-openshell-policy.yaml}" +LANDLOCK_COMPATIBILITY="${AIQ_OPENSHELL_LANDLOCK_COMPATIBILITY:-hard_requirement}" GATEWAY_NAME="${AIQ_OPENSHELL_GATEWAY_NAME:-aiq-local}" GATEWAY_PORT="${AIQ_OPENSHELL_GATEWAY_PORT:-8080}" DOCKER_BIN="${DOCKER_BIN:-}" @@ -57,7 +58,7 @@ GATEWAY_ENDPOINT="" GATEWAY_DISABLE_TLS=false RESTART_GATEWAY=true BUILD_IMAGE=true -CREATE_SANDBOX=true +CREATE_SANDBOX=false LIST_OPENSHELL_VERSIONS=false SUPPORTED_SERVICES="github,pypi,nvidia,tavily,serper,huggingface,arxiv,semantic-scholar,npm" @@ -76,7 +77,7 @@ Sets up OpenShell for AI-Q: 6. Generates an initial OpenShell policy. 7. Starts/verifies the OpenShell gateway. 8. Builds the AI-Q sandbox image. - 9. Creates a named policy-backed sandbox. + 9. Leaves per-job sandbox creation to the AI-Q runtime. Options: --openshell-version VERSION Exact OpenShell version, or "latest". @@ -89,7 +90,9 @@ Options: Services: $SUPPORTED_SERVICES --policy-file PATH Output policy file. Default: configs/openshell/generated/aiq-openshell-policy.yaml - --sandbox-name NAME OpenShell sandbox name (default: aiq-openshell-demo). + --landlock-compatibility MODE hard_requirement (default) or best_effort (local demo only). + --create-shared-debug-sandbox Create one named shared sandbox for explicit debug attachment. + --sandbox-name NAME Debug sandbox name (default: aiq-openshell-demo). --image-name NAME Docker image tag (default: aiq-openshell-demo:latest). --sandbox-log-level LEVEL In-container OpenShell log verbosity baked into the image via RUST_LOG (default: warn). Use "debug" to @@ -102,7 +105,7 @@ Options: --gateway-bin PATH OpenShell gateway launcher path. --no-restart-gateway Reuse the active gateway instead of starting one. --skip-build Do not build the sandbox image. - --skip-sandbox Do not create the named sandbox. + --skip-sandbox Deprecated no-op; per-job creation is already the default. --list-policies Print supported policy choices. --list-services Print supported services for --allow. --list-openshell-versions Print released OpenShell versions >= 0.0.72. @@ -149,6 +152,14 @@ while [[ $# -gt 0 ]]; do POLICY_FILE="$2" shift 2 ;; + --landlock-compatibility) + LANDLOCK_COMPATIBILITY="$2" + shift 2 + ;; + --create-shared-debug-sandbox) + CREATE_SANDBOX=true + shift + ;; --sandbox-name) SANDBOX_NAME="$2" shift 2 @@ -864,6 +875,13 @@ resolve_policy() { if [[ "$POLICY_ALLOWLIST" == *offline* && "$POLICY_ALLOWLIST" != "offline" ]]; then fail "Use either offline or a service allowlist, not both: $POLICY_ALLOWLIST" fi + case "$LANDLOCK_COMPATIBILITY" in + hard_requirement|best_effort) + ;; + *) + fail "Unsupported Landlock compatibility '$LANDLOCK_COMPATIBILITY'; use hard_requirement or best_effort" + ;; + esac } validate_service() { @@ -877,7 +895,7 @@ validate_service() { } emit_policy_header() { - cat >"$POLICY_FILE" <<'EOF' + cat >"$POLICY_FILE" < bool: + """Release this request's sandbox runtime exactly once.""" + return self.deepagents_runtime.finalize(interrupted=interrupted) + def _load_prompts(self) -> dict[str, str]: """Load all prompts for subagents.""" prompts = {} diff --git a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py index 33be229e8..8769adb11 100644 --- a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py +++ b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py @@ -19,6 +19,7 @@ import importlib.util import logging +import threading from collections.abc import Callable from pathlib import Path from typing import Any @@ -32,6 +33,7 @@ from pydantic import ConfigDict from pydantic import Field from pydantic import field_validator +from pydantic import model_validator from nat.data_models.function import FunctionBaseConfig @@ -85,19 +87,44 @@ class DeepResearchSandboxConfig(FunctionBaseConfig, name="deep_research_sandbox" default=(), description="Python packages to install into the Modal sandbox image.", ) - # OpenShell-specific (used when provider == "openshell"). The named sandbox is created - # out-of-band by scripts/setup_openshell.sh and attached to by name. - sandbox_name: str | None = Field(default=None, description="Existing named OpenShell sandbox to attach to.") + # OpenShell-specific (used when provider == "openshell"). The secure default creates + # a new policy-bound physical sandbox for each AI-Q job. + openshell_image: str = Field( + default="aiq-openshell-demo:latest", + description="Container image used for per-job OpenShell sandboxes.", + ) + existing_sandbox_name: str | None = Field( + default=None, + description="Debug-only existing OpenShell sandbox; requires allow_shared_sandbox=true.", + ) + sandbox_name: str | None = Field( + default=None, + description="Deprecated alias for existing_sandbox_name; requires allow_shared_sandbox=true.", + ) + allow_shared_sandbox: bool = Field( + default=False, + description="Allow debug attachment to a shared OpenShell sandbox; not job-isolated.", + ) gateway: str | None = Field( default=None, description="OpenShell gateway endpoint/name (null uses the locally selected gateway).", ) - policy: str | None = Field(default=None, description="OpenShell policy file path (requires a named sandbox).") + policy: str | None = Field(default=None, description="OpenShell policy YAML applied at per-job creation.") ready_timeout_seconds: float = Field( default=300.0, description="Seconds to wait for the OpenShell sandbox to become ready.", ) - delete_on_exit: bool = Field(default=False, description="Delete the OpenShell sandbox when the session closes.") + delete_on_exit: bool = Field(default=True, description="Delete the OpenShell sandbox when the session closes.") + attest: bool = Field(default=True, description="Fail closed unless the OpenShell policy revision is loaded.") + expected_policy_version: int | None = Field( + default=None, + ge=1, + description="Optional exact OpenShell policy revision pin.", + ) + require_hard_landlock: bool = Field( + default=True, + description="Require landlock.compatibility=hard_requirement in the configured policy.", + ) shell: tuple[str, ...] = Field( default=("bash", "-c"), description="Shell argv prefix passed to the langchain-nvidia-openshell adapter.", @@ -106,19 +133,33 @@ class DeepResearchSandboxConfig(FunctionBaseConfig, name="deep_research_sandbox" workdir: str | None = Field(default=None, description="Working directory inside the sandbox") timeout: int = Field(default=1200, description="Maximum sandbox lifetime in seconds") idle_timeout: int = Field(default=1800, description="Sandbox idle timeout in seconds") - network: Literal["blocked", "enabled"] = Field( + network: Literal["blocked", "allowlist", "open", "enabled"] = Field( default="blocked", - description="Outbound network policy for the sandbox.", + description="Outbound network policy; 'enabled' is a deprecated alias for 'open'.", + ) + network_allow: tuple[str, ...] = Field( + default=(), + description="Hostnames allowed when network='allowlist'.", ) artifact_capture: ArtifactCaptureConfig = Field( default_factory=ArtifactCaptureConfig, description="Durable harvesting of generated artifacts (charts/CSVs). Disabled by default.", ) + @model_validator(mode="after") + def _validate_openshell_and_network(self) -> DeepResearchSandboxConfig: + if self.network == "allowlist" and not self.network_allow: + raise ValueError("network='allowlist' requires a non-empty network_allow list") + if self.provider == "openshell": + shared_name = self.existing_sandbox_name or self.sandbox_name + if not shared_name and not self.attest: + raise ValueError("Per-job OpenShell creation requires attest=true") + return self + @property - def block_network(self) -> bool: - """Return the block_network flag for this public network setting.""" - return self.network == "blocked" + def network_mode(self) -> Literal["blocked", "allowlist", "open"]: + """Return the normalized provider-neutral network mode.""" + return "open" if self.network == "enabled" else self.network class DeepAgentsRuntime: @@ -147,6 +188,9 @@ def __init__( self._job_id = str(job_id) if job_id is not None else str(uuid4()) self._backend: Any | None = None self._sandbox_provider: Any | None = None + self._event_emit = artifact_emit + self._finalize_lock = threading.Lock() + self._finalized = False self.artifact_manager: Any | None = None self._skill_sources_by_agent = _resolve_agent_skill_sources(skills) self._skill_sources = tuple( @@ -160,6 +204,8 @@ def __init__( # runtime can expose its job-scoped workdir/artifact_dir and own its lifecycle. if sandbox is not None: self._sandbox_provider = _create_sandbox_backend(sandbox, self._job_id) + if hasattr(self._sandbox_provider, "set_event_emitter"): + self._sandbox_provider.set_event_emitter(artifact_emit) self.artifact_manager = _maybe_build_artifact_manager( provider=self._sandbox_provider, job_id=self._job_id, @@ -229,6 +275,48 @@ def terminate(self) -> None: if provider is not None and hasattr(provider, "terminate"): provider.terminate() + def finalize(self, *, interrupted: bool) -> bool: + """Release the sandbox once and emit a truthful, sanitized cleanup outcome.""" + with self._finalize_lock: + if self._finalized: + return bool(getattr(self._sandbox_provider, "cleanup_succeeded", True)) + self._finalized = True + + self._emit_cleanup("started", interrupted=interrupted) + try: + if interrupted: + self.terminate() + else: + self.close() + except Exception: # noqa: BLE001 - terminal cleanup cannot replace the job result + logger.warning("Sandbox cleanup failed for job %s", self._job_id, exc_info=True) + self._emit_cleanup("failed", interrupted=interrupted) + return False + + succeeded = bool(getattr(self._sandbox_provider, "cleanup_succeeded", True)) + self._emit_cleanup("succeeded" if succeeded else "failed", interrupted=interrupted) + return succeeded + + def _emit_cleanup(self, status: str, *, interrupted: bool) -> None: + """Emit cleanup metadata without policy contents, environment, or error text.""" + if self._event_emit is None: + return + try: + self._event_emit( + { + "type": "sandbox.cleanup", + "data": { + "status": status, + "interrupted": interrupted, + "provider": getattr(self._sandbox_provider, "provider_name", None), + "sandbox": getattr(self._sandbox_provider, "physical_sandbox_name", None) + or getattr(self._sandbox_provider, "sandbox_name", None), + }, + } + ) + except Exception: # noqa: BLE001 - observability must not break terminal cleanup + logger.warning("Sandbox cleanup event emission failed for job %s", self._job_id, exc_info=True) + def prepare_state_files(self, files: dict[str, Any]) -> dict[str, Any]: """Normalize seeded virtual filesystem files for the configured backend.""" return _normalize_state_files(files, strip_shared_route=self._sandbox is not None) @@ -404,10 +492,16 @@ def _create_sandbox_backend(config: DeepResearchSandboxConfig, job_id: str) -> A providers = { "openshell": { "gateway": config.gateway, + "existing_sandbox_name": config.existing_sandbox_name, "sandbox_name": config.sandbox_name, + "allow_shared_sandbox": config.allow_shared_sandbox, "policy": config.policy, + "image": config.openshell_image, "ready_timeout_seconds": config.ready_timeout_seconds, "delete_on_exit": config.delete_on_exit, + "attest": config.attest, + "expected_policy_version": config.expected_policy_version, + "require_hard_landlock": config.require_hard_landlock, "shell": list(config.shell), } } @@ -420,7 +514,7 @@ def _create_sandbox_backend(config: DeepResearchSandboxConfig, job_id: str) -> A "workdir": workdir, "timeout": config.timeout, "idle_timeout": config.idle_timeout, - "network": {"mode": "blocked" if config.block_network else "open"}, + "network": {"mode": config.network_mode, "allow": config.network_allow}, "artifact_capture": config.artifact_capture.model_dump(), "providers": providers, } diff --git a/src/aiq_agent/agents/deep_researcher/register.py b/src/aiq_agent/agents/deep_researcher/register.py index 4134b797f..14e382464 100644 --- a/src/aiq_agent/agents/deep_researcher/register.py +++ b/src/aiq_agent/agents/deep_researcher/register.py @@ -15,6 +15,7 @@ """NAT register function for deep research agent.""" +import asyncio import logging from typing import TypeVar @@ -236,10 +237,12 @@ async def deep_research_agent(config: DeepResearchAgentConfig, builder: Builder) async def _run(state: DeepResearchAgentState) -> DeepResearchAgentState: """Run deep research with a list of messages or payload.""" + active_agent = agent + owns_active_agent = False + interrupted = False try: data_sources = state.data_sources selected_tools = filter_tools_by_sources(tools, data_sources) - active_agent = agent if sandbox_config is not None or (data_sources is not None and selected_tools != tools): # Scope the Modal sandbox to the async job_id when one is in # NAT context (set by aiq_api/jobs/runner.py). Falls back to a @@ -266,6 +269,7 @@ async def _run(state: DeepResearchAgentState) -> DeepResearchAgentState: max_concurrent_source_tool_calls=config.max_concurrent_source_tool_calls, max_source_tool_batch_size=config.max_source_tool_batch_size, ) + owns_active_agent = True if all_mapped_tools_filtered_out(tools, selected_tools, data_sources): logger.warning("Deep research received data_sources with no matching tools") @@ -291,9 +295,15 @@ async def _run(state: DeepResearchAgentState) -> DeepResearchAgentState: result = await active_agent.run(state) return result + except asyncio.CancelledError: + interrupted = True + raise except Exception: logger.exception("Error in deep research execution") raise + finally: + if owns_active_agent: + await asyncio.to_thread(active_agent.finalize, interrupted=interrupted) yield FunctionInfo.from_fn(_run, description="Deep research agent for comprehensive multi-phase research.") diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/README.md b/src/aiq_agent/agents/deep_researcher/sandbox/README.md index 3c0ed2b1d..9aa15ee47 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/README.md +++ b/src/aiq_agent/agents/deep_researcher/sandbox/README.md @@ -37,15 +37,17 @@ creates these on session start (`_prepare_workspace`, an idempotent `mkdir -p`) runtime injects them into prompts/skills as `sandbox_workdir`/`sandbox_artifact_dir`. This prevents accidental filename collisions and keeps harvesting scoped to the current job. -Modal creates a fresh sandbox for each job. The experimental OpenShell configuration instead -attaches jobs to one pre-created named sandbox because the SDK cannot apply the configured -policy when creating an anonymous sandbox. Per-job directories inside that sandbox are not -an access-control boundary: executed code can access sibling job directories allowed by the -shared policy. Use OpenShell only for local, single-operator testing, and do not run mutually -untrusted jobs concurrently. Physical per-job OpenShell isolation and attach-time policy -verification are follow-up work. The default policy also sets `landlock.compatibility: -best_effort`, so on hosts without Landlock (e.g. Docker Desktop on macOS) filesystem -confinement is silently dropped; production must use `hard_requirement` to fail closed. +Modal and OpenShell both create a fresh physical sandbox for each job. OpenShell parses the +configured policy with the installed SDK schema, applies it in the creation `SandboxSpec`, +waits for `READY`, and requires a positive loaded policy revision before exposing the backend. +An optional `expected_policy_version` provides an exact revision pin. Any creation or +attestation failure closes the owning SDK context so the partially created sandbox is deleted. + +OpenShell shared attachment remains available only as an explicit debug escape hatch: +`existing_sandbox_name` plus `allow_shared_sandbox: true`. It is not a job isolation boundary +and must not be used for mutually untrusted jobs. Production policy validation also requires +`landlock.compatibility: hard_requirement`; a local demo may explicitly set both the policy +and `require_hard_landlock: false` to accept `best_effort`. The agent only ever sees a `read_file`/`write_file`/`edit_file`/`execute` tool surface plus `/shared/` for durable text. Binary artifacts are harvested host-side via @@ -60,7 +62,7 @@ plus `/shared/` for durable text. Binary artifacts are harvested host-side via | `config.py` | `SandboxConfig`: common fields + nested `providers.` + `artifact_capture` + `lifecycle_scope`; legacy flat-config shim; provider validated against the registry. | | `capabilities.py` | `SandboxCapabilities` + `verify_capabilities` (fail-closed: refuse to run if a required guarantee like `block_network` is unsupported). | | `providers/modal.py` | Modal provider (cloud). Create-fresh semantics (no silent attach-by-name). | -| `providers/openshell.py` | OpenShell provider (enterprise/on-prem). Lazy, ad-hoc deps; policy requires a named sandbox. | +| `providers/openshell.py` | OpenShell provider (enterprise/on-prem). Lazy, ad-hoc deps; per-job policy creation, readiness/revision attestation, and confined transfer. | | `artifacts/models.py` | `Artifact` record (id, mime, sha256, size, provenance, status). Metadata only. | | `artifacts/manifest.py` | `manifest.json` schema + parser. | | `artifacts/store.py` | `SqlArtifactStore` on the shared job `db_url` (metadata table + capped BLOB; pluggable to S3). | @@ -123,8 +125,8 @@ sandbox: provider: openshell # registry key workdir: /sandbox # injected into prompts + skills network: # normalized, provider-neutral egress policy - mode: blocked # blocked | allowlist | open (legacy `block_network: true` => blocked) - # allow: [pypi.org] # required for mode: allowlist; needs supports_network_allowlist + mode: allowlist # blocked | allowlist | open (legacy `block_network: true` => blocked) + allow: [api.github.com, github.com] # policy grants must be a subset of this list timeout: 1200 idle_timeout: 1800 resources: # optional CPU/memory caps; omit for no limit @@ -141,8 +143,12 @@ sandbox: python_packages: [matplotlib, numpy, pandas, pillow, tabulate] openshell: gateway: null # null = locally selected gateway - sandbox_name: aiq-openshell-demo + image: aiq-openshell-demo:latest policy: configs/openshell/generated/aiq-openshell-policy.yaml + delete_on_exit: true + attest: true + # expected_policy_version: 1 + require_hard_landlock: true ``` The legacy flat shape (top-level `app_name`/`image`/`python_packages`) still loads and @@ -151,12 +157,12 @@ is lifted into `providers.modal`. ## Artifact runtime - Generated code writes binaries + a `manifest.json` to `artifact_dir`. -- Once at the end of the agent run (`agent.run()` -> `ArtifactManager.final_harvest`), +- Once at the end of a successful agent run (`agent.run()` -> `ArtifactManager.final_harvest`), the `ArtifactManager` pulls bytes via `download_files`, runs the validation pipeline (path-traversal confinement -> extension allowlist -> size cap -> MIME-from-bytes/spoof reject -> quota -> SVG sanitize -> sha256), stores via `SqlArtifactStore`, then emits an - `artifact` SSE event (`to_sse_payload`, metadata + `content_url`, never bytes). -- Failed or cancelled runs are not harvested in the current implementation. + artifact event (metadata + content URL, never bytes). +- Failed or cancelled runs are not harvested until the terminal-harvest follow-up lands. - Reports reference artifacts as `![caption](artifact://)`; the report postprocessor rewrites filename refs to durable ids and drops unknown/foreign refs. - Endpoints: `GET /v1/jobs/async/job/{job_id}/artifacts` and `.../artifacts/{id}/content` @@ -202,12 +208,13 @@ shared helper, `MarkdownRenderer/artifact-url.ts`, builds the content path): Requires `modal` + `langchain-modal` (in `pyproject`) and `modal setup`. See `docs/source/examples/skills-sandbox/index.md`. -### OpenShell (experimental, local single-operator) +### OpenShell (experimental) -> OpenShell jobs currently attach to one pre-created named sandbox. Its policy is applied by -> the setup command and is not verified when AI-Q attaches. Job directories prevent accidental -> collisions but do not isolate mutually untrusted jobs. Do not use this path as a multi-tenant -> security boundary. +Each job creates a new policy-bound OpenShell sandbox and deletes it on terminal cleanup. +AI-Q refuses startup if the YAML does not match the installed SDK schema, the policy grants a +host outside the declared public allowlist, production Landlock mode is not fail-closed, or +the entered sandbox is not ready with a loaded policy revision. The creation spec deliberately +has no copied host environment or credential providers. Two ad-hoc deps (never in `pyproject`): the `openshell` SDK and the official `langchain-nvidia-openshell` adapter (`OpenShellSandbox`), the OpenShell partner package in @@ -228,16 +235,23 @@ One-command setup: ``` The setup script prints the environment variables needed by any later shell that starts -AI-Q. If you start the backend in a different terminal/session, export the printed values -before running `start_e2e.sh` (or put them in your local env file): +AI-Q. It builds the reusable image and generates the policy; AI-Q creates the physical +sandbox per job. If you start the backend in a different terminal/session, export the +printed values before running `start_e2e.sh` (or put them in your local env file): ```bash -export AIQ_OPENSHELL_SANDBOX_NAME="aiq-openshell-demo" +export AIQ_OPENSHELL_IMAGE="aiq-openshell-demo:latest" export AIQ_OPENSHELL_POLICY_FILE="$PWD/configs/openshell/generated/aiq-openshell-policy.yaml" ``` +For a local host that cannot enforce Landlock, an explicit non-production demo can use +`--landlock-compatibility best_effort` together with `require_hard_landlock: false` in the +AI-Q config. `--create-shared-debug-sandbox` creates the old named attachment target only +for deliberate debugging; configure `existing_sandbox_name` and `allow_shared_sandbox: true` +to attach to it. + Inference is routed host-side (e.g. NVIDIA Build or an internal inference hub set in the -config); the network-blocked sandbox never sees the key. +config); sandbox policy egress never requires or receives the inference key. **File-transfer gotcha:** the provider overrides file transfer with an env-free shim that passes the path via `argv`. OpenShell 0.0.57-0.0.67 strip @@ -260,7 +274,8 @@ in force. Once the upstream adapter provides equivalent guards, drop the shim an inside the OpenShell container, rebuild the sandbox image with a higher `RUST_LOG`: `./scripts/setup_openshell.sh --sandbox-log-level debug` (or `--build-arg OPENSHELL_SANDBOX_LOG_LEVEL=debug`). Default `warn` keeps OpenShell's stock behavior. - Read the container logs with `openshell logs `, the OpenShell TUI, or inside + Read the generated sandbox name from AI-Q's attestation/cleanup events, then use + `openshell logs `, the OpenShell TUI, or inside the sandbox at `/var/log/openshell.*.log` (e.g. `grep "OCSF PROC:"` for process activity). ## Testing @@ -269,8 +284,8 @@ in force. Once the upstream adapter provides equivalent guards, drop the shim an pytest tests/aiq_agent/agents/deep_researcher/sandbox/ -q ``` -All provider/artifact tests run without a live Modal/OpenShell gateway (OpenShell -compliance auto-skips when the SDK is absent). +Core provider/artifact tests use fake SDK objects and run without a live Modal/OpenShell +gateway. The exact checked-policy/protobuf schema assertion is optional when the SDK is absent. ## Troubleshooting @@ -278,10 +293,13 @@ compliance auto-skips when the SDK is absent). the workspace plugin packages aren't installed. Install them (don't re-run `setup.sh`, which recreates `.venv`): `uv pip install -e ./frontends/aiq_api -e ./sources/tavily_web_search -e "./sources/knowledge_layer[llamaindex,foundational_rag]" -e ./sources/exa_web_search -e ./sources/google_scholar_paper_search` -- **`langchain-nvidia-openshell was not found in the package registry`**: the adapter is - the OpenShell partner package in `langchain-ai/langchain-nvidia` (PR #303), not yet on - PyPI. The setup script installs it from a git spec by default; override with - `LANGCHAIN_NVIDIA_REPO=` or `--langchain-nvidia /path/to/checkout`. +- **`langchain-nvidia-openshell was not found in the package registry`**: use the adapter + source override supported by the setup script: set + `LANGCHAIN_NVIDIA_REPO=` or pass + `--langchain-nvidia /path/to/checkout`. +- **OpenShell policy rejected as broader than `network_allow`**: add the intended hostname + to the public allowlist or remove it from the policy. AI-Q never silently accepts policy + egress broader than the public declaration. - **`unbound variable` in `setup_openshell.sh` on macOS**: the system bash is 3.2; run under bash 5 (`brew install bash` then `/opt/homebrew/bin/bash ./scripts/setup_openshell.sh ...`). - **`network.mode` rejected at startup**: the selected provider doesn't declare the diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/base.py b/src/aiq_agent/agents/deep_researcher/sandbox/base.py index 20d726f77..e2baf3eaa 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/base.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/base.py @@ -99,6 +99,8 @@ def __init__(self, config: SandboxConfig, job_id: str) -> None: # state lock, so the two can never deadlock. self._state_lock = threading.Lock() self._terminated = False + self._event_emit: Callable[[dict[str, object]], None] | None = None + self._cleanup_failed = False # ------------------------------------------------------------------ # # Required surface (the only things a provider must implement) @@ -140,6 +142,19 @@ def is_recoverable_error(self, exc: Exception) -> bool: """ return False + def set_event_emitter(self, emit: Callable[[dict[str, object]], None] | None) -> None: + """Attach the job-scoped event sink used for sanitized lifecycle events.""" + self._event_emit = emit + + def _emit_event(self, event: dict[str, object]) -> None: + """Best-effort event delivery; observability must never break execution.""" + if self._event_emit is None: + return + try: + self._event_emit(event) + except Exception: # noqa: BLE001 - event persistence is non-critical + logger.warning("Sandbox event emission failed for %s", self.sandbox_name, exc_info=True) + def close(self) -> None: """Release the underlying sandbox session, if any (idempotent). @@ -192,8 +207,14 @@ def _safe_close(self, session: BaseSandbox | None) -> None: try: session.close() except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + self._cleanup_failed = True logger.warning("Sandbox %s cleanup failed", self.sandbox_name, exc_info=True) + @property + def cleanup_succeeded(self) -> bool: + """Whether every cleanup operation completed without an observed error.""" + return not self._cleanup_failed + @property def id(self) -> str: """Stable identifier: the live session id once created, else the scoped name.""" diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/config.py b/src/aiq_agent/agents/deep_researcher/sandbox/config.py index c6cca98cd..016dae067 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/config.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/config.py @@ -90,22 +90,66 @@ class OpenShellProviderConfig(BaseModel): """OpenShell-specific sandbox settings (enterprise/on-prem example provider).""" gateway: str | None = Field(default=None, description="OpenShell gateway/cluster endpoint or name") + existing_sandbox_name: str | None = Field( + default=None, + description="Debug-only existing OpenShell sandbox to attach to; requires allow_shared_sandbox=true.", + ) sandbox_name: str | None = Field( default=None, - description="Existing named OpenShell sandbox to attach to (required when a policy file is used).", + description="Deprecated alias for existing_sandbox_name; requires allow_shared_sandbox=true.", + ) + allow_shared_sandbox: bool = Field( + default=False, + description="Allow debug attachment to a shared sandbox; this does not provide per-job isolation.", ) policy: str | None = Field( default=None, - description="OpenShell policy file path. Requires a pre-created named sandbox (sandbox_name).", + description="OpenShell policy YAML applied when the per-job sandbox is created.", ) - image: str = Field(default="base", description="OpenShell image identifier") + image: str = Field(default="aiq-openshell-demo:latest", description="OpenShell image identifier") ready_timeout_seconds: float = Field(default=300.0, description="Seconds to wait for the sandbox to become ready") delete_on_exit: bool = Field(default=True, description="Delete the sandbox when its session context closes") + attest: bool = Field(default=True, description="Require READY state and a loaded policy revision before execution") + expected_policy_version: int | None = Field( + default=None, + ge=1, + description="Optional exact OpenShell policy revision pin.", + ) + require_hard_landlock: bool = Field( + default=True, + description="Reject policy files that do not require Landlock filesystem enforcement.", + ) shell: tuple[str, ...] = Field( default=("bash", "-c"), description="Shell argv prefix passed to the langchain-nvidia-openshell adapter.", ) + @model_validator(mode="after") + def _validate_lifecycle_mode(self) -> OpenShellProviderConfig: + """Keep shared attachment explicit and per-job creation fail-closed.""" + if self.sandbox_name and self.existing_sandbox_name and self.sandbox_name != self.existing_sandbox_name: + raise ValueError("Set only one of sandbox_name or existing_sandbox_name.") + shared_name = self.existing_sandbox_name or self.sandbox_name + if shared_name and not self.allow_shared_sandbox: + raise ValueError( + "Attaching to an existing OpenShell sandbox is debug-only and requires allow_shared_sandbox=true." + ) + if not shared_name: + if not self.delete_on_exit: + raise ValueError("Per-job OpenShell sandboxes require delete_on_exit=true.") + if not self.attest: + raise ValueError("Per-job OpenShell sandboxes require attest=true.") + if self.expected_policy_version is not None and not self.attest: + raise ValueError("expected_policy_version requires attest=true.") + if not self.shell or any(not part.strip() for part in self.shell): + raise ValueError("OpenShell shell must contain at least one non-empty argv element.") + return self + + @property + def shared_sandbox_name(self) -> str | None: + """Return the explicitly configured debug attachment target, if any.""" + return self.existing_sandbox_name or self.sandbox_name + class SandboxProvidersConfig(BaseModel): """Per-provider configuration blocks. Add a provider by adding an optional field here.""" diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py b/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py index 4d870402f..d4af6d5ca 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py @@ -33,7 +33,9 @@ import logging import os import re +from pathlib import Path from typing import TYPE_CHECKING +from typing import Any from deepagents.backends.protocol import FileDownloadResponse from deepagents.backends.protocol import FileUploadResponse @@ -125,14 +127,139 @@ def _is_openshell_not_found_error(exc: Exception) -> bool: return "not found" in text and ("sandbox" in text or exc.__class__.__module__.startswith("openshell")) -class OpenShellSandboxProvider(SandboxProvider): - """OpenShell backend that attaches to a configured sandbox. +def _read_policy_data(policy_path: str, *, require_hard_landlock: bool) -> dict[str, Any]: + """Read and normalize OpenShell policy YAML without importing the optional SDK.""" + try: + import yaml + except ImportError as exc: + raise ImportError(_OPENSHELL_IMPORT_HINT) from exc + + path = Path(policy_path).expanduser() + try: + raw = yaml.safe_load(path.read_text(encoding="utf-8")) + except OSError as exc: + raise ValueError(f"Could not read OpenShell policy file: {path}") from exc + except yaml.YAMLError as exc: + raise ValueError(f"Invalid OpenShell policy YAML: {path}") from exc + + if not isinstance(raw, dict): + raise ValueError(f"OpenShell policy must be a YAML mapping: {path}") + if raw.get("version") != 1: + raise ValueError("OpenShell policy version must be exactly 1") + + landlock = raw.get("landlock") + compatibility = landlock.get("compatibility") if isinstance(landlock, dict) else None + if require_hard_landlock and compatibility != "hard_requirement": + raise ValueError( + "OpenShell production policy requires landlock.compatibility=hard_requirement; " + "set require_hard_landlock=false only for an explicit local demo." + ) + + filesystem = raw.get("filesystem_policy", raw.get("filesystem")) + if not isinstance(filesystem, dict) or not any(filesystem.get(key) for key in ("read_only", "read_write")): + raise ValueError("OpenShell production policy requires non-empty filesystem read_only or read_write rules") + + process = raw.get("process") + if not isinstance(process, dict): + raise ValueError("OpenShell production policy requires a process policy") + for field in ("run_as_user", "run_as_group"): + identity = process.get(field) + if not isinstance(identity, str) or not identity.strip() or identity.strip().lower() in {"0", "root"}: + raise ValueError(f"OpenShell production policy requires a non-root process.{field}") + + network_policies = raw.get("network_policies") or {} + if not isinstance(network_policies, dict): + raise ValueError("OpenShell network_policies must be a mapping") + for policy_name, network_policy in network_policies.items(): + if not isinstance(network_policy, dict): + raise ValueError(f"OpenShell network policy {policy_name!r} must be a mapping") + endpoints = network_policy.get("endpoints") or [] + if not isinstance(endpoints, list): + raise ValueError(f"OpenShell network policy {policy_name!r} endpoints must be a list") + for endpoint in endpoints: + if not isinstance(endpoint, dict): + raise ValueError(f"OpenShell network policy {policy_name!r} contains an invalid endpoint") + if endpoint.get("enforcement") != "enforce" or endpoint.get("access") != "read-only": + raise ValueError( + f"OpenShell network policy {policy_name!r} endpoints must use " + "enforcement=enforce and access=read-only" + ) + + # OpenShell's YAML schema calls this field filesystem_policy while the 0.0.72 + # Python proto calls it filesystem. Keep this compatibility translation in one + # place and reject every other unknown field through ParseDict below. + policy_data = dict(raw) + if "filesystem_policy" in policy_data: + if "filesystem" in policy_data: + raise ValueError("OpenShell policy cannot contain both filesystem_policy and filesystem") + policy_data["filesystem"] = policy_data.pop("filesystem_policy") + return policy_data + + +def _parse_policy_proto(policy_data: dict[str, Any], *, policy_path: str) -> Any: + """Parse one validated policy snapshot into the SDK proto with strict field validation.""" + try: + from google.protobuf.json_format import ParseDict + from openshell._proto import sandbox_pb2 + except ImportError as exc: + raise ImportError(_OPENSHELL_IMPORT_HINT) from exc + + try: + return ParseDict(policy_data, sandbox_pb2.SandboxPolicy(), ignore_unknown_fields=False) + except Exception as exc: # noqa: BLE001 - protobuf raises several parse exception types + raise ValueError(f"OpenShell policy does not match the installed SDK schema: {policy_path}") from exc + + +def _policy_network_hosts(policy_data: dict[str, Any]) -> set[str]: + """Return every hostname authorized by an OpenShell network policy.""" + policies = policy_data.get("network_policies") or {} + if not isinstance(policies, dict): + return set() + hosts: set[str] = set() + for policy in policies.values(): + if not isinstance(policy, dict): + continue + endpoints = policy.get("endpoints") or [] + if not isinstance(endpoints, list): + continue + for endpoint in endpoints: + if isinstance(endpoint, dict) and isinstance(endpoint.get("host"), str): + hosts.add(endpoint["host"].lower().rstrip(".")) + return hosts + + +def _validate_policy_network(policy_data: dict[str, Any], *, mode: str, allow: tuple[str, ...]) -> None: + """Fail closed when the policy grants more egress than the public config declares.""" + policy_hosts = _policy_network_hosts(policy_data) + if mode == "blocked" and policy_hosts: + raise ValueError( + f"OpenShell policy grants network endpoints while sandbox.network is 'blocked': {sorted(policy_hosts)}" + ) + if mode == "allowlist": + configured_hosts = {host.lower().rstrip(".") for host in allow} + unexpected = policy_hosts - configured_hosts + if unexpected: + raise ValueError(f"OpenShell policy grants hosts outside sandbox.network.allow: {sorted(unexpected)}") + + +def _build_sandbox_spec(*, policy: Any, image: str, job_id: str) -> Any: + """Build a secret-free per-job OpenShell spec using the installed SDK schema.""" + try: + from openshell._proto import openshell_pb2 + except ImportError as exc: + raise ImportError(_OPENSHELL_IMPORT_HINT) from exc - OpenShell enforces filesystem/process/network policy at the gateway, so this - provider declares those capabilities. The SDK cannot apply or verify a policy - file while attaching: ``policy`` requires a pre-created named sandbox whose - policy is managed externally. - """ + template = openshell_pb2.SandboxTemplate( + image=image, + labels={"aiq": "deep-research", "aiq-job-id": _normalize_openshell_name(job_id, prefix="")}, + ) + # Deliberately omit environment and providers. Research/model credentials stay + # on the host unless a future explicit credential-provider feature is configured. + return openshell_pb2.SandboxSpec(template=template, policy=policy) + + +class OpenShellSandboxProvider(SandboxProvider): + """OpenShell backend that creates and attests a policy-bound sandbox per job.""" provider_name = "openshell" @@ -161,6 +288,7 @@ def capabilities(self) -> SandboxCapabilities: supports_process_policy=True, supports_artifact_download=True, supports_cleanup=True, + supports_terminate=True, ) def is_recoverable_error(self, exc: Exception) -> bool: @@ -228,11 +356,7 @@ def _download_files_envfree(self, paths: list[str]) -> list[FileDownloadResponse return responses def _create_session(self) -> BaseSandbox: - """Create/attach the OpenShell sandbox and wrap it in the official adapter. - - A configured ``policy`` requires a pre-created named sandbox, because the - SDK cannot apply policy files to anonymous sandboxes. - """ + """Create and attest a per-job OpenShell sandbox, or explicitly attach for debug.""" try: import openshell from langchain_nvidia_openshell import OpenShellSandbox @@ -241,36 +365,103 @@ def _create_session(self) -> BaseSandbox: cfg = self.config oscfg = cfg.providers.openshell - if oscfg.policy and not oscfg.sandbox_name: - raise ValueError( - "OpenShell `policy` requires `sandbox_name`. The SDK cannot apply a policy file to an " - "anonymous sandbox. Create the named sandbox with `openshell sandbox create --policy ` " - "first, then set providers.openshell.sandbox_name." - ) + + if not oscfg.shell or any(not part.strip() for part in oscfg.shell): + raise ValueError("OpenShell shell must contain at least one non-empty argv element") # Release any prior context (covers the recoverable-error reset path). self._exit_context() sandbox_kwargs: dict[str, object] = { "cluster": oscfg.gateway, - "delete_on_exit": oscfg.delete_on_exit, "ready_timeout_seconds": oscfg.ready_timeout_seconds, } - if oscfg.sandbox_name: - sandbox_kwargs["sandbox"] = oscfg.sandbox_name + shared_name = oscfg.shared_sandbox_name + if shared_name is not None: + logger.warning( + "OpenShell shared-sandbox debug attachment enabled: sandbox=%s job=%s; physical job isolation is off", + shared_name, + self.job_id, + ) + sandbox_kwargs.update(sandbox=shared_name, delete_on_exit=oscfg.delete_on_exit) + else: + if not oscfg.policy: + raise ValueError("Per-job OpenShell creation requires a policy file") + policy_data = _read_policy_data(oscfg.policy, require_hard_landlock=oscfg.require_hard_landlock) + _validate_policy_network( + policy_data, + mode=cfg.network.mode, + allow=cfg.network.allow, + ) + policy = _parse_policy_proto(policy_data, policy_path=oscfg.policy) + sandbox_kwargs.update( + spec=_build_sandbox_spec(policy=policy, image=oscfg.image, job_id=self.job_id), + delete_on_exit=True, + ) os_sandbox = openshell.Sandbox(**sandbox_kwargs) - os_sandbox.__enter__() + # Record ownership before entering so a partially successful __enter__ is + # still torn down if readiness or attestation raises. self._os_context = os_sandbox - backend = OpenShellSandbox(sandbox=os_sandbox, timeout=cfg.timeout, shell=oscfg.shell) - logger.info( - "OpenShell sandbox READY: id=%s gateway=%s sandbox_name=%s policy=%s", - backend.id, - oscfg.gateway, - oscfg.sandbox_name, - oscfg.policy, + try: + os_sandbox.__enter__() + self.physical_sandbox_name = getattr(os_sandbox.sandbox, "name", None) + self._attest(os_sandbox) + backend = OpenShellSandbox(sandbox=os_sandbox, timeout=cfg.timeout, shell=oscfg.shell) + sandbox_ref = os_sandbox.sandbox + logger.info( + "OpenShell sandbox attested: id=%s name=%s gateway=%s policy_version=%s shared=%s", + backend.id, + getattr(sandbox_ref, "name", None), + oscfg.gateway, + getattr(sandbox_ref, "current_policy_version", None), + shared_name is not None, + ) + return backend + except Exception: + self._exit_context() + raise + + def _attest(self, os_sandbox: Any) -> None: + """Fail closed unless the entered sandbox is READY with the expected policy revision.""" + try: + from openshell._proto import openshell_pb2 + except ImportError as exc: + raise ImportError(_OPENSHELL_IMPORT_HINT) from exc + + sandbox_ref = os_sandbox.sandbox + phase = getattr(sandbox_ref, "phase", None) + policy_version = getattr(sandbox_ref, "current_policy_version", 0) + if phase != openshell_pb2.SANDBOX_PHASE_READY: + self._emit_attestation(phase=phase, policy_version=policy_version, status="failed") + raise RuntimeError(f"OpenShell sandbox attestation failed: phase={phase!r} is not READY") + + oscfg = self.config.providers.openshell + if oscfg.attest and (not isinstance(policy_version, int) or policy_version <= 0): + self._emit_attestation(phase=phase, policy_version=policy_version, status="failed") + raise RuntimeError("OpenShell sandbox attestation failed: no loaded policy revision") + if oscfg.expected_policy_version is not None and policy_version != oscfg.expected_policy_version: + self._emit_attestation(phase=phase, policy_version=policy_version, status="failed") + raise RuntimeError( + "OpenShell sandbox attestation failed: " + f"policy revision {policy_version!r} != expected {oscfg.expected_policy_version}" + ) + self._emit_attestation(phase=phase, policy_version=policy_version, status="succeeded") + + def _emit_attestation(self, *, phase: object, policy_version: object, status: str) -> None: + """Emit a secret-free OpenShell attestation outcome.""" + self._emit_event( + { + "type": "sandbox.attestation", + "data": { + "provider": self.provider_name, + "sandbox": getattr(self, "physical_sandbox_name", None) or self.sandbox_name, + "phase": phase, + "policy_version": policy_version, + "status": status, + }, + } ) - return backend def close(self) -> None: """Terminate the session and exit the OpenShell context manager.""" @@ -290,6 +481,7 @@ def _exit_context(self) -> None: try: ctx.__exit__(None, None, None) except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + self._cleanup_failed = True logger.warning("OpenShell sandbox %s context cleanup failed", self.sandbox_name, exc_info=True) diff --git a/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py b/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py index d8a1fef7f..dd91f04d9 100644 --- a/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py +++ b/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py @@ -13,29 +13,31 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""OpenShell provider tests: the env-free upload/download shim. - -OpenShell 0.0.57's exec does not propagate ``env`` to the child process, which -breaks the official adapter's env-var file-transfer bootstraps. Our provider -overrides ``upload_files``/``download_files`` to pass the path via argv (and data -via stdin). These tests assert that contract with a fake sandbox, so they require -only the optional SDK + adapter to be importable. -""" +"""OpenShell provider tests: creation/attestation plus confined file transfer.""" from __future__ import annotations import base64 +import importlib.util +import sys +from contextlib import contextmanager from dataclasses import dataclass +from pathlib import Path +from types import ModuleType +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock +from unittest.mock import patch import pytest -pytest.importorskip("openshell") -pytest.importorskip("langchain_nvidia_openshell") - -from aiq_agent.agents.deep_researcher.sandbox.config import SandboxConfig # noqa: E402 -from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import OpenShellSandboxProvider # noqa: E402 +from aiq_agent.agents.deep_researcher.sandbox.base import SandboxProvider +from aiq_agent.agents.deep_researcher.sandbox.config import SandboxConfig +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import OpenShellSandboxProvider +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _build_sandbox_spec +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _parse_policy_proto +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _read_policy_data +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _validate_policy_network @dataclass @@ -63,18 +65,382 @@ def __exit__(self, *_args: object) -> None: self.exit_calls += 1 -def _provider() -> OpenShellSandboxProvider: +def _provider(**openshell_config: Any) -> OpenShellSandboxProvider: cfg = SandboxConfig( provider="openshell", network={"mode": "blocked"}, - providers={"openshell": {"sandbox_name": "demo", "delete_on_exit": False}}, + providers={"openshell": openshell_config}, ) - provider = OpenShellSandboxProvider(cfg, "job-1") + # Construct through the provider-neutral base so these unit tests run even when + # the optional OpenShell SDK/adapter are absent from normal CI. + provider = object.__new__(OpenShellSandboxProvider) + SandboxProvider.__init__(provider, cfg, "job-1") + provider._os_context = None # Avoid real session creation: a non-None _session short-circuits _session_or_create. provider._session = MagicMock() # type: ignore[assignment] return provider +class _FakeCreatedContext(_FakeOpenShellSandbox): + """Entered SDK context with a public sandbox reference for attestation.""" + + def __init__(self, *, phase: int = 2, policy_version: int = 1, name: str = "generated") -> None: + super().__init__() + self.enter_calls = 0 + self.sandbox = SimpleNamespace( + phase=phase, + current_policy_version=policy_version, + name=name, + ) + + def __enter__(self): + self.enter_calls += 1 + return self + + +class _FakeAdapter: + """Minimal langchain-nvidia-openshell adapter used by provider lifecycle tests.""" + + def __init__(self, *, sandbox: _FakeCreatedContext, timeout: int, shell: tuple[str, ...]) -> None: + self.id = sandbox.id + self.sandbox = sandbox + self.timeout = timeout + self.shell = shell + + +@contextmanager +def _fake_optional_modules(context_factory, adapter_cls: type = _FakeAdapter): + openshell_module = ModuleType("openshell") + openshell_module.Sandbox = context_factory # type: ignore[attr-defined] + adapter_module = ModuleType("langchain_nvidia_openshell") + adapter_module.OpenShellSandbox = adapter_cls # type: ignore[attr-defined] + proto_module = ModuleType("openshell._proto") + proto_module.openshell_pb2 = SimpleNamespace(SANDBOX_PHASE_READY=2) # type: ignore[attr-defined] + with patch.dict( + sys.modules, + { + "openshell": openshell_module, + "openshell._proto": proto_module, + "langchain_nvidia_openshell": adapter_module, + }, + ): + yield + + +def _write_policy(path: Path, *, compatibility: str = "hard_requirement", extra: str = "") -> None: + path.write_text( + "\n".join( + [ + "version: 1", + "filesystem_policy:", + " include_workdir: true", + " read_write: [/sandbox]", + "landlock:", + f" compatibility: {compatibility}", + "process:", + " run_as_user: sandbox", + " run_as_group: sandbox", + "network_policies: {}", + extra, + ] + ), + encoding="utf-8", + ) + + +def test_policy_reader_normalizes_filesystem_alias_and_requires_hard_landlock(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + + policy = _read_policy_data(str(policy_path), require_hard_landlock=True) + + assert "filesystem" in policy + assert "filesystem_policy" not in policy + assert policy["landlock"]["compatibility"] == "hard_requirement" + + +def test_policy_reader_rejects_best_effort_in_production(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path, compatibility="best_effort") + + with pytest.raises(ValueError, match="hard_requirement"): + _read_policy_data(str(policy_path), require_hard_landlock=True) + + +def test_policy_reader_rejects_missing_file_and_wrong_version(tmp_path: Path) -> None: + with pytest.raises(ValueError, match="Could not read"): + _read_policy_data(str(tmp_path / "missing.yaml"), require_hard_landlock=True) + + policy_path = tmp_path / "policy.yaml" + policy_path.write_text("version: 2\n", encoding="utf-8") + with pytest.raises(ValueError, match="exactly 1"): + _read_policy_data(str(policy_path), require_hard_landlock=True) + + +@pytest.mark.parametrize( + ("policy", "message"), + [ + ( + "version: 1\nlandlock: {compatibility: hard_requirement}\n" + "process: {run_as_user: sandbox, run_as_group: sandbox}\n", + "filesystem", + ), + ( + "version: 1\nfilesystem_policy: {read_write: [/sandbox]}\n" + "landlock: {compatibility: hard_requirement}\n" + "process: {run_as_user: root, run_as_group: sandbox}\n", + "non-root", + ), + ( + "version: 1\nfilesystem_policy: {read_write: [/sandbox]}\n" + "landlock: {compatibility: hard_requirement}\n" + "process: {run_as_user: sandbox, run_as_group: sandbox}\n" + "network_policies: {bad: {endpoints: [{host: example.com, enforcement: audit, access: read-write}]}}\n", + "enforcement=enforce", + ), + ], +) +def test_policy_reader_enforces_production_floor(tmp_path: Path, policy: str, message: str) -> None: + policy_path = tmp_path / "policy.yaml" + policy_path.write_text(policy, encoding="utf-8") + + with pytest.raises(ValueError, match=message): + _read_policy_data(str(policy_path), require_hard_landlock=True) + + +def test_checked_policy_matches_installed_sdk_schema() -> None: + if importlib.util.find_spec("openshell") is None: + pytest.skip("optional OpenShell SDK is not installed") + + policy_path = "configs/openshell/aiq-research-policy.yaml" + policy_data = _read_policy_data(policy_path, require_hard_landlock=True) + policy = _parse_policy_proto(policy_data, policy_path=policy_path) + + assert policy.version == 1 + + spec = _build_sandbox_spec(policy=policy, image="aiq:test", job_id="job-1") + assert spec.template.image == "aiq:test" + assert dict(spec.template.labels) == {"aiq": "deep-research", "aiq-job-id": "job-1"} + assert not spec.template.environment + assert not spec.environment + assert not spec.providers + + +def test_sandbox_spec_normalizes_long_job_id_label() -> None: + if importlib.util.find_spec("openshell") is None: + pytest.skip("optional OpenShell SDK is not installed") + from openshell._proto import sandbox_pb2 + + spec = _build_sandbox_spec( + policy=sandbox_pb2.SandboxPolicy(version=1), + image="aiq:test", + job_id="JOB_WITH_UNSAFE_CHARS_" * 8, + ) + job_label = spec.template.labels["aiq-job-id"] + assert len(job_label) <= 63 + assert job_label == job_label.lower() + assert "_" not in job_label + + +def test_policy_network_must_not_exceed_declared_allowlist() -> None: + policy = {"network_policies": {"github": {"endpoints": [{"host": "api.github.com"}, {"host": "github.com"}]}}} + + _validate_policy_network( + policy, + mode="allowlist", + allow=("api.github.com", "github.com"), + ) + with pytest.raises(ValueError, match="github.com"): + _validate_policy_network(policy, mode="allowlist", allow=("api.github.com",)) + with pytest.raises(ValueError, match="network endpoints"): + _validate_policy_network(policy, mode="blocked", allow=()) + + +def test_per_job_session_uses_policy_spec_and_attests_before_return(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + created: list[tuple[_FakeCreatedContext, dict[str, Any]]] = [] + + def context_factory(**kwargs: Any) -> _FakeCreatedContext: + context = _FakeCreatedContext() + created.append((context, kwargs)) + return context + + with ( + _fake_optional_modules(context_factory), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig( + provider="openshell", + providers={"openshell": {"policy": str(policy_path), "image": "aiq:test"}}, + ), + "job-123", + ) + backend = provider._create_session() + + context, kwargs = created[0] + assert backend.id == context.id + assert context.enter_calls == 1 + assert kwargs["spec"] == "job-spec" + assert kwargs["delete_on_exit"] is True + assert "sandbox" not in kwargs + assert provider.physical_sandbox_name == "generated" + + +def test_two_jobs_create_distinct_specs_without_named_attachment(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + created: list[dict[str, Any]] = [] + spec_jobs: list[str] = [] + + def context_factory(**kwargs: Any) -> _FakeCreatedContext: + created.append(kwargs) + return _FakeCreatedContext(name=f"generated-{len(created)}") + + def build_spec(*, policy: Any, image: str, job_id: str) -> str: + del policy, image + spec_jobs.append(job_id) + return f"spec-{job_id}" + + with ( + _fake_optional_modules(context_factory), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + side_effect=build_spec, + ), + ): + for job_id in ("job-a", "job-b"): + provider = OpenShellSandboxProvider( + SandboxConfig( + provider="openshell", + providers={"openshell": {"policy": str(policy_path), "image": "aiq:test"}}, + ), + job_id, + ) + provider._create_session() + + assert spec_jobs == ["job-a", "job-b"] + assert [kwargs["spec"] for kwargs in created] == ["spec-job-a", "spec-job-b"] + assert all("sandbox" not in kwargs for kwargs in created) + + +@pytest.mark.parametrize( + ("phase", "policy_version", "expected", "message"), + [ + (3, 1, None, "not READY"), + (2, 0, None, "no loaded policy"), + (2, 2, 1, "expected 1"), + ], +) +def test_attestation_failure_deletes_before_raising( + tmp_path: Path, + phase: int, + policy_version: int, + expected: int | None, + message: str, +) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + context = _FakeCreatedContext(phase=phase, policy_version=policy_version) + + with ( + _fake_optional_modules(lambda **_kwargs: context), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig( + provider="openshell", + providers={ + "openshell": { + "policy": str(policy_path), + "expected_policy_version": expected, + } + }, + ), + "job-123", + ) + with pytest.raises(RuntimeError, match=message): + provider._create_session() + + assert context.exit_calls == 1 + assert provider._os_context is None + + +def test_shared_attachment_requires_explicit_debug_opt_in() -> None: + with pytest.raises(ValueError, match="allow_shared_sandbox"): + SandboxConfig(provider="openshell", providers={"openshell": {"existing_sandbox_name": "shared"}}) + + config = SandboxConfig( + provider="openshell", + providers={ + "openshell": { + "existing_sandbox_name": "shared", + "allow_shared_sandbox": True, + "delete_on_exit": False, + } + }, + ) + assert config.providers.openshell.shared_sandbox_name == "shared" + + +def test_per_job_mode_rejects_disabled_attestation() -> None: + with pytest.raises(ValueError, match="attest=true"): + SandboxConfig( + provider="openshell", + providers={"openshell": {"policy": "policy.yaml", "attest": False}}, + ) + + +def test_adapter_construction_failure_deletes_sandbox(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + context = _FakeCreatedContext() + + class FailingAdapter: + def __init__(self, **_kwargs: Any) -> None: + raise ValueError("bad adapter configuration") + + with ( + _fake_optional_modules(lambda **_kwargs: context, FailingAdapter), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + with pytest.raises(ValueError, match="bad adapter"): + provider._create_session() + + assert context.exit_calls == 1 + assert provider._os_context is None + + def test_upload_passes_path_via_argv_and_data_via_stdin_no_env() -> None: provider = _provider() fake = _FakeOpenShellSandbox() @@ -195,3 +561,18 @@ def test_terminate_exits_openshell_context_once() -> None: assert fake.exit_calls == 1 assert provider._os_context is None + + +def test_context_exit_failure_marks_cleanup_failed() -> None: + provider = _provider() + + class FailingContext: + def __exit__(self, *_args: object) -> None: + raise RuntimeError("gateway delete failed") + + provider._os_context = FailingContext() + + provider.close() + + assert provider.cleanup_succeeded is False + assert provider._os_context is None diff --git a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py index 5e98ef489..308ba3509 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py +++ b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py @@ -32,6 +32,7 @@ from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepAgentsRuntime from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSandboxConfig from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSkillsConfig +from aiq_agent.agents.deep_researcher.deepagents_runtime import _create_sandbox_backend from aiq_agent.agents.deep_researcher.deepagents_runtime import discover_skill_collections from aiq_agent.agents.deep_researcher.deepagents_runtime import resolve_skill_collections @@ -301,3 +302,76 @@ def find_spec(module_name: str): pytest.raises(ImportError, match="langchain-modal"), ): _ = DeepAgentsRuntime(sandbox=DeepResearchSandboxConfig(provider="modal")).backend + + def test_public_openshell_config_maps_isolation_and_attestation(self) -> None: + public = DeepResearchSandboxConfig( + policy="policy.yaml", + openshell_image="aiq:test", + attest=True, + expected_policy_version=3, + network="allowlist", + network_allow=("api.github.com",), + ) + + with patch( + "aiq_agent.agents.deep_researcher.sandbox.create_sandbox_backend", + return_value="backend", + ) as create: + assert _create_sandbox_backend(public, "job-1") == "backend" + + resolved = create.call_args.args[0] + assert resolved.network.mode == "allowlist" + assert resolved.network.allow == ("api.github.com",) + assert resolved.providers.openshell.image == "aiq:test" + assert resolved.providers.openshell.attest is True + assert resolved.providers.openshell.expected_policy_version == 3 + assert resolved.providers.openshell.delete_on_exit is True + + def test_public_allowlist_requires_hosts(self) -> None: + with pytest.raises(ValueError, match="network_allow"): + DeepResearchSandboxConfig(network="allowlist") + + +class TestDeepAgentsRuntimeCleanup: + """Terminal cleanup is idempotent and reports the provider's actual outcome.""" + + def test_finalize_closes_once_and_emits_success(self) -> None: + provider = MagicMock() + provider.provider_name = "openshell" + provider.physical_sandbox_name = "sandbox-1" + provider.cleanup_succeeded = True + events: list[dict[str, object]] = [] + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime( + sandbox=DeepResearchSandboxConfig(), + artifact_emit=events.append, + ) + + assert runtime.finalize(interrupted=False) is True + assert runtime.finalize(interrupted=False) is True + provider.close.assert_called_once_with() + assert provider.terminate.call_count == 0 + assert [event["data"]["status"] for event in events] == ["started", "succeeded"] # type: ignore[index] + + def test_finalize_emits_failed_when_provider_observed_cleanup_error(self) -> None: + provider = MagicMock() + provider.provider_name = "openshell" + provider.sandbox_name = "logical-job" + provider.physical_sandbox_name = None + provider.cleanup_succeeded = False + events: list[dict[str, object]] = [] + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime( + sandbox=DeepResearchSandboxConfig(), + artifact_emit=events.append, + ) + + assert runtime.finalize(interrupted=True) is False + provider.terminate.assert_called_once_with() + assert [event["data"]["status"] for event in events] == ["started", "failed"] # type: ignore[index] diff --git a/tests/aiq_agent/jobs/test_runner.py b/tests/aiq_agent/jobs/test_runner.py index 4c3e895c8..dd26b2bac 100644 --- a/tests/aiq_agent/jobs/test_runner.py +++ b/tests/aiq_agent/jobs/test_runner.py @@ -1908,6 +1908,16 @@ def test_none_runtime_is_noop(self): # Must not raise when no sandbox runtime is present (non-sandbox agents). _teardown_sandbox(None, job_id="job-1", interrupted=False) + def test_runtime_finalizer_owns_cleanup_when_available(self): + from aiq_api.jobs.runner import _teardown_sandbox + + runtime = MagicMock(spec=["finalize", "close", "terminate"]) + _teardown_sandbox(runtime, job_id="job-1", interrupted=True) + + runtime.finalize.assert_called_once_with(interrupted=True) + runtime.close.assert_not_called() + runtime.terminate.assert_not_called() + def test_normal_path_calls_close(self): from aiq_api.jobs.runner import _teardown_sandbox From b6b058e367981da8d2884e0d47345e7f0cc8c1b6 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Wed, 1 Jul 2026 16:08:57 -0700 Subject: [PATCH 2/8] test: add live OpenShell isolation smoke Signed-off-by: Kyle Zheng --- scripts/README.md | 27 ++++ scripts/smoke_openshell_isolation.py | 177 +++++++++++++++++++++++++++ 2 files changed, 204 insertions(+) create mode 100755 scripts/smoke_openshell_isolation.py diff --git a/scripts/README.md b/scripts/README.md index 70afbc972..d5a2d8136 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -81,6 +81,33 @@ Useful version examples: In the interactive version prompt, pressing Enter selects `0.0.72`. +Deterministic live acceptance (no LLM or external research API involved): + +```bash +.venv/bin/python scripts/smoke_openshell_isolation.py --gateway aiq-local +``` + +The probe fails unless the gateway service itself reports OpenShell `0.0.72`. It creates two +jobs concurrently, proves they have distinct physical sandbox IDs and positive loaded policy +revisions, cancels one, proves the other still executes, and then proves both were deleted. +Success is four explicit lines: + +```text +PASS gateway version: 0.0.72 +PASS distinct attested sandboxes: A=@r, B=@r +PASS cancellation isolation: A deleted; B remained usable +PASS terminal cleanup: both probe sandboxes deleted +``` + +On a non-production local host that intentionally uses the generated `best_effort` policy: + +```bash +.venv/bin/python scripts/smoke_openshell_isolation.py \ + --gateway aiq-local \ + --policy configs/openshell/generated/aiq-openshell-policy.yaml \ + --allow-best-effort-landlock +``` + The setup installs the `openshell` SDK plus the official `langchain-nvidia-openshell` adapter (`OpenShellSandbox`), published on PyPI. The script installs it from PyPI by default; set `LANGCHAIN_NVIDIA_REPO` or pass `--langchain-nvidia` to use another diff --git a/scripts/smoke_openshell_isolation.py b/scripts/smoke_openshell_isolation.py new file mode 100755 index 000000000..e11415b45 --- /dev/null +++ b/scripts/smoke_openshell_isolation.py @@ -0,0 +1,177 @@ +#!/usr/bin/env python3 +# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Live, deterministic proof of OpenShell per-job isolation and cleanup. + +This intentionally bypasses the research LLM. It exercises the production AI-Q +OpenShell provider against a real gateway, using fixed shell commands as the oracle. +""" + +from __future__ import annotations + +import argparse +import os +import shlex +import sys +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from uuid import uuid4 + +import grpc + +from aiq_agent.agents.deep_researcher.sandbox import SandboxConfig +from aiq_agent.agents.deep_researcher.sandbox import create_sandbox_backend +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _policy_network_hosts +from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _read_policy_data + + +def _args() -> argparse.Namespace: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--gateway", help="Registered OpenShell gateway name; default uses the active gateway") + parser.add_argument( + "--policy", + default="configs/openshell/aiq-research-policy.yaml", + help="Policy applied to both probe sandboxes", + ) + parser.add_argument( + "--image", + default=os.getenv("AIQ_OPENSHELL_IMAGE", "aiq-openshell-demo:latest"), + help="Prebuilt OpenShell sandbox image", + ) + parser.add_argument( + "--expected-gateway-version", + default="0.0.72", + help="Fail unless the live gateway reports this exact version", + ) + parser.add_argument( + "--allow-best-effort-landlock", + action="store_true", + help="Permit a local-demo best_effort policy; never use this for production acceptance", + ) + return parser.parse_args() + + +def _assert_deleted(client: object, name: str) -> None: + try: + client.get(name) # type: ignore[attr-defined] + except grpc.RpcError as exc: + if isinstance(exc, grpc.Call) and exc.code() == grpc.StatusCode.NOT_FOUND: + return + raise + raise AssertionError(f"sandbox still exists after cleanup: {name}") + + +def _attested(events: list[dict[str, object]]) -> bool: + for event in events: + if event.get("type") != "sandbox.attestation": + continue + data = event.get("data") + if isinstance(data, dict) and data.get("status") == "succeeded": + return isinstance(data.get("policy_version"), int) and data["policy_version"] > 0 + return False + + +def main() -> int: + args = _args() + policy_path = Path(args.policy).resolve() + require_hard_landlock = not args.allow_best_effort_landlock + policy_data = _read_policy_data(str(policy_path), require_hard_landlock=require_hard_landlock) + hosts = tuple(sorted(_policy_network_hosts(policy_data))) + network = {"mode": "allowlist", "allow": hosts} if hosts else {"mode": "blocked"} + + try: + from openshell.sandbox import SandboxClient + except ImportError as exc: + raise RuntimeError("Run scripts/setup_openshell.sh before this smoke test") from exc + + with SandboxClient.from_active_cluster(cluster=args.gateway) as client: + version = client.health().version + if version != args.expected_gateway_version: + raise AssertionError( + f"gateway is {version}, expected {args.expected_gateway_version}; " + "upgrade the gateway service before claiming acceptance" + ) + print(f"PASS gateway version: {version}") + + suffix = uuid4().hex[:10] + job_ids = (f"aiq-isolation-a-{suffix}", f"aiq-isolation-b-{suffix}") + events: tuple[list[dict[str, object]], list[dict[str, object]]] = ([], []) + + def provider(job_id: str, sink: list[dict[str, object]]): + config = SandboxConfig( + provider="openshell", + workdir="/sandbox", + network=network, + providers={ + "openshell": { + "gateway": args.gateway, + "policy": str(policy_path), + "image": args.image, + "require_hard_landlock": require_hard_landlock, + } + }, + ) + created = create_sandbox_backend(config, job_id) + created.set_event_emitter(sink.append) + return created + + sandboxes = (provider(job_ids[0], events[0]), provider(job_ids[1], events[1])) + markers = (f"owner-a-{suffix}", f"owner-b-{suffix}") + + def initialize(index: int) -> str: + sandbox = sandboxes[index] + marker_file = f"{sandbox.workdir}/owner.txt" + command = ( + f"printf %s {shlex.quote(markers[index])} > {shlex.quote(marker_file)}; cat {shlex.quote(marker_file)}" + ) + result = sandbox.execute(command, timeout=30) + if result.exit_code != 0 or result.output.strip() != markers[index]: + raise AssertionError(f"job {index} workspace probe failed: exit={result.exit_code}") + return sandbox.physical_sandbox_name + + try: + with ThreadPoolExecutor(max_workers=2) as pool: + names = tuple(pool.map(initialize, range(2))) + if names[0] == names[1]: + raise AssertionError(f"jobs attached to the same physical sandbox: {names[0]}") + + with SandboxClient.from_active_cluster(cluster=args.gateway) as client: + refs = (client.get(names[0]), client.get(names[1])) + if refs[0].id == refs[1].id: + raise AssertionError(f"jobs share physical sandbox id {refs[0].id}") + if not all(ref.current_policy_version > 0 for ref in refs): + raise AssertionError("a live sandbox has no loaded policy revision") + if not all(_attested(job_events) for job_events in events): + raise AssertionError("AI-Q did not emit successful positive-revision attestation events") + print( + "PASS distinct attested sandboxes: " + f"A={names[0]}@r{refs[0].current_policy_version}, " + f"B={names[1]}@r{refs[1].current_policy_version}" + ) + + sandboxes[0].terminate() + _assert_deleted(client, names[0]) + if client.get(names[1]).id != refs[1].id: + raise AssertionError("job B changed after cancelling job A") + result = sandboxes[1].execute("printf %s still-alive", timeout=30) + if result.exit_code != 0 or result.output.strip() != "still-alive": + raise AssertionError("job B stopped working after job A was cancelled") + print("PASS cancellation isolation: A deleted; B remained usable") + + sandboxes[1].close() + _assert_deleted(client, names[1]) + print("PASS terminal cleanup: both probe sandboxes deleted") + finally: + for sandbox in sandboxes: + sandbox.terminate() + + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except Exception as exc: # noqa: BLE001 - smoke script should print one direct failure + print(f"FAIL {type(exc).__name__}: {exc}", file=sys.stderr) + raise SystemExit(1) from exc From 8e2287b32cb843bc1c70190937653b5d6ea4ef54 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Wed, 1 Jul 2026 16:44:40 -0700 Subject: [PATCH 3/8] fix: harden OpenShell cleanup lifecycle Signed-off-by: Kyle Zheng --- frontends/aiq_api/src/aiq_api/jobs/runner.py | 19 +- scripts/smoke_openshell_isolation.py | 32 +- .../deep_researcher/deepagents_runtime.py | 45 ++- .../agents/deep_researcher/sandbox/base.py | 2 +- .../sandbox/providers/openshell.py | 154 +++++++-- .../sandbox/test_openshell_provider.py | 317 +++++++++++++++++- .../agents/deep_researcher/test_agent.py | 46 +++ .../test_deepagents_runtime.py | 86 +++++ tests/aiq_agent/jobs/test_runner.py | 25 ++ 9 files changed, 674 insertions(+), 52 deletions(-) diff --git a/frontends/aiq_api/src/aiq_api/jobs/runner.py b/frontends/aiq_api/src/aiq_api/jobs/runner.py index b68b2c5cc..7c63e44ff 100644 --- a/frontends/aiq_api/src/aiq_api/jobs/runner.py +++ b/frontends/aiq_api/src/aiq_api/jobs/runner.py @@ -638,16 +638,14 @@ async def run_agent_job( finally: # Ensure terminal-path events are not left in the batch buffer. - if event_store is not None and hasattr(event_store, "flush"): - event_store.flush() + await _flush_event_store(event_store, job_id=job_id) if cancellation_monitor: cancellation_monitor.stop() # Release the sandbox off the event loop so the SDK session close never blocks the Dask # worker. The single artifact harvest already ran in agent.run() before this point, so # teardown only closes/terminates; interrupted jobs terminate() to preempt a live execute. await asyncio.to_thread(_teardown_sandbox, sandbox_runtime, job_id=job_id, interrupted=interrupted) - if event_store is not None and hasattr(event_store, "flush"): - event_store.flush() + await _flush_event_store(event_store, job_id=job_id) # Clean up job-scoped auth token if _auth_token_reset is not None: from ._auth_context import job_auth_token @@ -655,6 +653,16 @@ async def run_agent_job( job_auth_token.reset(_auth_token_reset) +async def _flush_event_store(event_store: Any | None, *, job_id: str) -> None: + """Flush terminal events off-loop without replacing the job result.""" + if event_store is None or not hasattr(event_store, "flush"): + return + try: + await asyncio.to_thread(event_store.flush) + except Exception as exc: # noqa: BLE001 - terminal observability must not replace the job result + logger.warning("Event store flush failed for job %s (%s)", job_id, type(exc).__name__) + + def _teardown_sandbox(sandbox_runtime: Any | None, *, job_id: str, interrupted: bool) -> None: """Release sandbox resources on a terminal path (best-effort, never raises). @@ -667,7 +675,8 @@ def _teardown_sandbox(sandbox_runtime: Any | None, *, job_id: str, interrupted: finalize = getattr(sandbox_runtime, "finalize", None) if finalize is not None: try: - finalize(interrupted=interrupted) + if not finalize(interrupted=interrupted): + logger.warning("Sandbox cleanup reported failure for job %s", job_id) except Exception: # noqa: BLE001 - cleanup must never replace the job result logger.warning("Sandbox cleanup failed for job %s", job_id, exc_info=True) return diff --git a/scripts/smoke_openshell_isolation.py b/scripts/smoke_openshell_isolation.py index e11415b45..f56e66b1b 100755 --- a/scripts/smoke_openshell_isolation.py +++ b/scripts/smoke_openshell_isolation.py @@ -72,6 +72,29 @@ def _attested(events: list[dict[str, object]]) -> bool: return False +def _loaded_policy_revision(client: object, name: str) -> int: + """Return the loaded revision, including the OpenShell 0.0.72 status-RPC fallback.""" + from openshell._proto import openshell_pb2 + + sandbox = client.get(name) # type: ignore[attr-defined] + if sandbox.current_policy_version > 0: + return sandbox.current_policy_version + + # OpenShell 0.0.72 can leave SandboxStatus.current_policy_version at zero even + # after the authoritative policy status RPC records the initial revision as loaded. + stub = getattr(client, "_stub", None) + if stub is None or not hasattr(stub, "GetSandboxPolicyStatus"): + return 0 + response = stub.GetSandboxPolicyStatus( + openshell_pb2.GetSandboxPolicyStatusRequest(name=name, version=0), + timeout=30, + ) + revision = response.revision + if revision.status != openshell_pb2.POLICY_STATUS_LOADED: + return 0 + return revision.version + + def main() -> int: args = _args() policy_path = Path(args.policy).resolve() @@ -140,15 +163,12 @@ def initialize(index: int) -> str: refs = (client.get(names[0]), client.get(names[1])) if refs[0].id == refs[1].id: raise AssertionError(f"jobs share physical sandbox id {refs[0].id}") - if not all(ref.current_policy_version > 0 for ref in refs): + revisions = tuple(_loaded_policy_revision(client, name) for name in names) + if not all(revision > 0 for revision in revisions): raise AssertionError("a live sandbox has no loaded policy revision") if not all(_attested(job_events) for job_events in events): raise AssertionError("AI-Q did not emit successful positive-revision attestation events") - print( - "PASS distinct attested sandboxes: " - f"A={names[0]}@r{refs[0].current_policy_version}, " - f"B={names[1]}@r{refs[1].current_policy_version}" - ) + print(f"PASS distinct attested sandboxes: A={names[0]}@r{revisions[0]}, B={names[1]}@r{revisions[1]}") sandboxes[0].terminate() _assert_deleted(client, names[0]) diff --git a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py index 8769adb11..d4ffa30f8 100644 --- a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py +++ b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py @@ -151,7 +151,15 @@ def _validate_openshell_and_network(self) -> DeepResearchSandboxConfig: if self.network == "allowlist" and not self.network_allow: raise ValueError("network='allowlist' requires a non-empty network_allow list") if self.provider == "openshell": + if ( + self.existing_sandbox_name is not None + and self.sandbox_name is not None + and self.existing_sandbox_name != self.sandbox_name + ): + raise ValueError("existing_sandbox_name and sandbox_name must match when both are set") shared_name = self.existing_sandbox_name or self.sandbox_name + if shared_name and not self.allow_shared_sandbox: + raise ValueError("existing_sandbox_name/sandbox_name requires allow_shared_sandbox=true") if not shared_name and not self.attest: raise ValueError("Per-job OpenShell creation requires attest=true") return self @@ -191,6 +199,7 @@ def __init__( self._event_emit = artifact_emit self._finalize_lock = threading.Lock() self._finalized = False + self._finalize_result: bool | None = None self.artifact_manager: Any | None = None self._skill_sources_by_agent = _resolve_agent_skill_sources(skills) self._skill_sources = tuple( @@ -279,23 +288,25 @@ def finalize(self, *, interrupted: bool) -> bool: """Release the sandbox once and emit a truthful, sanitized cleanup outcome.""" with self._finalize_lock: if self._finalized: - return bool(getattr(self._sandbox_provider, "cleanup_succeeded", True)) - self._finalized = True - - self._emit_cleanup("started", interrupted=interrupted) - try: - if interrupted: - self.terminate() + assert self._finalize_result is not None + return self._finalize_result + + self._emit_cleanup("started", interrupted=interrupted) + try: + if interrupted: + self.terminate() + else: + self.close() + except Exception as exc: # noqa: BLE001 - terminal cleanup cannot replace the job result + logger.warning("Sandbox cleanup failed for job %s (%s)", self._job_id, type(exc).__name__) + succeeded = False else: - self.close() - except Exception: # noqa: BLE001 - terminal cleanup cannot replace the job result - logger.warning("Sandbox cleanup failed for job %s", self._job_id, exc_info=True) - self._emit_cleanup("failed", interrupted=interrupted) - return False + succeeded = bool(getattr(self._sandbox_provider, "cleanup_succeeded", True)) - succeeded = bool(getattr(self._sandbox_provider, "cleanup_succeeded", True)) - self._emit_cleanup("succeeded" if succeeded else "failed", interrupted=interrupted) - return succeeded + self._emit_cleanup("succeeded" if succeeded else "failed", interrupted=interrupted) + self._finalize_result = succeeded + self._finalized = True + return succeeded def _emit_cleanup(self, status: str, *, interrupted: bool) -> None: """Emit cleanup metadata without policy contents, environment, or error text.""" @@ -314,8 +325,8 @@ def _emit_cleanup(self, status: str, *, interrupted: bool) -> None: }, } ) - except Exception: # noqa: BLE001 - observability must not break terminal cleanup - logger.warning("Sandbox cleanup event emission failed for job %s", self._job_id, exc_info=True) + except Exception as exc: # noqa: BLE001 - observability must not break terminal cleanup + logger.warning("Sandbox cleanup event emission failed for job %s (%s)", self._job_id, type(exc).__name__) def prepare_state_files(self, files: dict[str, Any]) -> dict[str, Any]: """Normalize seeded virtual filesystem files for the configured backend.""" diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/base.py b/src/aiq_agent/agents/deep_researcher/sandbox/base.py index e2baf3eaa..c43f865d6 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/base.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/base.py @@ -212,7 +212,7 @@ def _safe_close(self, session: BaseSandbox | None) -> None: @property def cleanup_succeeded(self) -> bool: - """Whether every cleanup operation completed without an observed error.""" + """Whether every cleanup attempt, including retry teardown, completed without an error.""" return not self._cleanup_failed @property diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py b/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py index d4af6d5ca..beca4d0e7 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py @@ -33,6 +33,7 @@ import logging import os import re +import time from pathlib import Path from typing import TYPE_CHECKING from typing import Any @@ -42,6 +43,7 @@ from deepagents.backends.sandbox import BaseSandbox from ..base import SandboxProvider +from ..base import SandboxTerminatedError from ..capabilities import SandboxCapabilities from ..registry import register_sandbox_provider @@ -267,6 +269,8 @@ def __init__(self, config: SandboxConfig, job_id: str) -> None: """Initialize the provider, requiring the OpenShell SDK and adapter to import.""" super().__init__(config, job_id) self._os_context: object | None = None + self._os_context_entering = False + self._os_context_exit_requested = False try: import langchain_nvidia_openshell # noqa: F401 import openshell # noqa: F401 @@ -309,7 +313,7 @@ def download_files(self, paths: list[str]) -> list[FileDownloadResponse]: def _upload_files_envfree(self, files: list[tuple[str, bytes]]) -> list[FileUploadResponse]: """Upload files via argv + stdin so no ``OPENSHELL_*`` env is required.""" - sandbox = self._os_context + sandbox = self._active_os_context() responses: list[FileUploadResponse] = [] for path, content in files: if not path.startswith("/"): @@ -327,7 +331,7 @@ def _upload_files_envfree(self, files: list[tuple[str, bytes]]) -> list[FileUplo def _download_files_envfree(self, paths: list[str]) -> list[FileDownloadResponse]: """Download files via an argv bootstrap that enforces size/symlink limits in-sandbox.""" - sandbox = self._os_context + sandbox = self._active_os_context() # Cap passed to the bootstrap so oversized files are refused before transfer. max_bytes = self.config.artifact_capture.max_file_bytes responses: list[FileDownloadResponse] = [] @@ -383,7 +387,9 @@ def _create_session(self) -> BaseSandbox: shared_name, self.job_id, ) - sandbox_kwargs.update(sandbox=shared_name, delete_on_exit=oscfg.delete_on_exit) + # Attachment does not transfer ownership: never delete a shared sandbox + # that this job did not create. + sandbox_kwargs.update(sandbox=shared_name, delete_on_exit=False) else: if not oscfg.policy: raise ValueError("Per-job OpenShell creation requires a policy file") @@ -396,33 +402,35 @@ def _create_session(self) -> BaseSandbox: policy = _parse_policy_proto(policy_data, policy_path=oscfg.policy) sandbox_kwargs.update( spec=_build_sandbox_spec(policy=policy, image=oscfg.image, job_id=self.job_id), - delete_on_exit=True, + delete_on_exit=oscfg.delete_on_exit, ) os_sandbox = openshell.Sandbox(**sandbox_kwargs) - # Record ownership before entering so a partially successful __enter__ is - # still torn down if readiness or attestation raises. - self._os_context = os_sandbox + self._enter_context(os_sandbox) + backend: BaseSandbox | None = None try: - os_sandbox.__enter__() + self._ensure_context_active(os_sandbox) self.physical_sandbox_name = getattr(os_sandbox.sandbox, "name", None) - self._attest(os_sandbox) + phase, policy_version = self._attest(os_sandbox) + self._ensure_context_active(os_sandbox) backend = OpenShellSandbox(sandbox=os_sandbox, timeout=cfg.timeout, shell=oscfg.shell) + self._ensure_context_active(os_sandbox) sandbox_ref = os_sandbox.sandbox logger.info( - "OpenShell sandbox attested: id=%s name=%s gateway=%s policy_version=%s shared=%s", + "OpenShell sandbox attested: id=%s name=%s policy_version=%s shared=%s", backend.id, getattr(sandbox_ref, "name", None), - oscfg.gateway, - getattr(sandbox_ref, "current_policy_version", None), + policy_version, shared_name is not None, ) + self._emit_attestation(phase=phase, policy_version=policy_version, status="succeeded") return backend - except Exception: + except BaseException: + self._safe_close(backend) self._exit_context() raise - def _attest(self, os_sandbox: Any) -> None: + def _attest(self, os_sandbox: Any) -> tuple[object, object]: """Fail closed unless the entered sandbox is READY with the expected policy revision.""" try: from openshell._proto import openshell_pb2 @@ -437,6 +445,11 @@ def _attest(self, os_sandbox: Any) -> None: raise RuntimeError(f"OpenShell sandbox attestation failed: phase={phase!r} is not READY") oscfg = self.config.providers.openshell + if oscfg.attest and (not isinstance(policy_version, int) or policy_version <= 0): + phase, policy_version = self._wait_for_loaded_policy(os_sandbox, sandbox_ref) + if phase != openshell_pb2.SANDBOX_PHASE_READY: + self._emit_attestation(phase=phase, policy_version=policy_version, status="failed") + raise RuntimeError(f"OpenShell sandbox attestation failed: phase={phase!r} is not READY") if oscfg.attest and (not isinstance(policy_version, int) or policy_version <= 0): self._emit_attestation(phase=phase, policy_version=policy_version, status="failed") raise RuntimeError("OpenShell sandbox attestation failed: no loaded policy revision") @@ -446,7 +459,47 @@ def _attest(self, os_sandbox: Any) -> None: "OpenShell sandbox attestation failed: " f"policy revision {policy_version!r} != expected {oscfg.expected_policy_version}" ) - self._emit_attestation(phase=phase, policy_version=policy_version, status="succeeded") + return phase, policy_version + + def _wait_for_loaded_policy(self, os_sandbox: Any, sandbox_ref: Any) -> tuple[object, object]: + """Wait for READY plus a loaded policy revision on OpenShell 0.0.72.""" + from openshell._proto import openshell_pb2 + + oscfg = self.config.providers.openshell + phase = getattr(sandbox_ref, "phase", None) + policy_version = getattr(sandbox_ref, "current_policy_version", 0) + client = getattr(os_sandbox, "_client", None) + # OpenShell 0.0.72 does not expose policy status on SandboxClient even though the + # generated RPC is authoritative; isolate that compatibility access here. + stub = getattr(client, "_stub", None) + if client is None or stub is None or not hasattr(stub, "GetSandboxPolicyStatus"): + return phase, policy_version + + request = openshell_pb2.GetSandboxPolicyStatusRequest(name=sandbox_ref.name, version=0) + deadline = time.monotonic() + oscfg.ready_timeout_seconds + while True: + self._ensure_context_active(os_sandbox) + sandbox_ref = client.get(sandbox_ref.name) + phase = getattr(sandbox_ref, "phase", None) + policy_version = getattr(sandbox_ref, "current_policy_version", 0) + if phase == openshell_pb2.SANDBOX_PHASE_READY and policy_version > 0: + return phase, policy_version + remaining = deadline - time.monotonic() + if remaining <= 0: + return phase, policy_version + + response = stub.GetSandboxPolicyStatus(request, timeout=min(5.0, remaining)) + revision = getattr(response, "revision", None) + revision_status = getattr(revision, "status", openshell_pb2.POLICY_STATUS_UNSPECIFIED) + if revision_status == openshell_pb2.POLICY_STATUS_LOADED: + policy_version = getattr(revision, "version", 0) + if phase == openshell_pb2.SANDBOX_PHASE_READY and policy_version > 0: + return phase, policy_version + elif revision_status == openshell_pb2.POLICY_STATUS_FAILED: + return phase, 0 + if phase == openshell_pb2.SANDBOX_PHASE_ERROR: + return phase, policy_version + time.sleep(min(0.5, remaining)) def _emit_attestation(self, *, phase: object, policy_version: object, status: str) -> None: """Emit a secret-free OpenShell attestation outcome.""" @@ -473,11 +526,74 @@ def _terminate_session(self, session: BaseSandbox | None) -> None: super()._terminate_session(session) self._exit_context() + def _active_os_context(self) -> Any: + """Return the current SDK context without racing an out-of-band teardown.""" + with self._state_lock: + ctx = self._os_context + if ctx is None: + raise SandboxTerminatedError(f"OpenShell sandbox {self.sandbox_name} has no active context") + return ctx + + def _enter_context(self, ctx: object) -> None: + """Enter an SDK context while allowing terminate() to request deferred cleanup.""" + with self._state_lock: + if self._terminated: + raise SandboxTerminatedError(f"Sandbox {self.sandbox_name} has been terminated") + if self._os_context is not None: + raise RuntimeError(f"OpenShell sandbox {self.sandbox_name} context creation is already in progress") + self._os_context = ctx + self._os_context_entering = True + self._os_context_exit_requested = False + + try: + ctx.__enter__() # type: ignore[attr-defined] + except BaseException: + self._finish_context_entry(ctx, entered=False) + raise + + if not self._finish_context_entry(ctx, entered=True): + raise SandboxTerminatedError(f"Sandbox {self.sandbox_name} was closed during creation") + + def _finish_context_entry(self, ctx: object, *, entered: bool) -> bool: + """Publish an entered context, or honor a pending teardown exactly once.""" + with self._state_lock: + owns_context = self._os_context is ctx + if owns_context: + self._os_context_entering = False + release_context = not entered or self._os_context_exit_requested or self._terminated + if release_context: + self._os_context = None + self._os_context_exit_requested = False + else: + release_context = entered + + if release_context: + self._close_os_context(ctx) + return entered and owns_context and not release_context + + def _ensure_context_active(self, ctx: object) -> None: + """Abort session publication when teardown won a creation race.""" + with self._state_lock: + active = self._os_context is ctx and not self._os_context_exit_requested and not self._terminated + if not active: + raise SandboxTerminatedError(f"Sandbox {self.sandbox_name} was closed during creation") + def _exit_context(self) -> None: - """Exit the OpenShell context once, swallowing cleanup errors on the terminal path.""" - ctx = self._os_context - self._os_context = None - if ctx is not None and hasattr(ctx, "__exit__"): + """Exit once, deferring deletion to the creator while ``__enter__`` is in flight.""" + with self._state_lock: + ctx = self._os_context + if ctx is None: + return + if self._os_context_entering: + self._os_context_exit_requested = True + return + self._os_context = None + self._os_context_exit_requested = False + self._close_os_context(ctx) + + def _close_os_context(self, ctx: object) -> None: + """Drive one detached SDK context exit without replacing the job result.""" + if hasattr(ctx, "__exit__"): try: ctx.__exit__(None, None, None) except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path diff --git a/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py b/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py index dd91f04d9..73f4ce86f 100644 --- a/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py +++ b/tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py @@ -23,6 +23,8 @@ from contextlib import contextmanager from dataclasses import dataclass from pathlib import Path +from threading import Event +from threading import Thread from types import ModuleType from types import SimpleNamespace from typing import Any @@ -32,6 +34,7 @@ import pytest from aiq_agent.agents.deep_researcher.sandbox.base import SandboxProvider +from aiq_agent.agents.deep_researcher.sandbox.base import SandboxTerminatedError from aiq_agent.agents.deep_researcher.sandbox.config import SandboxConfig from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import OpenShellSandboxProvider from aiq_agent.agents.deep_researcher.sandbox.providers.openshell import _build_sandbox_spec @@ -76,6 +79,8 @@ def _provider(**openshell_config: Any) -> OpenShellSandboxProvider: provider = object.__new__(OpenShellSandboxProvider) SandboxProvider.__init__(provider, cfg, "job-1") provider._os_context = None + provider._os_context_entering = False + provider._os_context_exit_requested = False # Avoid real session creation: a non-None _session short-circuits _session_or_create. provider._session = MagicMock() # type: ignore[assignment] return provider @@ -115,7 +120,14 @@ def _fake_optional_modules(context_factory, adapter_cls: type = _FakeAdapter): adapter_module = ModuleType("langchain_nvidia_openshell") adapter_module.OpenShellSandbox = adapter_cls # type: ignore[attr-defined] proto_module = ModuleType("openshell._proto") - proto_module.openshell_pb2 = SimpleNamespace(SANDBOX_PHASE_READY=2) # type: ignore[attr-defined] + proto_module.openshell_pb2 = SimpleNamespace( # type: ignore[attr-defined] + SANDBOX_PHASE_READY=2, + SANDBOX_PHASE_ERROR=3, + POLICY_STATUS_UNSPECIFIED=0, + POLICY_STATUS_LOADED=2, + POLICY_STATUS_FAILED=3, + GetSandboxPolicyStatusRequest=lambda **kwargs: SimpleNamespace(**kwargs), + ) with patch.dict( sys.modules, { @@ -256,7 +268,10 @@ def test_policy_network_must_not_exceed_declared_allowlist() -> None: _validate_policy_network(policy, mode="blocked", allow=()) -def test_per_job_session_uses_policy_spec_and_attests_before_return(tmp_path: Path) -> None: +def test_per_job_session_uses_policy_spec_and_attests_before_return( + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: policy_path = tmp_path / "policy.yaml" _write_policy(policy_path) created: list[tuple[_FakeCreatedContext, dict[str, Any]]] = [] @@ -280,11 +295,18 @@ def context_factory(**kwargs: Any) -> _FakeCreatedContext: provider = OpenShellSandboxProvider( SandboxConfig( provider="openshell", - providers={"openshell": {"policy": str(policy_path), "image": "aiq:test"}}, + providers={ + "openshell": { + "policy": str(policy_path), + "image": "aiq:test", + "gateway": "sensitive-gateway-name", + } + }, ), "job-123", ) - backend = provider._create_session() + with caplog.at_level("INFO"): + backend = provider._create_session() context, kwargs = created[0] assert backend.id == context.id @@ -293,6 +315,7 @@ def context_factory(**kwargs: Any) -> _FakeCreatedContext: assert kwargs["delete_on_exit"] is True assert "sandbox" not in kwargs assert provider.physical_sandbox_name == "generated" + assert "sensitive-gateway-name" not in caplog.text def test_two_jobs_create_distinct_specs_without_named_attachment(tmp_path: Path) -> None: @@ -385,6 +408,47 @@ def test_attestation_failure_deletes_before_raising( assert provider._os_context is None +def test_attestation_refreshes_ready_sandbox_until_policy_is_loaded(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + context = _FakeCreatedContext(policy_version=0) + refreshed = SimpleNamespace(phase=2, current_policy_version=0, name=context.sandbox.name) + policy_status = MagicMock( + return_value=SimpleNamespace(revision=SimpleNamespace(version=1, status=2)), + ) + context._client = SimpleNamespace( # type: ignore[attr-defined] + get=MagicMock(return_value=refreshed), + _stub=SimpleNamespace(GetSandboxPolicyStatus=policy_status), + ) + events: list[dict[str, object]] = [] + + with ( + _fake_optional_modules(lambda **_kwargs: context), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + provider.set_event_emitter(events.append) + + provider._create_session() + + request = policy_status.call_args.args[0] + assert request.name == context.sandbox.name + assert request.version == 0 + succeeded = [event for event in events if event["data"]["status"] == "succeeded"] # type: ignore[index] + assert len(succeeded) == 1 + assert succeeded[0]["data"]["policy_version"] == 1 # type: ignore[index] + + def test_shared_attachment_requires_explicit_debug_opt_in() -> None: with pytest.raises(ValueError, match="allow_shared_sandbox"): SandboxConfig(provider="openshell", providers={"openshell": {"existing_sandbox_name": "shared"}}) @@ -402,6 +466,35 @@ def test_shared_attachment_requires_explicit_debug_opt_in() -> None: assert config.providers.openshell.shared_sandbox_name == "shared" +def test_shared_attachment_never_deletes_unowned_sandbox() -> None: + created: list[dict[str, Any]] = [] + + def context_factory(**kwargs: Any) -> _FakeCreatedContext: + created.append(kwargs) + return _FakeCreatedContext(name="shared") + + with _fake_optional_modules(context_factory): + provider = OpenShellSandboxProvider( + SandboxConfig( + provider="openshell", + providers={ + "openshell": { + "existing_sandbox_name": "shared", + "allow_shared_sandbox": True, + "delete_on_exit": True, + } + }, + ), + "job-123", + ) + provider._create_session() + provider.close() + + assert created[0]["sandbox"] == "shared" + assert created[0]["delete_on_exit"] is False + assert "spec" not in created[0] + + def test_per_job_mode_rejects_disabled_attestation() -> None: with pytest.raises(ValueError, match="attest=true"): SandboxConfig( @@ -409,6 +502,12 @@ def test_per_job_mode_rejects_disabled_attestation() -> None: providers={"openshell": {"policy": "policy.yaml", "attest": False}}, ) + with pytest.raises(ValueError, match="delete_on_exit=true"): + SandboxConfig( + provider="openshell", + providers={"openshell": {"policy": "policy.yaml", "delete_on_exit": False}}, + ) + def test_adapter_construction_failure_deletes_sandbox(tmp_path: Path) -> None: policy_path = tmp_path / "policy.yaml" @@ -434,11 +533,191 @@ def __init__(self, **_kwargs: Any) -> None: SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), "job-123", ) + events: list[dict[str, object]] = [] + provider.set_event_emitter(events.append) with pytest.raises(ValueError, match="bad adapter"): provider._create_session() assert context.exit_calls == 1 assert provider._os_context is None + assert not any(event["data"]["status"] == "succeeded" for event in events) # type: ignore[index] + + +def test_attestation_success_is_emitted_after_adapter_construction(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + context = _FakeCreatedContext() + order: list[str] = [] + + class RecordingAdapter(_FakeAdapter): + def __init__(self, **kwargs: Any) -> None: + super().__init__(**kwargs) + order.append("adapter") + + with ( + _fake_optional_modules(lambda **_kwargs: context, RecordingAdapter), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + provider.set_event_emitter(lambda event: order.append(event["data"]["status"])) # type: ignore[index] + provider._create_session() + + assert order == ["adapter", "succeeded"] + + +def test_terminate_before_context_entry_prevents_creation(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + context = _FakeCreatedContext() + + class UnexpectedAdapter: + def __init__(self, **_kwargs: Any) -> None: + raise AssertionError("adapter must not be constructed after termination") + + with ( + _fake_optional_modules(lambda **_kwargs: context, UnexpectedAdapter), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + provider.terminate() + + with pytest.raises(SandboxTerminatedError): + provider._create_session() + + assert context.enter_calls == 0 + assert context.exit_calls == 0 + + +def test_terminate_during_context_entry_defers_one_exit(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + entry_started = Event() + allow_entry = Event() + + class BlockingContext(_FakeCreatedContext): + def __enter__(self): + self.enter_calls += 1 + entry_started.set() + if not allow_entry.wait(timeout=2): + raise AssertionError("context entry was not released") + return self + + class UnexpectedAdapter: + def __init__(self, **_kwargs: Any) -> None: + raise AssertionError("adapter must not be constructed after termination") + + context = BlockingContext() + errors: list[BaseException] = [] + + with ( + _fake_optional_modules(lambda **_kwargs: context, UnexpectedAdapter), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + + def create() -> None: + try: + provider._session_or_create() + except BaseException as exc: # noqa: BLE001 - capture the worker outcome for assertion + errors.append(exc) + + worker = Thread(target=create) + worker.start() + assert entry_started.wait(timeout=2) + + provider.terminate() + assert context.exit_calls == 0 + allow_entry.set() + worker.join(timeout=2) + + assert not worker.is_alive() + assert len(errors) == 1 and isinstance(errors[0], SandboxTerminatedError) + assert context.exit_calls == 1 + assert provider._session is None + + +def test_terminate_during_adapter_construction_cannot_publish_session(tmp_path: Path) -> None: + policy_path = tmp_path / "policy.yaml" + _write_policy(policy_path) + adapter_started = Event() + allow_adapter = Event() + context = _FakeCreatedContext() + errors: list[BaseException] = [] + events: list[dict[str, object]] = [] + + class BlockingAdapter(_FakeAdapter): + def __init__(self, **kwargs: Any) -> None: + adapter_started.set() + if not allow_adapter.wait(timeout=2): + raise AssertionError("adapter construction was not released") + super().__init__(**kwargs) + + with ( + _fake_optional_modules(lambda **_kwargs: context, BlockingAdapter), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._parse_policy_proto", + return_value="policy-proto", + ), + patch( + "aiq_agent.agents.deep_researcher.sandbox.providers.openshell._build_sandbox_spec", + return_value="job-spec", + ), + ): + provider = OpenShellSandboxProvider( + SandboxConfig(provider="openshell", providers={"openshell": {"policy": str(policy_path)}}), + "job-123", + ) + provider.set_event_emitter(events.append) + + def create() -> None: + try: + provider._session_or_create() + except BaseException as exc: # noqa: BLE001 - capture the worker outcome for assertion + errors.append(exc) + + worker = Thread(target=create) + worker.start() + assert adapter_started.wait(timeout=2) + + provider.terminate() + allow_adapter.set() + worker.join(timeout=2) + + assert not worker.is_alive() + assert len(errors) == 1 and isinstance(errors[0], SandboxTerminatedError) + assert context.exit_calls == 1 + assert provider._session is None + assert not any(event["data"]["status"] == "succeeded" for event in events) # type: ignore[index] def test_upload_passes_path_via_argv_and_data_via_stdin_no_env() -> None: @@ -576,3 +855,33 @@ def __exit__(self, *_args: object) -> None: assert provider.cleanup_succeeded is False assert provider._os_context is None + + +def test_retry_cleanup_failure_remains_terminal_failure() -> None: + from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepAgentsRuntime + from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSandboxConfig + + provider = _provider() + stale = MagicMock() + stale.close.side_effect = RuntimeError("stale sandbox close failed") + replacement = MagicMock() + provider._session = stale + provider._create_session = MagicMock(return_value=replacement) # type: ignore[method-assign] + provider._prepare_workspace = MagicMock() # type: ignore[method-assign] + events: list[dict[str, object]] = [] + + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime( + sandbox=DeepResearchSandboxConfig(), + artifact_emit=events.append, + ) + + provider._reset_session() + + assert runtime.finalize(interrupted=False) is False + replacement.close.assert_called_once_with() + assert provider.cleanup_succeeded is False + assert [event["data"]["status"] for event in events] == ["started", "failed"] # type: ignore[index] diff --git a/tests/aiq_agent/agents/deep_researcher/test_agent.py b/tests/aiq_agent/agents/deep_researcher/test_agent.py index 754c3d8e7..a0f3f2efb 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_agent.py +++ b/tests/aiq_agent/agents/deep_researcher/test_agent.py @@ -289,6 +289,52 @@ def test_register_resolves_named_runtime_config_refs(self): assert resolved_skills is skills assert resolved_sandbox is sandbox + @pytest.mark.parametrize("owns_active_agent", [False, True]) + @pytest.mark.asyncio + async def test_registered_run_cancellation_finalizes_only_request_owned_agent(self, owns_active_agent): + """Cancellation is re-raised and only a request-scoped agent owns terminal cleanup.""" + from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSandboxConfig + from aiq_agent.agents.deep_researcher.register import DeepResearchAgentConfig + from aiq_agent.agents.deep_researcher.register import deep_research_agent + + builder = MagicMock() + builder.get_tools = AsyncMock(return_value=[web_search_tool]) + builder.get_llm = AsyncMock(return_value=MagicMock()) + template_agent = MagicMock() + request_agent = MagicMock() + active_agent = request_agent if owns_active_agent else template_agent + active_agent.run = AsyncMock(side_effect=asyncio.CancelledError()) + agents = [template_agent, request_agent] if owns_active_agent else [template_agent] + config = DeepResearchAgentConfig( + orchestrator_llm="llm", + tools=["web_search_tool"], + verbose=False, + sandbox=DeepResearchSandboxConfig() if owns_active_agent else None, + ) + state = DeepResearchAgentState(messages=[HumanMessage(content="cancel this request")]) + + with ( + patch( + "aiq_agent.agents.deep_researcher.register.DeepResearcherAgent", + side_effect=agents, + ), + patch("aiq_agent.common.validate_tool_availability", return_value=(True, 1, [])), + ): + registration = deep_research_agent.__wrapped__(config, builder) + function_info = await anext(registration) + assert function_info.single_fn is not None + try: + with pytest.raises(asyncio.CancelledError): + await function_info.single_fn(state) + finally: + await registration.aclose() + + template_agent.finalize.assert_not_called() + if owns_active_agent: + request_agent.finalize.assert_called_once_with(interrupted=True) + else: + request_agent.finalize.assert_not_called() + def test_modal_sandbox_name_is_job_id(self): """Modal sandbox names use the resolved job ID directly.""" from aiq_agent.agents.deep_researcher.sandbox.providers.modal import _validate_modal_sandbox_name diff --git a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py index 308ba3509..4ccc440aa 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py +++ b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py @@ -17,6 +17,9 @@ from __future__ import annotations +import logging +from threading import Event +from threading import Thread from typing import Any from unittest.mock import MagicMock from unittest.mock import patch @@ -331,6 +334,22 @@ def test_public_allowlist_requires_hosts(self) -> None: with pytest.raises(ValueError, match="network_allow"): DeepResearchSandboxConfig(network="allowlist") + @pytest.mark.parametrize("shared_field", ["existing_sandbox_name", "sandbox_name"]) + def test_public_shared_attachment_requires_debug_opt_in(self, shared_field: str) -> None: + with pytest.raises(ValueError, match="allow_shared_sandbox=true"): + DeepResearchSandboxConfig(**{shared_field: "shared"}) + + sandbox = DeepResearchSandboxConfig(**{shared_field: "shared", "allow_shared_sandbox": True}) + assert getattr(sandbox, shared_field) == "shared" + + def test_public_shared_attachment_rejects_conflicting_aliases(self) -> None: + with pytest.raises(ValueError, match="must match"): + DeepResearchSandboxConfig( + existing_sandbox_name="shared-a", + sandbox_name="shared-b", + allow_shared_sandbox=True, + ) + class TestDeepAgentsRuntimeCleanup: """Terminal cleanup is idempotent and reports the provider's actual outcome.""" @@ -375,3 +394,70 @@ def test_finalize_emits_failed_when_provider_observed_cleanup_error(self) -> Non assert runtime.finalize(interrupted=True) is False provider.terminate.assert_called_once_with() assert [event["data"]["status"] for event in events] == ["started", "failed"] # type: ignore[index] + + def test_finalize_logs_only_cleanup_exception_type(self, caplog: pytest.LogCaptureFixture) -> None: + provider = MagicMock() + provider.provider_name = "openshell" + provider.physical_sandbox_name = "sandbox-1" + provider.close.side_effect = RuntimeError("credential=do-not-log") + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime(sandbox=DeepResearchSandboxConfig()) + + with caplog.at_level(logging.WARNING): + assert runtime.finalize(interrupted=False) is False + + assert "RuntimeError" in caplog.text + assert "credential=do-not-log" not in caplog.text + + def test_concurrent_finalize_waits_for_and_reuses_exact_result(self) -> None: + provider = MagicMock() + provider.provider_name = "openshell" + provider.physical_sandbox_name = "sandbox-1" + provider.cleanup_succeeded = True + cleanup_started = Event() + allow_cleanup = Event() + second_caller_started = Event() + events: list[dict[str, object]] = [] + results: list[bool] = [] + + def close() -> None: + cleanup_started.set() + if not allow_cleanup.wait(timeout=2): + raise AssertionError("cleanup was not released") + provider.cleanup_succeeded = False + + provider.close.side_effect = close + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime( + sandbox=DeepResearchSandboxConfig(), + artifact_emit=events.append, + ) + + first = Thread(target=lambda: results.append(runtime.finalize(interrupted=False))) + + def finalize_again() -> None: + second_caller_started.set() + results.append(runtime.finalize(interrupted=True)) + + second = Thread(target=finalize_again) + first.start() + assert cleanup_started.wait(timeout=2) + second.start() + assert second_caller_started.wait(timeout=2) + assert results == [] + + allow_cleanup.set() + first.join(timeout=2) + second.join(timeout=2) + + assert not first.is_alive() and not second.is_alive() + assert results == [False, False] + provider.close.assert_called_once_with() + provider.terminate.assert_not_called() + assert [event["data"]["status"] for event in events] == ["started", "failed"] # type: ignore[index] diff --git a/tests/aiq_agent/jobs/test_runner.py b/tests/aiq_agent/jobs/test_runner.py index dd26b2bac..79449c4c9 100644 --- a/tests/aiq_agent/jobs/test_runner.py +++ b/tests/aiq_agent/jobs/test_runner.py @@ -1908,6 +1908,20 @@ def test_none_runtime_is_noop(self): # Must not raise when no sandbox runtime is present (non-sandbox agents). _teardown_sandbox(None, job_id="job-1", interrupted=False) + @pytest.mark.asyncio + async def test_terminal_event_flush_failure_is_nonfatal_and_sanitized(self, caplog): + from aiq_api.jobs.runner import _flush_event_store + + event_store = MagicMock() + event_store.flush.side_effect = RuntimeError("secret-bearing database detail") + + with caplog.at_level("WARNING", logger="aiq_api.jobs.runner"): + await _flush_event_store(event_store, job_id="job-1") + + event_store.flush.assert_called_once_with() + assert "Event store flush failed for job job-1 (RuntimeError)" in caplog.text + assert "secret-bearing database detail" not in caplog.text + def test_runtime_finalizer_owns_cleanup_when_available(self): from aiq_api.jobs.runner import _teardown_sandbox @@ -1918,6 +1932,17 @@ def test_runtime_finalizer_owns_cleanup_when_available(self): runtime.close.assert_not_called() runtime.terminate.assert_not_called() + def test_runtime_finalizer_false_result_is_logged(self, caplog): + from aiq_api.jobs.runner import _teardown_sandbox + + runtime = MagicMock(spec=["finalize"]) + runtime.finalize.return_value = False + + with caplog.at_level("WARNING", logger="aiq_api.jobs.runner"): + _teardown_sandbox(runtime, job_id="job-1", interrupted=False) + + assert "Sandbox cleanup reported failure for job job-1" in caplog.text + def test_normal_path_calls_close(self): from aiq_api.jobs.runner import _teardown_sandbox From 55f54ce1153f3a4caed5e1f7a008f25c68c23619 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Wed, 1 Jul 2026 21:46:33 -0700 Subject: [PATCH 4/8] fix: drop nonexistent /app from generated OpenShell read_only policy The generated policy listed /app under filesystem_policy.read_only, but the aiq-openshell-demo image has no /app directory. Under the default landlock.compatibility: hard_requirement this fails closed at sandbox prepare ("Landlock path unavailable in hard_requirement mode: /app"), so the container reaches Ready then immediately errors. Removing the path fixes prepare on all hosts; production keeps hard_requirement, best_effort stays opt-in for local demos. Signed-off-by: Kyle Zheng --- scripts/setup_openshell.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts/setup_openshell.sh b/scripts/setup_openshell.sh index 7bf3ebd7c..0e13b0dea 100755 --- a/scripts/setup_openshell.sh +++ b/scripts/setup_openshell.sh @@ -909,7 +909,6 @@ filesystem_policy: - /usr - /lib - /etc - - /app - /var/log - /proc/self - /dev/urandom From 5b373720d0eaf41dd7d72dcb01c0b8ce9b409893 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Thu, 2 Jul 2026 11:43:48 -0700 Subject: [PATCH 5/8] drop /app from the policy and and also fix for non-sandbox jobs Signed-off-by: Kyle Zheng --- configs/openshell/aiq-research-policy.yaml | 1 - .../agents/deep_researcher/deepagents_runtime.py | 7 +++++++ .../agents/deep_researcher/test_deepagents_runtime.py | 8 ++++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/configs/openshell/aiq-research-policy.yaml b/configs/openshell/aiq-research-policy.yaml index ef3017ab4..332b33c28 100644 --- a/configs/openshell/aiq-research-policy.yaml +++ b/configs/openshell/aiq-research-policy.yaml @@ -11,7 +11,6 @@ filesystem_policy: - /usr - /lib - /etc - - /app - /var/log - /proc/self - /dev/urandom diff --git a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py index d4ffa30f8..c77a97c7a 100644 --- a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py +++ b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py @@ -291,6 +291,13 @@ def finalize(self, *, interrupted: bool) -> bool: assert self._finalize_result is not None return self._finalize_result + # No provider means there is no sandbox lifecycle to report; skip the + # cleanup events so non-sandbox jobs never emit an empty sandbox.cleanup. + if self._sandbox_provider is None: + self._finalize_result = True + self._finalized = True + return True + self._emit_cleanup("started", interrupted=interrupted) try: if interrupted: diff --git a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py index 4ccc440aa..01a074705 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py +++ b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py @@ -375,6 +375,14 @@ def test_finalize_closes_once_and_emits_success(self) -> None: assert provider.terminate.call_count == 0 assert [event["data"]["status"] for event in events] == ["started", "succeeded"] # type: ignore[index] + def test_finalize_without_provider_emits_no_cleanup_events(self) -> None: + events: list[dict[str, object]] = [] + runtime = DeepAgentsRuntime(sandbox=None, artifact_emit=events.append) + + assert runtime.finalize(interrupted=False) is True + assert runtime.finalize(interrupted=True) is True + assert events == [] + def test_finalize_emits_failed_when_provider_observed_cleanup_error(self) -> None: provider = MagicMock() provider.provider_name = "openshell" From 9cec3b422688b286966e2a2b95358b2afd4c6c41 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Thu, 2 Jul 2026 11:54:19 -0700 Subject: [PATCH 6/8] fix: sanitize sandbox teardown failures Signed-off-by: Kyle Zheng --- frontends/aiq_api/src/aiq_api/jobs/runner.py | 8 +++---- tests/aiq_agent/jobs/test_runner.py | 24 ++++++++++++++++---- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/frontends/aiq_api/src/aiq_api/jobs/runner.py b/frontends/aiq_api/src/aiq_api/jobs/runner.py index 7c63e44ff..43f629c3c 100644 --- a/frontends/aiq_api/src/aiq_api/jobs/runner.py +++ b/frontends/aiq_api/src/aiq_api/jobs/runner.py @@ -677,8 +677,8 @@ def _teardown_sandbox(sandbox_runtime: Any | None, *, job_id: str, interrupted: try: if not finalize(interrupted=interrupted): logger.warning("Sandbox cleanup reported failure for job %s", job_id) - except Exception: # noqa: BLE001 - cleanup must never replace the job result - logger.warning("Sandbox cleanup failed for job %s", job_id, exc_info=True) + except Exception as exc: # noqa: BLE001 - cleanup must never replace the job result + logger.warning("Sandbox cleanup failed for job %s (%s)", job_id, type(exc).__name__) return teardown = getattr(sandbox_runtime, "terminate", None) if interrupted else None if teardown is None: @@ -687,8 +687,8 @@ def _teardown_sandbox(sandbox_runtime: Any | None, *, job_id: str, interrupted: return try: teardown() - except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path - logger.warning("Sandbox cleanup failed for job %s", job_id, exc_info=True) + except Exception as exc: # noqa: BLE001 - cleanup must never raise on the terminal path + logger.warning("Sandbox cleanup failed for job %s (%s)", job_id, type(exc).__name__) def _create_agent_instance( diff --git a/tests/aiq_agent/jobs/test_runner.py b/tests/aiq_agent/jobs/test_runner.py index 79449c4c9..c40fb5ec5 100644 --- a/tests/aiq_agent/jobs/test_runner.py +++ b/tests/aiq_agent/jobs/test_runner.py @@ -1943,6 +1943,18 @@ def test_runtime_finalizer_false_result_is_logged(self, caplog): assert "Sandbox cleanup reported failure for job job-1" in caplog.text + def test_runtime_finalizer_exception_is_nonfatal_and_sanitized(self, caplog): + from aiq_api.jobs.runner import _teardown_sandbox + + runtime = MagicMock(spec=["finalize"]) + runtime.finalize.side_effect = RuntimeError("credential=do-not-log") + + with caplog.at_level("WARNING", logger="aiq_api.jobs.runner"): + _teardown_sandbox(runtime, job_id="job-1", interrupted=False) + + assert "Sandbox cleanup failed for job job-1 (RuntimeError)" in caplog.text + assert "credential=do-not-log" not in caplog.text + def test_normal_path_calls_close(self): from aiq_api.jobs.runner import _teardown_sandbox @@ -1969,13 +1981,17 @@ def test_interrupted_without_terminate_falls_back_to_close(self): runtime.close.assert_called_once_with() - def test_never_raises_when_teardown_fails(self): + def test_fallback_teardown_exception_is_nonfatal_and_sanitized(self, caplog): from aiq_api.jobs.runner import _teardown_sandbox runtime = MagicMock(spec=["close", "terminate"]) - runtime.close.side_effect = RuntimeError("sdk session close failed") - # Must swallow the error; teardown is best-effort on the terminal path. - _teardown_sandbox(runtime, job_id="job-1", interrupted=False) + runtime.close.side_effect = RuntimeError("credential=do-not-log") + + with caplog.at_level("WARNING", logger="aiq_api.jobs.runner"): + _teardown_sandbox(runtime, job_id="job-1", interrupted=False) + + assert "Sandbox cleanup failed for job job-1 (RuntimeError)" in caplog.text + assert "credential=do-not-log" not in caplog.text def test_does_not_harvest(self): from aiq_api.jobs.runner import _teardown_sandbox From 1e3118fb47ab8ac937a286fd8e193f625e0443c5 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Wed, 1 Jul 2026 15:38:00 -0700 Subject: [PATCH 7/8] feat: finalize durable sandbox artifacts Signed-off-by: Kyle Zheng --- docs/source/architecture/agents/sandbox.md | 5 +- frontends/aiq_api/README.md | 4 +- frontends/aiq_api/src/aiq_api/jobs/runner.py | 36 +++++---- frontends/aiq_api/src/aiq_api/routes/jobs.py | 13 ++-- .../adapters/api/deep-research-client.spec.ts | 70 ++++++++++++++++- .../src/adapters/api/deep-research-client.ts | 54 +++++++++++-- frontends/ui/src/adapters/api/index.ts | 1 + .../chat/hooks/use-deep-research.spec.ts | 8 +- .../features/chat/hooks/use-deep-research.ts | 13 ++-- .../features/chat/hooks/use-load-job-data.ts | 12 +-- frontends/ui/src/features/chat/store.ts | 4 +- frontends/ui/src/features/chat/types.ts | 12 ++- .../layout/components/FileCard.spec.tsx | 24 ++++++ .../features/layout/components/FileCard.tsx | 57 +++++++++++--- src/aiq_agent/agents/deep_researcher/agent.py | 1 + .../deep_researcher/custom_middleware.py | 22 ++++++ .../deep_researcher/deepagents_runtime.py | 31 ++++++-- .../agents/deep_researcher/factory.py | 9 ++- .../agents/deep_researcher/register.py | 7 +- .../agents/deep_researcher/sandbox/README.md | 10 ++- .../sandbox/artifacts/manager.py | 19 ++++- .../sandbox/artifacts/models.py | 32 ++++---- .../agents/deep_researcher/sandbox/base.py | 11 +++ .../deep_researcher/sandbox/test_artifacts.py | 25 ++++++- .../deep_researcher/test_custom_middleware.py | 70 +++++++++++++++++ .../test_deepagents_runtime.py | 75 +++++++++++++++++++ .../agents/deep_researcher/test_factory.py | 23 ++++++ .../fastapi_extensions/test_deep_research.py | 59 +++++++++++++++ tests/aiq_agent/jobs/test_runner.py | 31 ++++++++ 29 files changed, 647 insertions(+), 91 deletions(-) diff --git a/docs/source/architecture/agents/sandbox.md b/docs/source/architecture/agents/sandbox.md index 4d77a9c15..da9caaeeb 100644 --- a/docs/source/architecture/agents/sandbox.md +++ b/docs/source/architecture/agents/sandbox.md @@ -41,7 +41,10 @@ runtime (`.../job/{job_id}/artifacts`), which is also auth-scoped to the job. caps (`AIQ_MAX_SANDBOXES_PER_PRINCIPAL` / `AIQ_MAX_SANDBOXES_GLOBAL`, default-off) bound concurrency and cost. - Custom client-supplied job IDs must not be reused for a new job. -- The runtime closes provider sessions on success, failure, cancellation, and timeout. +- Manifest checkpoints preserve completed artifacts after successful sandbox commands. The + terminal finalizer harvests once before cleanup on success/failure; cancellation harvests + only when the provider is idle and otherwise terminates immediately. +- Job status becomes terminal only after artifact and cleanup events are flushed. - Per-job OpenShell mode requires `delete_on_exit: true`. A persistent shared sandbox is possible only through the explicit debug attachment settings. diff --git a/frontends/aiq_api/README.md b/frontends/aiq_api/README.md index abc157b41..e7ed0946b 100644 --- a/frontends/aiq_api/README.md +++ b/frontends/aiq_api/README.md @@ -72,7 +72,7 @@ Base path: `/v1/jobs/async` | `/v1/jobs/async/job/{id}` | GET | Get job status | | `/v1/jobs/async/job/{id}/stream` | GET | SSE stream from beginning | | `/v1/jobs/async/job/{id}/stream/{last_event_id}` | GET | SSE stream from event ID | -| `/v1/jobs/async/job/{id}/cancel` | POST | Cancel running job | +| `/v1/jobs/async/job/{id}/cancel` | POST | Request cancellation; worker publishes `INTERRUPTED` after cleanup | | `/v1/jobs/async/job/{id}/state` | GET | Get current UI state | | `/v1/jobs/async/job/{id}/report` | GET | Get final report | @@ -149,7 +149,7 @@ Events streamed during job execution: | `workflow.start` / `workflow.end` | Workflow lifecycle | | `llm.start` / `llm.chunk` / `llm.end` | LLM inference progress | | `tool.start` / `tool.end` | Tool invocations | -| `artifact.update` | Todos, files, citations, output updates | +| `artifact.update` | Todos, citations, output, and generated-file metadata (`content_url` for durable bytes) | | `job.error` | Error occurred | ## Configuration diff --git a/frontends/aiq_api/src/aiq_api/jobs/runner.py b/frontends/aiq_api/src/aiq_api/jobs/runner.py index 43f629c3c..ad1d10676 100644 --- a/frontends/aiq_api/src/aiq_api/jobs/runner.py +++ b/frontends/aiq_api/src/aiq_api/jobs/runner.py @@ -580,7 +580,14 @@ async def run_agent_job( # Signal event stream completion event_stream.on_complete() - # Flush any buffered events before updating status + # Finalize before SUCCESS so clients cannot stop streaming before + # artifact and cleanup events are durable. + await asyncio.to_thread( + _teardown_sandbox, + sandbox_runtime, + job_id=job_id, + interrupted=False, + ) if hasattr(event_store, "flush"): event_store.flush() @@ -596,17 +603,10 @@ async def run_agent_job( except asyncio.CancelledError: logger.info("Job %s cancelled", job_id) interrupted = True - if job_store: - try: - job = await job_store.get_job(job_id) - if job and job.status != JobStatus.INTERRUPTED.value: - await job_store.update_status(job_id, JobStatus.INTERRUPTED, error="cancelled by user") - except (ConnectionError, TimeoutError, RuntimeError): - pass - if event_store is None: event_store = BatchingEventStore(EventStore(db_url, job_id)) + await asyncio.to_thread(_teardown_sandbox, sandbox_runtime, job_id=job_id, interrupted=True) event_store.store( { "type": "job.cancelled", @@ -616,14 +616,20 @@ async def run_agent_job( if hasattr(event_store, "flush"): event_store.flush() - except Exception as e: - logger.exception("Job %s failed: %s", job_id, type(e).__name__) if job_store: - await job_store.update_status(job_id, JobStatus.FAILURE, error=str(e)) + try: + job = await job_store.get_job(job_id) + if job and job.status != JobStatus.INTERRUPTED.value: + await job_store.update_status(job_id, JobStatus.INTERRUPTED, error="cancelled by user") + except (ConnectionError, TimeoutError, RuntimeError): + pass + except Exception as e: + logger.exception("Job %s failed: %s", job_id, type(e).__name__) if event_store is None: event_store = BatchingEventStore(EventStore(db_url, job_id)) + await asyncio.to_thread(_teardown_sandbox, sandbox_runtime, job_id=job_id, interrupted=False) event_store.store( { "type": "job.error", @@ -635,15 +641,15 @@ async def run_agent_job( ) if hasattr(event_store, "flush"): event_store.flush() + if job_store: + await job_store.update_status(job_id, JobStatus.FAILURE, error=str(e)) finally: # Ensure terminal-path events are not left in the batch buffer. await _flush_event_store(event_store, job_id=job_id) if cancellation_monitor: cancellation_monitor.stop() - # Release the sandbox off the event loop so the SDK session close never blocks the Dask - # worker. The single artifact harvest already ran in agent.run() before this point, so - # teardown only closes/terminates; interrupted jobs terminate() to preempt a live execute. + # Idempotent fallback for failures before a terminal branch finalized the runtime. await asyncio.to_thread(_teardown_sandbox, sandbox_runtime, job_id=job_id, interrupted=interrupted) await _flush_event_store(event_store, job_id=job_id) # Clean up job-scoped auth token diff --git a/frontends/aiq_api/src/aiq_api/routes/jobs.py b/frontends/aiq_api/src/aiq_api/routes/jobs.py index f7d46e584..300891217 100644 --- a/frontends/aiq_api/src/aiq_api/routes/jobs.py +++ b/frontends/aiq_api/src/aiq_api/routes/jobs.py @@ -758,7 +758,7 @@ async def stream_job_events_from(job_id: str, last_event_id: int) -> StreamingRe "/v1/jobs/async/job/{job_id}/cancel", tags=["async jobs"], summary="Cancel a running job", - description="Request cancellation of a running job. The job status will be set to INTERRUPTED.", + description="Request cancellation. The worker publishes INTERRUPTED after terminal cleanup completes.", responses={ 400: {"description": "Job is not in RUNNING state"}, 404: {"description": "Job not found"}, @@ -772,8 +772,6 @@ async def cancel_job(job_id: str) -> dict: if job.status != JobStatus.RUNNING.value: raise HTTPException(400, f"Job not running: {job_id} (status: {job.status})") - await job_store.update_status(job_id, JobStatus.INTERRUPTED, error="cancelled by user") - event_store = EventStore(db_url, job_id) event_store.store( { @@ -784,9 +782,14 @@ async def cancel_job(job_id: str) -> dict: task_cancelled = await _cancel_dask_task(scheduler_address, job_id) - logger.info("Cancel requested for job %s: status updated, task_cancelled=%s", job_id, task_cancelled) + logger.info("Cancel requested for job %s: task_cancelled=%s", job_id, task_cancelled) - return {"job_id": job_id, "status": JobStatus.INTERRUPTED.value, "task_cancelled": task_cancelled} + return { + "job_id": job_id, + "status": job.status, + "cancellation_requested": True, + "task_cancelled": task_cancelled, + } @app.get( "/v1/jobs/async/job/{job_id}/state", diff --git a/frontends/ui/src/adapters/api/deep-research-client.spec.ts b/frontends/ui/src/adapters/api/deep-research-client.spec.ts index a57b0f035..9e3f4e131 100644 --- a/frontends/ui/src/adapters/api/deep-research-client.spec.ts +++ b/frontends/ui/src/adapters/api/deep-research-client.spec.ts @@ -2,7 +2,29 @@ // SPDX-License-Identifier: Apache-2.0 import { afterEach, describe, expect, test, vi } from 'vitest' -import { getJobStatus } from './deep-research-client' +import { createDeepResearchClient, getJobStatus } from './deep-research-client' + +class FakeEventSource { + static latest: FakeEventSource | null = null + onopen: (() => void) | null = null + onmessage: ((event: MessageEvent) => void) | null = null + onerror: (() => void) | null = null + private listeners = new Map void>() + + constructor(_url: string) { + FakeEventSource.latest = this + } + + addEventListener(type: string, listener: EventListener): void { + this.listeners.set(type, listener as (event: MessageEvent) => void) + } + + close(): void {} + + emit(type: string, data: unknown): void { + this.listeners.get(type)?.(new MessageEvent(type, { data: JSON.stringify(data) })) + } +} describe('deep research REST client', () => { afterEach(() => { @@ -32,4 +54,50 @@ describe('deep research REST client', () => { 'Failed to get job status: 500 - PROXY_ERROR: fetch failed' ) }) + + test('maps generated binary file events to durable artifact metadata', () => { + vi.stubGlobal('EventSource', FakeEventSource) + const onFileUpdate = vi.fn() + const client = createDeepResearchClient({ jobId: 'job-1', callbacks: { onFileUpdate } }) + client.connect() + + FakeEventSource.latest?.emit('artifact.update', { + data: { + type: 'file', + file_path: 'chart.png', + artifact_id: 'art_123', + content_url: '/v1/jobs/async/job/job-1/artifacts/art_123/content', + mime_type: 'image/png', + size_bytes: 2048, + sha256: 'a'.repeat(64), + inline: true, + }, + }) + + expect(onFileUpdate).toHaveBeenCalledWith({ + filename: 'chart.png', + content: undefined, + artifactId: 'art_123', + contentUrl: '/api/jobs/async/job/job-1/artifacts/art_123/content', + mimeType: 'image/png', + sizeBytes: 2048, + sha256: 'a'.repeat(64), + inline: true, + }) + }) + + test('preserves legacy text file events', () => { + vi.stubGlobal('EventSource', FakeEventSource) + const onFileUpdate = vi.fn() + const client = createDeepResearchClient({ jobId: 'job-1', callbacks: { onFileUpdate } }) + client.connect() + + FakeEventSource.latest?.emit('artifact.update', { + data: { type: 'file', file_path: 'report.md', content: '# Report' }, + }) + + expect(onFileUpdate).toHaveBeenCalledWith( + expect.objectContaining({ filename: 'report.md', content: '# Report' }) + ) + }) }) diff --git a/frontends/ui/src/adapters/api/deep-research-client.ts b/frontends/ui/src/adapters/api/deep-research-client.ts index 5377e3d7c..65b9724dd 100644 --- a/frontends/ui/src/adapters/api/deep-research-client.ts +++ b/frontends/ui/src/adapters/api/deep-research-client.ts @@ -12,6 +12,7 @@ */ import { apiConfig } from './config' +import { artifactContentPath } from '@/shared/components/MarkdownRenderer/artifact-url' // ============================================================ // Types @@ -149,6 +150,18 @@ export interface TodoItem { status: 'pending' | 'in_progress' | 'completed' | 'cancelled' } +/** File update emitted by artifact.update. Durable generated files carry metadata, not bytes. */ +export interface FileArtifactUpdate { + filename: string + content?: string + artifactId?: string + contentUrl?: string + mimeType?: string + sizeBytes?: number + sha256?: string + inline?: boolean +} + /** artifact.update event */ export interface ArtifactUpdateEvent extends DeepResearchSSEEvent { event: 'artifact.update' @@ -157,8 +170,9 @@ export interface ArtifactUpdateEvent extends DeepResearchSSEEvent { timestamp: string data: { type: ArtifactType - content: string | TodoItem[] + content?: string | TodoItem[] url?: string // For citation_source and citation_use types + content_url?: string } metadata?: { workflow?: string @@ -208,7 +222,7 @@ export interface DeepResearchCallbacks { /** Called on artifact updates */ onTodoUpdate?: (todos: TodoItem[], workflow?: string) => void onCitationUpdate?: (url: string, content: string, isCited?: boolean) => void - onFileUpdate?: (filename: string, content: string) => void + onFileUpdate?: (file: FileArtifactUpdate) => void onOutputUpdate?: (content: string, outputCategory?: string, workflow?: string) => void /** Called on job heartbeat (confirms job is alive during long operations) */ onHeartbeat?: (uptimeSeconds: number) => void @@ -485,10 +499,17 @@ export const createDeepResearchClient = (options: DeepResearchStreamOptions): De case 'artifact.update': { // artifact.update has nested structure: { id, timestamp, data: { type, content, url?, output_category? }, metadata?: { workflow } } const artifactWrapper = rawData as { - data?: { type: ArtifactType; content: string | TodoItem[]; url?: string; output_category?: string } + data?: { + type: ArtifactType + content?: string | TodoItem[] + url?: string + content_url?: string + output_category?: string + } type?: ArtifactType content?: string | TodoItem[] url?: string + content_url?: string output_category?: string metadata?: { workflow?: string } } @@ -509,11 +530,27 @@ export const createDeepResearchClient = (options: DeepResearchStreamOptions): De callbacks.onCitationUpdate?.(artifactData.url || '', artifactData.content as string, true) break case 'file': { - // file artifacts are written during research — extract filename from path + // Generated artifacts carry durable metadata; legacy text-file events carry content. const raw = artifactData as Record const filePath = (raw.file_path || raw.path || artifactData.url || 'unknown') as string const fileName = filePath.split('/').pop() || filePath - callbacks.onFileUpdate?.(fileName, artifactData.content as string) + const artifactId = typeof raw.artifact_id === 'string' ? raw.artifact_id : undefined + callbacks.onFileUpdate?.({ + filename: fileName, + content: typeof artifactData.content === 'string' ? artifactData.content : undefined, + artifactId, + contentUrl: artifactId + ? artifactContentPath(jobId, artifactId) + : typeof raw.content_url === 'string' + ? raw.content_url + : typeof raw.url === 'string' + ? raw.url + : undefined, + mimeType: typeof raw.mime_type === 'string' ? raw.mime_type : undefined, + sizeBytes: typeof raw.size_bytes === 'number' ? raw.size_bytes : undefined, + sha256: typeof raw.sha256 === 'string' ? raw.sha256 : undefined, + inline: typeof raw.inline === 'boolean' ? raw.inline : undefined, + }) break } case 'output': @@ -747,7 +784,12 @@ export const getJobReport = async ( export const cancelJob = async ( jobId: string, authToken?: string -): Promise<{ cancelled: boolean }> => { +): Promise<{ + job_id: string + status: DeepResearchJobStatus + cancellation_requested: boolean + task_cancelled: boolean +}> => { const url = `${getDeepResearchBaseUrl()}/job/${jobId}/cancel` const headers: HeadersInit = { 'Content-Type': 'application/json', diff --git a/frontends/ui/src/adapters/api/index.ts b/frontends/ui/src/adapters/api/index.ts index 8a803c9d4..47de92cd0 100644 --- a/frontends/ui/src/adapters/api/index.ts +++ b/frontends/ui/src/adapters/api/index.ts @@ -110,6 +110,7 @@ export type { ToolStartEvent, ToolEndEvent, TodoItem, + FileArtifactUpdate, ArtifactUpdateEvent, DeepResearchEvent, DeepResearchCallbacks, diff --git a/frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts b/frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts index 5fbce086e..b7fa0f040 100644 --- a/frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts +++ b/frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts @@ -937,7 +937,7 @@ describe('useDeepResearch', () => { await setupConnectedHook() act(() => { - mockClient?.callbacks.onFileUpdate?.('report.md', '# Report content') + mockClient?.callbacks.onFileUpdate?.({ filename: 'report.md', content: '# Report content' }) }) expect(mockAddDeepResearchFile).toHaveBeenCalledWith({ @@ -950,7 +950,7 @@ describe('useDeepResearch', () => { await setupConnectedHook() act(() => { - mockClient?.callbacks.onFileUpdate?.('report.md', '# Final report') + mockClient?.callbacks.onFileUpdate?.({ filename: 'report.md', content: '# Final report' }) }) expect(mockSetCurrentStatus).toHaveBeenCalledWith('writing') @@ -960,7 +960,7 @@ describe('useDeepResearch', () => { await setupConnectedHook() act(() => { - mockClient?.callbacks.onFileUpdate?.('artifacts/report.md', '# Final report') + mockClient?.callbacks.onFileUpdate?.({ filename: 'artifacts/report.md', content: '# Final report' }) }) expect(mockSetCurrentStatus).toHaveBeenCalledWith('writing') @@ -970,7 +970,7 @@ describe('useDeepResearch', () => { await setupConnectedHook() act(() => { - mockClient?.callbacks.onFileUpdate?.('notes.md', '# Some notes') + mockClient?.callbacks.onFileUpdate?.({ filename: 'notes.md', content: '# Some notes' }) }) expect(mockAddDeepResearchFile).toHaveBeenCalledWith({ diff --git a/frontends/ui/src/features/chat/hooks/use-deep-research.ts b/frontends/ui/src/features/chat/hooks/use-deep-research.ts index 4cbcae37c..93b6f5c37 100644 --- a/frontends/ui/src/features/chat/hooks/use-deep-research.ts +++ b/frontends/ui/src/features/chat/hooks/use-deep-research.ts @@ -19,6 +19,7 @@ import { cancelJob, type DeepResearchClient, type DeepResearchJobStatus, + type FileArtifactUpdate, type TodoItem, } from '@/adapters/api' import { useChatStore } from '../store' @@ -212,7 +213,7 @@ export const useDeepResearch = (): UseDeepResearchReturn => { toolCalls: new Map; output?: string; workflow?: string; agentId?: string; isSandbox?: boolean }>(), todos: null as TodoItem[] | null, citations: [] as Array<{ url: string; content: string; isCited: boolean }>, - files: new Map(), + files: new Map(), reportContent: null as string | null, } @@ -238,7 +239,7 @@ export const useDeepResearch = (): UseDeepResearchReturn => { const llmSteps = Array.from(buf.llmSteps.entries()).map(([id, s]) => ({ id, name: s.name, workflow: s.workflow, content: s.content, thinking: s.thinking, usage: s.usage, isComplete: true, timestamp: now })) const toolCalls = Array.from(buf.toolCalls.entries()).map(([id, t]) => ({ id, name: t.name, input: t.input, output: t.output, workflow: t.workflow, agentId: t.agentId, isSandbox: t.isSandbox, status: 'complete' as const, timestamp: now })) const citations = buf.citations.map((c, i) => ({ id: `citation-${i}`, url: c.url, content: c.content, isCited: c.isCited, timestamp: now })) - const files = Array.from(buf.files.entries()).map(([filename, content], i) => ({ id: `file-${i}`, filename, content, timestamp: now })) + const files = Array.from(buf.files.values()).map((file, i) => ({ id: `file-${i}`, ...file, timestamp: now })) const todos = buf.todos ? normalizeDeepResearchTodos(buf.todos) : undefined useChatStore.setState((state) => ({ @@ -486,13 +487,13 @@ export const useDeepResearch = (): UseDeepResearchReturn => { resetTimeout(); addDeepResearchCitation(url, content, isCited) }, - onFileUpdate: (filename, content) => { - if (buf.active) { buf.files.set(filename, content); return } + onFileUpdate: (file) => { + if (buf.active) { buf.files.set(file.filename, file); return } if (!isActiveJob()) return - resetTimeout(); addDeepResearchFile({ filename, content }) + resetTimeout(); addDeepResearchFile(file) // report.md artifact arrives 1-2 min before the final_report output event — // use it as an early signal to switch the UI to "writing" status. - if (filename.endsWith('report.md')) { + if (file.filename.endsWith('report.md')) { setCurrentStatus('writing') } }, diff --git a/frontends/ui/src/features/chat/hooks/use-load-job-data.ts b/frontends/ui/src/features/chat/hooks/use-load-job-data.ts index b39b76543..b9a041d56 100644 --- a/frontends/ui/src/features/chat/hooks/use-load-job-data.ts +++ b/frontends/ui/src/features/chat/hooks/use-load-job-data.ts @@ -28,6 +28,7 @@ import { createDeepResearchClient, type DeepResearchClient, type DeepResearchJobStatus, + type FileArtifactUpdate, type TodoItem, } from '@/adapters/api' import { useChatStore } from '../store' @@ -371,7 +372,7 @@ export const useLoadJobData = (): UseLoadJobDataReturn => { >(), todos: null as TodoItem[] | null, citations: [] as Array<{ url: string; content: string; isCited: boolean }>, - files: new Map(), // filename -> latest content (deduped) + files: new Map(), // filename -> latest event (deduped) reportContent: null as string | null, } @@ -427,10 +428,9 @@ export const useLoadJobData = (): UseLoadJobDataReturn => { timestamp: now, })) - const files = Array.from(buffer.files.entries()).map(([filename, content], idx) => ({ + const files = Array.from(buffer.files.values()).map((file, idx) => ({ id: `file-${idx}`, - filename, - content, + ...file, timestamp: now, })) @@ -574,8 +574,8 @@ export const useLoadJobData = (): UseLoadJobDataReturn => { buffer.citations.push({ url, content, isCited: isCited ?? false }) }, - onFileUpdate: (filename, content) => { - buffer.files.set(filename, content) + onFileUpdate: (file) => { + buffer.files.set(file.filename, file) }, onOutputUpdate: (content, outputCategory) => { diff --git a/frontends/ui/src/features/chat/store.ts b/frontends/ui/src/features/chat/store.ts index 2e8ef16c4..f3afd71b3 100644 --- a/frontends/ui/src/features/chat/store.ts +++ b/frontends/ui/src/features/chat/store.ts @@ -2898,9 +2898,9 @@ export const useChatStore = create()( const existingIndex = deepResearchFiles.findIndex((f) => f.filename === file.filename) if (existingIndex >= 0) { - // Update existing file with latest content + // Update existing text or durable generated-artifact metadata. const updatedFiles = deepResearchFiles.map((f, i) => - i === existingIndex ? { ...f, content: file.content, timestamp: new Date() } : f + i === existingIndex ? { ...f, ...file, timestamp: new Date() } : f ) set({ deepResearchFiles: updatedFiles }, false, 'addDeepResearchFile:update') return deepResearchFiles[existingIndex].id diff --git a/frontends/ui/src/features/chat/types.ts b/frontends/ui/src/features/chat/types.ts index a62785299..499b44b01 100644 --- a/frontends/ui/src/features/chat/types.ts +++ b/frontends/ui/src/features/chat/types.ts @@ -360,8 +360,16 @@ export interface DeepResearchFile { id: string /** File name/path */ filename: string - /** File content */ - content: string + /** Inline text content for legacy text-file events */ + content?: string + /** Durable generated-artifact identifier */ + artifactId?: string + /** Authenticated endpoint for the generated bytes */ + contentUrl?: string + mimeType?: string + sizeBytes?: number + sha256?: string + inline?: boolean /** When file was created/updated */ timestamp: Date } diff --git a/frontends/ui/src/features/layout/components/FileCard.spec.tsx b/frontends/ui/src/features/layout/components/FileCard.spec.tsx index 6dc785f14..e49ac2b32 100644 --- a/frontends/ui/src/features/layout/components/FileCard.spec.tsx +++ b/frontends/ui/src/features/layout/components/FileCard.spec.tsx @@ -62,6 +62,30 @@ describe('FileCard', () => { const lineCount = screen.getByText(/lines/) expect(lineCount).toBeInTheDocument() }) + + test('renders durable artifact metadata and opens its authenticated content URL', async () => { + const user = userEvent.setup() + const open = vi.spyOn(window, 'open').mockImplementation(() => null) + render( + + ) + + expect(screen.getByText('image/png · 2.0 KiB')).toBeInTheDocument() + await user.click(screen.getByRole('button', { name: 'Open file' })) + expect(open).toHaveBeenCalledWith( + '/api/jobs/async/job/job-1/artifacts/art-1/content', + '_blank', + 'noopener,noreferrer' + ) + }) }) describe('expand/collapse behavior', () => { diff --git a/frontends/ui/src/features/layout/components/FileCard.tsx b/frontends/ui/src/features/layout/components/FileCard.tsx index 0d370173d..59e442fa1 100644 --- a/frontends/ui/src/features/layout/components/FileCard.tsx +++ b/frontends/ui/src/features/layout/components/FileCard.tsx @@ -24,8 +24,14 @@ export interface FileInfo { id: string /** File name/path */ filename: string - /** File content */ - content: string + /** Inline content for legacy text-file events */ + content?: string + artifactId?: string + contentUrl?: string + mimeType?: string + sizeBytes?: number + sha256?: string + inline?: boolean /** When file was created/updated */ timestamp?: Date | string } @@ -59,6 +65,12 @@ const isMarkdownFile = (filename: string): boolean => { return ['md', 'markdown'].includes(ext) } +const formatBytes = (size: number): string => { + if (size < 1024) return `${size} B` + if (size < 1024 * 1024) return `${(size / 1024).toFixed(1)} KiB` + return `${(size / (1024 * 1024)).toFixed(1)} MiB` +} + /** * Expandable card showing a file artifact's details. */ @@ -81,7 +93,7 @@ export const FileCard: FC = ({ file }) => { + {file.contentUrl && ( + + + + )} + {/* Collapsed preview */} {!isExpanded && contentPreview && ( diff --git a/src/aiq_agent/agents/deep_researcher/agent.py b/src/aiq_agent/agents/deep_researcher/agent.py index 6378fa759..6b33f524b 100644 --- a/src/aiq_agent/agents/deep_researcher/agent.py +++ b/src/aiq_agent/agents/deep_researcher/agent.py @@ -138,6 +138,7 @@ def __init__( self.middleware_set = build_deep_research_middleware_set( tool_set=self.tool_set, source_registry_middleware=self.source_registry_middleware, + artifact_manager=self.deepagents_runtime.artifact_manager, ) self.source_tool_names = self.tool_set.source_tool_names diff --git a/src/aiq_agent/agents/deep_researcher/custom_middleware.py b/src/aiq_agent/agents/deep_researcher/custom_middleware.py index c2379fce7..383c134d4 100644 --- a/src/aiq_agent/agents/deep_researcher/custom_middleware.py +++ b/src/aiq_agent/agents/deep_researcher/custom_middleware.py @@ -467,6 +467,28 @@ def get_source_list_text(self, mode: str = "compact") -> str | None: return self._render_source_list_text(self.get_source_entries(mode=mode)) +class ArtifactHarvestMiddleware(AgentMiddleware): + """Checkpoint durable artifacts after successful sandbox execute calls.""" + + def __init__(self, artifact_manager: object) -> None: + """Store the artifact manager used for best-effort checkpoints.""" + self.artifact_manager = artifact_manager + + async def awrap_tool_call(self, request, handler): + """Run the tool, then checkpoint manifest-declared artifacts after execute.""" + result = await handler(request) + tool_name = "" + if hasattr(request, "tool_call") and isinstance(request.tool_call, dict): + tool_name = request.tool_call.get("name", "") + result_status = result.get("status") if isinstance(result, dict) else getattr(result, "status", None) + if tool_name == "execute" and result_status != "error": + try: + await asyncio.to_thread(self.artifact_manager.harvest_after_execute) + except Exception as exc: # noqa: BLE001 - artifact capture must not fail the agent + logger.warning("Artifact checkpoint harvest failed (%s)", type(exc).__name__) + return result + + class PlanPersistenceMiddleware(AgentMiddleware): """Persists the planner's structured ResearchPlan to the shared filesystem. diff --git a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py index c77a97c7a..e2aabf968 100644 --- a/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py +++ b/src/aiq_agent/agents/deep_researcher/deepagents_runtime.py @@ -269,8 +269,8 @@ def final_harvest(self) -> None: return try: manager.final_harvest() - except Exception: # noqa: BLE001 - harvest is best-effort on the terminal path - logger.warning("Final artifact harvest failed for job %s", self._job_id, exc_info=True) + except Exception as exc: # noqa: BLE001 - harvest is best-effort on the terminal path + logger.warning("Final artifact harvest failed for job %s (%s)", self._job_id, type(exc).__name__) def close(self) -> None: """Release the sandbox provider on a normal terminal job path (idempotent).""" @@ -285,7 +285,7 @@ def terminate(self) -> None: provider.terminate() def finalize(self, *, interrupted: bool) -> bool: - """Release the sandbox once and emit a truthful, sanitized cleanup outcome.""" + """Harvest, release the sandbox once, and emit a truthful cleanup outcome.""" with self._finalize_lock: if self._finalized: assert self._finalize_result is not None @@ -301,8 +301,10 @@ def finalize(self, *, interrupted: bool) -> bool: self._emit_cleanup("started", interrupted=interrupted) try: if interrupted: + self._try_cancellation_harvest() self.terminate() else: + self.final_harvest() self.close() except Exception as exc: # noqa: BLE001 - terminal cleanup cannot replace the job result logger.warning("Sandbox cleanup failed for job %s (%s)", self._job_id, type(exc).__name__) @@ -315,6 +317,20 @@ def finalize(self, *, interrupted: bool) -> bool: self._finalized = True return succeeded + def _try_cancellation_harvest(self) -> bool: + """Harvest only when the sandbox is idle; never wait behind an active execute.""" + if self.artifact_manager is None or self._sandbox_provider is None: + return False + lease = getattr(self._sandbox_provider, "try_operation_lease", None) + if not callable(lease): + return False + with lease() as acquired: + if not acquired: + logger.info("Skipping terminal harvest for busy cancelled sandbox job %s", self._job_id) + return False + self.final_harvest() + return True + def _emit_cleanup(self, status: str, *, interrupted: bool) -> None: """Emit cleanup metadata without policy contents, environment, or error text.""" if self._event_emit is None: @@ -373,11 +389,16 @@ def _maybe_build_artifact_manager( Defaults to ``None`` (no harvesting) so adding the sandbox alone never requires a DB. """ - if provider is None or artifact_db_url is None: + if provider is None: return None capture = getattr(getattr(provider, "config", None), "artifact_capture", None) - if capture is None or not getattr(capture, "enabled", False): + if not isinstance(capture, ArtifactCaptureConfig) or not capture.enabled: return None + if artifact_db_url is None: + raise ValueError( + "sandbox artifact_capture.enabled requires a durable artifact_db_url; " + "disable capture or invoke the agent through a host that supplies an artifact store" + ) from .sandbox.artifacts import ArtifactManager from .sandbox.artifacts import SqlArtifactStore diff --git a/src/aiq_agent/agents/deep_researcher/factory.py b/src/aiq_agent/agents/deep_researcher/factory.py index 88dc6ab8c..095b3b8ab 100644 --- a/src/aiq_agent/agents/deep_researcher/factory.py +++ b/src/aiq_agent/agents/deep_researcher/factory.py @@ -41,6 +41,7 @@ from aiq_agent.common import LLMRole from aiq_agent.common import render_prompt_template +from .custom_middleware import ArtifactHarvestMiddleware from .custom_middleware import EmptyContentFixMiddleware from .custom_middleware import PlanPersistenceMiddleware from .custom_middleware import SourceRegistryMiddleware @@ -199,13 +200,14 @@ def build_common_middleware( *, tool_set: DeepResearchToolSet, source_registry_middleware: SourceRegistryMiddleware, + artifact_manager: object | None = None, extra_valid_tool_names: Sequence[str] = (), ) -> list[Any]: """Build the shared middleware stack with agent-specific valid tool names.""" valid_tool_names = {tool.name for tool in [*tool_set.all_tools, *tool_set.researcher_tools]} valid_tool_names.update(FILESYSTEM_TOOL_NAMES) valid_tool_names.update(extra_valid_tool_names) - return [ + middleware: list[Any] = [ EmptyContentFixMiddleware(), ToolNameSanitizationMiddleware(valid_tool_names=sorted(valid_tool_names)), ToolRetryMiddleware(max_retries=3, backoff_factor=2.0, initial_delay=1.0), @@ -213,6 +215,9 @@ def build_common_middleware( ToolResultPruningMiddleware(keep_last_n=10, max_chars=2000), ModelRetryMiddleware(max_retries=2, backoff_factor=2.0, initial_delay=1.0), ] + if artifact_manager is not None: + middleware.append(ArtifactHarvestMiddleware(artifact_manager)) + return middleware def build_source_router_middleware(*, extra_valid_tool_names: Sequence[str] = ()) -> list[Any]: @@ -229,6 +234,7 @@ def build_deep_research_middleware_set( *, tool_set: DeepResearchToolSet, source_registry_middleware: SourceRegistryMiddleware, + artifact_manager: object | None = None, ) -> DeepResearchMiddlewareSet: """Build researcher, writer, and orchestrator middleware stacks.""" @@ -237,6 +243,7 @@ def common(extra_valid_tool_names: Sequence[str] = ()) -> list[Any]: return build_common_middleware( tool_set=tool_set, source_registry_middleware=source_registry_middleware, + artifact_manager=artifact_manager, extra_valid_tool_names=extra_valid_tool_names, ) diff --git a/src/aiq_agent/agents/deep_researcher/register.py b/src/aiq_agent/agents/deep_researcher/register.py index 14e382464..eded3bee4 100644 --- a/src/aiq_agent/agents/deep_researcher/register.py +++ b/src/aiq_agent/agents/deep_researcher/register.py @@ -220,6 +220,9 @@ async def deep_research_agent(config: DeepResearchAgentConfig, builder: Builder) verbose = is_verbose(config.verbose) callbacks = [VerboseTraceCallback()] if verbose else [] + # A sandbox-enabled agent is request-scoped below. Keep the reusable template free of + # sandbox/store state so startup does not construct an unused artifact runtime. + template_skills = None if sandbox_config is not None else skills_config agent = DeepResearcherAgent( llm_provider=provider, tools=tools, @@ -228,8 +231,8 @@ async def deep_research_agent(config: DeepResearchAgentConfig, builder: Builder) domain_catalog_path=config.domain_catalog_path, enable_source_router=config.enable_source_router, enable_citation_verification=config.enable_citation_verification, - skills=skills_config, - sandbox=sandbox_config, + skills=template_skills, + sandbox=None, max_research_concurrency=config.max_research_concurrency, max_concurrent_source_tool_calls=config.max_concurrent_source_tool_calls, max_source_tool_batch_size=config.max_source_tool_batch_size, diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/README.md b/src/aiq_agent/agents/deep_researcher/sandbox/README.md index 9aa15ee47..48175203d 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/README.md +++ b/src/aiq_agent/agents/deep_researcher/sandbox/README.md @@ -157,12 +157,14 @@ is lifted into `providers.modal`. ## Artifact runtime - Generated code writes binaries + a `manifest.json` to `artifact_dir`. -- Once at the end of a successful agent run (`agent.run()` -> `ArtifactManager.final_harvest`), - the `ArtifactManager` pulls bytes via `download_files`, runs the validation pipeline +- Successful `execute` calls trigger a manifest-only checkpoint. Terminal finalization runs + one manifest + directory scan on success or failure. Cancellation attempts that scan only + when the provider operation lease is immediately available; a busy sandbox is terminated + without a background harvest thread, so no artifact can arrive after terminal status. +- The `ArtifactManager` pulls bytes via `download_files`, runs the validation pipeline (path-traversal confinement -> extension allowlist -> size cap -> MIME-from-bytes/spoof reject -> quota -> SVG sanitize -> sha256), stores via `SqlArtifactStore`, then emits an - artifact event (metadata + content URL, never bytes). -- Failed or cancelled runs are not harvested until the terminal-harvest follow-up lands. + `artifact.update` event (durable metadata + `content_url`, never bytes or URL-as-text). - Reports reference artifacts as `![caption](artifact://)`; the report postprocessor rewrites filename refs to durable ids and drops unknown/foreign refs. - Endpoints: `GET /v1/jobs/async/job/{job_id}/artifacts` and `.../artifacts/{id}/content` diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py b/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py index 5cae44941..66dbd1291 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py @@ -178,15 +178,30 @@ def __init__( self._emit = emit self._content_url_template = content_url_template self._lock = threading.Lock() + self._final_harvest_lock = threading.Lock() + self._final_harvest_done = False + self._final_harvest_result: list[Artifact] = [] self._seen: set[tuple[str, str]] = set() self._total_bytes = 0 self._count = 0 def final_harvest(self) -> list[Artifact]: - """Harvest at the end of a successful agent run, with a directory scan fallback.""" + """Harvest at a terminal boundary, with a directory scan fallback.""" if not self.config.enabled: return [] - return self._harvest(scan=True) + with self._final_harvest_lock: + if self._final_harvest_done: + return list(self._final_harvest_result) + captured = self._harvest(scan=True) + self._final_harvest_result = list(captured) + self._final_harvest_done = True + return captured + + def harvest_after_execute(self) -> list[Artifact]: + """Checkpoint manifest-declared artifacts after a sandbox execute call.""" + if not self.config.enabled: + return [] + return self._harvest(scan=False) def resolve_report_references(self, markdown: str, artifacts: list[Artifact] | None = None) -> str: """Validate ``artifact://`` image references against this job's artifacts. diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py b/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py index 0ca080d05..95c3a4bf3 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py @@ -84,24 +84,30 @@ class Artifact(BaseModel): status: ArtifactStatus = Field(default=ArtifactStatus.PENDING) def to_sse_payload(self, content_url: str) -> dict[str, object]: - """Build the richer ``artifact`` SSE payload for live UI updates. + """Build the canonical ``artifact.update`` SSE payload for live UI updates. Args: content_url: Authenticated URL where the bytes can be fetched. Returns: - A JSON-serializable payload (no bytes) matching the design's artifact event. + A JSON-serializable payload (no bytes) matching the API/UI event contract. """ return { - "type": "artifact", - "artifact_id": self.artifact_id, - "kind": self.kind.value, - "filename": self.filename, - "mime_type": self.mime_type, - "size_bytes": self.size_bytes, - "sha256": self.sha256, - "title": self.title, - "caption": self.caption, - "inline": self.inline, - "content_url": content_url, + "type": "artifact.update", + "name": self.filename, + "data": { + "type": "file", + "url": content_url, + "content_url": content_url, + "file_path": self.filename, + "artifact_id": self.artifact_id, + "job_id": self.job_id, + "kind": self.kind.value, + "mime_type": self.mime_type, + "size_bytes": self.size_bytes, + "sha256": self.sha256, + "title": self.title, + "caption": self.caption, + "inline": self.inline, + }, } diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/base.py b/src/aiq_agent/agents/deep_researcher/sandbox/base.py index c43f865d6..bc1bec746 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/base.py +++ b/src/aiq_agent/agents/deep_researcher/sandbox/base.py @@ -33,6 +33,7 @@ from abc import ABC from abc import abstractmethod from collections.abc import Callable +from contextlib import contextmanager from typing import TYPE_CHECKING from typing import TypeVar @@ -155,6 +156,16 @@ def _emit_event(self, event: dict[str, object]) -> None: except Exception: # noqa: BLE001 - event persistence is non-critical logger.warning("Sandbox event emission failed for %s", self.sandbox_name, exc_info=True) + @contextmanager + def try_operation_lease(self): + """Yield whether the provider is idle without waiting behind an in-flight execute.""" + acquired = self._lock.acquire(blocking=False) + try: + yield acquired + finally: + if acquired: + self._lock.release() + def close(self) -> None: """Release the underlying sandbox session, if any (idempotent). diff --git a/tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py b/tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py index 6a5628ed2..18e702ba5 100644 --- a/tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py +++ b/tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py @@ -40,14 +40,18 @@ class _FakeBackend: def __init__(self, files: dict[str, bytes]) -> None: self.files = files + self.execute_calls: list[str] = [] + self.download_calls: list[list[str]] = [] def download_files(self, paths: list[str]) -> list[Any]: + self.download_calls.append(list(paths)) return [ SimpleNamespace(path=p, content=self.files.get(p), error=None if p in self.files else "not found") for p in paths ] def execute(self, command: str, *, timeout: int | None = None) -> Any: + self.execute_calls.append(command) return SimpleNamespace(output="\n".join(self.files), exit_code=0) @@ -118,8 +122,12 @@ def test_captures_manifest_artifact(self, tmp_path: Any) -> None: assert captured[0].mime_type == "image/png" assert captured[0].kind == ArtifactKind.IMAGE assert store.list("job-1")[0].filename == "chart.png" - assert emitted and emitted[0]["type"] == "artifact" - assert "content" not in emitted[0] # bytes never in the event payload + assert emitted and emitted[0]["type"] == "artifact.update" + assert emitted[0]["name"] == "chart.png" + assert emitted[0]["data"]["type"] == "file" + assert emitted[0]["data"]["artifact_id"] == captured[0].artifact_id + assert emitted[0]["data"]["content_url"].endswith(f"/{captured[0].artifact_id}/content") + assert "content" not in emitted[0]["data"] # bytes and URL-as-text never enter the payload def test_rejects_path_traversal(self, tmp_path: Any) -> None: store = SqlArtifactStore(f"sqlite:///{tmp_path}/jobs.db") @@ -160,8 +168,21 @@ def test_dedups_identical_content(self, tmp_path: Any) -> None: files = {f"{_ARTIFACT_DIR}/manifest.json": _manifest_bytes(png_path), png_path: _PNG} manager, _ = _make_manager(store, files) manager.final_harvest() + first_downloads = list(manager.backend.download_calls) manager.final_harvest() # same bytes again assert len(store.list("job-1")) == 1 + assert manager.backend.download_calls == first_downloads + + def test_checkpoint_harvest_uses_manifest_without_directory_scan(self, tmp_path: Any) -> None: + store = SqlArtifactStore(f"sqlite:///{tmp_path}/jobs.db") + png_path = f"{_ARTIFACT_DIR}/chart.png" + files = {f"{_ARTIFACT_DIR}/manifest.json": _manifest_bytes(png_path), png_path: _PNG} + manager, _ = _make_manager(store, files) + + captured = manager.harvest_after_execute() + + assert [artifact.filename for artifact in captured] == ["chart.png"] + assert manager.backend.execute_calls == [] def test_scan_fallback_without_manifest(self, tmp_path: Any) -> None: store = SqlArtifactStore(f"sqlite:///{tmp_path}/jobs.db") diff --git a/tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py b/tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py index 61d0376ce..88426fc31 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py +++ b/tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py @@ -25,6 +25,7 @@ from langchain_core.messages import SystemMessage from langchain_core.messages import ToolMessage +from aiq_agent.agents.deep_researcher.custom_middleware import ArtifactHarvestMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import PlanPersistenceMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import SourceRegistryMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import TodoSuppressionMiddleware @@ -546,6 +547,75 @@ async def test_content_returned_unchanged(self, middleware): assert result.content == content +class TestArtifactHarvestMiddleware: + """Checkpoint harvesting runs only after successful execute tool calls.""" + + @pytest.mark.asyncio + async def test_execute_checkpoints_after_handler(self) -> None: + manager = MagicMock() + middleware = ArtifactHarvestMiddleware(manager) + request = MagicMock() + request.tool_call = {"name": "execute"} + handler = AsyncMock(return_value="ok") + + result = await middleware.awrap_tool_call(request, handler) + + assert result == "ok" + manager.harvest_after_execute.assert_called_once_with() + + @pytest.mark.asyncio + async def test_non_execute_tool_does_not_harvest(self) -> None: + manager = MagicMock() + middleware = ArtifactHarvestMiddleware(manager) + request = MagicMock() + request.tool_call = {"name": "read_file"} + + await middleware.awrap_tool_call(request, AsyncMock(return_value="ok")) + + manager.harvest_after_execute.assert_not_called() + + @pytest.mark.asyncio + async def test_handler_failure_does_not_harvest(self) -> None: + manager = MagicMock() + middleware = ArtifactHarvestMiddleware(manager) + request = MagicMock() + request.tool_call = {"name": "execute"} + + with pytest.raises(RuntimeError, match="tool failed"): + await middleware.awrap_tool_call(request, AsyncMock(side_effect=RuntimeError("tool failed"))) + + manager.harvest_after_execute.assert_not_called() + + @pytest.mark.asyncio + async def test_execute_error_result_does_not_harvest(self) -> None: + manager = MagicMock() + middleware = ArtifactHarvestMiddleware(manager) + request = MagicMock() + request.tool_call = {"name": "execute"} + + await middleware.awrap_tool_call( + request, + AsyncMock(return_value=ToolMessage(content="failed", tool_call_id="tc1", status="error")), + ) + + manager.harvest_after_execute.assert_not_called() + + @pytest.mark.asyncio + async def test_checkpoint_failure_logs_only_exception_type(self, caplog: pytest.LogCaptureFixture) -> None: + manager = MagicMock() + manager.harvest_after_execute.side_effect = RuntimeError("credential=do-not-log") + middleware = ArtifactHarvestMiddleware(manager) + request = MagicMock() + request.tool_call = {"name": "execute"} + + with caplog.at_level(logging.WARNING): + result = await middleware.awrap_tool_call(request, AsyncMock(return_value="ok")) + + assert result == "ok" + assert "RuntimeError" in caplog.text + assert "credential=do-not-log" not in caplog.text + + class _RecordingBackend: """Minimal backend stub capturing upload_files calls (overwrite-safe).""" diff --git a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py index 01a074705..8ee486ed7 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py +++ b/tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py @@ -18,8 +18,10 @@ from __future__ import annotations import logging +from contextlib import nullcontext from threading import Event from threading import Thread +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock from unittest.mock import patch @@ -36,8 +38,10 @@ from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSandboxConfig from aiq_agent.agents.deep_researcher.deepagents_runtime import DeepResearchSkillsConfig from aiq_agent.agents.deep_researcher.deepagents_runtime import _create_sandbox_backend +from aiq_agent.agents.deep_researcher.deepagents_runtime import _maybe_build_artifact_manager from aiq_agent.agents.deep_researcher.deepagents_runtime import discover_skill_collections from aiq_agent.agents.deep_researcher.deepagents_runtime import resolve_skill_collections +from aiq_agent.agents.deep_researcher.sandbox.config import ArtifactCaptureConfig SYNTHESIS_SKILL_SOURCE = f"{BUILTIN_SKILL_SOURCE}synthesis/" @@ -469,3 +473,74 @@ def finalize_again() -> None: provider.close.assert_called_once_with() provider.terminate.assert_not_called() assert [event["data"]["status"] for event in events] == ["started", "failed"] # type: ignore[index] + + def test_normal_finalize_harvests_before_close(self) -> None: + provider = MagicMock() + provider.cleanup_succeeded = True + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime(sandbox=DeepResearchSandboxConfig()) + order: list[str] = [] + runtime.artifact_manager = MagicMock() + runtime.artifact_manager.final_harvest.side_effect = lambda: order.append("harvest") + provider.close.side_effect = lambda: order.append("close") + + runtime.finalize(interrupted=False) + runtime.finalize(interrupted=False) + + assert order == ["harvest", "close"] + + def test_final_harvest_logs_only_exception_type(self, caplog: pytest.LogCaptureFixture) -> None: + provider = MagicMock() + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime(sandbox=DeepResearchSandboxConfig()) + runtime.artifact_manager = MagicMock() + runtime.artifact_manager.final_harvest.side_effect = RuntimeError("credential=do-not-log") + + with caplog.at_level(logging.WARNING): + runtime.final_harvest() + + assert "RuntimeError" in caplog.text + assert "credential=do-not-log" not in caplog.text + + @pytest.mark.parametrize(("lease_acquired", "expected"), [(True, ["harvest", "terminate"]), (False, ["terminate"])]) + def test_interrupted_finalize_harvests_only_when_provider_is_idle( + self, + lease_acquired: bool, + expected: list[str], + ) -> None: + provider = MagicMock() + provider.cleanup_succeeded = True + provider.try_operation_lease.return_value = nullcontext(lease_acquired) + with patch( + "aiq_agent.agents.deep_researcher.deepagents_runtime._create_sandbox_backend", + return_value=provider, + ): + runtime = DeepAgentsRuntime(sandbox=DeepResearchSandboxConfig()) + order: list[str] = [] + runtime.artifact_manager = MagicMock() + runtime.artifact_manager.final_harvest.side_effect = lambda: order.append("harvest") + provider.terminate.side_effect = lambda: order.append("terminate") + + runtime.finalize(interrupted=True) + + assert order == expected + + def test_enabled_capture_requires_durable_store_url(self) -> None: + provider = SimpleNamespace( + config=SimpleNamespace(artifact_capture=ArtifactCaptureConfig(enabled=True)), + ) + + with pytest.raises(ValueError, match="durable artifact_db_url"): + _maybe_build_artifact_manager( + provider=provider, + job_id="job-1", + artifact_dir="/sandbox/job-1/aiq-artifacts", + artifact_db_url=None, + artifact_emit=None, + ) diff --git a/tests/aiq_agent/agents/deep_researcher/test_factory.py b/tests/aiq_agent/agents/deep_researcher/test_factory.py index 72614b1c2..826b21c97 100644 --- a/tests/aiq_agent/agents/deep_researcher/test_factory.py +++ b/tests/aiq_agent/agents/deep_researcher/test_factory.py @@ -23,6 +23,7 @@ from langchain.agents.middleware import AgentMiddleware from langchain_core.tools import tool +from aiq_agent.agents.deep_researcher.custom_middleware import ArtifactHarvestMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import SourceRegistryMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import ToolNameSanitizationMiddleware from aiq_agent.agents.deep_researcher.custom_middleware import ToolVisibilityMiddleware @@ -150,6 +151,28 @@ def test_middleware_set_adds_orchestrator_batch_tool_name(): assert tool_set.writer_tools != tool_set.researcher_tools +def test_middleware_set_wires_artifact_checkpoint_when_manager_present(): + """Artifact harvesting is attached to every execute-capable middleware stack.""" + registry = SourceRegistryMiddleware(source_tool_names={web_search_tool.name}) + tool_set = build_deep_research_tool_set( + [web_search_tool], + source_registry_middleware=registry, + max_concurrent_source_tool_calls=2, + max_source_tool_batch_size=3, + ) + manager = MagicMock() + + middleware_set = build_deep_research_middleware_set( + tool_set=tool_set, + source_registry_middleware=registry, + artifact_manager=manager, + ) + + for middleware in (middleware_set.researcher, middleware_set.planner, middleware_set.writer): + checkpoint = next(item for item in middleware if isinstance(item, ArtifactHarvestMiddleware)) + assert checkpoint.artifact_manager is manager + + def test_subagents_route_tools_and_writer_skills(): """Source-router excludes source tools, planner receives source tools, and writer receives configured skills.""" runtime = DeepAgentsRuntime( diff --git a/tests/aiq_agent/fastapi_extensions/test_deep_research.py b/tests/aiq_agent/fastapi_extensions/test_deep_research.py index b8fb6c682..9f37e4d6c 100644 --- a/tests/aiq_agent/fastapi_extensions/test_deep_research.py +++ b/tests/aiq_agent/fastapi_extensions/test_deep_research.py @@ -52,7 +52,10 @@ - Routes registered when infrastructure available """ +from types import SimpleNamespace +from unittest.mock import AsyncMock from unittest.mock import MagicMock +from unittest.mock import patch import pytest @@ -247,6 +250,62 @@ async def test_routes_registered_with_dask(self): assert mock_app.post.call_count >= 2 assert mock_app.get.call_count >= 6 + @pytest.mark.asyncio + async def test_cancel_request_remains_nonterminal_until_worker_finalizes(self): + """The API records intent; only the worker may publish INTERRUPTED.""" + from aiq_api.routes.jobs import register_job_routes + + routes: dict[str, object] = {} + mock_app = MagicMock() + + def register(path: str, **_kwargs): + def decorator(fn): + routes[path] = fn + return fn + + return decorator + + mock_app.get.side_effect = register + mock_app.post.side_effect = register + mock_builder = MagicMock() + mock_builder.get_function_config.side_effect = KeyError("Not found") + job_store = MagicMock() + job_store.update_status = AsyncMock() + mock_worker = MagicMock() + mock_worker._dask_available = True + mock_worker._job_store = job_store + mock_worker._scheduler_address = "tcp://localhost:8786" + mock_worker._db_url = "sqlite:///./test.db" + mock_worker._config_file_path = "/path/to/config.yml" + mock_worker._log_level = 20 + mock_worker._use_dask_threads = False + mock_worker._front_end_config = MagicMock(expiry_seconds=86400) + event_store = MagicMock() + + with ( + patch("aiq_api.routes.jobs.require_verified_principal", return_value=MagicMock()), + patch( + "aiq_api.jobs.access.authorize_job_access", + new=AsyncMock(return_value=SimpleNamespace(status="running")), + ), + patch("aiq_api.jobs.event_store.EventStore", return_value=event_store), + patch("aiq_api.routes.jobs._cancel_dask_task", new=AsyncMock(return_value=True)), + ): + await register_job_routes(mock_app, mock_builder, mock_worker) + cancel = routes["/v1/jobs/async/job/{job_id}/cancel"] + response = await cancel("job-1") # type: ignore[operator] + + job_store.update_status.assert_not_awaited() + event_store.store.assert_called_once_with( + {"type": "job.cancellation_requested", "data": {"reason": "cancelled by user"}} + ) + assert response == { + "job_id": "job-1", + "status": "running", + "cancellation_requested": True, + "task_cancelled": True, + } + class TestArtifactHelpers: """Tests for artifact extraction helper functions.""" diff --git a/tests/aiq_agent/jobs/test_runner.py b/tests/aiq_agent/jobs/test_runner.py index c40fb5ec5..aecfa6013 100644 --- a/tests/aiq_agent/jobs/test_runner.py +++ b/tests/aiq_agent/jobs/test_runner.py @@ -609,6 +609,37 @@ def test_store_event(self, tmp_path): assert len(events) == 1 assert events[0]["type"] == "test.event" + def test_artifact_update_survives_event_store_round_trip(self, tmp_path): + """Generated-file metadata remains reconstructable after durable storage.""" + from aiq_agent.agents.deep_researcher.sandbox.artifacts import Artifact + from aiq_agent.agents.deep_researcher.sandbox.artifacts import ArtifactKind + from aiq_api.jobs.event_store import EventStore + + db_url = f"sqlite:///{tmp_path / 'test.db'}" + content_url = "/v1/jobs/async/job/job-1/artifacts/artifact-1/content" + artifact = Artifact( + artifact_id="artifact-1", + job_id="job-1", + kind=ArtifactKind.IMAGE, + mime_type="image/png", + filename="chart.png", + sandbox_path="/sandbox/job-1/aiq-artifacts/chart.png", + storage_uri="db://artifacts/artifact-1", + sha256="a" * 64, + size_bytes=128, + inline=True, + ) + + EventStore(db_url, "job-1").store(artifact.to_sse_payload(content_url)) + + event = EventStore.get_events(db_url, "job-1")[0] + assert event["type"] == "artifact.update" + assert event["name"] == "chart.png" + assert event["data"]["type"] == "file" + assert event["data"]["content_url"] == content_url + assert "content" not in event["data"] + assert event["data"]["artifact_id"] == "artifact-1" + def test_get_events_empty(self, tmp_path): """Test get_events returns empty list for unknown job.""" from aiq_api.jobs.event_store import EventStore From 23dd4d55787c51e5de99adc0cd6ce7f6be2ecf00 Mon Sep 17 00:00:00 2001 From: Kyle Zheng Date: Thu, 2 Jul 2026 15:24:44 -0700 Subject: [PATCH 8/8] docs: clarify OpenShell Landlock best_effort for local macOS/Docker Desktop Make the existing --landlock-compatibility flag discoverable and document the two-knob requirement (policy landlock.compatibility vs AI-Q require_hard_landlock) so local hosts without a Landlock LSM don't hit sandbox prepare failures. Signed-off-by: Kyle Zheng --- docs/source/architecture/agents/sandbox.md | 5 +++++ scripts/setup_openshell.sh | 4 +++- .../agents/deep_researcher/sandbox/README.md | 21 +++++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/docs/source/architecture/agents/sandbox.md b/docs/source/architecture/agents/sandbox.md index da9caaeeb..5a370398a 100644 --- a/docs/source/architecture/agents/sandbox.md +++ b/docs/source/architecture/agents/sandbox.md @@ -47,6 +47,11 @@ runtime (`.../job/{job_id}/artifacts`), which is also auth-scoped to the job. - Job status becomes terminal only after artifact and cleanup events are flushed. - Per-job OpenShell mode requires `delete_on_exit: true`. A persistent shared sandbox is possible only through the explicit debug attachment settings. +- Hosts without a Landlock LSM (local macOS / Docker Desktop) cannot satisfy the production + default `landlock.compatibility: hard_requirement`; every sandbox fails prepare there. + Generate a local policy with `./scripts/setup_openshell.sh --landlock-compatibility best_effort` + and set `require_hard_landlock: false`. See the deep researcher sandbox README ("Local + macOS / Docker Desktop") for the two-knob detail. ## Current Safeguards diff --git a/scripts/setup_openshell.sh b/scripts/setup_openshell.sh index 0e13b0dea..d81a605ca 100755 --- a/scripts/setup_openshell.sh +++ b/scripts/setup_openshell.sh @@ -90,7 +90,9 @@ Options: Services: $SUPPORTED_SERVICES --policy-file PATH Output policy file. Default: configs/openshell/generated/aiq-openshell-policy.yaml - --landlock-compatibility MODE hard_requirement (default) or best_effort (local demo only). + --landlock-compatibility MODE hard_requirement (default; production/Linux, fails closed) + or best_effort (required for local macOS / Docker Desktop, + which have no Landlock LSM; never use in production). --create-shared-debug-sandbox Create one named shared sandbox for explicit debug attachment. --sandbox-name NAME Debug sandbox name (default: aiq-openshell-demo). --image-name NAME Docker image tag (default: aiq-openshell-demo:latest). diff --git a/src/aiq_agent/agents/deep_researcher/sandbox/README.md b/src/aiq_agent/agents/deep_researcher/sandbox/README.md index 48175203d..bca718aaa 100644 --- a/src/aiq_agent/agents/deep_researcher/sandbox/README.md +++ b/src/aiq_agent/agents/deep_researcher/sandbox/README.md @@ -246,6 +246,27 @@ export AIQ_OPENSHELL_IMAGE="aiq-openshell-demo:latest" export AIQ_OPENSHELL_POLICY_FILE="$PWD/configs/openshell/generated/aiq-openshell-policy.yaml" ``` +### Local macOS / Docker Desktop + +macOS (and Docker Desktop) has no Landlock LSM, so a policy with the production default +`landlock.compatibility: hard_requirement` fails closed at sandbox prepare: every sandbox +goes straight to `SANDBOX_PHASE_ERROR` and no `execute` ever runs. Generate a local policy +with `best_effort` instead: + +```bash +./scripts/setup_openshell.sh --landlock-compatibility best_effort +# or, persistently, so regeneration keeps it: +export AIQ_OPENSHELL_LANDLOCK_COMPATIBILITY=best_effort +``` + +Two independent knobs must agree for a local run; setting only one is a common trap: + +- `landlock.compatibility: best_effort` in the **policy file** is what the OpenShell gateway + actually enforces (this is what lets prepare succeed without Landlock). +- `require_hard_landlock: false` in the **AI-Q config** only relaxes AI-Q's own acceptance + gate so it will load a non-`hard_requirement` policy. It does not change what the gateway + enforces, so it is insufficient on its own. + For a local host that cannot enforce Landlock, an explicit non-production demo can use `--landlock-compatibility best_effort` together with `require_hard_landlock: false` in the AI-Q config. `--create-shared-debug-sandbox` creates the old named attachment target only