Repository navigation
fix(deep-research): make quantitative artifacts reliable - #373
KyleZheng1284 wants to merge 6 commits into
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:
WalkthroughDeep research adds canonical quantitative dataset validation, writer-only durable artifact publication, bounded chart retries, deterministic report fallback, workflow deadlines, and bounded sandbox teardown. Configurations, prompts, documentation, and regression tests reflect these runtime contracts. ChangesDeep Research Runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/custom_middleware.py`:
- Around line 1123-1137: Update _contains_all_published_canonical_tables to
normalize both baseline and each markdown_table by converting \r\n and \r to \n
before substring matching, matching the normalization used by
_baseline_contains_tables. Preserve the existing published-digest coverage
checks and compare only the normalized values.
🪄 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: dba1af55-9a62-4189-b5bb-2df7e3828da0
📒 Files selected for processing (40)
CHANGELOG.mdREADME.mdconfigs/config_domain_routing_and_skills.ymlconfigs/config_openshell.ymldocs/source/architecture/agents/deep-researcher.mddocs/source/architecture/agents/sandbox.mddocs/source/architecture/data-flow.mddocs/source/customization/configuration-reference.mddocs/source/deployment/openshell.mddocs/source/examples/skills-sandbox/index.mddocs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.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/agents/deep_researcher/tools/test_research.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/jobs/test_runner.py
💤 Files with no reviewable changes (1)
- src/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Script Validation
- GitHub Check: Pytest and Coverage
- GitHub Check: Lint and Hooks
🧰 Additional context used
📓 Path-based instructions (8)
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/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/factory.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.mddocs/source/customization/configuration-reference.mddocs/source/deployment/openshell.mddocs/source/integration/rest-api.mddocs/source/architecture/data-flow.mddocs/source/architecture/agents/deep-researcher.mddocs/source/examples/skills-sandbox/index.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.mddocs/source/customization/configuration-reference.mddocs/source/deployment/openshell.mddocs/source/integration/rest-api.mddocs/source/architecture/data-flow.mddocs/source/architecture/agents/deep-researcher.mdREADME.mddocs/source/examples/skills-sandbox/index.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:
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/agents/deep_researcher/test_factory.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/factory.pyfrontends/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/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/factory.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/config_domain_routing_and_skills.yml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.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
🧠 Learnings (1)
📚 Learning: 2026-07-06T23:55:42.908Z
Learnt from: cdgamarose-nv
Repo: NVIDIA-AI-Blueprints/aiq PR: 311
File: src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2:74-81
Timestamp: 2026-07-06T23:55:42.908Z
Learning: In agent test files (e.g., tests/aiq_agent/agents/*/test_agent.py), avoid brittle assertions that match exact substrings from prompt template files (such as *.j2 prompt wording). Prompt wording can change frequently, so instead assert structural/behavioral properties (e.g., that the prompt builder is called, that required sections/fields are present via stable markers, that the model output/agent behavior conforms to an expected schema, or that key actions are taken) rather than matching literal prompt text.
Applied to files:
tests/aiq_agent/agents/deep_researcher/test_agent.py
🪛 ast-grep (0.44.1)
tests/aiq_agent/fastapi_extensions/test_deep_research.py
[info] 296-308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": csv_path,
"kind": "dataset",
"inline": False,
"expected_sha256": digest,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 368-368: Do not make http calls without encryption
Context: "http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 368-368: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py
[info] 1252-1252: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cls._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1303-1303: use jsonify instead of json.dumps for JSON output
Context: json.dumps(invalid_note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1350-1350: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1397-1397: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1414-1425: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/quarterly_revenue.csv",
"kind": "dataset",
"expected_sha256": self._CSV_DIGEST,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1441-1452: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/unknown.csv",
"kind": "dataset",
"expected_sha256": "a" * 64,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/aiq_agent/agents/deep_researcher/custom_middleware.py
[warning] 151-151: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(boundary_pattern, command)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py
[info] 70-70: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": [entry]})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 325-333: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "table"},
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 350-358: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": digest},
{"path": csv_path, "kind": "dataset", "expected_sha256": other_digest},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 452-460: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
{"path": f"{_ARTIFACT_DIR}/bad.png", "kind": "not-a-kind"},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 471-471: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 580-580: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": entries})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py
[info] 104-104: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": artifacts})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 164-164: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md
[style] ~21-~21: The double modal “Required Input” is nonstandard (only accepted in certain dialects). Consider “to be Input”.
Context: ...ow or value in csv_text. ## Required Input Read every ResearchNotes file and se...
(NEEDS_FIXED)
docs/source/customization/configuration-reference.md
[style] ~495-~495: Consider a different adjective to strengthen your wording.
Context: ... Optional wall-clock deadline for async deep research. When timeout wins the termina...
(DEEP_PROFOUND)
🔇 Additional comments (50)
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.py (1)
24-24: LGTM!Also applies to: 53-53
src/aiq_agent/agents/deep_researcher/sandbox/README.md (1)
171-177: LGTM!tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py (1)
67-71: 📐 Maintainability & Code QualityRun the required Python validation.
Please provide passing Ruff check/format validation and pytest results for these behavior changes. As per coding guidelines, “Run ruff check and ruff format validation for Python code changes” and “Run pytest for all behavior changes in Python code.”
Also applies to: 97-102, 248-259, 277-299, 300-321, 322-343, 344-366, 367-383, 385-402, 403-430, 431-448, 449-477, 478-497, 499-529, 530-543, 545-573, 575-587, 589-603, 604-615, 616-625
Source: Coding guidelines
src/aiq_agent/agents/deep_researcher/tools/research.py (1)
41-98: LGTM!Also applies to: 143-176, 210-253, 300-300, 327-335
tests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.py (1)
56-92: LGTM!Also applies to: 239-318, 320-387
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
66-123: LGTM!Also applies to: 275-283, 315-352, 500-514, 575-588, 1480-1745
tests/aiq_agent/agents/deep_researcher/tools/test_research.py (1)
39-139: LGTM!Also applies to: 142-333
CHANGELOG.md (1)
26-27: LGTM!README.md (1)
69-69: LGTM!Also applies to: 288-288, 464-466
docs/source/architecture/agents/sandbox.md (1)
65-71: LGTM!docs/source/architecture/data-flow.md (1)
44-44: LGTM!Also applies to: 63-69, 187-191
docs/source/deployment/openshell.md (1)
394-395: LGTM!Also applies to: 410-415
docs/source/integration/rest-api.md (1)
288-293: LGTM!tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py (2)
210-338: LGTM!
346-376: LGTM!tests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py (1)
4-146: LGTM!tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py (4)
921-996: LGTM!
1024-1100: LGTM!
1133-1180: LGTM!
1183-1653: LGTM!tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
64-64: LGTM!Also applies to: 233-241
tests/aiq_agent/agents/deep_researcher/test_factory.py (3)
198-253: LGTM!
290-299: LGTM!Also applies to: 378-394
452-489: LGTM!tests/aiq_agent/fastapi_extensions/test_deep_research.py (1)
276-379: LGTM!configs/config_domain_routing_and_skills.yml (1)
172-172: LGTM!Also applies to: 199-199
configs/config_openshell.yml (1)
123-126: LGTM!Also applies to: 164-164
docs/source/architecture/agents/deep-researcher.md (1)
200-201: LGTM!Also applies to: 221-222, 279-327
docs/source/customization/configuration-reference.md (1)
471-473: LGTM!Also applies to: 494-495
src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md (1)
1-181: LGTM!src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.py (1)
46-49: LGTM!Also applies to: 66-79
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py (1)
56-77: LGTM!Also applies to: 210-275, 427-541, 596-736
src/aiq_agent/agents/deep_researcher/custom_middleware.py (1)
58-209: LGTM!Also applies to: 838-1115, 1139-1265
src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2 (1)
18-19: LGTM!Also applies to: 137-137
docs/source/examples/skills-sandbox/index.md (1)
87-109: LGTM!src/aiq_agent/agents/deep_researcher/register.py (1)
112-122: LGTM!Also applies to: 248-248, 284-284
src/aiq_agent/agents/deep_researcher/agent.py (1)
216-458: LGTM!src/aiq_agent/agents/deep_researcher/models/subagent_contracts.py (1)
202-419: LGTM!src/aiq_agent/agents/deep_researcher/models/__init__.py (1)
21-21: LGTM!Also applies to: 38-38
src/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.md (1)
22-50: LGTM!Also applies to: 80-99, 128-136, 182-184
src/aiq_agent/agents/deep_researcher/factory.py (1)
277-304: LGTM!Also applies to: 363-420, 474-630
src/aiq_agent/agents/deep_researcher/prompts/researcher.j2 (1)
54-54: 🎯 Functional CorrectnessPrompt context is already passed through Both prompts get
execution_enabled,sandbox_workdir, andsandbox_artifact_dirfromrender_prompt, so the concern about empty paths or a dropped writer block does not apply.> Likely an incorrect or invalid review comment.frontends/aiq_api/src/aiq_api/jobs/runner.py (6)
48-69: LGTM!
264-282: LGTM!
935-963: Terminal-state race handling is coherent. The inner handler claims the timeout terminal state (flags +_handle_workflow_timeout) before external context managers unwind; the outer handler stays idempotent viatimeout_terminal_recorded; and the generic handler returns early so a late cleanup failure cannot overwrite the stable timeout reason.finallyteardown is correctly gated onsandbox_teardown_started.Also applies to: 1033-1059, 1077-1086, 1120-1122
1183-1370: LGTM!
83-83: LGTM!Also applies to: 989-997, 1414-1414, 1490-1490, 1514-1514, 1555-1576
302-377: 🩺 Stability & AvailabilityValidate
workflow_timeout_secondsat the config boundary.run_with_cancellation()rejects non-finite or<= 0values, so any config path that can pass those through will fail late as a genericValueErrorinstead of being rejected at load time.tests/aiq_agent/jobs/test_runner.py (2)
1593-2235: Solid watchdog coverage. Cancellation-vs-timeout distinction, bounded teardown ordering, CAS claim/preserve semantics, and the secret-leak assertion for the generic path are all exercised with finite waits — no hang risk.
2638-2638: LGTM!Also applies to: 2655-2655, 2667-2667, 2693-2693, 3087-3087, 3220-3221
#### Overview Deep-research currently accepts any non-empty `/shared/output.md` as the final report, including stale planner/orchestrator content from the same state. This prerequisite establishes a small writer-owned commit boundary before the broader convergence/artifact work in #373. - Create one run-local `FinalReportCommitTracker` per `DeepResearcherAgent.run()`. - Deny non-writer `write_file`/`edit_file` mutations of `/shared/output.md` and its route-local `/output.md` state alias with `final_report_writer_only`. - Upsert writer `write_file` calls through the existing backend, refresh the exact UTF-8 SHA-256 after successful edits, and sanitize backend failures as `writer_output_commit_failed`. - Require a successful writer mutation, non-empty current content, and an exact digest match before writer completion or final extraction. - Preserve the existing one-turn correction, then fail closed with `writer_output_not_committed`. - Remove production inline-orchestrator report salvage. - Document that the proof is run-local and byte-exact; it is not persistent across restart and does not claim cross-provider filesystem atomicity. This changes no API fields, configuration schema, database schema, deployment behavior, or sandbox-provider implementation. After this prerequisite merges, #373 can rebase onto `release/2.2` and use its validated ResearchNotes fallback when no committed writer report exists. #### DCO sign-off for the squash commit Signed-off-by: Kyle Zheng <kyzheng@nvidia.com> #### Validation - [x] `uv run ruff check .` — passed - [x] `uv run ruff format --check .` — passed (372 files) - [x] Focused deep-research/job-runner regressions — 265 passed - [x] `uv run pytest` — 1,836 passed, 13 skipped - [x] `pre-commit run --all-files` — all 13 hooks passed, including secrets, lockfiles, links, and notebook output - [x] `cd docs && make html` — passed - [x] Live Modal smoke — `deep_researcher` reached success with a non-empty 12,351-byte report - [x] Live OpenShell 0.0.80 local-demo smoke — strict readiness passed; job reached success with a non-empty 7,360-byte report; attestation and cleanup events were recorded; no owned sandbox remained - [x] I added or updated tests for behavior changes. - [x] I updated documentation for contributor-facing architecture behavior. - [x] I confirmed this PR does not include secrets, credentials, local configs, generated policy files, or internal-only data. - [x] I certify this contribution under the Developer Certificate of Origin (DCO) and signed the commit with `git commit -s`. - [x] I replaced the DCO sign-off placeholder with my GitHub commit identity and kept the required angle brackets. #### Where should reviewers start? 1. `FinalReportCommitTracker`, `FinalReportOwnershipGuardMiddleware`, and `FinalReportCommitMiddleware` in `custom_middleware.py`. 2. Per-run tracker construction/extraction in `agent.py` and role-specific wiring in `factory.py`. 3. The routed `StateBackend`/`CompositeBackend` stale-planner overwrite regression in `test_custom_middleware.py`. #### Related Issues - Prerequisite for #373. - Independent of #372; no artifact-capture configuration is included here. ## Summary by CodeRabbit - **Bug Fixes** - Final research reports are now accepted only after the writer commits `/shared/output.md` (or the supported backend alias) during the current run. - Commit verification is digest-based, rejecting stale/modified/missing/whitespace-only output and detecting any post-write tampering. - Only the designated writer can create/update the final report artifact; completion is blocked until the verified commit occurs. - Removed inline fallback/salvage behavior—if the commit proof isn’t present, runs fail with `writer_output_not_committed`. - **Documentation** - Updated deep-research architecture docs to describe the stricter writer/output commit contract, ownership guardrails, and failure-closed handoffs. - **Tests** - Expanded coverage for digest-verified commit tracking, writer-only ownership enforcement, and updated failure reason/validation scenarios. Authors: - Kyle Zheng (https://github.com/KyleZheng1284) - Ajay Thorve (https://github.com/AjayThorve) Approvers: - Ajay Thorve (https://github.com/AjayThorve) URL: #375
2f51e04 to
515a4cb
Compare
515a4cb to
209feaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/register.py`:
- Around line 254-255: Update the async deep-research factory registration to
forward config.max_researcher_execute_attempts alongside
max_writer_execute_attempts. Ensure the use_async_deep_research job path
receives the configured researcher retry limit instead of relying on the
default.
In `@src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py`:
- Around line 73-76: The helper _manifest_error_is_not_found must stop inferring
absence from arbitrary normalized error text or substring markers. Update the
download_files error flow to use a structured not-found signal, or match only
the backend’s explicit not-found enum, while preserving fail-closed behavior for
unreadable or other backend failures and only enabling scan fallback for
confirmed absence.
In
`@src/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.md`:
- Around line 47-50: Update the dataset constraints in SKILL.md to document the
aggregate per-note CSV limit enforced by
ResearchNotes._validate_quantitative_dataset_references: all datasets in one
note together must not exceed 128 KiB of UTF-8 CSV, in addition to the existing
per-dataset limits.
🪄 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: 1c213ad3-d2d4-4670-88a1-67ce19674aec
📒 Files selected for processing (40)
CHANGELOG.mdREADME.mdconfigs/config_domain_routing_and_skills.ymlconfigs/config_openshell.ymldocs/source/architecture/agents/deep-researcher.mddocs/source/architecture/agents/sandbox.mddocs/source/architecture/data-flow.mddocs/source/customization/configuration-reference.mddocs/source/deployment/openshell.mddocs/source/examples/skills-sandbox/index.mddocs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.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/agents/deep_researcher/tools/test_research.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/jobs/test_runner.py
💤 Files with no reviewable changes (1)
- src/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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:
src/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/fastapi_extensions/test_deep_research.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pytests/aiq_agent/jobs/test_runner.pysrc/aiq_agent/agents/deep_researcher/agent.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pytests/aiq_agent/agents/deep_researcher/test_agent.pysrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.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/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.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/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.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.mddocs/source/deployment/openshell.mddocs/source/integration/rest-api.mddocs/source/architecture/agents/deep-researcher.mddocs/source/customization/configuration-reference.mddocs/source/architecture/data-flow.mddocs/source/examples/skills-sandbox/index.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.mddocs/source/deployment/openshell.mddocs/source/integration/rest-api.mddocs/source/architecture/agents/deep-researcher.mddocs/source/customization/configuration-reference.mdREADME.mddocs/source/architecture/data-flow.mddocs/source/examples/skills-sandbox/index.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_domain_routing_and_skills.ymlconfigs/config_openshell.yml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.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
🧠 Learnings (1)
📚 Learning: 2026-07-06T23:55:42.908Z
Learnt from: cdgamarose-nv
Repo: NVIDIA-AI-Blueprints/aiq PR: 311
File: src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2:74-81
Timestamp: 2026-07-06T23:55:42.908Z
Learning: In agent test files (e.g., tests/aiq_agent/agents/*/test_agent.py), avoid brittle assertions that match exact substrings from prompt template files (such as *.j2 prompt wording). Prompt wording can change frequently, so instead assert structural/behavioral properties (e.g., that the prompt builder is called, that required sections/fields are present via stable markers, that the model output/agent behavior conforms to an expected schema, or that key actions are taken) rather than matching literal prompt text.
Applied to files:
tests/aiq_agent/agents/deep_researcher/test_agent.py
🪛 ast-grep (0.44.1)
tests/aiq_agent/fastapi_extensions/test_deep_research.py
[warning] 368-368: Do not make http calls without encryption
Context: "http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 368-368: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
[info] 296-308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": csv_path,
"kind": "dataset",
"inline": False,
"expected_sha256": digest,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py
[info] 104-104: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": artifacts})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 164-164: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py
[info] 1559-1559: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cls._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1608-1608: use jsonify instead of json.dumps for JSON output
Context: json.dumps(invalid_note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1685-1685: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1732-1732: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1749-1760: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/quarterly_revenue.csv",
"kind": "dataset",
"expected_sha256": self._CSV_DIGEST,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1776-1787: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/unknown.csv",
"kind": "dataset",
"expected_sha256": "a" * 64,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1803-1808: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [{"path": "/shared/aiq-artifacts/report.pdf", "kind": "document"}],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py
[info] 70-70: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": [entry]})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 366-374: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "table"},
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 391-399: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": digest},
{"path": csv_path, "kind": "dataset", "expected_sha256": other_digest},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 493-501: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
{"path": f"{_ARTIFACT_DIR}/bad.png", "kind": "not-a-kind"},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 512-512: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 621-621: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": entries})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/aiq_agent/agents/deep_researcher/custom_middleware.py
[warning] 177-177: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(boundary_pattern, command)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
🪛 LanguageTool
src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md
[style] ~21-~21: The double modal “Required Input” is nonstandard (only accepted in certain dialects). Consider “to be Input”.
Context: ...ow or value in csv_text. ## Required Input Read every ResearchNotes file and se...
(NEEDS_FIXED)
docs/source/customization/configuration-reference.md
[style] ~497-~497: Consider a different adjective to strengthen your wording.
Context: ... Optional wall-clock deadline for async deep research. When timeout wins the termina...
(DEEP_PROFOUND)
🔇 Additional comments (44)
CHANGELOG.md (1)
26-27: LGTM!README.md (1)
69-69: LGTM!Also applies to: 288-288, 464-466
docs/source/architecture/agents/sandbox.md (1)
12-13: LGTM!Also applies to: 66-72
src/aiq_agent/agents/deep_researcher/sandbox/README.md (1)
156-156: LGTM!Also applies to: 172-178
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (3)
58-70: LGTM!
73-96: LGTM!
258-266: LGTM!Also applies to: 363-390
tests/aiq_agent/fastapi_extensions/test_deep_research.py (2)
55-60: LGTM!
276-381: LGTM!docs/source/architecture/data-flow.md (1)
44-69: LGTM!Also applies to: 187-192
docs/source/deployment/openshell.md (1)
120-138: LGTM!Also applies to: 190-190, 407-428
docs/source/integration/rest-api.md (1)
288-293: LGTM!tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py (1)
56-105: LGTM!Also applies to: 169-196, 210-338, 346-376
tests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py (1)
4-63: LGTM!Also applies to: 65-147
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py (1)
18-56: LGTM!Also applies to: 294-472, 475-649, 1162-1341, 1374-1488, 1490-2022
tests/aiq_agent/agents/deep_researcher/test_factory.py (1)
24-56: LGTM!Also applies to: 116-141, 204-264, 301-334, 376-448, 470-543
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.py (1)
46-49: LGTM!Also applies to: 66-79
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py (1)
56-72: LGTM!Also applies to: 210-299, 424-565, 592-757, 759-802
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.py (1)
24-24: LGTM!Also applies to: 53-53
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
48-69: LGTM!Also applies to: 83-83, 264-281, 298-377, 935-963, 989-997, 1033-1122, 1183-1189, 1212-1368, 1414-1414, 1490-1490, 1514-1514, 1555-1575
tests/aiq_agent/jobs/test_runner.py (1)
73-73: LGTM!Also applies to: 1594-2235, 2639-2694, 3088-3088, 3221-3222
tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py (1)
67-71: LGTM!Also applies to: 97-102, 248-300, 318-681
src/aiq_agent/agents/deep_researcher/custom_middleware.py (2)
1422-1441: Newline normalization now applied to both baseline and tables — the prior CRLF-mismatch concern is resolved.
65-116: LGTM!Also applies to: 131-235, 1264-1525
configs/config_domain_routing_and_skills.yml (1)
172-172: LGTM!Also applies to: 186-189, 203-204
configs/config_openshell.yml (1)
123-126: LGTM!Also applies to: 165-166
docs/source/architecture/agents/deep-researcher.md (1)
207-209: LGTM!Also applies to: 229-231
src/aiq_agent/agents/deep_researcher/prompts/writer.j2 (1)
6-17: LGTM!Also applies to: 60-73, 97-126, 147-148
src/aiq_agent/agents/deep_researcher/register.py (1)
113-128: LGTM!src/aiq_agent/agents/deep_researcher/factory.py (1)
296-320: LGTM!Also applies to: 373-431, 484-542, 604-653
src/aiq_agent/agents/deep_researcher/agent.py (1)
222-245: LGTM!Also applies to: 247-336, 337-456
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
286-296: LGTM!Also applies to: 344-370, 518-532, 593-606, 1573-1802
tests/aiq_agent/agents/deep_researcher/tools/test_research.py (1)
90-141: LGTM!Also applies to: 143-159, 251-326, 339-345
docs/source/customization/configuration-reference.md (1)
471-474: LGTM!Also applies to: 495-497
docs/source/examples/skills-sandbox/index.md (1)
9-11: LGTM!Also applies to: 26-31, 74-74, 95-95, 109-118, 128-130, 166-172, 182-191, 224-231, 236-256
src/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.md (1)
22-46: LGTM!Also applies to: 80-99, 128-136, 182-184
src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md (1)
1-181: LGTM!src/aiq_agent/agents/deep_researcher/models/subagent_contracts.py (1)
18-44: LGTM!Also applies to: 202-367, 383-419
src/aiq_agent/agents/deep_researcher/models/__init__.py (1)
21-21: LGTM!Also applies to: 38-38
src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2 (1)
18-19: LGTM!Also applies to: 137-137
src/aiq_agent/agents/deep_researcher/prompts/researcher.j2 (1)
46-55: LGTM!Also applies to: 80-81
src/aiq_agent/agents/deep_researcher/tools/research.py (2)
25-99: LGTM!Also applies to: 144-178, 212-263, 310-310
337-345: 🗄️ Data Integrity & IntegrationNo issue: this branch is unreachable from the current wiring.
artifact_manageris only created when a sandbox provider andartifact_db_urlare present, andcontext.backendalways resolves to a concrete backend (StateBackendorCompositeBackend), neverNone.> Likely an incorrect or invalid review comment.tests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.py (1)
18-26: LGTM!Also applies to: 56-92, 239-318, 320-387
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/agent.py (1)
273-276: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when a published digest has no validated dataset.
Filtering candidates drops unmatched published digests;
required_tablesthen becomes empty and Line 241 accepts a committed report without any canonical table. Treat a published digest missing from validatedResearchNotesas an invalid writer output/fallback state instead of silently accepting it.🤖 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/agent.py` around lines 273 - 276, Update the candidate filtering flow around _published_canonical_digests so every published digest must match a validated ResearchNotes dataset. Detect unmatched published digests before allowing required_tables to be evaluated, and mark the writer output as invalid or trigger the existing fallback path instead of proceeding with an empty requirement. Preserve normal filtering when all published digests have validated candidates.
🤖 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/agent.py`:
- Around line 273-276: Update the candidate filtering flow around
_published_canonical_digests so every published digest must match a validated
ResearchNotes dataset. Detect unmatched published digests before allowing
required_tables to be evaluated, and mark the writer output as invalid or
trigger the existing fallback path instead of proceeding with an empty
requirement. Preserve normal filtering when all published digests have validated
candidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1c26ae70-b6ca-4203-b042-44a5e95c7a9b
📒 Files selected for processing (15)
configs/config_domain_routing_and_skills.ymlconfigs/config_openshell.ymldocs/source/architecture/agents/deep-researcher.mddocs/source/customization/configuration-reference.mddocs/source/examples/skills-sandbox/index.mdsrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.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.py
💤 Files with no reviewable changes (3)
- configs/config_domain_routing_and_skills.yml
- tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py
- configs/config_openshell.yml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
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/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.py
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/customization/configuration-reference.mddocs/source/architecture/agents/deep-researcher.mddocs/source/examples/skills-sandbox/index.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/customization/configuration-reference.mddocs/source/architecture/agents/deep-researcher.mddocs/source/examples/skills-sandbox/index.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:
src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/agent.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.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/register.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.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_custom_middleware.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py
🧠 Learnings (1)
📚 Learning: 2026-07-06T23:55:42.908Z
Learnt from: cdgamarose-nv
Repo: NVIDIA-AI-Blueprints/aiq PR: 311
File: src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2:74-81
Timestamp: 2026-07-06T23:55:42.908Z
Learning: In agent test files (e.g., tests/aiq_agent/agents/*/test_agent.py), avoid brittle assertions that match exact substrings from prompt template files (such as *.j2 prompt wording). Prompt wording can change frequently, so instead assert structural/behavioral properties (e.g., that the prompt builder is called, that required sections/fields are present via stable markers, that the model output/agent behavior conforms to an expected schema, or that key actions are taken) rather than matching literal prompt text.
Applied to files:
tests/aiq_agent/agents/deep_researcher/test_agent.py
🔇 Additional comments (12)
tests/aiq_agent/agents/deep_researcher/test_factory.py (1)
24-42: LGTM!Also applies to: 56-56, 205-205, 214-287, 324-333, 455-471, 529-566
tests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py (1)
4-23: LGTM!Also applies to: 25-39, 41-54, 56-63, 65-101, 103-124, 126-146, 148-167
src/aiq_agent/agents/deep_researcher/prompts/researcher.j2 (1)
46-48: LGTM!Also applies to: 52-57, 82-83
docs/source/architecture/agents/deep-researcher.md (1)
207-209: LGTM!Also applies to: 234-257, 342-369
docs/source/customization/configuration-reference.md (1)
471-474: LGTM!Also applies to: 495-510
docs/source/examples/skills-sandbox/index.md (1)
9-11: LGTM!Also applies to: 26-31, 74-74, 108-117, 127-129, 165-171, 181-190, 223-255
src/aiq_agent/agents/deep_researcher/register.py (1)
113-128: LGTM!Also applies to: 254-255, 291-292
src/aiq_agent/agents/deep_researcher/factory.py (1)
147-147: LGTM!Also applies to: 215-233, 280-315, 374-415, 424-431, 485-508, 561-652
src/aiq_agent/agents/deep_researcher/custom_middleware.py (1)
1280-1285: LGTM!Also applies to: 1436-1441, 1454-1525, 1535-1561
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py (1)
1181-1235: LGTM!Also applies to: 1281-1421, 1479-1487, 1928-1938
src/aiq_agent/agents/deep_researcher/agent.py (1)
56-57: LGTM!Also applies to: 94-219
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
286-369: LGTM!Also applies to: 518-606, 1573-1843
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>
1da07f1 to
7f72f47
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/models/subagent_contracts.py`:
- Around line 332-366: Update _validate_csv_quote_placement to handle the
after_quote state with an explicit state == after_quote branch before its
transitions. Preserve the existing behavior: a following quote returns to
quoted, a delimiter or line break starts a new field, and any other character
raises ValueError.
In `@src/aiq_agent/agents/deep_researcher/sandbox/README.md`:
- Around line 174-178: Clarify the checkpointing paragraph in the README so
“terminal finalization” explicitly applies to non-cancelled success or failure,
while cancellation is an exception: a busy sandbox may skip the manifest and
directory scan and request immediate termination. Preserve the existing behavior
for an immediately available provider operation lease and retained writer
outputs.
In `@tests/aiq_agent/fastapi_extensions/test_deep_research.py`:
- Around line 314-322: Update the download_files stub’s missing-file error value
to the structured "file_not_found" token expected by
manager._manifest_error_is_not_found, matching the sibling test doubles. Keep
successful file entries unchanged while ensuring absent paths produce the exact
not-found signal.
🪄 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: 673c8fd9-95a2-4cfe-9a34-e1cf1f9c592f
📒 Files selected for processing (40)
CHANGELOG.mdREADME.mdconfigs/config_domain_routing_and_skills.ymlconfigs/config_openshell.ymldocs/source/architecture/agents/deep-researcher.mddocs/source/architecture/agents/sandbox.mddocs/source/architecture/data-flow.mddocs/source/customization/configuration-reference.mddocs/source/deployment/openshell.mddocs/source/examples/skills-sandbox/index.mddocs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.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/agents/deep_researcher/tools/test_research.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/jobs/test_runner.py
💤 Files with no reviewable changes (1)
- src/aiq_agent/agents/deep_researcher/skills/research/chart-generation/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.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:
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/test_agent.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/tools/research.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/deep_researcher/test_custom_middleware.pytests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pytests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.pysrc/aiq_agent/agents/deep_researcher/factory.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/__init__.pysrc/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.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/__init__.pysrc/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2src/aiq_agent/agents/deep_researcher/prompts/researcher.j2src/aiq_agent/agents/deep_researcher/models/__init__.pysrc/aiq_agent/agents/deep_researcher/sandbox/README.mdsrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.pysrc/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.mdsrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.mdsrc/aiq_agent/agents/deep_researcher/models/subagent_contracts.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/tools/research.pysrc/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.pysrc/aiq_agent/agents/deep_researcher/custom_middleware.pysrc/aiq_agent/agents/deep_researcher/factory.py
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update the docs under docs/source/ when behavior, configuration, or workflows change
Files:
docs/source/integration/rest-api.mddocs/source/architecture/agents/sandbox.mddocs/source/architecture/data-flow.mddocs/source/deployment/openshell.mddocs/source/architecture/agents/deep-researcher.mddocs/source/customization/configuration-reference.mddocs/source/examples/skills-sandbox/index.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/integration/rest-api.mddocs/source/architecture/agents/sandbox.mdREADME.mddocs/source/architecture/data-flow.mddocs/source/deployment/openshell.mddocs/source/architecture/agents/deep-researcher.mddocs/source/customization/configuration-reference.mddocs/source/examples/skills-sandbox/index.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_domain_routing_and_skills.ymlconfigs/config_openshell.yml
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/fastapi_extensions/test_deep_research.pytests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/deep_researcher/tools/test_research.pytests/aiq_agent/agents/deep_researcher/test_factory.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.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_artifact_publication_regression.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
🧠 Learnings (1)
📚 Learning: 2026-07-06T23:55:42.908Z
Learnt from: cdgamarose-nv
Repo: NVIDIA-AI-Blueprints/aiq PR: 311
File: src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2:74-81
Timestamp: 2026-07-06T23:55:42.908Z
Learning: In agent test files (e.g., tests/aiq_agent/agents/*/test_agent.py), avoid brittle assertions that match exact substrings from prompt template files (such as *.j2 prompt wording). Prompt wording can change frequently, so instead assert structural/behavioral properties (e.g., that the prompt builder is called, that required sections/fields are present via stable markers, that the model output/agent behavior conforms to an expected schema, or that key actions are taken) rather than matching literal prompt text.
Applied to files:
tests/aiq_agent/agents/deep_researcher/test_agent.py
🪛 ast-grep (0.44.1)
tests/aiq_agent/fastapi_extensions/test_deep_research.py
[info] 296-308: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": csv_path,
"kind": "dataset",
"inline": False,
"expected_sha256": digest,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[warning] 368-368: Do not make http calls without encryption
Context: "http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 368-368: Configuring an LLM/agent client endpoint over http:// sends prompts and responses (and often API keys) in cleartext, exposing them to interception. Use https for the base_url.
Context: base_url="http://test"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(llm-client-insecure-http-python)
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py
[info] 1559-1559: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cls._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1608-1608: use jsonify instead of json.dumps for JSON output
Context: json.dumps(invalid_note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1685-1685: use jsonify instead of json.dumps for JSON output
Context: json.dumps(self._research_note())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1732-1732: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1749-1760: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/quarterly_revenue.csv",
"kind": "dataset",
"expected_sha256": self._CSV_DIGEST,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1776-1787: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{
"path": "/shared/aiq-artifacts/unknown.csv",
"kind": "dataset",
"expected_sha256": "a" * 64,
}
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 1803-1808: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [{"path": "/shared/aiq-artifacts/report.pdf", "kind": "document"}],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py
[info] 70-70: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": [entry]})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 366-374: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "table"},
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 391-399: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": digest},
{"path": csv_path, "kind": "dataset", "expected_sha256": other_digest},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 495-503: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"version": 1,
"artifacts": [
{"path": csv_path, "kind": "dataset", "expected_sha256": "a" * 64},
{"path": f"{_ARTIFACT_DIR}/bad.png", "kind": "not-a-kind"},
],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 514-514: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": []})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 623-623: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": entries})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/aiq_agent/agents/deep_researcher/custom_middleware.py
[warning] 177-177: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling.
Context: re.search(boundary_pattern, command)
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-non-literal-regex-python)
tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py
[info] 108-108: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"version": 1, "artifacts": artifacts})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 168-168: use jsonify instead of json.dumps for JSON output
Context: json.dumps(note)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md
[style] ~21-~21: The double modal “Required Input” is nonstandard (only accepted in certain dialects). Consider “to be Input”.
Context: ...ow or value in csv_text. ## Required Input Read every ResearchNotes file and se...
(NEEDS_FIXED)
docs/source/customization/configuration-reference.md
[style] ~497-~497: Consider a different adjective to strengthen your wording.
Context: ... Optional wall-clock deadline for async deep research. When timeout wins the termina...
(DEEP_PROFOUND)
🔇 Additional comments (51)
configs/config_domain_routing_and_skills.yml (1)
170-172: LGTM!configs/config_openshell.yml (1)
122-126: LGTM!docs/source/architecture/agents/deep-researcher.md (1)
207-209: LGTM!Also applies to: 234-257, 309-363
src/aiq_agent/agents/deep_researcher/prompts/writer.j2 (1)
6-17: LGTM!Also applies to: 60-73, 97-126, 147-148
src/aiq_agent/agents/deep_researcher/register.py (2)
113-128: LGTM!
254-255: LGTM!Also applies to: 291-292
src/aiq_agent/agents/deep_researcher/tools/research.py (3)
51-98: LGTM!
144-178: LGTM!
212-263: LGTM!Also applies to: 337-345
tests/aiq_agent/agents/deep_researcher/tools/test_research.py (1)
90-345: LGTM!tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
286-370: LGTM!Also applies to: 518-532, 593-606, 1573-1882
frontends/aiq_api/src/aiq_api/jobs/runner.py (4)
48-69: LGTM!Also applies to: 265-283
303-377: LGTM!
936-964: LGTM!Also applies to: 990-994, 1034-1123
1184-1371: LGTM!Also applies to: 1557-1577
tests/aiq_agent/jobs/test_runner.py (1)
1594-2235: LGTM!Also applies to: 2639-2698, 3092-3093, 3226-3227
docs/source/customization/configuration-reference.md (1)
471-510: LGTM!docs/source/examples/skills-sandbox/index.md (1)
9-256: LGTM!src/aiq_agent/agents/deep_researcher/skills/research/data-table-analysis/SKILL.md (1)
9-240: LGTM! The previously-flagged missing aggregate CSV limit documentation is now present (line 51).src/aiq_agent/agents/deep_researcher/skills/synthesis/chart-generation/SKILL.md (1)
1-181: LGTM!src/aiq_agent/agents/deep_researcher/models/subagent_contracts.py (2)
202-324: LGTM! Digest computation, freezing, and CSV structural validation are consistent with the trust-boundary requirements described incustom_middleware.py/research.py.
383-419: LGTM!src/aiq_agent/agents/deep_researcher/models/__init__.py (1)
21-38: LGTM!src/aiq_agent/agents/deep_researcher/prompts/orchestrator.j2 (1)
18-20: LGTM! Consistent with the writer-exclusivesandbox_artifact_dirownership contract enforced inresearcher.j2and the SKILL.md files.Also applies to: 137-137
src/aiq_agent/agents/deep_researcher/prompts/researcher.j2 (1)
46-57: LGTM!Also applies to: 82-83
tests/aiq_agent/agents/deep_researcher/models/test_subagent_contracts.py (1)
18-92: LGTM! Boundary cases (exact 64KiB/128KiB, exact row/column limits, CRLF/Unicode CSV, quote-escaping) are well covered and match the validator constants.Also applies to: 239-387
src/aiq_agent/agents/deep_researcher/custom_middleware.py (2)
1422-1441: CRLF normalization is now applied to both baseline and eachmarkdown_tablehere (matching_baseline_contains_tables), which addresses the earlier nit.
65-253: LGTM!Also applies to: 1030-1262, 1264-1561
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/__init__.py (1)
24-24: LGTM!Also applies to: 53-53
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manifest.py (1)
46-49: LGTM!Also applies to: 66-79
src/aiq_agent/agents/deep_researcher/sandbox/artifacts/manager.py (2)
208-276: LGTM!Also applies to: 449-514, 611-755
72-74: 🩺 Stability & AvailabilityConfirm Modal’s missing-file error literal before relying on exact-match fallback. OpenShell maps absent files to
file_not_found, but Modal’sdownload_fileserror shape still needs a check; if it uses a different token, manifest-less jobs will fail closed instead of scanning.CHANGELOG.md (1)
26-27: LGTM!README.md (1)
69-69: LGTM!Also applies to: 288-288, 464-466
tests/aiq_agent/fastapi_extensions/test_deep_research.py (1)
55-60: LGTM!Also applies to: 276-313, 323-381
tests/aiq_agent/agents/deep_researcher/test_custom_middleware.py (1)
1162-1236: LGTM!Also applies to: 1265-1341, 1374-1488, 1490-2034
tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py (1)
58-58: LGTM!Also applies to: 67-71, 248-300, 318-424, 426-520, 521-631, 632-683
tests/aiq_agent/agents/deep_researcher/test_artifact_publication_regression.py (1)
56-134: LGTM!Also applies to: 173-341, 344-380
src/aiq_agent/agents/deep_researcher/factory.py (3)
147-147: LGTM!Also applies to: 283-315, 485-488, 508-508, 561-561, 594-594, 605-610, 623-623, 652-652
424-431: LGTM!
374-406: 🩺 Stability & Availability
ModelRetryMiddleware.retry_onalready accepts callables. The copied middleware can use_exclude_structured_output_from_model_retries(...)as-is; there’s noisinstancemismatch here.> Likely an incorrect or invalid review comment.src/aiq_agent/agents/deep_researcher/agent.py (3)
40-69: LGTM!Also applies to: 94-127, 219-219
229-248: LGTM!
250-466: LGTM!tests/aiq_agent/agents/deep_researcher/test_factory.py (1)
204-287: LGTM!Also applies to: 324-333, 455-471, 529-566
docs/source/architecture/agents/sandbox.md (1)
66-72: LGTM!docs/source/architecture/data-flow.md (1)
44-44: LGTM!Also applies to: 63-69, 187-191
docs/source/deployment/openshell.md (1)
407-408: LGTM!Also applies to: 423-428
docs/source/integration/rest-api.md (1)
307-312: LGTM!tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
254-263: LGTM!tests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py (1)
4-101: LGTM!Also applies to: 103-167
| def _validate_csv_quote_placement(csv_text: str) -> None: | ||
| """Reject quote placement that ``csv.reader(strict=True)`` accepts leniently. | ||
|
|
||
| The standard-library parser treats a quote inside an unquoted field as a | ||
| literal character even in strict mode. Such input is not portable CSV and | ||
| downstream readers may interpret it differently. This state check inspects | ||
| the original text without normalizing or reserializing it. | ||
| """ | ||
| field_start = "field_start" | ||
| unquoted = "unquoted" | ||
| quoted = "quoted" | ||
| after_quote = "after_quote" | ||
| state = field_start | ||
|
|
||
| for character in csv_text: | ||
| if state == field_start: | ||
| if character == '"': | ||
| state = quoted | ||
| elif character not in {",", "\r", "\n"}: | ||
| state = unquoted | ||
| elif state == unquoted: | ||
| if character == '"': | ||
| raise ValueError("csv_text contains malformed quote placement") | ||
| if character in {",", "\r", "\n"}: | ||
| state = field_start | ||
| elif state == quoted: | ||
| if character == '"': | ||
| state = after_quote | ||
| elif character == '"': | ||
| state = quoted | ||
| elif character in {",", "\r", "\n"}: | ||
| state = field_start | ||
| else: | ||
| raise ValueError("csv_text contains malformed quote placement") | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Quote-placement state machine implicitly relies on state elimination for after_quote.
The after_quote state is never checked by name (state == "after_quote"); its three transitions are reached only because the preceding elif state == quoted etc. have already been ruled out. I traced this against all current test cases and it's correct, but the implicit "whatever's left must be after_quote" style is easy to break silently if a 5th state or new field-start branch is ever added.
♻️ Suggested explicit state check
- elif state == quoted:
- if character == '"':
- state = after_quote
- elif character == '"':
- state = quoted
- elif character in {",", "\r", "\n"}:
- state = field_start
- else:
- raise ValueError("csv_text contains malformed quote placement")
+ elif state == quoted:
+ if character == '"':
+ state = after_quote
+ elif state == after_quote:
+ if character == '"':
+ state = quoted
+ elif character in {",", "\r", "\n"}:
+ state = field_start
+ else:
+ raise ValueError("csv_text contains malformed quote placement")📝 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.
| def _validate_csv_quote_placement(csv_text: str) -> None: | |
| """Reject quote placement that ``csv.reader(strict=True)`` accepts leniently. | |
| The standard-library parser treats a quote inside an unquoted field as a | |
| literal character even in strict mode. Such input is not portable CSV and | |
| downstream readers may interpret it differently. This state check inspects | |
| the original text without normalizing or reserializing it. | |
| """ | |
| field_start = "field_start" | |
| unquoted = "unquoted" | |
| quoted = "quoted" | |
| after_quote = "after_quote" | |
| state = field_start | |
| for character in csv_text: | |
| if state == field_start: | |
| if character == '"': | |
| state = quoted | |
| elif character not in {",", "\r", "\n"}: | |
| state = unquoted | |
| elif state == unquoted: | |
| if character == '"': | |
| raise ValueError("csv_text contains malformed quote placement") | |
| if character in {",", "\r", "\n"}: | |
| state = field_start | |
| elif state == quoted: | |
| if character == '"': | |
| state = after_quote | |
| elif character == '"': | |
| state = quoted | |
| elif character in {",", "\r", "\n"}: | |
| state = field_start | |
| else: | |
| raise ValueError("csv_text contains malformed quote placement") | |
| def _validate_csv_quote_placement(csv_text: str) -> None: | |
| """Reject quote placement that ``csv.reader(strict=True)`` accepts leniently. | |
| The standard-library parser treats a quote inside an unquoted field as a | |
| literal character even in strict mode. Such input is not portable CSV and | |
| downstream readers may interpret it differently. This state check inspects | |
| the original text without normalizing or reserializing it. | |
| """ | |
| field_start = "field_start" | |
| unquoted = "unquoted" | |
| quoted = "quoted" | |
| after_quote = "after_quote" | |
| state = field_start | |
| for character in csv_text: | |
| if state == field_start: | |
| if character == '"': | |
| state = quoted | |
| elif character not in {",", "\r", "\n"}: | |
| state = unquoted | |
| elif state == unquoted: | |
| if character == '"': | |
| raise ValueError("csv_text contains malformed quote placement") | |
| if character in {",", "\r", "\n"}: | |
| state = field_start | |
| elif state == quoted: | |
| if character == '"': | |
| state = after_quote | |
| elif state == after_quote: | |
| if character == '"': | |
| state = quoted | |
| elif character in {",", "\r", "\n"}: | |
| state = field_start | |
| else: | |
| raise ValueError("csv_text contains malformed quote placement") |
🤖 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/models/subagent_contracts.py` around
lines 332 - 366, Update _validate_csv_quote_placement to handle the after_quote
state with an explicit state == after_quote branch before its transitions.
Preserve the existing behavior: a following quote returns to quoted, a delimiter
or line break starts a new field, and any other character raises ValueError.
| - Writer manifest writes and successful writer chart executions trigger a manifest-only | ||
| checkpoint. Terminal finalization runs one manifest + directory scan on success or failure. | ||
| On cancellation, that scan runs only when the provider operation lease is immediately | ||
| available; for a busy sandbox the runtime requests immediate termination without extending | ||
| the worker's bounded wait, while completed writer outputs remain preserved by earlier checkpoints. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the terminal-scan exception for cancellation.
“Terminal finalization runs one manifest + directory scan on success or failure” includes cancellation, but the following sentence says a busy sandbox skips that scan. State explicitly that the scan applies to non-cancelled completion, or that cancellation is an exception.
As per path instructions, agent changes must be reviewed for workflow correctness 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/README.md` around lines 174 -
178, Clarify the checkpointing paragraph in the README so “terminal
finalization” explicitly applies to non-cancelled success or failure, while
cancellation is an exception: a busy sandbox may skip the manifest and directory
scan and request immediate termination. Preserve the existing behavior for an
immediately available provider operation lease and retained writer outputs.
Source: Path instructions
| def download_files(self, paths): | ||
| return [ | ||
| SimpleNamespace( | ||
| path=path, | ||
| content=self.files.get(path), | ||
| error=None if path in self.files else "not found", | ||
| ) | ||
| for path in paths | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align this stub's error token with the structured "file_not_found" signal. manager._manifest_error_is_not_found matches that exact token, and the sibling doubles (test_artifacts.py, test_artifact_publication_regression.py) already use it. "not found" only passes here because every path is present; as a reusable pattern it silently misclassifies genuine absence as unreadable/fail-closed.
Suggested change
- error=None if path in self.files else "not found",
+ error=None if path in self.files else "file_not_found",📝 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.
| def download_files(self, paths): | |
| return [ | |
| SimpleNamespace( | |
| path=path, | |
| content=self.files.get(path), | |
| error=None if path in self.files else "not found", | |
| ) | |
| for path in paths | |
| ] | |
| def download_files(self, paths): | |
| return [ | |
| SimpleNamespace( | |
| path=path, | |
| content=self.files.get(path), | |
| error=None if path in self.files else "file_not_found", | |
| ) | |
| for path in paths | |
| ] |
🤖 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/fastapi_extensions/test_deep_research.py` around lines 314 -
322, Update the download_files stub’s missing-file error value to the structured
"file_not_found" token expected by manager._manifest_error_is_not_found,
matching the sibling test doubles. Keep successful file entries unchanged while
ensuring absent paths produce the exact not-found signal.
|
/ok to test 7f72f47 |
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
Signed-off-by: Kyle Zheng <kyzheng@nvidia.com>
175f4fc to
f3d9c85
Compare
#### Overview Document AI-Q's actual OpenShell execution boundary for architecture and security review, and present OpenShell before Modal in the deep-research sandbox implementation reference. This documentation-only change now: - makes the shared AI-Q provider lifecycle explicit: job scope, lazy single-flight creation, serialized operations, idempotency-gated retry, artifact harvesting, and terminal cleanup apply to both OpenShell and Modal; - documents the complete agent-visible filesystem/code surface (`ls`, `glob`, `grep`, `read_file`, `write_file`, `edit_file`, and `execute`) and distinguishes provider-backed paths from host-side `/shared/` and `/skills/` routes; - distinguishes public `deep_research_sandbox` workflow YAML from the internal normalized `SandboxConfig`, and links the validated `configs/config_openshell.yml` public profile; - defines the distinct AI-Q API principal, asynchronous job, job-scoped OpenShell sandbox, and external tenant boundaries; - documents the current async submission order, including the race between Dask worker startup and `job_access` persistence; - separates the architecture into two rendered sequence diagrams: submission/provisioning/policy verification and execution/artifacts/teardown; - shows the real OpenShell data and control path: AI-Q runtime and SDK -> authenticated Gateway -> sandbox Supervisor -> restricted child process; - describes the local policy preflight and the exact Gateway phase, source, protobuf, hash, and version agreement required before AI-Q constructs the OpenShell adapter; - distinguishes fail-closed, point-in-time control-plane policy verification from hardware-backed remote attestation or independent kernel measurement; - documents that end-user authorization remains in AI-Q and that the Gateway authenticates the configured AI-Q service/operator connection, not the API caller; - describes conditional artifact checkpointing, terminal harvesting, EventStore persistence, and the host-side `/shared/` and `/skills/` routes; - states the handled cleanup guarantees and the process-loss limit: SDK context exit requests deletion, but the AI-Q ghost-job reaper does not delete OpenShell resources, so production orphan reconciliation remains external; - qualifies labels as discovery metadata rather than authorization or uniqueness boundaries; and - consistently presents OpenShell before Modal in the isolation overview, module map, configuration example, and provider sections. The reference also corrects two adjacent artifact statements: `/skills/` uses a host-side filesystem backend rather than `StateBackend`, and SVG is currently rejected until a vetted sanitizer exists rather than sanitized or stored as download-only content. No runtime, configuration, policy, API, or dependency behavior changes. #### DCO sign-off for the squash commit Signed-off-by: Kyle Zheng <kyzheng@nvidia.com> Signed-off-by: Kyle Zheng <126034466+KyleZheng1284@users.noreply.github.com> #### Validation - [x] `.venv/bin/pytest -q tests/aiq_agent/agents/deep_researcher/sandbox/test_openshell_provider.py tests/aiq_agent/agents/deep_researcher/sandbox/test_sandbox_runtime.py tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py tests/aiq_agent/jobs/test_runner.py frontends/aiq_api/tests/test_job_access.py frontends/aiq_api/tests/test_submit_collision.py frontends/aiq_api/tests/test_submit_owner_user_id.py` — 328 passed, 2 skipped. - [x] `.venv/bin/pytest -q tests/aiq_agent/agents/deep_researcher/sandbox/test_artifacts.py tests/aiq_agent/agents/deep_researcher/sandbox/test_sandbox_runtime.py tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py` — 114 passed after the review-fix commit. - [x] `.venv/bin/pytest -q tests/aiq_agent/agents/deep_researcher/test_factory.py::test_middleware_set_adds_orchestrator_batch_tool_name` — 1 passed after the filesystem-tool documentation correction. - [x] `.venv/bin/python .agents/skills/aiq-configure-workflow/scripts/validate_config.py configs/config_openshell.yml` — no errors and 0 warnings. - [x] Schema probe — the README example validates as internal `SandboxConfig`, the linked OpenShell block validates as public `DeepResearchSandboxConfig`, and the internal shape is rejected by the public schema. - [x] `.venv/bin/pre-commit run --files src/aiq_agent/agents/deep_researcher/sandbox/README.md` — all applicable hooks passed, including merge-conflict, large-file, whitespace, secret, and network-backed Markdown link checks. - [x] Extracted both `mermaid` blocks and rendered each with `npx --yes @mermaid-js/mermaid-cli` — both SVGs generated without parse errors. - [x] `git diff --check origin/release/2.2..HEAD` — passed. - [x] Verified the rebased branch contains five DCO-signed commits and changes only `src/aiq_agent/agents/deep_researcher/sandbox/README.md`. - [x] I ran the relevant local checks or explained why they are not applicable. - [x] I added or updated tests for behavior changes. No tests were added because this PR changes documentation only; the focused existing suites above validate the described behavior. - [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. - [x] I replaced the DCO sign-off placeholder with my GitHub commit identity and kept the required angle brackets around the email address. #### Where should reviewers start? Start in `src/aiq_agent/agents/deep_researcher/sandbox/README.md` at `Providers`: 1. `Workspace organization and isolation limits` for the complete filesystem/code tool set and provider-backed versus host-side routing. 2. `Shared AI-Q provider lifecycle` for the job scope, lazy creation, serialization/retry, artifact, and terminal contracts common to OpenShell and Modal. 3. `Internal normalized SandboxConfig` for the explicit boundary between public workflow YAML and the runtime's internal provider model. 4. `OpenShell > Boundary model` for the principal/job/sandbox/tenant definitions and identity split. 5. `OpenShell > Submission, provisioning, and policy verification` for the actual Dask and Gateway/Supervisor ordering. 6. `OpenShell > OpenShell transport during execution and teardown` for the provider relay around the shared artifact and terminal lifecycle. 7. `OpenShell > What AI-Q policy verification proves` for the exact assurance boundary and point-in-time limitation. 8. `OpenShell > Lifecycle guarantees and reconciliation limits` for OpenShell deletion requests, labels, and orphan handling. 9. `Modal (cloud)` for its provider-specific creation, controls, retry classification, and teardown adapter. The main review question is whether the shared AI-Q lifecycle is clearly separated from provider-specific behavior, and whether the OpenShell sections describe the implementation precisely without overstating end-user identity propagation, continuous policy attestation, or deletion guarantees. #### Related Issues - Relates to #373; the documentation remains split into this PR to keep the artifact-convergence implementation review scoped. ## Summary by CodeRabbit * **Documentation** * Clarified the Deep Research Sandbox security boundary, keeping host orchestration state separate from provider execution and restricting what sandbox creation requests can include. * Updated guidance on workspace isolation, scoped reattachment, and fail-closed attestation/policy verification. * Strengthened instructions for artifact harvesting and report post-processing, including manifest-first validation, safer reference rewriting, and `artifact://<id>` handling. * Refreshed operational/troubleshooting notes for OpenShell and Modal/cloud, including sandbox lifecycle ordering and single-retry semantics for idempotent transfers. Authors: - Kyle Zheng (https://github.com/KyleZheng1284) Approvers: - Soumili Nandi (https://github.com/soumilinandi) URL: #387
Overview
Make quantitative deep-research output converge and publish durably across sandbox providers. OpenShell exposed the loop, but the failure is provider-neutral: quantitative evidence needs a validated handoff into writer-owned publication, operators need optional convergence controls, and async jobs need a configurable terminal deadline.
What actually changes
The core fix is one explicit ownership handoff:
QuantitativeDatasetinResearchNotesresearchandsynthesisskill collections and could redo analysis during publicationsynthesis, writes the report baseline, publishes the exact validated CSV, and optionally charts that CSVThe visible skill-directory change supports that ownership boundary; it is not the whole fix:
Skill routing is collection-based. Moving
chart-generationfromresearchtosynthesisis therefore the smallest way to keep analysis researcher-owned and publication writer-owned. Leaving it underresearchwould require giving the writer the entire research collection again; adding per-skill routing would introduce a larger configuration/schema change.This branch is rebased onto the current
release/2.2head380a801and preserves the release fixes already merged through:Supporting hardening
ResearchNotesdataset now invalidates both writer-report extraction and deterministic fallback.file_not_foundreason; arbitrary backend text containingnot foundor404remains fail-closed and cannot enable directory scanning.This PR intentionally does not add a deterministic publisher tool, a file-backed large-dataset plane, row-level lineage, per-skill routing, a chart-skill rename, or a new partial-success API status. The implementation is split into four signed commits: release rebase/integration, focused convergence-gap fixes, the compatibility adjustment that makes convergence safeguards opt-in, and focused review hardening.
DCO sign-off for the squash commit
Signed-off-by: Kyle Zheng kyzheng@nvidia.com
Signed-off-by: Kyle Zheng 126034466+KyleZheng1284@users.noreply.github.com
Validation
.venv/bin/ruff check --no-cache .— passed.venv/bin/ruff format --check --no-cache .— 375 files already formatted.venv/bin/python -m pytest -q— 2,011 passed, 13 skipped, 23 dependency warnings.venv/bin/nat validate --config_file configs/config_domain_routing_and_skills.yml— passed.venv/bin/nat validate --config_file configs/config_openshell.yml— passed.venv/bin/sphinx-build -W --keep-going -b html docs/source docs/build/html— passed before the review-only skill documentation addition; no Sphinx source changed afterward.venv/bin/pre-commit run --all-files— all hooks passed, including root/MCP lock checks, secret detection, skill validation, notebook output clearing, and the network-backed Markdown link checkgit commit -sor an equivalent sign-off.Where should reviewers start?
Review the ownership handoff in this order:
skills/research/data-table-analysis/SKILL.mdandmodels/subagent_contracts.py— researcher output contract.factory.pyand the two shipped configs — collection routing from researcher to writer.skills/synthesis/chart-generation/SKILL.mdandcustom_middleware.py— writer-owned publication without re-analysis.sandbox/artifacts/manager.py— digest-protected capture, structured manifest absence, and scan behavior.agent.pyandfrontends/aiq_api/src/aiq_api/jobs/runner.py— report fail-closed behavior, deterministic fallback, optional execution budgets, and terminal hardening.The provider-neutral acceptance path is in
test_artifact_publication_regression.py; default compatibility and explicit-budget behavior are covered intest_agent.py,test_factory.py,test_custom_middleware.py,test_chart_artifact_contract.py, andtest_runner.py; the artifact list/content API round-trip is intest_deep_research.py.Related Issues
Summary by CodeRabbit