feat: isolate and attest OpenShell jobs#298
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOpenShell deep-research sandboxing now uses per-job policy-bound sandboxes with attestation and allowlist networking. Cleanup now flows through ChangesPer-job OpenShell sandbox and finalize lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
204-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_cleanup_failedis sticky across mid-life retries, causing false-negative terminal cleanup reports.
_safe_close(and its cousin inopenshell.py's_exit_context) setsself._cleanup_failed = Trueon any close exception, but_safe_closeis also invoked from_reset_session()when tearing down a stale session before recreating it on a recoverable error — a mid-job event unrelated to terminal cleanup. Since_cleanup_failedis never reset, a transient failure during that retry path permanently poisonscleanup_succeeded, sofinalize()will report "failed" for a job whose actual terminalclose()succeeded without error. This undermines the truthful-cleanup-reporting goal called out in the PR description.Consider tracking retry-teardown failures separately from terminal-cleanup failures, or resetting
_cleanup_failedat the start of_reset_session()since a fresh session is about to replace the stale one.🤖 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/base.py` around lines 204 - 216, The cleanup failure flag in `_safe_close` is being left sticky across session retries, so a stale-session close error can incorrectly make `cleanup_succeeded` fail later. Update `BaseSandbox._safe_close` and the retry path in `_reset_session()` to distinguish mid-life teardown from final terminal cleanup, or reset `_cleanup_failed` when starting a fresh session replacement. Keep the terminal reporting used by `cleanup_succeeded`/`finalize()` tied only to the actual final close, and apply the same handling to the corresponding `_exit_context` logic in `openshell.py` if it shares the same flag.
🤖 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/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 667-673: In runner.py, the sandbox cleanup path around finalize()
only logs when an exception is raised, so a falsy cleanup result is missed.
Update the finalize handling in the job runner to inspect the return value from
sandbox_runtime.finalize(...) and emit the same warning path when it returns
False or another falsy cleanup_succeeded value, while still keeping exception
handling non-fatal. Use the existing finalize, job_id, and logger.warning call
site to keep the behavior aligned with the current cleanup flow.
In `@src/aiq_agent/agents/deep_researcher/deepagents_runtime.py`:
- Around line 278-298: The finalize() idempotency guard in DeepResearcherRuntime
only protects the _finalized flag, so concurrent calls can skip the actual
cleanup and return a stale success from cleanup_succeeded. Update finalize() to
keep the same lock across the full cleanup path, including terminate()/close(),
the cleanup result read, and the cleanup event emission, so only one caller
performs cleanup and others wait for the real outcome. Use the existing
finalize(), _finalize_lock, _finalized, _emit_cleanup, terminate(), and close()
symbols to make the change.
In `@src/aiq_agent/agents/deep_researcher/register.py`:
- Around line 298-306: Add a unit test in test_agent.py that covers _run
cancellation behavior in DeepResearcher register.py: simulate
asyncio.CancelledError during _run, assert the exception is re-raised, and
verify active_agent.finalize is called with interrupted=True only when
owns_active_agent is true. Use the _run coroutine and the owns_active_agent /
active_agent.finalize path to confirm no finalization happens for non-owned
agents and that the finally block triggers the correct interrupted flag for
owned agents.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 368-423: `_os_context` is being accessed concurrently between
`_create_session()` and out-of-band cleanup paths like `close()`, `terminate()`,
and `_terminate_session()`, which can race during cancellation. Protect all
reads/writes/clears of `self._os_context` with the existing `self._state_lock`,
matching how `self._session` is guarded in the base class, and ensure
`_exit_context()` and the `_create_session()` setup/teardown path in
`OpenShellSandboxProvider` use the same lock consistently.
- Around line 406-424: The attestation success event is emitted too early in
OpenShellSandbox creation, before session construction is guaranteed to succeed.
In openshell.py, adjust the _create_session flow so _attest(self, os_sandbox)
and its “succeeded” emission happen only after OpenShellSandbox(...) is
successfully constructed, or add a failure compensation path if adapter
construction throws. Keep the logging and cleanup around os_sandbox.__enter__
and _exit_context() unchanged, but ensure the attestation state reflects the
final outcome of _create_session rather than the pre-construction step.
In `@tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py`:
- Around line 151-441: Add a regression test around OpenShellSandboxProvider
that exercises _reset_session() after a stale-session close failure, then
performs a later successful terminal close and verifies cleanup_succeeded
remains True. Use the existing _reset_session and close behavior in the
provider/test setup to force the first cleanup failure, then assert the second
close does not stay poisoned by _cleanup_failed.
---
Outside diff comments:
In `@src/aiq_agent/agents/deep_researcher/sandbox/base.py`:
- Around line 204-216: The cleanup failure flag in `_safe_close` is being left
sticky across session retries, so a stale-session close error can incorrectly
make `cleanup_succeeded` fail later. Update `BaseSandbox._safe_close` and the
retry path in `_reset_session()` to distinguish mid-life teardown from final
terminal cleanup, or reset `_cleanup_failed` when starting a fresh session
replacement. Keep the terminal reporting used by
`cleanup_succeeded`/`finalize()` tied only to the actual final close, and apply
the same handling to the corresponding `_exit_context` logic in `openshell.py`
if it shares the same flag.
🪄 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: 63446849-608f-4056-8b4b-58895c90df18
📒 Files selected for processing (16)
configs/config_openshell.ymlconfigs/openshell/aiq-research-policy.yamldocs/source/architecture/agents/sandbox.mdfrontends/aiq_api/src/aiq_api/jobs/runner.pyscripts/README.mdscripts/setup_openshell.shsrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/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_openshell_provider.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
**
⚙️ 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:
scripts/README.mdtests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/agent.pyconfigs/config_openshell.ymlfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyconfigs/openshell/aiq-research-policy.yamldocs/source/architecture/agents/sandbox.mdsrc/aiq_agent/agents/deep_researcher/sandbox/config.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pyscripts/setup_openshell.shsrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.md
**/*.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/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/agent.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/config.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.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/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/register.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/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/register.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.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.md
{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
{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/jobs/runner.py
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
🔇 Additional comments (20)
configs/config_openshell.yml (1)
1-2: LGTM!Also applies to: 142-165
configs/openshell/aiq-research-policy.yaml (1)
24-29: LGTM!scripts/setup_openshell.sh (2)
17-17: LGTM!Also applies to: 51-51, 61-61, 80-80, 93-95, 108-108, 1010-1010, 1062-1062, 1075-1075
155-162: LGTM!Validation of
LANDLOCK_COMPATIBILITYruns inresolve_policy()beforeemit_policy_header()writes it into the now-unquoted heredoc, so only the two whitelisted literals ever reach the generated policy file — no injection risk from the quoting change.Also applies to: 878-884, 898-898, 922-925
docs/source/architecture/agents/sandbox.md (1)
9-11: LGTM!Description of per-job physical sandboxes, attestation, fail-closed network/Landlock behavior, and
sandbox.attestation/sandbox.cleanupevents is consistent with the config and setup-script changes in this cohort.Also applies to: 24-30, 40-46, 58-58
scripts/README.md (1)
54-65: LGTM!Matches the setup script's new default (
CREATE_SANDBOX=false),--landlock-compatibility/--create-shared-debug-sandboxflags, and Landlock defaults.Also applies to: 190-190
src/aiq_agent/agents/deep_researcher/sandbox/README.md (2)
40-50: LGTM!Fail-closed conditions, attestation/policy-revision requirements, debug shared-sandbox opt-in, and troubleshooting guidance are consistent with the per-job sandbox design described across the config, policy, and setup-script changes in this cohort.
Also applies to: 65-65, 160-165, 211-217, 238-254, 277-279, 287-302
122-152: 📐 Maintainability & Code QualityThe sandbox example matches the current schema.
SandboxConfigexposesnetwork: NetworkPolicywithmode/allow, andproviders.openshell.imageis a real field, so this example is consistent with the code.> Likely an incorrect or invalid review comment.src/aiq_agent/agents/deep_researcher/sandbox/config.py (1)
93-152: LGTM!src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
22-22: LGTM!Also applies to: 36-36, 90-127, 136-162, 191-193, 207-208, 495-504, 517-517
src/aiq_agent/agents/deep_researcher/agent.py (1)
155-158: LGTM!src/aiq_agent/agents/deep_researcher/register.py (1)
18-18: LGTM!Also applies to: 240-245, 272-272
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
649-650: LGTM!src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (3)
130-196: LGTM!
199-262: LGTM!Also applies to: 291-292, 359-359
231-243: 🎯 Functional Correctness
openis an explicit unrestricted mode, so_validate_policy_networkonly needs to enforceblockedandallowlist.> Likely an incorrect or invalid review comment.src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
102-103: LGTM!Also applies to: 145-157
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (1)
16-40: LGTM!Also applies to: 68-148, 564-578
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
35-35: LGTM!Also applies to: 306-377
tests/aiq_agent/jobs/test_runner.py (1)
1911-1920: LGTM!
|
/ok to test b6b058e |
|
Final CodeRabbit remediation is in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
390-390: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not delete attached debug sandboxes by default.
This shared-name branch attaches to an existing sandbox, but
oscfg.delete_on_exitnow defaults toTrue, so normal cleanup can delete a sandbox the job did not create. Keep deletion disabled for shared/debug attachment unless there is a separate explicit opt-in.Suggested fix
- sandbox_kwargs.update(sandbox=shared_name, delete_on_exit=oscfg.delete_on_exit) + sandbox_kwargs.update(sandbox=shared_name, delete_on_exit=False)🤖 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` at line 390, The shared-name attachment path in openshell.py is passing through delete_on_exit from oscfg, which can accidentally delete an existing debug sandbox that this job did not create. Update the sandbox_kwargs.update(...) call in the shared-name branch to disable deletion by default for attached sandboxes, and only allow cleanup when there is an explicit opt-in separate from the shared/debug attach flow.
🤖 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.
Outside diff comments:
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Line 390: The shared-name attachment path in openshell.py is passing through
delete_on_exit from oscfg, which can accidentally delete an existing debug
sandbox that this job did not create. Update the sandbox_kwargs.update(...) call
in the shared-name branch to disable deletion by default for attached sandboxes,
and only allow cleanup when there is an explicit opt-in separate from the
shared/debug attach flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 2ad4ae39-8901-4264-be74-600780d237d0
📒 Files selected for processing (9)
frontends/aiq_api/src/aiq_api/jobs/runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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:
frontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py
**
⚙️ 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/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.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/jobs/runner.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/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.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/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py
🔇 Additional comments (9)
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
658-683: LGTM!src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
150-162: LGTM!Also applies to: 194-209, 279-322
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
36-46: LGTM!Also applies to: 271-295, 316-334, 407-430, 432-501, 528-600
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
102-103: LGTM!Also applies to: 145-157, 210-216
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (1)
26-27: LGTM!Also applies to: 37-37, 82-83, 123-130, 400-438, 490-497, 500-529, 532-562, 565-620, 623-674, 814-841
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
20-21: LGTM!Also applies to: 308-330, 332-334, 340-359, 361-380, 381-429
tests/aiq_agent/jobs/test_runner.py (1)
1911-1919: LGTM!Also applies to: 1921-1930
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
292-336: LGTM!scripts/smoke_openshell_isolation.py (1)
29-52: LGTM!Also applies to: 55-72, 75-96, 98-142, 145-154, 156-188
KyleZheng1284
force-pushed
the
codex/aiq-openshell-isolation
branch
from
e5b69ab to
b05c93b
Compare
There was a problem hiding this comment.
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/jobs/runner.py (1)
649-650: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded
event_store.flush()in terminalfinallyblock can mask the job result.Every other cleanup call in this
finallypath (_teardown_sandbox) is deliberately exception-safe, per its own docstring: cleanup must never replace the job result. This newevent_store.flush()call has no try/except — if_flush()raises, the exception propagates out of thefinallyblock and overrides whatever result/exception the job actually produced. It's also invoked synchronously on the event loop rather than viaasyncio.to_threadlike the sandbox teardown right above it, so if_flush()does any blocking I/O it will stall the worker.🛠️ Proposed fix: make flush best-effort and non-blocking
- if event_store is not None and hasattr(event_store, "flush"): - event_store.flush() + if event_store is not None and hasattr(event_store, "flush"): + try: + await asyncio.to_thread(event_store.flush) + except Exception: # noqa: BLE001 - flush must never replace the job result + logger.warning("Event store flush failed for job %s", job_id, exc_info=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/jobs/runner.py` around lines 649 - 650, The new event_store.flush() call in the runner’s terminal finally path should be made best-effort like _teardown_sandbox so it cannot override the job outcome. Update the cleanup in runner.py around the event_store handling to wrap flush in exception-safe handling, and if the flush implementation may block, invoke it off the event loop (consistent with the sandbox teardown pattern) instead of calling it synchronously. Use the existing runner cleanup block and event_store.flush as the locating symbols.src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
403-405: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRespect
delete_on_exitfor job-owned sandboxes.The per-job path always passes
delete_on_exit=True, so a configureddelete_on_exit=Falseis ignored. Keep the shared-attachment override atFalse, but useoscfg.delete_on_exitfor owned sandboxes.Proposed fix
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, )As per path instructions, “Review agent changes for research workflow correctness, graph state transitions, prompt/tool contracts, HITL behavior, and failure handling.”
🤖 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 403 - 405, The sandbox setup in openshell.py is hardcoding delete_on_exit=True for the job-owned path, which overrides the configured behavior. Update the sandbox_kwargs.update call in the owned-sandbox creation flow to use oscfg.delete_on_exit, while keeping the shared-attachment override path explicitly set to False. Make sure the change is applied in the same branch that builds the sandbox spec via _build_sandbox_spec and assigns self.job_id.Source: Path instructions
src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
96-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce the shared-sandbox debug opt-in in this config model.
Line 154 treats
existing_sandbox_name/sandbox_nameas a shared attachment, but the validator never rejectsallow_shared_sandbox=False. Add an explicit check here so the NAT/runtime config cannot accept a non-isolated sandbox unless the debug opt-in is present.Proposed fix
if self.provider == "openshell": 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")As per path instructions, “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.”
Also applies to: 149-157
🤖 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/deepagents_runtime.py` around lines 96 - 106, The shared-sandbox debug opt-in is not enforced in this config model, so a non-isolated attachment can be accepted even when allow_shared_sandbox is false. Add an explicit validation check in the same config class that defines existing_sandbox_name, sandbox_name, and allow_shared_sandbox to reject any use of the shared sandbox fields unless the debug opt-in is enabled. Make sure the validator covers both the primary and deprecated alias fields consistently.Source: Path instructions
🤖 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.
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 649-650: The new event_store.flush() call in the runner’s terminal
finally path should be made best-effort like _teardown_sandbox so it cannot
override the job outcome. Update the cleanup in runner.py around the event_store
handling to wrap flush in exception-safe handling, and if the flush
implementation may block, invoke it off the event loop (consistent with the
sandbox teardown pattern) instead of calling it synchronously. Use the existing
runner cleanup block and event_store.flush as the locating symbols.
In `@src/aiq_agent/agents/deep_researcher/deepagents_runtime.py`:
- Around line 96-106: The shared-sandbox debug opt-in is not enforced in this
config model, so a non-isolated attachment can be accepted even when
allow_shared_sandbox is false. Add an explicit validation check in the same
config class that defines existing_sandbox_name, sandbox_name, and
allow_shared_sandbox to reject any use of the shared sandbox fields unless the
debug opt-in is enabled. Make sure the validator covers both the primary and
deprecated alias fields consistently.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 403-405: The sandbox setup in openshell.py is hardcoding
delete_on_exit=True for the job-owned path, which overrides the configured
behavior. Update the sandbox_kwargs.update call in the owned-sandbox creation
flow to use oscfg.delete_on_exit, while keeping the shared-attachment override
path explicitly set to False. Make sure the change is applied in the same branch
that builds the sandbox spec via _build_sandbox_spec and assigns self.job_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1a4f355e-9777-4ffb-8f9e-c7419c6c4804
📒 Files selected for processing (9)
frontends/aiq_api/src/aiq_api/jobs/runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/jobs/test_runner.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyscripts/smoke_openshell_isolation.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py
**
⚙️ 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:
tests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyscripts/smoke_openshell_isolation.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.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/jobs/runner.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/base.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/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.py
🔇 Additional comments (9)
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
658-683: LGTM! Thefinalizefast-path now logs a warning on a falsycleanup_succeededresult, matching the previously requested fix and theTestTerminalTeardowntest suite (test_runtime_finalizer_false_result_is_logged).src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
36-36: LGTM!Also applies to: 92-95, 108-143, 160-162, 191-209, 279-322
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
36-36: LGTM!Also applies to: 46-46, 132-260, 272-273, 295-295, 316-316, 334-334, 390-392, 408-503, 530-602
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
102-103: LGTM!Also applies to: 145-157, 204-216
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (1)
16-43: LGTM!Also applies to: 71-161, 163-270, 271-705, 828-870
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
20-21: LGTM!Also applies to: 37-37, 307-331, 332-335, 337-429
tests/aiq_agent/jobs/test_runner.py (1)
1911-1919: LGTM!Also applies to: 1921-1931
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
292-337: LGTM!scripts/smoke_openshell_isolation.py (1)
1-52: LGTM!Also applies to: 55-73, 75-96, 98-142, 145-155, 156-184, 185-188, 192-197
KyleZheng1284
force-pushed
the
codex/aiq-openshell-isolation
branch
from
b05c93b to
24264c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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/jobs/runner.py (1)
666-672: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring doesn't mention the new
finalize()-first path.The docstring only describes the terminate()/close() routing, but the primary path (lines 675-682) now delegates to
finalize()when present, falling back to terminate/close only for runtimes without it. Worth a one-line update for future maintainers.📝 Suggested docstring update
"""Release sandbox resources on a terminal path (best-effort, never raises). + Prefers ``finalize(interrupted=...)`` when the runtime exposes it (logs a warning on a + falsy/failed result). Falls back to the legacy routing below for runtimes without a + ``finalize`` method: Interrupted jobs (cancel/timeout) call ``terminate()`` so a still-running ``execute`` is forcibly preempted; normal paths call ``close()`` gracefully. Both are idempotent. This runs off the event loop (``asyncio.to_thread``) so the SDK session close cannot block the worker. """🤖 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/jobs/runner.py` around lines 666 - 672, Update the _teardown_sandbox docstring in runner.py to mention the new finalize()-first behavior before the existing terminate()/close() fallback description. Keep the note brief but explicit that sandbox_runtime.finalize() is used when available, with terminate() for interrupted jobs and close() for the remaining fallback path. Use the _teardown_sandbox symbol so maintainers can quickly find the routing logic.src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
155-156: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize base lifecycle exception logging.
Both event emission and session cleanup are best-effort paths; avoid
exc_info=Trueso secret-bearing callback/SDK details are not written to logs.Suggested fix
- except Exception: # noqa: BLE001 - event persistence is non-critical - logger.warning("Sandbox event emission failed for %s", self.sandbox_name, exc_info=True) + except Exception as exc: # noqa: BLE001 - event persistence is non-critical + logger.warning("Sandbox event emission failed for %s (%s)", self.sandbox_name, type(exc).__name__) @@ - except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + except Exception as exc: # 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) + logger.warning("Sandbox %s cleanup failed (%s)", self.sandbox_name, type(exc).__name__)As per coding guidelines, “Never print or log secret values, including in tool output or error messages.”
Also applies to: 209-211
🤖 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/base.py` around lines 155 - 156, The best-effort exception logging in the sandbox lifecycle currently includes exc_info=True, which can leak secret-bearing callback or SDK details into logs. Update the exception handlers in the base lifecycle methods (including the sandbox event emission path and the related session cleanup path) to log only a sanitized warning message without exception stack traces or raw exception objects, keeping the existing logger.warning calls and using the same identifiers like self.sandbox_name for context.Source: Coding guidelines
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
598-602: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize SDK cleanup exception logging.
The cleanup path should report failure without logging raw SDK exception text or traceback details.
Suggested fix
- except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + except Exception as exc: # 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) + logger.warning( + "OpenShell sandbox %s context cleanup failed (%s)", + self.sandbox_name, + type(exc).__name__, + )As per coding guidelines, “Never print or log secret values, including in tool output or error messages.”
🤖 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 598 - 602, The OpenShell cleanup path in the ctx.__exit__ exception handler is logging raw SDK exception details via exc_info=True, which can expose sensitive data. Update the cleanup logging in openshell.py to report only that sandbox context cleanup failed, without including the exception text or traceback, while still preserving the _cleanup_failed flag and the existing warning in the cleanup flow.Source: Coding guidelines
🤖 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 `@src/aiq_agent/agents/deep_researcher/deepagents_runtime.py`:
- Around line 294-295: The cleanup warning in the terminal exception handler is
logging raw exception details via exc_info=True, which can leak sensitive
SDK/event-sink messages. Update the exception handling in the sandbox cleanup
path around the cleanup logic in deepagents_runtime.py (including the related
block near the other cleanup handler mentioned in the comment) to log only the
exception type or a sanitized summary, and remove full traceback/exception
payload logging from logger.warning while keeping the failure signal.
- Around line 154-156: The shared-sandbox alias handling in DeepAgentsRuntime is
too permissive because self.existing_sandbox_name or self.sandbox_name silently
prefers one value when both are set. Update the validation around shared_name so
DeepAgentsRuntime explicitly detects when existing_sandbox_name and deprecated
sandbox_name are both provided with different values and raises a ValueError
instead of choosing one. Keep the existing allow_shared_sandbox guard, and use
the existing_sandbox_name/sandbox_name checks in the same block to enforce a
single unambiguous sandbox name.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 419-424: The attestation log in the OpenShell sandbox flow is
including an environment-specific gateway value that should not be emitted.
Update the logger.info call in the attestation path of the OpenShell provider to
remove oscfg.gateway from the message and its argument list, while keeping the
remaining identifiers like backend.id, sandbox_ref.name, policy_version, and
shared so the log stays useful without exposing deployment identifiers.
---
Outside diff comments:
In `@frontends/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 666-672: Update the _teardown_sandbox docstring in runner.py to
mention the new finalize()-first behavior before the existing
terminate()/close() fallback description. Keep the note brief but explicit that
sandbox_runtime.finalize() is used when available, with terminate() for
interrupted jobs and close() for the remaining fallback path. Use the
_teardown_sandbox symbol so maintainers can quickly find the routing logic.
In `@src/aiq_agent/agents/deep_researcher/sandbox/base.py`:
- Around line 155-156: The best-effort exception logging in the sandbox
lifecycle currently includes exc_info=True, which can leak secret-bearing
callback or SDK details into logs. Update the exception handlers in the base
lifecycle methods (including the sandbox event emission path and the related
session cleanup path) to log only a sanitized warning message without exception
stack traces or raw exception objects, keeping the existing logger.warning calls
and using the same identifiers like self.sandbox_name for context.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 598-602: The OpenShell cleanup path in the ctx.__exit__ exception
handler is logging raw SDK exception details via exc_info=True, which can expose
sensitive data. Update the cleanup logging in openshell.py to report only that
sandbox context cleanup failed, without including the exception text or
traceback, while still preserving the _cleanup_failed flag and the existing
warning in the cleanup flow.
🪄 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: 870175a1-89e7-4cd7-b6c1-709d76de1100
📒 Files selected for processing (9)
frontends/aiq_api/src/aiq_api/jobs/runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/agents/deep_researcher/test_deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pytests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pyscripts/smoke_openshell_isolation.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.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/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py
**
⚙️ 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:
tests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pytests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pyscripts/smoke_openshell_isolation.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.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/base.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/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.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/jobs/runner.py
🔇 Additional comments (10)
frontends/aiq_api/src/aiq_api/jobs/runner.py (2)
675-682: 🩺 Stability & AvailabilityFalsy
finalize()return now logged — past review comment resolved.This correctly closes the gap flagged previously: a non-raising
finalize()failure is no longer silently dropped.
639-664: LGTM!src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
193-196: LGTM!Also applies to: 210-211, 281-293, 297-303
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
36-46: LGTM!Also applies to: 271-295, 316-334, 390-418, 427-503, 530-597
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
102-103: LGTM!Also applies to: 145-154, 213-216
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (1)
26-27: LGTM!Also applies to: 37-37, 82-83, 123-130, 400-438, 458-498, 525-532, 535-709, 849-876
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
20-21: LGTM!Also applies to: 336-343, 388-437
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
292-337: LGTM!tests/aiq_agent/jobs/test_runner.py (1)
1911-1924: LGTM!Also applies to: 1935-1945
scripts/smoke_openshell_isolation.py (1)
75-95: LGTM!Also applies to: 166-171
KyleZheng1284
force-pushed
the
codex/aiq-openshell-isolation
branch
from
24264c4 to
8e2287b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
597-601: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize OpenShell context cleanup failures.
exc_info=Truecan log raw SDK exception messages during terminal cleanup. Log only the exception type while preserving_cleanup_failed = True. As per coding guidelines, “Never print or log secret values, including in tool output or error messages.”Suggested fix
- except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + except Exception as exc: # 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) + logger.warning( + "OpenShell sandbox %s context cleanup failed (%s)", + self.sandbox_name, + type(exc).__name__, + )🤖 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 597 - 601, The OpenShell cleanup warning in the context exit path is leaking raw exception details via exc_info=True, which can expose sensitive SDK messages. In the ctx.__exit__ exception handler inside the OpenShell sandbox cleanup logic, keep setting _cleanup_failed = True but change the logger.warning call to report only the exception type (using the caught exception object) without full traceback or message details, and keep the context tied to the existing sandbox_name identifier.Source: Coding guidelines
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
149-156: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log raw event-sink or cleanup exceptions.
Both handlers use
exc_info=True, which can emit secret-bearing exception messages from SDKs or persistence sinks. Keep these best-effort paths non-fatal but log onlytype(exc).__name__. As per coding guidelines, “Never print or log secret values, including in tool output or error messages.”Suggested fix
- except Exception: # noqa: BLE001 - event persistence is non-critical - logger.warning("Sandbox event emission failed for %s", self.sandbox_name, exc_info=True) + except Exception as exc: # noqa: BLE001 - event persistence is non-critical + logger.warning("Sandbox event emission failed for %s (%s)", self.sandbox_name, type(exc).__name__) @@ - except Exception: # noqa: BLE001 - cleanup must never raise on the terminal path + except Exception as exc: # 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) + logger.warning("Sandbox %s cleanup failed (%s)", self.sandbox_name, type(exc).__name__)Also applies to: 204-211
🤖 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/base.py` around lines 149 - 156, The best-effort error handlers in _emit_event and the cleanup path are logging exception details with exc_info=True, which can leak secret-bearing messages from SDKs or persistence sinks. Update these handlers to keep failures non-fatal but log only the exception type name (for example via the caught Exception object), and remove traceback/raw exception output from the logger.warning calls while preserving sandbox_name context.Source: Coding guidelines
🤖 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/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 680-681: The sandbox cleanup finalizer in `runner.py` still logs
full tracebacks via `logger.warning(..., exc_info=True)`, which can leak
secret-bearing details from unexpected SDK/event-sink exceptions. Update the
`finalize()` exception handler to log only the exception type/class name (using
the existing `job_id` context) and remove traceback emission so `Sandbox cleanup
failed for job %s` stays sanitized.
In `@tests/aiq_agent/jobs/test_runner.py`:
- Around line 1935-1945: The _teardown_sandbox path in aiq_api.jobs.runner still
logs finalize() exceptions with exc_info=True, which can leak raw exception
messages. Update the exception handling around runtime.finalize() to log only
the exception type or a sanitized summary, and add a regression test alongside
test_runtime_finalizer_false_result_is_logged that raises a RuntimeError with
sensitive text to confirm the warning does not expose the secret.
---
Outside diff comments:
In `@src/aiq_agent/agents/deep_researcher/sandbox/base.py`:
- Around line 149-156: The best-effort error handlers in _emit_event and the
cleanup path are logging exception details with exc_info=True, which can leak
secret-bearing messages from SDKs or persistence sinks. Update these handlers to
keep failures non-fatal but log only the exception type name (for example via
the caught Exception object), and remove traceback/raw exception output from the
logger.warning calls while preserving sandbox_name context.
In `@src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py`:
- Around line 597-601: The OpenShell cleanup warning in the context exit path is
leaking raw exception details via exc_info=True, which can expose sensitive SDK
messages. In the ctx.__exit__ exception handler inside the OpenShell sandbox
cleanup logic, keep setting _cleanup_failed = True but change the logger.warning
call to report only the exception type (using the caught exception object)
without full traceback or message details, and keep the context tied to the
existing sandbox_name identifier.
🪄 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: 30bb7492-02b8-46c4-87b0-b11bb1797bbf
📒 Files selected for processing (9)
frontends/aiq_api/src/aiq_api/jobs/runner.pyscripts/smoke_openshell_isolation.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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_deepagents_runtime.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyscripts/smoke_openshell_isolation.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py
**
⚙️ 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:
tests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/sandbox/base.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyscripts/smoke_openshell_isolation.pytests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/base.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/base.pysrc/aiq_agent/agents/deep_researcher/deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.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/jobs/runner.py
🔇 Additional comments (9)
src/aiq_agent/agents/deep_researcher/deepagents_runtime.py (1)
154-162: LGTM!Also applies to: 202-217, 287-309, 328-329
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.py (1)
36-46: LGTM!Also applies to: 272-295, 316-334, 390-502, 529-596
src/aiq_agent/agents/deep_researcher/sandbox/base.py (1)
102-103: LGTM!Also applies to: 213-216
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
641-663: LGTM!Also applies to: 678-679
scripts/smoke_openshell_isolation.py (1)
75-95: LGTM!Also applies to: 166-189
tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py (1)
26-27: LGTM!Also applies to: 37-37, 82-83, 123-130, 271-318, 411-449, 469-509, 536-720, 860-887
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
292-337: LGTM!tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
20-22: LGTM!Also applies to: 337-352, 398-463
tests/aiq_agent/jobs/test_runner.py (1)
1911-1934: LGTM!
|
/ok to test 9cec3b4 |
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
I found five remaining blockers on the current head.
|
Can we split durable OpenShell provisioning from gateway lifecycle before merging? |
|
Lifecycle follow-up: commit |
|
/ok to test 0c25ad5 |
|
/ok to test 7baf293 |
|
/ok to test 170bdbf |
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
One test-ownership concern on the current head.
| # 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. |
There was a problem hiding this comment.
This is a 300+ line live acceptance test: it imports internal provider helpers, creates real resources, asserts security and lifecycle behavior, and owns cleanup. Keeping that logic under scripts/ makes it look like an operator utility and leaves it outside pytest discovery, markers, and fixtures. Can we move the test implementation to tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_live.py, mark it integration, and gate it with AIQ_OPENSHELL_LIVE_TESTS=1, following the existing OpenSearch live-test pattern? If the one-command entry point is useful, keep a thin wrapper under scripts/, but the assertions and cleanup fixtures should be test-owned.
There was a problem hiding this comment.
Addressed in dc9839c.
All live assertions and resource ownership now live in tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_live.py. The module is marked integration, is gated by AIQ_OPENSHELL_LIVE_TESTS=1, and does not import the optional OpenShell SDK or contact a gateway before the gate. Fixtures own configuration, provider creation, direct sandbox creation, immediate resource registration, reverse-order teardown, and gateway-verified deletion; teardown verification fails the test even if the test body also failed.
scripts/smoke_openshell_isolation.py is now only a subprocess.run() launcher that maps the compatibility flags to the documented environment variables, targets the single pytest module, and propagates pytest's exit code. tests/scripts/test_openshell_smoke_wrapper.py covers argument/environment mapping, best-effort opt-in, target selection, and exit-code propagation.
Local result without opt-in: 10 passed, 3 live tests collected/skipped, with no gateway access. The broader OpenShell regression set is 292 passed, 2 skipped. The required Linux/OpenShell 0.0.72/hard-Landlock live run is still pending, so I am intentionally leaving this thread open until that run and refreshed CI are green.
|
Can we add a canonical OpenShell setup and deployment guide under |
|
Addressed in The guide now owns the operator contract: security boundary; Linux/Docker, macOS demo, Podman evaluation, remote gateway, and Windows/WSL support matrix; pinned CLI/SDK/gateway/adapter compatibility; lifecycle/service ownership; exact Linux, macOS, and remote flows; policy/config and Landlock pairing; expected per-job behavior; pytest-owned acceptance; sanitized inspection; and safe cleanup/troubleshooting. One-way references were added from The normal Sphinx build succeeds and the new guide/backlinks render without warnings. The requested warning-as-error build completes but remains nonzero on two pre-existing |
KyleZheng1284
added
the
BLOCKED
ChatGPT Codex Connector
BLOCKED — upstream OpenShell policy state and SDK capabilitiesMarking this PR blocked for the AI-Q 2.2 OpenShell integration. The policy-status failure has now been reproduced without AI-Q by creating a sandbox directly through the OpenShell 0.0.77 CLI on macOS/Docker Desktop:
AI-Q therefore correctly times out instead of exposing the execution adapter without authoritative The upstream lifecycle/API work is tracked in NVIDIA/OpenShell#2159. That issue also tracks the Python SDK gap for request-level labels and selector-based listing; template/container labels alone do not make Unblock criteria
Until those gates are met, this PR must not claim production acceptance or merge by weakening attestation, treating Current scope note: the policy-state contradiction is confirmed on macOS with OpenShell 0.0.77; a minimal Linux reproduction is still required to determine whether the runtime manifestation is driver/platform-specific. |
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>
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>
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>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
KyleZheng1284
added
AIQ2.2
and removed
BLOCKED
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
KyleZheng1284
force-pushed
the
codex/aiq-openshell-isolation
branch
from
7b0ca2f to
ddc1b6d
Compare
|
/ok to test ddc1b6d |
|
If i run |
| return getattr(message, field, None) is not None | ||
|
|
||
|
|
||
| def _deterministic_policy_hash(policy: Any) -> str: |
There was a problem hiding this comment.
Is it safe that AI-Q is reinventing the recipe to compute the hash? It seems like there could be chances of it drifting from the openshell one, and keeping it up to date would be a lot of maintenance work
There was a problem hiding this comment.
Removed the custom hash calculation and now validating that OpenShell’s effective and revision policies match the submitted policy and report the same non-empty hash.
Missing or mismatched hashes fail closed; if we need local hash verification later we can look into using an official OpenShell SDK helper
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
there are some deps mismatches since the published langchain-openshell pkg |
|
/ok to test d74c86c |
3 participants
Overview
Create and own one physical OpenShell sandbox per AI-Q deep-research job. This change provides per-job isolation, fail-closed policy attestation, discoverable ownership labels, truthful cleanup, authenticated gateway readiness checks, pytest-owned live acceptance, and a canonical operator guide.
The previous upstream blocker is resolved by NVIDIA/OpenShell#2170 and released in OpenShell
0.0.80. AI-Q now requiresopenshell>=0.0.80,<0.1; the setup script defaults to and enforces the0.0.80supported floor. Runtime security decisions remain capability- and state-based rather than branching on an OpenShell version.Key behavior:
aiq=deep-researchand a normalizedaiq-job-idto both OpenShell request metadata and template/container metadata. Active jobs can be queried withopenshell sandbox list --selector aiq=deep-research.allow_shared_sandbox=truedebug configuration. Shared attachment is non-production and does not transfer cleanup ownership to the job.network.allow;allowed_ips/CIDR exceptions are rejected.READY, aLOADEDrevision without a load error, sandbox policy source, matching positive current/active/revision/config versions, structurally identical policies, and matching deterministic hashes.PENDINGaspolicy_status_inconsistent; never weaken attestation because the sandbox is otherwise executable./procbaseline so OpenShell does not enrich the submitted policy and create an unexpected revision.sandbox.attestationandsandbox.cleanupevents without exception messages, policy contents, credentials, SDK response bodies, or tracebacks.develop/PR feat: persist and surface sandbox artifacts #314. Generated files remain job-scoped, receive durable artifact references, and can be rendered or downloaded after the physical sandbox is deleted.Wrote /shared/output.mdwithout creating a non-empty output file. A second false completion fails with the stablewriter_output_missingreason instead of restarting research.setup_openshell.shinstalls the pinned dependencies, generates policy, and builds the image;start_openshell_gateway.shvalidates an authenticated registered or packaged gateway and performs a disposable strict readiness probe;READY,LOADED, effective policy source/content/hash/version, command execution, deletion, and confirmed absence.scripts/smoke_openshell_isolation.pyis now only a thin compatibility launcher.docs/source/deployment/openshell.mdas the canonical operator guide for platform support, ownership, policy/config pairing, startup, acceptance, and troubleshooting.Reviewer Test Instructions
On macOS with Docker Desktop, this is a functional local-demo flow using explicit Landlock
best_effort:Open
http://localhost:3000, create two sessions, and start two concurrent deep-research jobs that request a CSV and PNG chart.While both jobs are running:
Expected behavior:
aiq=deep-researchand distinctaiq-job-idlabels.Run the automated macOS live suite with:
Expected result: all three live isolation/attestation, failure-cleanup/redaction, and shared-policy-mismatch tests pass.
A passing macOS run is functional demo evidence only. Production acceptance requires Linux, Docker, and a policy/config pairing that both require Landlock
hard_requirement, as documented indocs/source/deployment/openshell.md.Validation
Current-head scoped regression suite:
Result: 485 passed, 3 skipped in 6.58s. The three live tests were collected and skipped before optional OpenShell imports or gateway access because live testing was not enabled.
Lint, formatting, and shell syntax:
Result: all checks passed; 53 files already formatted; shell syntax passed.
Configuration validation:
Result: configuration valid.
Documentation:
Result: build succeeded. Sphinx reported two existing unresolved links in
docs/source/integration/agent-skills.md; neither warning originates from the OpenShell documentation.Live OpenShell validation recorded on macOS/Docker Desktop after the
0.0.80rebaseline:Linux/Docker acceptance with Landlock
hard_requirementhas not been run on this macOS host and remains the production acceptance gate.git commit -sor an equivalent sign-off.Where should reviewers start?
src/aiq_agent/agents/deep_researcher/sandbox/providers/openshell.pyscripts/check_openshell_readiness.pyandscripts/start_openshell_gateway.shtests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_live.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pydocs/source/deployment/openshell.mdRelated Issues
0.0.80: NVIDIA/OpenShell#2170