fix(research): improve empty-source outcomes and artifact docs - #397
Conversation
Classify empty-source failures and preserve sanitized generated answers across inline and async research paths. Persist actionable async failures, outcome reasons, and optional encrypted reports atomically, with regression coverage for empty selections and empty retrieval results. Signed-off-by: Tanner Leach <tleach@nvidia.com>
|
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. |
|
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:
WalkthroughThe change classifies empty-source research failures, exposes actionable public responses, preserves sanitized generated reports, and persists typed encrypted job failures. Research workflows now raise and handle ChangesEmpty-Source Failure Flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ResearchAgent
participant ResearchWorkflow
participant run_agent_job
participant JobAPI
ResearchAgent->>ResearchWorkflow: raise EmptySourceRegistryError
ResearchWorkflow->>ResearchWorkflow: build public_response
ResearchWorkflow-->>run_agent_job: typed failure and generated_answer
run_agent_job->>run_agent_job: persist encrypted failure and report
run_agent_job-->>JobAPI: FAILURE status and actionable error
JobAPI-->>JobAPI: serve preserved report via /report
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Tanner Leach <tleach@nvidia.com>
tanleach
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiq_agent/agents/deep_researcher/register.py (1)
239-285: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winFast-fail "no sources selected" guard placed after expensive per-request setup in both registrars. Both newly-added
data_sources == []checks run after avoidable work that a request known-to-fail shouldn't have to pay for.
src/aiq_agent/agents/deep_researcher/register.py#L239-L285: hoistif data_sources == []: raise EmptySourceRegistryError(...)to right afterdata_sources = state.data_sources, before the sandbox-scopedDeepResearcherAgent(...)construction (eager sandbox provisioning per test comments) and beforefilter_tools_by_sources.src/aiq_agent/agents/shallow_researcher/register.py#L103-L174: hoist the samedata_sources == []check above theasync with AsyncExitStack()block, beforerequire_verified_principal()/open_per_user_mcp_tools(...)are invoked.🤖 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/register.py` around lines 239 - 285, Move the data_sources == [] EmptySourceRegistryError guard in src/aiq_agent/agents/deep_researcher/register.py (lines 239-285) immediately after data_sources = state.data_sources, before filter_tools_by_sources and DeepResearcherAgent construction. Apply the same hoist in src/aiq_agent/agents/shallow_researcher/register.py (lines 103-174), placing the guard before AsyncExitStack, require_verified_principal(), and open_per_user_mcp_tools().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 276-299: Replace the duplicated event insertion and notification
logic in the writer with a connection-scoped EventStore method such as
store_in_connection(conn, event). Refactor EventStore.store to reuse this method
while preserving transaction ownership in the caller, and update the writer to
pass the stored event through it instead of accessing
_prepare_event_for_storage, _sync_engine, or _is_postgres directly.
- Around line 333-364: The source-failure handling around
_write_job_source_failure_if_running_sync currently loses the actionable typed
failure when event flushing or insertion fails. Make the job_info failure update
commit independently of the job_events/SSE write, and treat event persistence as
best-effort by retrying without final_report_event when needed; preserve
error.public_message and stored_output in the committed failure record.
In `@tests/aiq_agent/jobs/test_runner.py`:
- Around line 975-976: Ensure the test setup around
crypto.reset_content_encryption_manager_for_tests() and the corresponding
cleanup near the encryption-policy assertions always resets the global
encryption manager, including when assertions fail. Use a try/finally cleanup or
an autouse fixture so cached state keyed to test-key cannot leak into subsequent
tests.
---
Outside diff comments:
In `@src/aiq_agent/agents/deep_researcher/register.py`:
- Around line 239-285: Move the data_sources == [] EmptySourceRegistryError
guard in src/aiq_agent/agents/deep_researcher/register.py (lines 239-285)
immediately after data_sources = state.data_sources, before
filter_tools_by_sources and DeepResearcherAgent construction. Apply the same
hoist in src/aiq_agent/agents/shallow_researcher/register.py (lines 103-174),
placing the guard before AsyncExitStack, require_verified_principal(), and
open_per_user_mcp_tools().
🪄 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: b7b4b476-ee7b-4e65-9723-2be04e3c46fb
📒 Files selected for processing (16)
docs/source/integration/rest-api.mdfrontends/aiq_api/src/aiq_api/jobs/callbacks.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pyfrontends/aiq_api/tests/test_content_encryption_routes.pysrc/aiq_agent/agents/chat_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/shallow_researcher/agent.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/common/citation_verification.pytests/aiq_agent/agents/chat_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/shallow_researcher/test_agent.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pytests/aiq_agent/common/test_citation_verification.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: UI Lint
- GitHub Check: UI Type Check
- GitHub Check: UI Unit Tests
- GitHub Check: Pytest and Coverage
- GitHub Check: Script Validation
- GitHub Check: Lint and Hooks
🧰 Additional context used
📓 Path-based instructions (7)
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update canonical documentation under
docs/source/when behavior, configuration, or workflows change; do not duplicate full documentation pages into skills.
Files:
docs/source/integration/rest-api.md
**/*
📄 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:
docs/source/integration/rest-api.mdsrc/aiq_agent/agents/chat_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pyfrontends/aiq_api/tests/test_content_encryption_routes.pytests/aiq_agent/common/test_citation_verification.pyfrontends/aiq_api/src/aiq_api/jobs/callbacks.pysrc/aiq_agent/agents/shallow_researcher/agent.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/common/citation_verification.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/chat_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/shallow_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_agent.pysrc/aiq_agent/agents/deep_researcher/agent.py
{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.md
**/*.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:
src/aiq_agent/agents/chat_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pyfrontends/aiq_api/tests/test_content_encryption_routes.pytests/aiq_agent/common/test_citation_verification.pyfrontends/aiq_api/src/aiq_api/jobs/callbacks.pysrc/aiq_agent/agents/shallow_researcher/agent.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/common/citation_verification.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/chat_researcher/test_agent.pytests/aiq_agent/jobs/test_runner.pytests/aiq_agent/agents/shallow_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_agent.pysrc/aiq_agent/agents/deep_researcher/agent.py
src/aiq_agent/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/aiq_agent/**/*.py: Register new data sources indata_source_registryso the UI can toggle them.
Missing-secret paths must degrade gracefully by stubbing or skipping rather than crashing or leaking information.
Respect authenticated data sources by honoringrequires_auth, passing through per-user tokens, using backend token validators, and applying owner guardrails before loading protected report or artifact context into an agent.
Do not weaken or bypassAuthMiddleware, token validators, or authentication gating without prior design discussion.
Files:
src/aiq_agent/agents/chat_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/shallow_researcher/agent.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/common/citation_verification.pysrc/aiq_agent/agents/deep_researcher/agent.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/chat_researcher/agent.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/shallow_researcher/agent.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/agent.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/callbacks.pyfrontends/aiq_api/src/aiq_api/jobs/runner.py
🧠 Learnings (2)
📚 Learning: 2026-06-14T17:49:00.640Z
Learnt from: torkian
Repo: NVIDIA-AI-Blueprints/aiq PR: 273
File: frontends/aiq_api/tests/test_sse_reconnect_cursor.py:384-401
Timestamp: 2026-06-14T17:49:00.640Z
Learning: When using `unittest.mock.patch` for code that imports dependencies inside functions/generators (e.g., inside `aiq_api.routes.jobs`), don’t patch via an attribute that doesn’t exist on the consuming module. If the generator does `from ..jobs.event_store import EventStore` inside the generator body, then `aiq_api.routes.jobs` will not have an `EventStore` attribute; patch the source class/method in its defining module instead (e.g., `aiq_api.jobs.event_store.EventStore.get_events_async`). Patching `aiq_api.routes.jobs.EventStore...` would raise `AttributeError` because that symbol is not present at module scope.
Applied to files:
frontends/aiq_api/tests/test_content_encryption_routes.py
📚 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/chat_researcher/test_agent.pytests/aiq_agent/agents/shallow_researcher/test_agent.pytests/aiq_agent/agents/deep_researcher/test_agent.py
🪛 ast-grep (0.45.0)
frontends/aiq_api/tests/test_content_encryption_routes.py
[info] 613-618: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"report": "# Preserved generated answer",
"outcome_reason": outcome_reason,
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
frontends/aiq_api/src/aiq_api/jobs/runner.py
[info] 277-277: use jsonify instead of json.dumps for JSON output
Context: json.dumps(stored_event)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 288-288: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"id": event_id, "type": event_type})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (17)
frontends/aiq_api/src/aiq_api/jobs/runner.py (2)
30-30: LGTM!Also applies to: 39-42
1087-1104: LGTM!frontends/aiq_api/src/aiq_api/jobs/callbacks.py (1)
129-135: LGTM!Also applies to: 393-393
tests/aiq_agent/jobs/test_runner.py (1)
1103-1143: LGTM!Also applies to: 1145-1179
frontends/aiq_api/tests/test_content_encryption_routes.py (1)
83-91: LGTM!Also applies to: 123-124, 594-638
docs/source/integration/rest-api.md (1)
207-212: LGTM!src/aiq_agent/common/citation_verification.py (1)
43-43: LGTM!Also applies to: 109-144, 153-174
tests/aiq_agent/common/test_citation_verification.py (1)
22-27: LGTM! Solid contract coverage for the new classification/enum behavior.Also applies to: 43-114
src/aiq_agent/agents/deep_researcher/register.py (1)
341-354: LGTM!EmptySourceRegistryError→public_responsemapping for the evaluation wrapper is correct and matches the new test.tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
45-46: LGTM! Thorough coverage of the classification matrix, generated-answer sanitization, and the disabled-citation-verification fallback path. Consistent withEmptySourceRegistryReason/EmptySourceRegistryErrorbehavior.Also applies to: 356-410, 1308-1351, 1360-1385, 1387-1409
src/aiq_agent/agents/shallow_researcher/agent.py (1)
360-382: LGTM! Null-safe extraction plus correct sanitized-answer preservation and reason classification on the empty-registry path.Also applies to: 397-399
src/aiq_agent/agents/shallow_researcher/register.py (1)
224-238: LGTM! Evaluation wrapper correctly surfacesexc.public_responsefor empty-source failures.src/aiq_agent/agents/deep_researcher/agent.py (1)
276-294: LGTM! Sanitized-answer preservation and reason classification are correctly ordered relative to thewriter_output_not_committedfallback, and match the new test matrix.Also applies to: 321-324
src/aiq_agent/agents/chat_researcher/agent.py (1)
225-232: LGTM!public_responsecorrectly replaces the old ad-hoc formatting and matches the new remediation-text tests.Also applies to: 345-352
tests/aiq_agent/agents/shallow_researcher/test_agent.py (1)
33-33: LGTM! Good coverage of the classification matrix and generated-answer preservation, including the no-tool-call and empty-final-message edge cases.Also applies to: 46-51, 821-848, 850-890, 891-905
tests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.py (1)
24-25: LGTM! Correctly validates the typed-error contract for the aiq_api-absent registration path.Also applies to: 103-120, 123-137, 139-156
tests/aiq_agent/agents/chat_researcher/test_agent.py (1)
31-32: LGTM! Correctly asserts the typed remediation text and terminal failure outcome across both shallow/deep depths.Also applies to: 245-286, 287-316
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 `@docs/source/examples/skills-sandbox/index.md`:
- Around line 153-177: Add the documented authentication setup to every REST
example command in the job submission, status polling, artifact listing, and
artifact download sequence, reusing the same auth header or token consistently.
If authentication is intended to be disabled locally instead, explicitly
document that required configuration before the commands, while preserving the
distinction between the public API and authenticated job routes.
- Around line 166-170: Update the job-polling instructions around the curl
example to treat both SUCCESS and FAILURE as terminal statuses, rather than
waiting only for SUCCESS. Explain that users should inspect the response’s error
field when the job reaches FAILURE, preserving the existing polling command and
documenting the current failure contract.
🪄 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: 062b1d54-e7dc-45ec-8c4f-20ae5ee171e6
📒 Files selected for processing (1)
docs/source/examples/skills-sandbox/index.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Pytest and Coverage
- GitHub Check: Lint and Hooks
- GitHub Check: Script Validation
🧰 Additional context used
📓 Path-based instructions (3)
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update canonical documentation under
docs/source/when behavior, configuration, or workflows change; do not duplicate full documentation pages into skills.
Files:
docs/source/examples/skills-sandbox/index.md
**/*
📄 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:
docs/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/examples/skills-sandbox/index.md
🔇 Additional comments (1)
docs/source/examples/skills-sandbox/index.md (1)
110-111: LGTM!Also applies to: 122-144, 179-182, 275-278
Signed-off-by: Tanner Leach <tleach@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
298-344: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFlush failure still masks the typed source-failure message (past review comment not fully resolved).
event_store.flush()(line ~316) is awaited inside the sametrythat also performs the guarded typed write. If flush raises (e.g. a transient DB hiccup), the whole block falls into the genericexcept Exceptionbranch and persists a sanitized"job failed (RuntimeError); ..."message via_write_job_failure_if_running_sync— discardingerror.public_message(e.g. "No data sources are selected...") entirely. This is the same hazard flagged on a previous commit: "the shared transaction rolls back the typed failure payload with it." Only thejob_eventsinsert failure got hardened (viaEventStore.store()'s internal try/except); the flush call did not.This file already has
_flush_event_store(best-effort, never raises) a few hundred lines down — reuse it here instead of the raw flush call. No test currently exercisesevent_store.flush()raising in this path;test_source_failure_event_write_failure_preserves_typed_outcomepassesevent_store=None, bypassing this branch entirely.🔧 Proposed fix — reuse the existing best-effort flush helper
try: stored_output = serialize_job_output_for_storage(output, job_output_cipher) - if event_store is not None and hasattr(event_store, "flush"): - await asyncio.to_thread(event_store.flush) + if event_store is not None: + await _flush_event_store(event_store, job_id=job_id) final_report_event = ( build_final_report_event(error.generated_answer).to_sse_dict() if error.generated_answer else None )As per path instructions,
{src/aiq_agent/fastapi_extensions/**,frontends/aiq_api/src/aiq_api/**}changes should "Check...error responses...job state transitions" and "Require tests for...job state transitions when those surfaces change" — worth adding a regression test whereevent_store.flushitself raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/aiq_api/src/aiq_api/jobs/runner.py` around lines 298 - 344, Replace the raw event_store.flush call in _persist_empty_source_failure with the existing best-effort _flush_event_store helper so flush errors cannot enter the generic fallback and overwrite error.public_message. Add a regression test covering a raising event_store.flush while verifying the typed source-failure message and job state are preserved.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiq_agent/agents/deep_researcher/register.py`:
- Around line 246-254: Consolidate the duplicated empty-source and
tool-unavailability guards from deep_researcher and shallow_researcher into a
shared helper, parameterized by the agent-type string. Update both register
flows to call that helper while preserving EmptySourceRegistryError reasons and
existing behavior.
In `@src/aiq_agent/agents/shallow_researcher/register.py`:
- Around line 108-116: Consolidate the duplicated empty-source validation used
by shallow and deep researcher registration into a shared helper, and update
both guard locations in shallow_researcher/register.py to call it. Preserve the
agent-specific name and EmptySourceRegistryReason.NO_SOURCES_SELECTED behavior
while removing the repeated imports and exception construction.
---
Duplicate comments:
In `@frontends/aiq_api/src/aiq_api/jobs/runner.py`:
- Around line 298-344: Replace the raw event_store.flush call in
_persist_empty_source_failure with the existing best-effort _flush_event_store
helper so flush errors cannot enter the generic fallback and overwrite
error.public_message. Add a regression test covering a raising event_store.flush
while verifying the typed source-failure message and job state are preserved.
🪄 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: 7b676fb4-08e4-41d0-b1ee-00b17e9f0e9e
📒 Files selected for processing (6)
frontends/aiq_api/src/aiq_api/jobs/runner.pysrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/shallow_researcher/register.pytests/aiq_agent/agents/deep_researcher/test_agent.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pytests/aiq_agent/jobs/test_runner.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Lint and Hooks
- GitHub Check: Pytest and Coverage
- GitHub Check: Script Validation
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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:
src/aiq_agent/agents/shallow_researcher/register.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_agent.py
src/aiq_agent/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/aiq_agent/**/*.py: Register new data sources indata_source_registryso the UI can toggle them.
Missing-secret paths must degrade gracefully by stubbing or skipping rather than crashing or leaking information.
Respect authenticated data sources by honoringrequires_auth, passing through per-user tokens, using backend token validators, and applying owner guardrails before loading protected report or artifact context into an agent.
Do not weaken or bypassAuthMiddleware, token validators, or authentication gating without prior design discussion.
Files:
src/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/register.py
**/*
📄 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:
src/aiq_agent/agents/shallow_researcher/register.pytests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.pysrc/aiq_agent/agents/deep_researcher/register.pytests/aiq_agent/jobs/test_runner.pyfrontends/aiq_api/src/aiq_api/jobs/runner.pytests/aiq_agent/agents/deep_researcher/test_agent.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/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/register.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
🔇 Additional comments (6)
src/aiq_agent/agents/deep_researcher/register.py (1)
34-34: LGTM!Also applies to: 349-353
src/aiq_agent/agents/shallow_researcher/register.py (1)
29-29: LGTM!Also applies to: 231-237
tests/aiq_agent/agents/deep_researcher/test_agent.py (1)
45-46: LGTM!Also applies to: 391-433, 1331-1432
tests/aiq_agent/agents/shallow_researcher/test_register_per_user_mcp.py (1)
24-25: LGTM!Also applies to: 103-121, 124-137, 140-157
frontends/aiq_api/src/aiq_api/jobs/runner.py (1)
244-279: LGTM! Nicely resolves the prior duplicated event-insert/pg_notify logic by delegating toEventStore.store().tests/aiq_agent/jobs/test_runner.py (1)
93-102: LGTM! Fixture correctly guarantees encryption-manager teardown even on assertion failure, and the new persistence tests matchrunner.py's guarded-write behavior.Also applies to: 924-1234
tanleach
changed the title
Signed-off-by: Tanner Leach <tleach@nvidia.com>
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 `@docs/source/examples/skills-sandbox/index.md`:
- Around line 169-170: Update the job-status guidance in the documented polling
instructions to compare against the API’s exact lowercase values `success` and
`failure`, and use the lowercase value consistently when describing failure
handling.
🪄 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: 4e0eb1de-31da-48dc-a602-55c9e52ca1a6
📒 Files selected for processing (1)
docs/source/examples/skills-sandbox/index.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update canonical documentation under
docs/source/when behavior, configuration, or workflows change; do not duplicate full documentation pages into skills.
Files:
docs/source/examples/skills-sandbox/index.md
**/*
📄 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:
docs/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/examples/skills-sandbox/index.md
🔇 Additional comments (1)
docs/source/examples/skills-sandbox/index.md (1)
145-147: Authentication guidance is still missing from the commands.The prose says authenticated deployments must use
Authorization: Bearer $AIQ_TOKEN, but the submission, polling, artifact-listing, and download examples omit that header. Copy-pasting them withREQUIRE_AUTH=truewill fail. This remains the issue raised in the previous review.Source: Path instructions
Reuse typed source checks across deep and shallow researchers and align sandbox job statuses with API values. Signed-off-by: Tanner Leach <tleach@nvidia.com>
|
/ok to test 54c5382 |
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)
docs/source/examples/skills-sandbox/index.md (1)
145-147: 🎯 Functional Correctness | 🟠 MajorAdd the authentication header to every REST example.
The text requires
Authorization: Bearer $AIQ_TOKENwhenREQUIRE_AUTH=true, but the submission, polling, artifact-listing, and artifact-download commands omit it. Those routes require authentication, so the documented flow fails on authenticated deployments. Add the same header to eachcurlcommand.As per path instructions, documentation must preserve command accuracy and public-vs-internal API boundary clarity.
Also applies to: 159-180
🤖 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 `@docs/source/examples/skills-sandbox/index.md` around lines 145 - 147, Update every REST curl example in the documented submission, polling, artifact-listing, and artifact-download flow to include the Authorization: Bearer $AIQ_TOKEN header, matching the requirement stated near REQUIRE_AUTH. Preserve the existing commands, endpoints, and distinction between public and internal APIs while adding the header consistently to each command.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/source/examples/skills-sandbox/index.md`:
- Around line 145-147: Update every REST curl example in the documented
submission, polling, artifact-listing, and artifact-download flow to include the
Authorization: Bearer $AIQ_TOKEN header, matching the requirement stated near
REQUIRE_AUTH. Preserve the existing commands, endpoints, and distinction between
public and internal APIs while adding the header consistently to each command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9ee96d71-d55c-41ed-8a80-580b25048f08
📒 Files selected for processing (5)
docs/source/examples/skills-sandbox/index.mdsrc/aiq_agent/agents/deep_researcher/register.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/common/__init__.pysrc/aiq_agent/common/tool_validation.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Lint and Hooks
- GitHub Check: Script Validation
- GitHub Check: Pytest and Coverage
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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:
src/aiq_agent/common/__init__.pysrc/aiq_agent/common/tool_validation.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/register.py
src/aiq_agent/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/aiq_agent/**/*.py: Register new data sources indata_source_registryso the UI can toggle them.
Missing-secret paths must degrade gracefully by stubbing or skipping rather than crashing or leaking information.
Respect authenticated data sources by honoringrequires_auth, passing through per-user tokens, using backend token validators, and applying owner guardrails before loading protected report or artifact context into an agent.
Do not weaken or bypassAuthMiddleware, token validators, or authentication gating without prior design discussion.
Files:
src/aiq_agent/common/__init__.pysrc/aiq_agent/common/tool_validation.pysrc/aiq_agent/agents/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/register.py
**/*
📄 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:
src/aiq_agent/common/__init__.pysrc/aiq_agent/common/tool_validation.pysrc/aiq_agent/agents/shallow_researcher/register.pydocs/source/examples/skills-sandbox/index.mdsrc/aiq_agent/agents/deep_researcher/register.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/shallow_researcher/register.pysrc/aiq_agent/agents/deep_researcher/register.py
docs/source/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Update canonical documentation under
docs/source/when behavior, configuration, or workflows change; do not duplicate full documentation pages into skills.
Files:
docs/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/examples/skills-sandbox/index.md
🔇 Additional comments (10)
docs/source/examples/skills-sandbox/index.md (1)
140-144: LGTM!Also applies to: 149-154, 183-186, 279-282
src/aiq_agent/common/__init__.py (2)
62-62: LGTM!
98-98: LGTM!src/aiq_agent/agents/shallow_researcher/register.py (3)
29-29: LGTM!
109-109: LGTM!Also applies to: 178-178
214-215: LGTM!src/aiq_agent/agents/deep_researcher/register.py (3)
34-34: LGTM!
247-247: LGTM!Also applies to: 281-281
326-327: LGTM!src/aiq_agent/common/tool_validation.py (1)
103-131: 📐 Maintainability & Code QualityRun the required Python validation
uv run ruff check .,uv run ruff format --check ., anduv run pytest.
AjayThorve
left a comment
AjayThorve
left a comment
There was a problem hiding this comment.
One source-validation gap remains on the current head.
Signed-off-by: Tanner Leach <tleach@nvidia.com>
Validate source selection and availability at the shared agent entrypoint. Treat empty source results as typed failures even when citation verification is disabled. Signed-off-by: Tanner Leach <tleach@nvidia.com>
|
/ok to test 66606a6 |
|
/merge |
aa06ef8
into
NVIDIA-AI-Blueprints:develop
## Summary - classify empty-source research outcomes and return actionable, sanitized responses - persist typed async job failures safely and add regression coverage across research workflows - clarify that direct `nat run` artifacts are non-persistent and document the `nat serve` job/artifact retrieval workflow ## Validation - Sphinx HTML build passed - Sphinx linkcheck passed ## Related issue - https://linear.app/nvidia/issue/AIQ-27/docs-clarify-generated-artifact-persistence-requires-nat-servejob Signed-off-by: Tanner Leach <tanleach@users.noreply.github.com> ## Summary by CodeRabbit * **New Features** * Research runs now classify empty-source situations (no sources selected, tools unavailable, or no results) and return consistent, user-friendly remediation messages. * Failed jobs with actionable source-selection issues preserve sanitized answers and final report artifacts for later retrieval. * **Bug Fixes** * Job failures continue to avoid exposing internal exception details or plaintext output. * **Documentation** * Clarified REST “Get Job Status” semantics for typed source-condition failures and refined the skills-sandbox async/persistence example. * **Tests** * Expanded coverage for empty-source handling, public-facing messaging, and encrypted report/error preservation in API/job flows. Authors: - Tanner Leach (https://github.com/tanleach) Approvers: - Ajay Thorve (https://github.com/AjayThorve) URL: #397 (cherry picked from commit aa06ef8)
## Summary - classify empty-source research outcomes and return actionable, sanitized responses - persist typed async job failures safely and add regression coverage across research workflows - clarify that direct `nat run` artifacts are non-persistent and document the `nat serve` job/artifact retrieval workflow ## Validation - Sphinx HTML build passed - Sphinx linkcheck passed ## Related issue - https://linear.app/nvidia/issue/AIQ-27/docs-clarify-generated-artifact-persistence-requires-nat-servejob Signed-off-by: Tanner Leach <tanleach@users.noreply.github.com> ## Summary by CodeRabbit * **New Features** * Research runs now classify empty-source situations (no sources selected, tools unavailable, or no results) and return consistent, user-friendly remediation messages. * Failed jobs with actionable source-selection issues preserve sanitized answers and final report artifacts for later retrieval. * **Bug Fixes** * Job failures continue to avoid exposing internal exception details or plaintext output. * **Documentation** * Clarified REST “Get Job Status” semantics for typed source-condition failures and refined the skills-sandbox async/persistence example. * **Tests** * Expanded coverage for empty-source handling, public-facing messaging, and encrypted report/error preservation in API/job flows. Authors: - Tanner Leach (https://github.com/tanleach) Approvers: - Ajay Thorve (https://github.com/AjayThorve) URL: #397 (cherry picked from commit aa06ef8)
## Summary Backports #397 to release/2.2, including actionable empty-source outcomes, safe async failure persistence, regression coverage, and artifact workflow documentation. ## Verification - Focused tests: 1,032 passed, 6 skipped - Ruff lint passed - Ruff format passed ## Summary by CodeRabbit * **New Features** * Research now returns consistent, actionable messaging for empty, unselected, unavailable, or no-results source conditions, including the relevant draft when available. * Async job status surfaces typed source-condition failures, and preserved reports remain accessible on failure. * Final report content is now emitted reliably as an output artifact for async flows. * **Documentation** * Updated the skills sandbox example to clarify persistence behavior. * Expanded REST API documentation for typed source-condition failures and report preservation. * **Tests** * Added and extended coverage for empty-source scenarios and async job status/report handling. * **Chores** * Updated link-check configuration to ignore an additional API reference URL. Authors: - Tanner Leach (https://github.com/tanleach) Approvers: - https://github.com/peterychang URL: #405
2 participants
Summary
nat runartifacts are non-persistent and document thenat servejob/artifact retrieval workflowValidation
Related issue
Signed-off-by: Tanner Leach tleach@nvidia.com
Summary by CodeRabbit