feat: Persisted Artifact Lifecycle for sandbox#305
Conversation
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
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 <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
…esktop 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 <kyzheng@nvidia.com>
WalkthroughThis PR converts OpenShell sandboxes to per-job, policy-bound, attested instances (replacing shared local sandbox attachment), reworks sandbox finalize/cleanup and artifact checkpoint harvesting, changes artifact.update SSE payload to durable content_url-based metadata consumed across the UI, and switches job cancellation to an event-driven request/interrupt flow. ChangesOpenShell sandboxing, artifact harvesting, and cancellation flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JobsRoute
participant EventStore
participant JobRunner
participant DeepAgentsRuntime
participant OpenShellSandboxProvider
Client->>JobsRoute: POST cancel/job/{id}
JobsRoute->>EventStore: store job.cancellation_requested
JobsRoute->>JobRunner: cancel Dask task
JobRunner->>DeepAgentsRuntime: finalize(interrupted=True)
DeepAgentsRuntime->>DeepAgentsRuntime: try_operation_lease (harvest if idle)
DeepAgentsRuntime->>OpenShellSandboxProvider: terminate()
DeepAgentsRuntime->>EventStore: emit sandbox.cleanup succeeded/failed
JobRunner->>EventStore: publish INTERRUPTED status
sequenceDiagram
participant OpenShellSandboxProvider
participant OpenShellSDK
participant ArtifactManager
participant EventStore
participant UI
OpenShellSandboxProvider->>OpenShellSDK: create sandbox with policy spec
OpenShellSDK-->>OpenShellSandboxProvider: READY, loaded policy revision
OpenShellSandboxProvider->>EventStore: emit sandbox.attestation succeeded
ArtifactManager->>ArtifactManager: harvest_after_execute (checkpoint)
ArtifactManager->>EventStore: store artifact.update (content_url)
EventStore-->>UI: SSE artifact.update
UI->>UI: render FileCard with mimeType/size, Open file
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
775-792: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRestore a worker-visible cancellation signal The cancel endpoint now only records
job.cancellation_requestedand calls_cancel_dask_task(), butrunner.pystill only stops onJobStatus.INTERRUPTED. Since_cancel_dask_task()is justdistributed.Client.cancel(..., force=True), an already-running task can keep going while the API returnscancellation_requested: True.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/routes/jobs.py` around lines 775 - 792, The cancel flow only records job.cancellation_requested and calls _cancel_dask_task(), but runner.py still only exits on JobStatus.INTERRUPTED, so running work may continue after the API says cancellation was requested. Update the cancellation path in jobs.py and the worker-side check in runner.py so a cancellation_requested signal is visible to the worker loop, for example by persisting a status/flag that the runner polls and treats as a stop condition alongside JobStatus.INTERRUPTED. Ensure the symbols EventStore.store, _cancel_dask_task, and the runner cancellation check are kept in sync.tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py (1)
200-204: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a concurrency test for
harvest_after_executevsfinal_harvest.
harvest_after_execute()andfinal_harvest()both fall through to the shared_lock-protected_harvest(), but onlyfinal_harvest()'s own memoization is exercised sequentially by existing tests. A test that invokesharvest_after_execute()from a background thread concurrently withfinal_harvest()would directly verify the dedup/quota counters (_seen,_count,_total_bytes) stay consistent under the new checkpoint entrypoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py` around lines 200 - 204, Add a concurrency test around `ArtifactHarvester` to cover `harvest_after_execute()` racing with `final_harvest()`, since both funnel through the shared `_harvest()` path but only `final_harvest()` memoization is currently tested. Use the existing `test_artifacts.py` setup with `_ARTIFACT_DIR`, `_manifest_bytes`, and the harvester methods, then invoke `harvest_after_execute()` from a background thread while calling `final_harvest()` on the main thread. Assert the dedup/quota state (`_seen`, `_count`, `_total_bytes`) remains consistent and that the checkpoint entrypoint does not double-count artifacts.frontends/ui/src/adapters/api/deep-research-client.ts (1)
166-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ArtifactUpdateEventtype doesn't reflect the actualfilepayload shape.The runtime parser (Lines 501-515) reads
file_path,artifact_id,mime_type,size_bytes,sha256,inline,output_categoryoff the raw payload, but the exportedArtifactUpdateEvent.data.datatype (Lines 166-181) only declarescontent,url,content_url. Consumers importing this type directly (it's re-exported fromindex.ts) get an inaccurate contract forfile-type artifacts.♻️ Proposed fix
data: { type: ArtifactType content?: string | TodoItem[] url?: string // For citation_source and citation_use types content_url?: string + file_path?: string + path?: string + artifact_id?: string + mime_type?: string + size_bytes?: number + sha256?: string + inline?: boolean + output_category?: string }Also applies to: 499-515
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/ui/src/adapters/api/deep-research-client.ts` around lines 166 - 181, `ArtifactUpdateEvent` currently declares an incomplete `data.data` shape for file artifacts, so update the type to match the raw payload parsed in `parseArtifactUpdateEvent` and any related helpers. Add the file-specific fields observed at runtime (`file_path`, `artifact_id`, `mime_type`, `size_bytes`, `sha256`, `inline`, `output_category`) to the `ArtifactUpdateEvent` contract, while preserving the existing `content`, `url`, and `content_url` cases for other artifact types. Ensure the exported type from `deep-research-client.ts` remains aligned with the parser so consumers re-exported from `index.ts` get the accurate artifact payload shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/ui/src/adapters/api/deep-research-client.ts`:
- Around line 533-536: The `filePath` derivation in `deep-research-client.ts` is
relying on an unsafe `as string` assertion, which can let non-string
`file_path`/`path`/`url` values reach `split('/')` and fail in the SSE handler.
Update the artifact parsing logic around the `raw`/`filePath`/`fileName` block
to validate each candidate value is actually a string before using it, and fall
back safely to a default when none are strings. Keep the fix within the same
artifact-handling path so strict TypeScript behavior is preserved without
runtime casts.
In `@frontends/ui/src/features/chat/hooks/use-load-job-data.ts`:
- Line 375: The buffered file replay in useLoadJobData is replacing prior
FileArtifactUpdate entries by filename instead of merging them, so repeated
artifact.update events can lose earlier data. Update the handling around the
buffer.files Map in useLoadJobData to merge updates by file.filename the same
way addDeepResearchFile does, preserving existing content and metadata when a
later event arrives for the same file.
In `@src/aiq_agent/agents/deep_researcher/sandbox/config.py`:
- Around line 93-100: The deprecated sandbox_name alias in the sandbox config is
accepted silently, so add a deprecation notice when it is provided. Update the
validator logic in the config model around existing_sandbox_name and
sandbox_name to emit a warnings.warn with DeprecationWarning, or an equivalent
logger.warning, whenever sandbox_name is set and then map it through as the
fallback for existing_sandbox_name. Keep the warning tied to the config field
handling so users of the old alias get notified before it is removed.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 372-432: The initial argv validation in OpenShellProvider.setup
duplicates the non-empty shell check already enforced by
OpenShellProviderConfig._validate_lifecycle_mode, so trim the redundant
oscfg.shell guard from the setup path and rely on the config-level validator as
the single source of truth. Keep the rest of the sandbox initialization flow in
setup unchanged, including the shared_sandbox_name and policy handling branches.
---
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/routes/jobs.py`:
- Around line 775-792: The cancel flow only records job.cancellation_requested
and calls _cancel_dask_task(), but runner.py still only exits on
JobStatus.INTERRUPTED, so running work may continue after the API says
cancellation was requested. Update the cancellation path in jobs.py and the
worker-side check in runner.py so a cancellation_requested signal is visible to
the worker loop, for example by persisting a status/flag that the runner polls
and treats as a stop condition alongside JobStatus.INTERRUPTED. Ensure the
symbols EventStore.store, _cancel_dask_task, and the runner cancellation check
are kept in sync.
In `@frontends/ui/src/adapters/api/deep-research-client.ts`:
- Around line 166-181: `ArtifactUpdateEvent` currently declares an incomplete
`data.data` shape for file artifacts, so update the type to match the raw
payload parsed in `parseArtifactUpdateEvent` and any related helpers. Add the
file-specific fields observed at runtime (`file_path`, `artifact_id`,
`mime_type`, `size_bytes`, `sha256`, `inline`, `output_category`) to the
`ArtifactUpdateEvent` contract, while preserving the existing `content`, `url`,
and `content_url` cases for other artifact types. Ensure the exported type from
`deep-research-client.ts` remains aligned with the parser so consumers
re-exported from `index.ts` get the accurate artifact payload shape.
In `@tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py`:
- Around line 200-204: Add a concurrency test around `ArtifactHarvester` to
cover `harvest_after_execute()` racing with `final_harvest()`, since both funnel
through the shared `_harvest()` path but only `final_harvest()` memoization is
currently tested. Use the existing `test_artifacts.py` setup with
`_ARTIFACT_DIR`, `_manifest_bytes`, and the harvester methods, then invoke
`harvest_after_execute()` from a background thread while calling
`final_harvest()` on the main thread. Assert the dedup/quota state (`_seen`,
`_count`, `_total_bytes`) remains consistent and that the checkpoint entrypoint
does not double-count artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cb9a3b77-fad9-4a7e-9efe-9514bf9b7811
📒 Files selected for processing (38)
configs/config_openshell.ymlconfigs/openshell/aiq-research-policy.yamldocs/source/architecture/agents/sandbox.mdfrontends/aiq_api/README.mdfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/ui/src/adapters/api/deep-research-client.spec.tsfrontends/ui/src/adapters/api/deep-research-client.tsfrontends/ui/src/adapters/api/index.tsfrontends/ui/src/features/chat/hooks/use-deep-research.spec.tsfrontends/ui/src/features/chat/hooks/use-deep-research.tsfrontends/ui/src/features/chat/hooks/use-load-job-data.tsfrontends/ui/src/features/chat/store.tsfrontends/ui/src/features/chat/types.tsfrontends/ui/src/features/layout/components/FileCard.spec.tsxfrontends/ui/src/features/layout/components/FileCard.tsxscripts/README.mdscripts/setup_openshell.shscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (13)
frontends/ui/**/*.{js,ts,jsx,tsx,vue}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run npm lint, type-check, and build validation for UI changes in frontends/ui
Files:
frontends/ui/src/features/layout/components/FileCard.spec.tsxfrontends/ui/src/adapters/api/index.tsfrontends/ui/src/features/chat/types.tsfrontends/ui/src/features/chat/store.tsfrontends/ui/src/adapters/api/deep-research-client.spec.tsfrontends/ui/src/features/chat/hooks/use-deep-research.spec.tsfrontends/ui/src/features/chat/hooks/use-load-job-data.tsfrontends/ui/src/features/layout/components/FileCard.tsxfrontends/ui/src/features/chat/hooks/use-deep-research.tsfrontends/ui/src/adapters/api/deep-research-client.ts
frontends/ui/**/*.{ts,tsx,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
frontends/ui/**/*.{ts,tsx,jsx,js}: The UI is built with Next.js / React / TypeScript / Tailwind with KUI components; reuse existing KUI components and visual patterns rather than introducing new ones
Validate UI-affecting changes with npm run lint, npm run type-check, and npm run test:ci, and include a screenshot for visible changes
Files:
frontends/ui/src/features/layout/components/FileCard.spec.tsxfrontends/ui/src/adapters/api/index.tsfrontends/ui/src/features/chat/types.tsfrontends/ui/src/features/chat/store.tsfrontends/ui/src/adapters/api/deep-research-client.spec.tsfrontends/ui/src/features/chat/hooks/use-deep-research.spec.tsfrontends/ui/src/features/chat/hooks/use-load-job-data.tsfrontends/ui/src/features/layout/components/FileCard.tsxfrontends/ui/src/features/chat/hooks/use-deep-research.tsfrontends/ui/src/adapters/api/deep-research-client.ts
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
frontends/ui/src/features/layout/components/FileCard.spec.tsxtests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/fastapi_extensions/test_deep_research.pyfrontends/aiq_api/README.mdfrontends/ui/src/adapters/api/index.tstests/aiq_agent/agents/deep_researcher/test_factory.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pyfrontends/ui/src/features/chat/types.tsfrontends/ui/src/features/chat/store.tsconfigs/config_openshell.ymlfrontends/ui/src/adapters/api/deep-research-client.spec.tssrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pyscripts/README.mdfrontends/ui/src/features/chat/hooks/use-deep-research.spec.tsscripts/smoke_openshell_isolation.pyfrontends/ui/src/features/chat/hooks/use-load-job-data.tstests/aiq_agent/agents/deep_researcher/test_custom_middleware.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pydocs/source/architecture/agents/sandbox.mdconfigs/openshell/aiq-research-policy.yamlsrc/aiq_agent/agents/deep_researcher/register.pyfrontends/ui/src/features/layout/components/FileCard.tsxfrontends/ui/src/features/chat/hooks/use-deep-research.tssrc/aiq_agent/agents/deep_researcher/factory.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdscripts/setup_openshell.shfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pyfrontends/ui/src/adapters/api/deep-research-client.ts
frontends/ui/**/*
⚙️ CodeRabbit configuration file
frontends/ui/**/*: Review UI changes for strict TypeScript behavior, API contract alignment, auth/session handling, accessible controls,
resilient loading and error states, and report/chat state consistency. Prefer existing UI patterns and require tests
for changed user-visible workflows.
Files:
frontends/ui/src/features/layout/components/FileCard.spec.tsxfrontends/ui/src/adapters/api/index.tsfrontends/ui/src/features/chat/types.tsfrontends/ui/src/features/chat/store.tsfrontends/ui/src/adapters/api/deep-research-client.spec.tsfrontends/ui/src/features/chat/hooks/use-deep-research.spec.tsfrontends/ui/src/features/chat/hooks/use-load-job-data.tsfrontends/ui/src/features/layout/components/FileCard.tsxfrontends/ui/src/features/chat/hooks/use-deep-research.tsfrontends/ui/src/adapters/api/deep-research-client.ts
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/test_factory.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pyscripts/smoke_openshell_isolation.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/factory.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pyfrontends/aiq_api/src/aiq_api/routes/jobs.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py
src/aiq_agent/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/aiq_agent/**/*.py: Respect authenticated data sources by honoring requires_auth, per-user token pass-through, and backend token validators; apply owner guardrails before loading protected report or artifact context into an agent
Do not weaken or bypass AuthMiddleware, validators, or auth gating without a prior design discussion
Files:
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.py
src/aiq_agent/agents/**/*
⚙️ CodeRabbit configuration file
src/aiq_agent/agents/**/*: Review agent changes for research workflow correctness, graph state transitions, prompt/tool contracts,
HITL behavior, and failure handling. Flag changes that weaken source attribution, report generation,
async cancellation, checkpointing, or data-source selection without focused tests and docs.
Files:
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/deepagents_runtime.py
{deploy/**,configs/**}
⚙️ CodeRabbit configuration file
{deploy/**,configs/**}: Review deployment and config changes for secret separation, safe defaults, local-vs-production behavior, Helm and
Docker portability, and documentation parity. Flag committed credentials, environment-specific NVIDIA internals in
public defaults, and changes that make examples diverge from CI-tested paths.
Files:
configs/config_openshell.ymlconfigs/openshell/aiq-research-policy.yaml
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/architecture/agents/sandbox.md
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,CONTRIBUTING.md,SECURITY.md,CODE-OF-CONDUCT.md}: Review documentation for command accuracy, branch-name consistency, current CI and copy-pr-bot behavior, public
vs internal boundary clarity, stale examples, and links that no longer match the repository layout.
Files:
docs/source/architecture/agents/sandbox.md
**/*config*.py
📄 CodeRabbit inference engine (AGENTS.md)
Config schemas must inherit from FunctionBaseConfig and YAML _type names must come from the registered config class
Files:
src/aiq_agent/agents/deep_researcher/sandbox/config.py
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}
⚙️ CodeRabbit configuration file
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}: Treat API, auth, and job-runner changes as externally visible contracts. Check authorization boundaries,
request tracing, async job lifecycle, websocket reconnect behavior, error responses, and cross-user data isolation.
Require tests for route behavior, access decisions, and job state transitions when those surfaces change.
Files:
frontends/aiq_api/src/aiq_api/routes/jobs.pyfrontends/aiq_api/src/aiq_api/jobs/runner.py
🔇 Additional comments (50)
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
583-590: LGTM!Also applies to: 606-609, 619-632, 644-654, 662-670, 681-697
frontends/aiq_api/src/aiq_api/routes/jobs.py (1)
761-774: LGTM!frontends/ui/src/adapters/api/deep-research-client.spec.ts (1)
5-102: LGTM!frontends/ui/src/features/chat/hooks/use-deep-research.spec.ts (1)
940-974: LGTM!frontends/ui/src/features/layout/components/FileCard.spec.tsx (1)
65-88: LGTM!tests/aiq_agent/fastapi_extensions/test_deep_research.py (1)
55-58: LGTM!Also applies to: 253-308
tests/aiq_agent/jobs/test_runner.py (1)
612-642: LGTM!Also applies to: 1942-1988, 2015-2025
src/aiq_agent/agents/deep_researcher/agent.py (2)
138-142: LGTM!Also applies to: 156-159
327-346: 🚀 Performance & ScalabilityNo issue here:
ArtifactManager.final_harvest()is memoized, so the cleanup path’s later call returns cached artifacts instead of repeating harvest I/O or re-emittingartifact.updateevents.> Likely an incorrect or invalid review comment.src/aiq_agent/agents/deep_researcher/custom_middleware.py (1)
470-489: LGTM!src/aiq_agent/agents/deep_researcher/factory.py (1)
44-44: LGTM!Also applies to: 199-220, 233-248
src/aiq_agent/agents/deep_researcher/register.py (2)
18-18: LGTM!Also applies to: 223-225, 234-248, 301-309
259-275: 🩺 Stability & AvailabilityNo sandbox leak here
DeepAgentsRuntimeonly builds the provider object; the real OpenShell/Modal sandbox is created later in the provider’s_create_session(). IfDeepResearcherAgent.__init__fails, there isn’t a live sandbox yet to finalize.> Likely an incorrect or invalid review comment.frontends/ui/src/features/layout/components/FileCard.tsx (3)
154-164: 🔒 Security & PrivacyConfirm
contentUrlworks withwindow.openunder bearer-token auth.Opening
file.contentUrlviawindow.open(...)relies on the browser attaching credentials automatically, which only happens for cookies — not for anAuthorizationheader. Elsewhere in this codebase (generate-pdf.ts), authenticated artifact byte-fetching explicitly forwardsAuthorization/idTokenserver-side specifically because outbound requests don't auto-attach headers. If/v1/jobs/.../artifacts/.../contentrequires a bearer token rather than a cookie, this "Open file" action will 401 for authenticated deployments.As per path instructions, "Review UI changes for strict TypeScript behavior, API contract alignment, auth/session handling, accessible controls, resilient loading and error states, and report/chat state consistency."
Source: Path instructions
96-98: 🎯 Functional Correctness | ⚡ Quick winDangling
aria-controls/aria-expandedwhen file has no content.When
file.contentis absent,onClickis now a no-op and the chevron is hidden, but theButtonstill rendersaria-expanded={isExpanded}andaria-controls={file-content-${file.id}}pointing at an element that can never render (isExpandedcan never becometrue). This is a broken ARIA reference for assistive tech.As per path instructions, "Review UI changes for ... accessible controls."
♿ Proposed fix
<Button kind="tertiary" size="small" onClick={() => file.content && setIsExpanded(!isExpanded)} - aria-expanded={isExpanded} - aria-controls={`file-content-${file.id}`} + aria-expanded={file.content ? isExpanded : undefined} + aria-controls={file.content ? `file-content-${file.id}` : undefined} className="w-full justify-start text-left p-0" >Also applies to: 140-152
Source: Path instructions
27-34: LGTM!Also applies to: 68-73, 116-122
frontends/aiq_api/README.md (1)
75-75: LGTM!Also applies to: 152-152
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
292-337: LGTM!src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py (1)
181-204: LGTM!src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py (1)
87-112: LGTM!tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py (1)
43-54: LGTM!Also applies to: 125-130, 171-174, 176-185
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (2)
16-722: LGTM!Also applies to: 845-887
71-86: 📐 Maintainability & Code QualityNo change needed in
_provider()The helper seeds the same instance state asOpenShellSandboxProvider.__init__; it only skips the optional import check so these tests can run without the OpenShell SDK.> Likely an incorrect or invalid review comment.tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py (1)
28-28: LGTM!Also applies to: 550-618
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
20-24: LGTM!Also applies to: 40-44, 312-356, 358-475, 477-533, 534-546
tests/aiq_agent/agents/deep_researcher/test_factory.py (1)
26-26: LGTM!Also applies to: 154-175
frontends/ui/src/adapters/api/deep-research-client.ts (3)
15-15: LGTM!Also applies to: 153-164, 225-225, 787-792
537-553: LGTM!
793-812: LGTM!configs/openshell/aiq-research-policy.yaml (1)
11-16: LGTM!Also applies to: 23-28
configs/config_openshell.yml (1)
1-2: LGTM!Per-job image/policy, allowlist network, and
delete_on_exit: true+attest: true+require_hard_landlock: trueline up correctly with the fail-closed contract enforced inopenshell.py's_read_policy_data/_validate_policy_networkandconfig.py's_validate_lifecycle_mode.Also applies to: 142-165
src/aiq_agent/agents/deep_researcher/sandbox/config.py (2)
101-126: LGTM!Fail-closed lifecycle validation (
delete_on_exit/attestrequired for per-job mode,allow_shared_sandboxgate,expected_policy_versiondepends onattest) matches the provider's_create_session/_attestcontract and the accompanying tests.Also applies to: 148-152
127-146: 🎯 Functional CorrectnessNo issue here —
from __future__ import annotationsis already present, so_validate_lifecycle_mode(self) -> OpenShellProviderConfigdoes not raise at import time.> Likely an incorrect or invalid review comment.src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (5)
190-260: LGTM!Strict
ParseDict(..., ignore_unknown_fields=False), secret-freeSandboxSpecconstruction (no environment/providers copied), and network-hosts-vs-config-allowlist validation all correctly implement the fail-closed contract.
263-360: LGTM!
supports_terminate=Truematches the new terminate-aware context lifecycle, and routing upload/download through_active_os_context()instead ofself._os_contextcloses a prior race where a concurrently-torn-down context could be used directly.
433-518: LGTM!The double policy-version check (before vs. after
_wait_for_loaded_policy) is redundant-looking but correct: the second check is the only one that fires when a version was already loaded and the wait branch was skipped entirely.
529-601: LGTM!The enter/exit state machine (
_os_context_entering/_os_context_exit_requested) correctly handles the terminate-during-creation race: deferred exit while entering, single detached cleanup, and_ensure_context_activecheckpoints that abort publication if teardown won the race. Matches the accompanying test suite.
164-188: 🎯 Functional CorrectnessNo change needed The policy generator and checked-in OpenShell policy both use
process.run_as_user/run_as_group: sandboxandnetwork_policies[*].endpoints[*]withenforcement: enforceandaccess: read-only; this guard matches the repo’s contract.> Likely an incorrect or invalid review comment.docs/source/architecture/agents/sandbox.md (1)
9-11: LGTM!Accurately reflects the per-job attestation/policy-binding behavior and the two-knob Landlock guidance implemented in this cohort.
Also applies to: 24-30, 40-54, 66-66
src/aiq_agent/agents/deep_researcher/sandbox/README.md (1)
40-50: LGTM!Documentation is consistent with the code changes: per-job creation/attestation semantics, the two independent Landlock knobs, the debug-only shared-attachment escape hatch, and the
artifact.updateevent rename all match the implementation.Also applies to: 65-65, 128-129, 146-151, 160-167, 213-219, 240-277, 300-301, 310-325
src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
90-170: LGTM!Also applies to: 199-217, 272-273, 287-353, 392-401, 534-556
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
36-36: LGTM!Also applies to: 103-104, 146-168, 221-228
scripts/README.md (1)
54-65: LGTM!Also applies to: 84-110, 217-217
scripts/setup_openshell.sh (1)
17-17: LGTM! The<<EOFswitch is injection-safe:$LANDLOCK_COMPATIBILITYis validated tohard_requirement/best_effortbefore emission, and the heredoc header carries no other expandable tokens.Also applies to: 51-51, 61-61, 80-110, 157-164, 880-886, 900-926, 1011-1011, 1063-1063, 1076-1076
scripts/smoke_openshell_isolation.py (1)
1-197: LGTM! Lazy session creation gives real concurrent isolation, thefinallyterminate is idempotent, andraise SystemExit(main())correctly bypasses theexcept ExceptionFAIL handler on the success path.frontends/ui/src/adapters/api/index.ts (1)
96-121: LGTM!frontends/ui/src/features/chat/hooks/use-deep-research.ts (2)
22-22: LGTM!
216-216: 🎯 Functional CorrectnessMerge buffered file updates before flush
buf.files.set(file.filename, file)keeps only the last event per filename, while the live path merges fields inaddDeepResearchFile. Ifartifact.updatecan emit sparse updates for the same file, replayed sessions can drop earliercontentor metadata.frontends/ui/src/features/chat/store.ts (1)
2896-2907: LGTM!frontends/ui/src/features/chat/types.ts (1)
363-372: 🗄️ Data Integrity & Integration | ⚡ Quick winVerify all consumers handle now-optional
content.Making
contentoptional is correct for durable artifacts fetched viacontentUrl, but any renderer that previously assumedcontent: string(e.g.FileCard.tsx, not in this batch) must now branch oncontentUrl/artifactIdpresence instead. Please confirm downstream rendering degrades gracefully (loading/fetch state) whencontentis absent.As per path instructions, "Review UI changes for strict TypeScript behavior, API contract alignment, ... resilient loading and error states."
Source: Path instructions
| // Generated artifacts carry durable metadata; legacy text-file events carry content. | ||
| const raw = artifactData as Record<string, unknown> | ||
| const filePath = (raw.file_path || raw.path || artifactData.url || 'unknown') as string | ||
| const fileName = filePath.split('/').pop() || filePath |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unsafe as string cast on filePath — a non-string file_path/path/url would throw at runtime.
(raw.file_path || raw.path || artifactData.url || 'unknown') as string is a compile-time-only assertion. If the backend ever sends a non-string value for these fields, filePath.split('/') on the next line throws inside the SSE handler.
As per path instructions, frontends/ui/**/* changes should be reviewed for "strict TypeScript behavior."
🛡️ Proposed fix
- const raw = artifactData as Record<string, unknown>
- const filePath = (raw.file_path || raw.path || artifactData.url || 'unknown') as string
+ const raw = artifactData as Record<string, unknown>
+ const rawPath = raw.file_path ?? raw.path ?? artifactData.url
+ const filePath = typeof rawPath === 'string' ? rawPath : 'unknown'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Generated artifacts carry durable metadata; legacy text-file events carry content. | |
| const raw = artifactData as Record<string, unknown> | |
| const filePath = (raw.file_path || raw.path || artifactData.url || 'unknown') as string | |
| const fileName = filePath.split('/').pop() || filePath | |
| // Generated artifacts carry durable metadata; legacy text-file events carry content. | |
| const raw = artifactData as Record<string, unknown> | |
| const rawPath = raw.file_path ?? raw.path ?? artifactData.url | |
| const filePath = typeof rawPath === 'string' ? rawPath : 'unknown' | |
| const fileName = filePath.split('/').pop() || filePath |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontends/ui/src/adapters/api/deep-research-client.ts` around lines 533 -
536, The `filePath` derivation in `deep-research-client.ts` is relying on an
unsafe `as string` assertion, which can let non-string `file_path`/`path`/`url`
values reach `split('/')` and fail in the SSE handler. Update the artifact
parsing logic around the `raw`/`filePath`/`fileName` block to validate each
candidate value is actually a string before using it, and fall back safely to a
default when none are strings. Keep the fix within the same artifact-handling
path so strict TypeScript behavior is preserved without runtime casts.
Source: Path instructions
| todos: null as TodoItem[] | null, | ||
| citations: [] as Array<{ url: string; content: string; isCited: boolean }>, | ||
| files: new Map<string, string>(), // filename -> latest content (deduped) | ||
| files: new Map<string, FileArtifactUpdate>(), // filename -> latest event (deduped) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant hooks first, then inspect the narrowed slices.
ast-grep outline frontends/ui/src/features/chat/hooks/use-load-job-data.ts --view expanded || true
ast-grep outline frontends/ui/src/features/chat/hooks/use-deep-research.ts --view expanded || true
echo "---- use-load-job-data.ts relevant lines ----"
sed -n '340,620p' frontends/ui/src/features/chat/hooks/use-load-job-data.ts
echo "---- use-deep-research.ts relevant lines ----"
sed -n '180,540p' frontends/ui/src/features/chat/hooks/use-deep-research.tsRepository: NVIDIA-AI-Blueprints/aiq
Length of output: 30183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline frontends/ui/src/features/chat/hooks/use-load-job-data.ts --view expanded || true
ast-grep outline frontends/ui/src/features/chat/hooks/use-deep-research.ts --view expanded || true
echo "---- use-load-job-data.ts relevant lines ----"
sed -n '340,620p' frontends/ui/src/features/chat/hooks/use-load-job-data.ts
echo "---- use-deep-research.ts relevant lines ----"
sed -n '180,540p' frontends/ui/src/features/chat/hooks/use-deep-research.tsRepository: NVIDIA-AI-Blueprints/aiq
Length of output: 30183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "type FileArtifactUpdate|interface FileArtifactUpdate|FileArtifactUpdate" frontends/ui/src -g'*.ts' -g'*.tsx'
echo "---- use-load-job-data.ts ----"
sed -n '360,610p' frontends/ui/src/features/chat/hooks/use-load-job-data.ts
echo "---- use-deep-research.ts ----"
sed -n '200,520p' frontends/ui/src/features/chat/hooks/use-deep-research.tsRepository: NVIDIA-AI-Blueprints/aiq
Length of output: 26161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,250p' frontends/ui/src/adapters/api/deep-research-client.ts
echo "---- call sites ----"
rg -n "onFileUpdate\\(" frontends/ui/src -g'*.ts' -g'*.tsx'
echo "---- backend mentions of file artifact updates ----"
rg -n "FileArtifactUpdate|artifact update|onFileUpdate|filename" src frontends/ui/src -g'*.ts' -g'*.tsx'Repository: NVIDIA-AI-Blueprints/aiq
Length of output: 4124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,240p' frontends/ui/src/adapters/api/deep-research-client.ts
echo "---- deep research store actions ----"
rg -n "addDeepResearchFile|deepResearchFiles|setDeepResearchFile|updateDeepResearchFile" frontends/ui/src -g'*.ts' -g'*.tsx'Repository: NVIDIA-AI-Blueprints/aiq
Length of output: 8172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- FileArtifactUpdate definition ----"
sed -n '145,210p' frontends/ui/src/adapters/api/deep-research-client.ts
echo "---- store actions mentioning deepResearchFiles ----"
rg -n "deepResearchFiles|addDeepResearchFile|setDeepResearchFile|updateDeepResearchFile" frontends/ui/src -g'*.ts' -g'*.tsx' -A3 -B3Repository: NVIDIA-AI-Blueprints/aiq
Length of output: 27776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "file\\.filename|onFileUpdate|FileArtifactUpdate|artifact.*file|filename" src frontends/ui/src sources -g'*.py' -g'*.ts' -g'*.tsx'Repository: NVIDIA-AI-Blueprints/aiq
Length of output: 25584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '500,570p' frontends/ui/src/adapters/api/deep-research-client.ts
echo "---- file update tests ----"
sed -n '1,160p' frontends/ui/src/adapters/api/deep-research-client.spec.tsRepository: NVIDIA-AI-Blueprints/aiq
Length of output: 6632
Merge buffered file updates by filename. buffer.files.set(file.filename, file) replaces the prior snapshot, so repeated artifact.update events for the same file can drop earlier content or metadata. Use the same merge-by-filename behavior as addDeepResearchFile in this replay path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontends/ui/src/features/chat/hooks/use-load-job-data.ts` at line 375, The
buffered file replay in useLoadJobData is replacing prior FileArtifactUpdate
entries by filename instead of merging them, so repeated artifact.update events
can lose earlier data. Update the handling around the buffer.files Map in
useLoadJobData to merge updates by file.filename the same way
addDeepResearchFile does, preserving existing content and metadata when a later
event arrives for the same file.
| 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.", | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Deprecated sandbox_name alias never warns.
sandbox_name is documented as deprecated in favor of existing_sandbox_name, but nothing emits a deprecation warning when it's used. Consider a warnings.warn(..., DeprecationWarning) (or a logger.warning) in the validator when sandbox_name is set, so operators still on the old field notice before it's removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiq_agent/agents/deep_researcher/sandbox/config.py` around lines 93 -
100, The deprecated sandbox_name alias in the sandbox config is accepted
silently, so add a deprecation notice when it is provided. Update the validator
logic in the config model around existing_sandbox_name and sandbox_name to emit
a warnings.warn with DeprecationWarning, or an equivalent logger.warning,
whenever sandbox_name is set and then map it through as the fallback for
existing_sandbox_name. Keep the warning tied to the config field handling so
users of the old alias get notified before it is removed.
|
|
||
| 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, | ||
| ) | ||
| # 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") | ||
| 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=oscfg.delete_on_exit, | ||
| ) | ||
|
|
||
| os_sandbox = openshell.Sandbox(**sandbox_kwargs) | ||
| os_sandbox.__enter__() | ||
| 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, | ||
| self._enter_context(os_sandbox) | ||
| backend: BaseSandbox | None = None | ||
| try: | ||
| self._ensure_context_active(os_sandbox) | ||
| self.physical_sandbox_name = getattr(os_sandbox.sandbox, "name", None) | ||
| 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 policy_version=%s shared=%s", | ||
| backend.id, | ||
| getattr(sandbox_ref, "name", None), | ||
| policy_version, | ||
| shared_name is not None, | ||
| ) | ||
| self._emit_attestation(phase=phase, policy_version=policy_version, status="succeeded") | ||
| return backend | ||
| except BaseException: | ||
| self._safe_close(backend) | ||
| self._exit_context() | ||
| raise | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Shell argv check duplicates the config-level validator.
Lines 373-374 re-validate oscfg.shell even though OpenShellProviderConfig._validate_lifecycle_mode (config.py) already enforces non-empty argv at construction time. Harmless belt-and-suspenders, but if you want to trim it, the config validator is the single source of truth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py` around
lines 372 - 432, The initial argv validation in OpenShellProvider.setup
duplicates the non-empty shell check already enforced by
OpenShellProviderConfig._validate_lifecycle_mode, so trim the redundant
oscfg.shell guard from the setup path and rely on the config-level validator as
the single source of truth. Keep the rest of the sandbox initialization flow in
setup unchanged, including the shared_sandbox_name and policy handling branches.
#### Overview Extract the provider-neutral artifact lifecycle and UI delivery work from #305 onto current `develop`, so it can land independently of the OpenShell policy-attestation work in #298. This change: - checkpoints artifact manifests after successful sandbox `execute` calls and performs one idempotent terminal harvest before sandbox teardown; - avoids blocking cancellation on an in-flight provider operation while preserving already checkpointed artifacts; - emits canonical `artifact.update` metadata without embedding artifact bytes or credentials; - delivers live and replayed sandbox artifacts to the Files tab, using the authenticated same-origin artifact endpoint for file access; and - preserves the artifact store selected by current configuration, including the existing SQL and S3-compatible implementations on `develop`. The existing #305 branch is unchanged. This PR contains no OpenShell gateway, policy, attestation, Landlock, or cancellation-API changes. #### Validation ```bash PYTHONPATH=src:frontends/aiq_api/src uv run --no-sync pytest \ tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py \ tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py \ tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py \ tests/aiq_agent/agents/deep_researcher/test_factory.py \ tests/aiq_agent/jobs/test_runner.py -q # 225 passed uv run --no-sync ruff check . # All checks passed uv run --no-sync ruff format --check . # 307 files already formatted cd frontends/ui npm run type-check # passed npm run lint # passed with 0 errors (existing warnings remain) npm run test:ci # 79 files passed; 1,324 tests passed; 1 skipped cd ../../docs make html # succeeded; 3 existing cross-reference/version-switcher warnings ``` - [x] I ran the relevant local checks or explained why they are not applicable. - [x] I added or updated tests for behavior changes. - [x] I updated documentation for user-facing or contributor-facing changes. - [x] I confirmed this PR does not include secrets, credentials, or internal-only data. - [x] I certify this contribution under the Developer Certificate of Origin (DCO) and signed my commits with `git commit -s` or an equivalent sign-off. #### Where should reviewers start? 1. `src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py` for the canonical metadata-only SSE contract. 2. `src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py`, `custom_middleware.py`, and `frontends/aiq_api/src/aiq_api/jobs/runner.py` for checkpoint and terminal-harvest ownership. 3. `frontends/ui/src/adapters/api/deep-research-client.ts` and `frontends/ui/src/features/layout/components/FileCard.tsx` for live/replayed Files-tab delivery. #### Related Issues - Relates to #305 - Independent of #298 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Deep research file artifacts now stream as structured `artifact.update` events, enabling durable file links with richer metadata (MIME, size, hashes, titles/captions, etc.). * File cards can display MIME/size and show an **Open file** button when a durable content URL is available. * **Bug Fixes** * Improved artifact harvesting across success, failure, and cancellation to better preserve completed outputs. * Updated streaming/replay behavior now merges incremental file metadata updates without overwriting earlier content. * **Documentation** * Refreshed architecture and SSE documentation to reflect the updated `artifact.update` payload shape. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Kyle Zheng <kyzheng@nvidia.com> Co-authored-by: Ajay Thorve <AjayThorve@users.noreply.github.com>
#### Overview Extract the provider-neutral artifact lifecycle and UI delivery work from NVIDIA-AI-Blueprints#305 onto current `develop`, so it can land independently of the OpenShell policy-attestation work in NVIDIA-AI-Blueprints#298. This change: - checkpoints artifact manifests after successful sandbox `execute` calls and performs one idempotent terminal harvest before sandbox teardown; - avoids blocking cancellation on an in-flight provider operation while preserving already checkpointed artifacts; - emits canonical `artifact.update` metadata without embedding artifact bytes or credentials; - delivers live and replayed sandbox artifacts to the Files tab, using the authenticated same-origin artifact endpoint for file access; and - preserves the artifact store selected by current configuration, including the existing SQL and S3-compatible implementations on `develop`. The existing NVIDIA-AI-Blueprints#305 branch is unchanged. This PR contains no OpenShell gateway, policy, attestation, Landlock, or cancellation-API changes. #### Validation ```bash PYTHONPATH=src:frontends/aiq_api/src uv run --no-sync pytest \ tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py \ tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py \ tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py \ tests/aiq_agent/agents/deep_researcher/test_factory.py \ tests/aiq_agent/jobs/test_runner.py -q # 225 passed uv run --no-sync ruff check . # All checks passed uv run --no-sync ruff format --check . # 307 files already formatted cd frontends/ui npm run type-check # passed npm run lint # passed with 0 errors (existing warnings remain) npm run test:ci # 79 files passed; 1,324 tests passed; 1 skipped cd ../../docs make html # succeeded; 3 existing cross-reference/version-switcher warnings ``` - [x] I ran the relevant local checks or explained why they are not applicable. - [x] I added or updated tests for behavior changes. - [x] I updated documentation for user-facing or contributor-facing changes. - [x] I confirmed this PR does not include secrets, credentials, or internal-only data. - [x] I certify this contribution under the Developer Certificate of Origin (DCO) and signed my commits with `git commit -s` or an equivalent sign-off. #### Where should reviewers start? 1. `src/aiq_agent/agents/deep_researcher/sandbox/artifacts/models.py` for the canonical metadata-only SSE contract. 2. `src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py`, `custom_middleware.py`, and `frontends/aiq_api/src/aiq_api/jobs/runner.py` for checkpoint and terminal-harvest ownership. 3. `frontends/ui/src/adapters/api/deep-research-client.ts` and `frontends/ui/src/features/layout/components/FileCard.tsx` for live/replayed Files-tab delivery. #### Related Issues - Relates to NVIDIA-AI-Blueprints#305 - Independent of NVIDIA-AI-Blueprints#298 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Deep research file artifacts now stream as structured `artifact.update` events, enabling durable file links with richer metadata (MIME, size, hashes, titles/captions, etc.). * File cards can display MIME/size and show an **Open file** button when a durable content URL is available. * **Bug Fixes** * Improved artifact harvesting across success, failure, and cancellation to better preserve completed outputs. * Updated streaming/replay behavior now merges incremental file metadata updates without overwriting earlier content. * **Documentation** * Refreshed architecture and SSE documentation to reflect the updated `artifact.update` payload shape. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Kyle Zheng <kyzheng@nvidia.com> Co-authored-by: Ajay Thorve <AjayThorve@users.noreply.github.com> Signed-off-by: Tanner Leach <tleach@nvidia.com>
2 participants
Overview
Built on top of #298, wait for that to be merged in first. Associated JIRA
Adds a durable artifact runtime for deep research. Generated files (charts, CSVs,
ResearchNotes) are harvested off the sandbox and stored as BLOBs in the shared on-disk
job DB (
jobs.db), surfaced in the UI, and served via/v1/jobs/.../artifacts/<id>/content.Because the bytes live host-side, artifacts persist after the per-job sandbox container is
deleted and across a full backend restart. Also hardens teardown (sanitized failure logging,
no
sandbox.cleanupevents for non-sandbox jobs) and fixes local-macOS Landlock usability(drop nonexistent
/appfrom the policy; documentbest_effort).For Testing
Start the stack and run a deep research job with a chart prompt (e.g. "compare quarterly CapEx for two companies with a chart"), and wait for the report to finish.
Open View Report in the UI and confirm the chart/figure and any CSV files show up in the report and also under the files tab under the general thinking tab
Note the artifact ID from the report or logs (looks like art_53af6e93...)
3.. The openshell container should be automatically deleted once the job is complete (docker ps will show none afterward), hard-refresh the UI, and confirm the chart still renders in the report and that the generated chart/table still exists under there.
Optional one-liner proof: run sqlite3 jobs.db "select artifact_id, filename, size_bytes, length(content) from artifacts;" and confirm length(content) equals size_bytes (full bytes stored on disk).
Validation
pytestfor the affected suites: finalize/cleanup, OpenShell provider, artifacts, custommiddleware, factory, and job runner.
Durability proof: after a job's OpenShell container was deleted, its artifact bytes remained
in
jobs.db(length(content) == size_bytesfor every row) and the content endpoint stillreturned
200; survives backend restart since it reopens the same on-disk DB.ruff checkon changed Python;bash -n scripts/setup_openshell.sh.I ran the relevant local checks or explained why they are not applicable.
I added or updated tests for behavior changes.
I updated documentation for user-facing or contributor-facing changes.
I confirmed this PR does not include secrets, credentials, or internal-only data.
I certify this contribution under the Developer Certificate of Origin (DCO) and signed my commits with
git commit -sor an equivalent sign-off.Where should reviewers start?
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pyand.../artifacts/store.py(harvest → durable BLOB store), then
deepagents_runtime.finalize(terminal harvest + cleanup).Related Issues
Summary by CodeRabbit
New Features
Bug Fixes