refactor(deep-research): single-source the chart contract via an on-demand visualization skill - #389
Conversation
…emand visualization skill Move the chart-spec contract out of the always-on writer prompt into the visualization skill (sandbox PNG mode and non-sandbox inline-spec mode), and wire the skill into every default deep-research config so inline charts are preserved everywhere with the writer prompt shrunk to an on-demand pointer. Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
|
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 configurations expose the visualization skill to the writer agent. Runtime routing enables chart-skill detection, prompts delegate chart generation, and the skill defines sandbox artifact and inline chart-spec modes. Tests and documentation cover the updated contracts and migration. ChangesDeep research visualization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeepAgentsRuntime
participant DeepResearchWriter
participant ChartGenerationSkill
participant Sandbox
participant Report
DeepResearchWriter->>DeepAgentsRuntime: Check chart-skill availability
DeepAgentsRuntime-->>DeepResearchWriter: Provide chart_skill_enabled
DeepResearchWriter->>ChartGenerationSkill: Delegate chart generation
alt Sandbox execution enabled
ChartGenerationSkill->>Sandbox: Execute chart script
Sandbox-->>ChartGenerationSkill: Write PNG, CSV, and manifest.json
ChartGenerationSkill-->>Report: Add artifact:// image reference
else Inline mode
ChartGenerationSkill-->>Report: Add chart or chart-carousel JSON
end
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/skills/visualization/chart-generation/SKILL.md`:
- Line 226: Update the fenced chart examples in SKILL.md by inserting a blank
line between each example label and its opening ```chart fence, including the
examples near lines 226, 231, and 240. Preserve the existing labels and fence
contents.
In `@tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py`:
- Around line 73-78: Update the normalization around openshell_functions and the
deep_research_agent entry to assert that the OpenShell agent configuration
contains the expected visualization wiring before removing or normalizing that
block. Preserve the existing cleanup of skills and sandbox, but make the test
fail when writer-agent visualization is missing from config_openshell.yml.
🪄 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: 95c3be3b-6d6b-4779-a30f-dc7f36210b9e
📒 Files selected for processing (16)
configs/config_cli_default.ymlconfigs/config_domain_routing_and_skills.ymlconfigs/config_frontier_models.ymlconfigs/config_mcp.ymlconfigs/config_openshell.ymlconfigs/config_web_azure_ai_search.ymlconfigs/config_web_default_guardrails.ymlconfigs/config_web_default_llamaindex.ymlconfigs/config_web_frag.ymlconfigs/config_web_frag_mcp_auth.ymlconfigs/config_web_opensearch.ymlsrc/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.mdtests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/test_result_chart_prompt.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
configs/**/*.y{a,}ml
📄 CodeRabbit inference engine (AGENTS.md)
Use
_typevalues derived from the registered configuration class names in workflow YAML configuration.
Files:
configs/config_web_frag.ymlconfigs/config_openshell.ymlconfigs/config_domain_routing_and_skills.ymlconfigs/config_web_opensearch.ymlconfigs/config_cli_default.ymlconfigs/config_frontier_models.ymlconfigs/config_web_frag_mcp_auth.ymlconfigs/config_web_default_guardrails.ymlconfigs/config_web_azure_ai_search.ymlconfigs/config_mcp.ymlconfigs/config_web_default_llamaindex.yml
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Keep changes inside this repository, avoid editing adjacent repositories, and scope changes to the smallest relevant independent package, especially undersources/.
Run the narrowest relevant validation command first and broaden to the full suite only when a change crosses shared boundaries.
Keep pull requests scoped, exclude unrelated files and generated artifacts, never include secrets, and provide validation commands and results.
**/*: Do not include secrets, credentials, private hostnames, internal-only logs, customer data, or generated local artifacts.
Add or update tests for behavior changes.
Files:
configs/config_web_frag.ymlconfigs/config_openshell.ymlconfigs/config_domain_routing_and_skills.ymlconfigs/config_web_opensearch.ymlconfigs/config_cli_default.ymlconfigs/config_frontier_models.ymltests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pyconfigs/config_web_frag_mcp_auth.ymltests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pyconfigs/config_web_default_guardrails.ymlconfigs/config_web_azure_ai_search.ymlconfigs/config_mcp.ymltests/aiq_agent/agents/test_result_chart_prompt.pyconfigs/config_web_default_llamaindex.ymlsrc/aiq_agent/agents/deep_researcher/prompts/writer.j2src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
For deployment changes, run the relevant Helm or Compose validation and describe the environment used.
Files:
configs/config_web_frag.ymlconfigs/config_openshell.ymlconfigs/config_domain_routing_and_skills.ymlconfigs/config_web_opensearch.ymlconfigs/config_cli_default.ymlconfigs/config_frontier_models.ymlconfigs/config_web_frag_mcp_auth.ymlconfigs/config_web_default_guardrails.ymlconfigs/config_web_azure_ai_search.ymlconfigs/config_mcp.ymlconfigs/config_web_default_llamaindex.yml
{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_web_frag.ymlconfigs/config_openshell.ymlconfigs/config_domain_routing_and_skills.ymlconfigs/config_web_opensearch.ymlconfigs/config_cli_default.ymlconfigs/config_frontier_models.ymlconfigs/config_web_frag_mcp_auth.ymlconfigs/config_web_default_guardrails.ymlconfigs/config_web_azure_ai_search.ymlconfigs/config_mcp.ymlconfigs/config_web_default_llamaindex.yml
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Format and lint Python with Ruff using a 120-character line length, Python 3.11 target, rules E/F/W/I/PL/UP, and single-line imports; avoid reformatting unrelated code.
Never print or log secret values, including through tool output or error messages.
**/*.py: Runuv run ruff check .anduv run ruff format --check .for root Python changes.
Runuv run pytestfor root project Python changes.
Files:
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.pytests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.pytests/aiq_agent/agents/test_result_chart_prompt.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/prompts/writer.j2src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md
🪛 ast-grep (0.44.1)
tests/aiq_agent/agents/test_result_chart_prompt.py
[warning] 59-59: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _CHART_BLOCK.findall(text)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 102-102: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _CAROUSEL_BLOCK.findall(text)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🪛 LanguageTool
src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md
[style] ~39-~39: ‘a majority of’ might be wordy. Consider a shorter alternative.
Context: ...harts:** if a series is mostly missing (a majority of periods undisclosed) or mixes metric...
(EN_WORDINESS_PREMIUM_A_MAJORITY_OF)
[uncategorized] ~208-~208: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...in the web app, so ALSO place a compact markdown table of the same values immediately ...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.23.0)
src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md
[warning] 226-226: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 231-231: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (16)
configs/config_cli_default.yml (1)
117-120: LGTM!Also applies to: 132-132
configs/config_frontier_models.yml (1)
147-150: LGTM!Also applies to: 165-165
configs/config_mcp.yml (1)
92-95: LGTM!Also applies to: 107-107
configs/config_web_azure_ai_search.yml (1)
192-195: LGTM!Also applies to: 209-209
configs/config_web_default_guardrails.yml (1)
222-225: LGTM!Also applies to: 237-237
configs/config_web_default_llamaindex.yml (1)
191-194: LGTM!Also applies to: 208-208
configs/config_web_frag.yml (1)
159-162: LGTM!Also applies to: 175-175
configs/config_web_frag_mcp_auth.yml (1)
208-211: LGTM!Also applies to: 224-224
configs/config_web_opensearch.yml (1)
166-170: LGTM!Also applies to: 182-182
configs/config_domain_routing_and_skills.yml (1)
169-169: LGTM!configs/config_openshell.yml (1)
123-123: LGTM!src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md (1)
5-212: LGTM!src/aiq_agent/agents/deep_researcher/prompts/writer.j2 (1)
78-90: LGTM!Also applies to: 109-109
tests/aiq_agent/agents/deep_researcher/test_chart_artifact_contract.py (1)
16-16: LGTM!Also applies to: 29-29
tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py (1)
49-72: LGTM!Also applies to: 80-82, 118-118
tests/aiq_agent/agents/test_result_chart_prompt.py (1)
15-145: LGTM!Also applies to: 157-185
…pecific chart gating, prompt/skill ownership, migration docs, lint) Gate the writer chart section on a writer-specific capability instead of the global skills flag, and tighten the prompt/skill ownership boundary so the chart procedure lives only in the visualization skill. - deepagents_runtime: add CHART_SKILL_NAME and DeepAgentsRuntime.agent_has_chart_skill, which maps an agent's resolved skill sources back to collection dirs and checks for a chart-generation SKILL.md on disk (false when the agent has no sources). - factory: render the writer prompt with chart_skill_enabled derived from agent_has_chart_skill(WRITER_AGENT); leave the global skills_enabled injection for the other prompts unchanged. - writer.j2: gate the chart section on chart_skill_enabled with the markdown-table fallback preserved; trim it to the earned-figure rule, a pointer to the skill as the controlling procedure, and runtime context only (the real sandbox_workdir and sandbox_artifact_dir paths). The detailed sandbox procedure now lives solely in the skill. - chart-generation SKILL.md: make the sandbox-vs-inline mode precedence explicit (mutually exclusive, sandbox always wins when execute is available) and add the MD031 blank lines before the three chart example fences. - docs/CHANGELOG: document the chart-generation move from the research collection to visualization (charts are no longer sandbox-gated; external writer configs that relied on research for charts must now assign visualization). - tests: harden the OpenShell normalization assertion to require writer-agent visualization before discarding, add an agent_has_chart_skill unit test, and update the writer prompt gating tests to the chart_skill_enabled flag. Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
…cs (NVIDIA-AI-Blueprints#389 review) Wire the visualization skill into the four deep-research benchmark configs so inline charts are preserved there too, and correct the docs that still placed chart-generation in the research collection. Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
|
/ok to test 331c383 |
KyleZheng1284
left a comment
KyleZheng1284
left a comment
There was a problem hiding this comment.
LGTM, I like the idea of routing it through a visualization skill then checking if sandbox is configured or not
Wiring the visualization skill into config_mcp.yml adds a deep_research_skills function; update the public-config contract test to expect it. Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
|
/ok to test a84867b |
|
/ok to test dc17250 |
…kills and sandbox configs (NVIDIA-AI-Blueprints#389 review) Per Chantal's review, wire the on-demand visualization chart skill into only the two skills and sandbox example configs (config_domain_routing_and_skills and config_openshell) and remove the deep_research_skills block plus its skills ref from the nine default configs and the four benchmark configs. The writer.j2 chart_skill_enabled gating is unchanged, so the now-unwired configs fall back to the Markdown-table branch. Update the two affected tests and the CHANGELOG and configuration-reference migration note to match the new scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
|
Clarify or enforce the sandbox manifest checkpoint contract @Manushpm8 This is not a request to expose Could we either:
If the manifest remains required, the test should assert that the PNG is durable immediately after |
…ecovery as an accepted path (NVIDIA-AI-Blueprints#389 review) Kyle observed a live sandbox chart captured only by the terminal directory scan with default metadata. The backend treats the manifest as optional (scan is the documented fallback), so the skill's 'harvested reliably' wording overstated the requirement. Reframe the manifest as preferred (metadata plus the mid-run execute checkpoint) and note that terminal-scan recovery is an accepted successful path with default metadata. Signed-off-by: Manush Maheshwari <manushm@nvidia.com>
|
/ok to test aa6efec |
|
/ok to test aa6efec |
1 similar comment
|
/ok to test aa6efec |
|
can you pull in latest develop, CI should be fixed |
…onsolidation Signed-off-by: Manush Maheshwari <manushm@nvidia.com> # Conflicts: # tests/aiq_agent/agents/deep_researcher/test_deepagents_runtime.py
|
@AjayThorve done, pulled in latest develop ( |
|
/merge |
|
/ok to test #389 |
|
/merge |
|
/ok to test f02bd5a |
1 similar comment
|
/ok to test f02bd5a |
|
/ok to test 9731fae |
|
/ok to test 9731fae |
|
/merge |
|
/ merge |
|
/merge |
|
/merge |
2 similar comments
|
/merge |
|
/merge |
ff051b7
into
NVIDIA-AI-Blueprints:develop
4 participants
Overview
The inline chart-spec contract lived in the always-on
writer.j2prompt (and was duplicated in the sandbox## Figuresblock). That is prompt weight every deep-research writer paid on every turn, whether or not a chart was warranted, which works against keeping the writer prompt lean.This change single-sources that contract into the on-demand
visualizationchart skill and loads it only when a chart is actually needed:chart-generationinto its own sandbox-independentvisualizationskill collection (out of the sandbox-onlyresearchcollection) and makes itsSKILL.mdtwo-mode: the existing sandbox path (matplotlib PNG viaexecute,artifact://reference) plus a new inline path holding the fullchart/chart-carousel/ KPI-only spec contract, field reference, examples, and the portable markdown-table fallback.writer.j2from an embedded chart contract to one skills-gated pointer: sandbox writers get the per-job run command, non-sandbox writers are pointed at the inline spec in the skill, and a no-skills{% else %}branch degrades gracefully to a markdown table. NochartJSON syntax remains in the prompt.visualizationskill into every deep-research config so inline charts keep working everywhere. This is behavior-preserving: no config loses inline charts.visualizationis not sandbox-gated, so it is never added torequire_sandbox.The skill machinery is independent of the sandbox:
skills_enabledis driven by thedeep_research_skillsconfig, and the/skills/route is a filesystem-backed read, so the writer can load the chart skill with or without a sandbox.DCO sign-off for the squash commit
Signed-off-by: Manush Maheshwari manushm@nvidia.com
Validation
git commit -sor an equivalent sign-off.Unit and prompt tests (
tests/aiq_agent/agents/deep_researcher/plustest_result_chart_prompt.py): 399 passed, 4 skipped. Coverage asserts the chart contract lives in the skill (both modes) and the shallow prompt, thatwriter.j2no longer embeds it ("```chart" in rendered_promptisFalse), that the writer delegates to the skill, and that the skill examples validate against the UI zod schema.Config validation: all 11 deep-research configs parse and load; every
deep_research_agent.skillsreference resolves to a defineddeep_research_skillsfunction (no dangling refs).Live end-to-end run on the default model (
nvidia/nemotron-3-super-120b-a12bviaintegrate.api.nvidia.com), non-sandbox writer with thevisualizationskill wired and zero chart syntax in the base prompt. The writer navigated the skill route and read the contract before emitting a chart:The emitted spec passed the real UI
ChartSpecSchema/parseChartSpecand rendered through the actualChartBlock->ResultChartcomponent (not a fallback code block). Because the chart syntax exists only in the skill file, this confirms the on-demand skill mechanism produces charts end to end on the default model.Where should reviewers start?
src/aiq_agent/agents/deep_researcher/skills/visualization/chart-generation/SKILL.md(the two-mode contract, the single source of truth).src/aiq_agent/agents/deep_researcher/prompts/writer.j2(the skills-gated pointer that replaced the embedded contract).configs/config_cli_default.yml(thedeep_research_skillsblock +skills:reference pattern, applied to all deep-research configs).Related Issues
Summary by CodeRabbit