fix: clarifier searches before asking clarification questions#245
Conversation
Closes NVIDIA-AI-Blueprints#234. The clarifier agent had tool-calling bound to a search loop, but weak or non-frontier models routinely skipped the search and asked the user a clarification question immediately. The expected behavior is to gather context with the available tools first and only ask once a genuine ambiguity remains. This is fixed at the graph level so it works model-agnostically rather than relying on prompt engineering for any one model: - New force_search node in the LangGraph workflow. When the LLM tries to ask for clarification on the first turn, tools are configured, and no tool call has happened yet, decide_route sends the run through force_search before ask_for_clarification. - Guidance is injected ephemerally in agent_node (same pattern as the existing JSON_REMINDER_AFTER_TOOLS) rather than written into state.messages, so it cannot leak into helpers like get_latest_user_query that surface text back to the user. - A force_search_used boolean on ClarifierAgentState makes the nudge fire at most once per run, so a model that ignores the nudge falls through to ask_for_clarification instead of looping. - prompts/research_clarification.j2 is updated so the Tool Usage section instructs the model to "Search first, ask second" for any unfamiliar entity, acronym, or term. Frontier models follow this voluntarily and skip the structural nudge entirely. End-to-end reproducer added at scripts/e2e_clarifier_search_test.py. Against nvidia/nemotron-3-nano-30b-a3b (the project's default clarifier model) with a real Tavily web search tool: develop: 0 tool calls, user prompted immediately (bug) fix: 1 tool call, user never prompted (fixed) The mock-based unit suite covers the new helper, the four routing branches, and the regression flagged during review where the guidance message could otherwise leak into the invalid-format fallback text. All 148 clarifier tests pass. Signed-off-by: Torkian <torkian@mac.com>
Greptile SummaryThis PR fixes the bug where the clarifier agent would prompt the user for clarification before using its bound search tools, by injecting an inline force-search retry inside
Confidence Score: 5/5Safe to merge — the inline retry is tightly guarded by iteration and tool-invocation checks, the two API-contract bugs from prior review rounds are both addressed, and 147 tests including dedicated regression tests for each fix all pass. The force-search nudge fires exactly once per conversation (gated on iteration==0 and absence of prior tool calls), is ephemeral (FORCE_SEARCH_GUIDANCE never enters state), and only retry_response is persisted — eliminating the consecutive-assistant-message violation. The skip-command HumanMessage interleaving and the budget-exhaustion early-exit guard close the other two previously identified issues. No new execution paths are left unguarded. No files require special attention; the one note is a test-assertion coverage gap that does not affect production behavior. Important Files Changed
Reviews (5): Last reviewed commit: "Fix consecutive assistant messages on th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@torkian Thank you for your contribution. We will review soon. |
ruff (project config: force-single-line=true) flagged the combined `from langchain_core.messages import HumanMessage, ToolMessage` line in scripts/e2e_clarifier_search_test.py. Split it into two single-line imports to match the rest of the codebase, and re-run ruff format. Signed-off-by: Torkian <torkian@mac.com>
Greptile review on PR NVIDIA-AI-Blueprints#245 flagged two real issues: P1 (agent.py): the ephemeral force_search guidance injection in agent_node was missing the iteration==0 guard that decide_route already applies. After force_search fired once and a stubborn model still refused to call a tool, ask_for_clarification ran, the user replied, and iteration advanced to 1 - but the next agent_node call still saw force_search_used=True with no tool invocations in state.messages and re-injected the nudge. The model then received "issue one focused tool call now" immediately after the user's clarifying answer, causing a gratuitous search instead of synthesizing the reply. Fixed by adding state.iteration == 0 to the injection condition. P2 (e2e script): the PASS verdict gated on tool_calls >= 1 only, without verifying len(user_prompts) == 0. A run where the LLM both searched AND prompted the user would have falsely printed "before any user prompt" and exited 0. Now we require both conditions and print distinct FAIL messages for the two failure modes. Also cleaned up an unused mock side_effect in test_force_search_guidance_not_in_state_messages (the user replies "skip" which forces completion, so only two LLM calls actually happen), and added test_force_search_guidance_not_injected_after_user_reply as a regression test for the P1 fix. 149 clarifier tests pass. Signed-off-by: Torkian <torkian@mac.com>
|
Thanks for the review! Addressed both findings in 8f18f80:
Also cleaned up an unused mock side_effect in |
cdgamarose-nv
left a comment
cdgamarose-nv
left a comment
There was a problem hiding this comment.
Thanks for your PR! Left some comments
Address review feedback on NVIDIA-AI-Blueprints#245: replace the dedicated force_search graph node and the ClarifierAgentState.force_search_used flag with a single inline retry inside agent_node. When, on the first turn, the model asks for clarification without first using its bound search tools, agent_node now nudges it once with FORCE_SEARCH_GUIDANCE and re-invokes inline, returning both the original response and the retry. This keeps the behavior model-agnostic without adding graph state or an extra node hop. The one-shot guarantee holds from two conditions alone, so the force_search_used flag is no longer needed: - iteration == 0: only fires on the first turn; once the user replies, iteration advances and it never fires again. - not _has_tool_invocations(state.messages): once any tool call is in history (e.g. after a successful forced search, even while iteration is still 0), it never re-nudges. FORCE_SEARCH_GUIDANCE is sent only in the local retry_messages and is never returned to state, so it cannot leak into get_latest_user_query or other history lookups. Also remove scripts/e2e_clarifier_search_test.py from the PR per review; the end-to-end test approach and results are documented in the PR description instead. Behavior is unchanged (same LLM call sequence and final message history); all 147 clarifier tests pass, including the force-search behavioral and ephemeral-guidance regression tests. Signed-off-by: Torkian <torkian@mac.com>
Greptile review on NVIDIA-AI-Blueprints#245: the inline search-before-clarify retry returned both the skipped clarification response and the retry response, {"messages": [response, retry_response]}. When the retry carries a tool call (the happy path), the persisted history became [..., AIMessage(clarification), AIMessage(tool_call), ToolMessage, ...] — two consecutive assistant-role messages, which the OpenAI Chat Completions and Anthropic Messages APIs reject with a 400. NVIDIA NIM tolerates it, so it did not surface in the NIM-backed e2e run, and the mocked unit tests did not enforce message ordering. Return only the retry response. The first attempt was already shown to the model inside the local retry_messages (ephemeral, never persisted), so dropping it from state loses nothing and keeps a valid sequence regardless of whether the retry is a tool call or another clarification. Add a regression test asserting no two consecutive AIMessages are ever sent to the LLM across a forced-search run; it fails against the old [response, retry_response] return and passes with the fix. Signed-off-by: Torkian <torkian@mac.com>
While auditing the message ordering for the forced-search fix, a separate pre-existing bug surfaced in the skip-command branch. When a user types a skip command, ask_clarification returned an AIMessage(complete) without persisting the user's reply, leaving it directly after the prior AIMessage(clarification) — two consecutive assistant messages. The unconditional ask_for_clarification -> agent edge then re-entered agent_node with the clarification budget exhausted, which appended a third AIMessage(complete). With enable_plan_approval enabled this corrupted history flowed into the planner's ainvoke, producing a 400 from the OpenAI/Anthropic APIs (NVIDIA NIM tolerates it). Two coordinated fixes: - The skip branch now persists the user's reply as a HumanMessage before the completion AIMessage, so the prior and new assistant turns are separated by a human message. - agent_node's budget-exhausted branch no longer emits a completion when the last message is already a completion AIMessage (the skip branch's), avoiding the duplicate on graph re-entry. Add regression tests covering both the persisted history ordering and the planner ainvoke sequence under enable_plan_approval; both fail against the prior behavior and pass with the fix. Signed-off-by: Torkian <torkian@mac.com>
torkian
changed the title
|
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:
WalkthroughClarifierAgent now forces a search-first retry on the first clarification turn, preserves skip replies as human messages, avoids duplicate assistant completions, and updates prompt/test coverage for the new message-ordering rules. ChangesClarifier search-before-clarify flow
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/clarifier/agent.py`:
- Around line 665-669: Normalize blank skip replies in the clarifier flow before
creating the return payload in the agent logic, since _is_skip_command() already
treats empty and whitespace-only input as skip. Update the branch that appends
HumanMessage so it uses a non-empty sentinel instead of persisting an empty user
turn, and add a regression test covering user_prompt_callback == "" with
enable_plan_approval=True that verifies the planner history does not contain an
empty HumanMessage.
In `@tests/aiq_agent/agents/clarifier/test_agent.py`:
- Around line 1300-1304: The completion-message check in the clarifier test is
too loose and does not enforce the intended invariant. Update the assertion
around `completion_ais` in `test_agent.py` to match only the terminal completion
`AIMessage` (not any non-tool `AIMessage`), and require exactly one match
instead of allowing `>= 1`. Use `ClarifierAgent._has_tool_invocations` and the
`completion_ais` filter to locate and tighten this logic.
🪄 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: e74d3d1c-9f3a-410a-b4ed-c0cfabe38091
📒 Files selected for processing (3)
src/aiq_agent/agents/clarifier/agent.pysrc/aiq_agent/agents/clarifier/prompts/research_clarification.j2tests/aiq_agent/agents/clarifier/test_agent.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
src/aiq_agent/agents/clarifier/prompts/research_clarification.j2src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/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/clarifier/prompts/research_clarification.j2src/aiq_agent/agents/clarifier/agent.py
**/*.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/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.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/clarifier/agent.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/clarifier/test_agent.py
🔇 Additional comments (2)
src/aiq_agent/agents/clarifier/prompts/research_clarification.j2 (1)
32-36: LGTM!src/aiq_agent/agents/clarifier/agent.py (1)
107-115: LGTM!Also applies to: 496-513, 533-547, 561-630, 702-706
Addresses two CodeRabbit findings on NVIDIA-AI-Blueprints#245: - agent_node skip branch: `_is_skip_command` treats "" and whitespace-only replies as skip, so the branch could persist an empty HumanMessage which then flows into plan generation; some chat APIs reject empty message content. Substitute a non-empty "[skipped clarification]" sentinel when the reply is blank. Adds a regression test that, with enable_plan_approval, the planner ainvoke never receives an empty HumanMessage. - Tighten the completion-message assertion in the skip-ordering test: match only the terminal completion AIMessage (needs_clarification=false via _is_complete) and require exactly one, instead of matching any non-tool AIMessage with len >= 1. 151 clarifier tests pass; ruff clean. Signed-off-by: Torkian <torkian@mac.com>
|
Did a deep pre-merge review pass to clear remaining edge cases in one go. Pushed in Fixed (in scope of this change):
Intentionally out of scope (pre-existing, not touched by this PR — flagging rather than expanding scope):
Happy to file follow-ups for those if you'd like them addressed. |
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/clarifier/agent.py (1)
633-669: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEnforce search-before-HITL instead of relying on the retry prompt.
Line 669 returns
retry_responseeven when it has notool_calls; if that response still asks for clarification,decide_routeroutes straight toask_for_clarification, so non-compliant models can still prompt the user before any search. That contradicts the model-agnostic “search first, ask second” objective. Please enforce an actual tool/search path before HITL, or explicitly change the contract/tests to best-effort prompting. As per path instructions,src/aiq_agent/agents/**/*: Review agent changes for research workflow correctness, graph state transitions, prompt/tool contracts, HITL behavior, and failure handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiq_agent/agents/clarifier/agent.py` around lines 633 - 669, The search-before-clarify retry in clarifier/agent.py is still allowing a clarification-only retry to flow into HITL. Update the logic around the one-shot retry so Clarifier only returns a retry result when it actually triggers a tool/search path, and otherwise continues to the normal clarification route or explicitly falls back per the intended contract. Use the existing symbols _searched_since_last_user_turn, decide_route, and FORCE_SEARCH_GUIDANCE to locate the control flow and make the search-first behavior enforced rather than prompt-only.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 `@src/aiq_agent/agents/clarifier/agent.py`:
- Around line 633-669: The search-before-clarify retry in clarifier/agent.py is
still allowing a clarification-only retry to flow into HITL. Update the logic
around the one-shot retry so Clarifier only returns a retry result when it
actually triggers a tool/search path, and otherwise continues to the normal
clarification route or explicitly falls back per the intended contract. Use the
existing symbols _searched_since_last_user_turn, decide_route, and
FORCE_SEARCH_GUIDANCE to locate the control flow and make the search-first
behavior enforced rather than prompt-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a706f4fd-ab4f-402b-980e-af4d62609eef
📒 Files selected for processing (2)
src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.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/clarifier/agent.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/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/clarifier/agent.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/clarifier/test_agent.py
🔇 Additional comments (2)
src/aiq_agent/agents/clarifier/agent.py (1)
116-118: LGTM!Also applies to: 518-538, 558-611, 673-759
tests/aiq_agent/agents/clarifier/test_agent.py (1)
911-939: LGTM!Also applies to: 941-1031, 1034-1107, 1109-1201, 1203-1262, 1265-1388, 1391-1435, 1438-1544
Pre-merge review surfaced several edge cases in the search-before-clarify and skip handling. Fixes: - Scope the search-before-clarify guard to the current request. It used _has_tool_invocations over the whole history, so a tool call from an earlier conversation turn could suppress the nudge for a fresh user query. New _searched_since_last_user_turn only counts tool calls after the latest HumanMessage. Adds unit + behavioral regression tests. - Harden the budget-exhausted branch in agent_node so it never produces two consecutive assistant messages. When the last turn is a non-complete AIMessage at exhaustion, interleave a sentinel user turn before the completion (previously it appended an adjacent completion that could 400 the planner under plan approval). Adds a regression test. - Promote the blank-skip sentinel to a module constant (SKIPPED_CLARIFICATION_SENTINEL), reused by the exhausted-completion interleave; fixes the casing drift with the log literal. Test polish: tighten two planner call_count asserts to == 1, make the fallback-query assertion require both tokens (and, not or), drop an unused captured dict, and add docstrings to the graph-node functions and the new test fixtures/helpers. 157 clarifier tests pass; ruff check + format clean. Signed-off-by: Torkian <torkian@mac.com>
torkian
force-pushed
the
fix/clarifier-search-before-questions
branch
from
e0740ed to
6fef9e3
Compare
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/clarifier/agent.py`:
- Around line 596-611: The completion path in ClarifierAgent currently treats
any last AIMessage the same, but pending tool-call messages must be handled
separately before inserting SKIPPED_CLARIFICATION_SENTINEL. Update the logic
around the last_message check in the clarification completion branch so that
AIMessage instances with tool_calls are either drained/handled first or routed
through the tool-call path, instead of directly returning the sentinel plus
completion. Keep the existing non-tool-call behavior for plain clarification
turns, but avoid emitting a history that leaves unresolved tool calls adjacent
to the completion message.
🪄 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: b2ffc1fe-7cb5-438c-bf7c-f5c9e2fe69a9
📒 Files selected for processing (2)
src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.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/clarifier/agent.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/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/clarifier/agent.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/clarifier/test_agent.py
🔇 Additional comments (5)
tests/aiq_agent/agents/clarifier/test_agent.py (1)
25-31: LGTM!Also applies to: 911-939, 959-1031, 1034-1106, 1109-1200, 1203-1262, 1265-1388, 1391-1544
src/aiq_agent/agents/clarifier/agent.py (4)
116-119: LGTM!
518-538: LGTM!
659-671: LGTM!
704-723: LGTM!
…dedupe helper Apply reviewer feedback on NVIDIA-AI-Blueprints#245: - Inject the search-before-clarify nudge as a trailing SystemMessage instead of a HumanMessage. A SystemMessage reads as scaffolding, so models don't weight it as the real user query; guidance is still ephemeral (never persisted to state). - Trim the oversized rationale comment on the retry block from ~26 lines to ~6; the detail was longer than the code and duplicated elsewhere. - Hoist the thrice-duplicated _adjacent_assistant_pairs test helper to a single module-level function and reuse it. 157 clarifier tests pass; ruff check + format clean. Signed-off-by: Torkian <torkian@mac.com>
At clarification-budget exhaustion, if the last message is an AIMessage with pending tool_calls, agent_node no longer stacks a completion on top of it (which would leave the tool call unresolved and produce a history the OpenAI/Anthropic APIs reject). It now returns no new message so decide_route sends the pending call to the tools node; the tool result then re-enters and completion proceeds validly. Adds a regression test. Signed-off-by: Torkian <torkian@mac.com>
|
Thanks for the review @cdgamarose-nv — addressed all of it:
158 clarifier tests pass; ruff check + format clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/aiq_agent/agents/clarifier/test_agent.py (2)
1085-1116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNegative-guidance check misses
SystemMessage.The loop at lines 1114-1116 only inspects
HumanMessageinstances forFORCE_SEARCH_GUIDANCE, but production code now injects the guidance via a trailingSystemMessage(peragent.py:SystemMessage(content=FORCE_SEARCH_GUIDANCE)). This check would pass even if a stray guidanceSystemMessagewere present.🔧 Proposed fix
- for m in second_call_messages: - if isinstance(m, HumanMessage): - assert FORCE_SEARCH_GUIDANCE not in m.content + for m in second_call_messages: + if isinstance(m, (HumanMessage, SystemMessage)): + assert FORCE_SEARCH_GUIDANCE not in m.content🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/aiq_agent/agents/clarifier/test_agent.py` around lines 1085 - 1116, The negative-guidance assertion in test_force_search_not_triggered_when_llm_searches_first only checks HumanMessage entries, so it can miss FORCE_SEARCH_GUIDANCE if it is injected as a SystemMessage. Update the check to inspect all messages passed to mock_llm.ainvoke on the second call, especially the trailing SystemMessage added by ClarifierAgent/agent.py, and assert that none of them contain the guidance text.
1165-1211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegression test no longer covers the bug it targets.
This test's docstring explicitly states it's a regression test for re-injection of
FORCE_SEARCH_GUIDANCEafter the user replies (Greptile P1 finding), but the check at lines 1206-1210 only scansHumanMessageinstances. Since guidance is now injected as aSystemMessage, a reintroduced bug where guidance leaks into aSystemMessageon the post-reply turn would go undetected — defeating the stated purpose of this regression test.🔧 Proposed fix
- third_call_messages = mock_llm.ainvoke.call_args_list[2].args[0] - for m in third_call_messages: - if isinstance(m, HumanMessage): - assert FORCE_SEARCH_GUIDANCE not in m.content, ( - "force_search guidance must not be re-injected after the user replies" - ) + third_call_messages = mock_llm.ainvoke.call_args_list[2].args[0] + for m in third_call_messages: + if isinstance(m, (HumanMessage, SystemMessage)): + assert FORCE_SEARCH_GUIDANCE not in m.content, ( + "force_search guidance must not be re-injected after the user replies" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/aiq_agent/agents/clarifier/test_agent.py` around lines 1165 - 1211, The regression test in ClarifierAgent no longer verifies the actual bug path because it only inspects HumanMessage entries, while FORCE_SEARCH_GUIDANCE is injected via SystemMessage on the post-reply turn. Update test_force_search_guidance_not_injected_after_user_reply to examine the third mock_llm.ainvoke call’s full message list and assert the guidance is absent from any message content, especially SystemMessage, so the test still protects the ClarifierAgent.run flow after the user reply.
🤖 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/clarifier/agent.py`:
- Around line 648-649: The retry prompt in clarifier agent logic is appending a
trailing SystemMessage, which can break Anthropic-backed retries. Update the
retry path in the agent flow around retry_messages and retry_response so
FORCE_SEARCH_GUIDANCE is merged into the initial system prompt content instead
of being added after response, and keep any later retry messages as non-system
messages only.
---
Outside diff comments:
In `@tests/aiq_agent/agents/clarifier/test_agent.py`:
- Around line 1085-1116: The negative-guidance assertion in
test_force_search_not_triggered_when_llm_searches_first only checks HumanMessage
entries, so it can miss FORCE_SEARCH_GUIDANCE if it is injected as a
SystemMessage. Update the check to inspect all messages passed to
mock_llm.ainvoke on the second call, especially the trailing SystemMessage added
by ClarifierAgent/agent.py, and assert that none of them contain the guidance
text.
- Around line 1165-1211: The regression test in ClarifierAgent no longer
verifies the actual bug path because it only inspects HumanMessage entries,
while FORCE_SEARCH_GUIDANCE is injected via SystemMessage on the post-reply
turn. Update test_force_search_guidance_not_injected_after_user_reply to examine
the third mock_llm.ainvoke call’s full message list and assert the guidance is
absent from any message content, especially SystemMessage, so the test still
protects the ClarifierAgent.run flow after the user reply.
🪄 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: 08e7b9e8-a1b4-487f-9cf8-57bead036337
📒 Files selected for processing (2)
src/aiq_agent/agents/clarifier/agent.pytests/aiq_agent/agents/clarifier/test_agent.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run ruff check and ruff format validation for Python code changes
**/*.py: Python code must be linted and formatted with Ruff using line length 120, target Python 3.11, rule sets E,F,W,I,PL,UP, and isort force-single-line configuration
Never commit secrets, tokens, or environment-specific hostnames; use environment variables and SecretStr instead, resolving API keys at runtime
Never print or log secret values, including in tool output or error messages
Missing-secret paths must degrade gracefully (stub/skip), not crash or leak
Do not hand-reformat unrelated code when making changes; match the existing import and formatting style
Files:
tests/aiq_agent/agents/clarifier/test_agent.pysrc/aiq_agent/agents/clarifier/agent.py
**/*test*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run pytest for all behavior changes in Python code
Files:
tests/aiq_agent/agents/clarifier/test_agent.py
**
⚙️ CodeRabbit configuration file
**:AI-Q Agent Guidance
Repository-global instructions for coding agents and for humans reviewing
agent-authored changes. These rules apply to every task in this repository.
Task-specific runbooks live in.agents/skills/— load the
relevant skill before starting a workflow it covers.Project overview
AI-Q is an NVIDIA AI Blueprint: an enterprise research agent built on the
NeMo Agent Toolkit (NAT). The deployed product is a research blueprint, not
a general skill runtime. New retrieval sources and tools are NAT functions;
agent behavior is driven by workflow YAML, Jinja2 prompts, and a data-source
registry — not by hard-coded logic.Primary boundaries:
- Backend Python package:
src/aiq_agent/.- Data-source and tool packages:
sources/(each is its own package).- Frontends and tooling:
frontends/(web UI infrontends/ui/, eval harnesses
infrontends/benchmarks/).- Configs, deployment, docs:
configs/,deploy/,docs/.Stay inside this repository. If your workspace also contains adjacent repos
(for example a sibling NeMo-Relay checkout), do not edit them as part of an AI-Q
change. Treatsources/*as independent packages: prefer the smallest change
scoped to the package you are touching.Repository structure
Path Purpose src/aiq_agent/Backend agent, FastAPI extensions, auth, observability, knowledge sources/Data-source / tool packages (e.g. tavily_web_search,google_scholar_paper_search)configs/Workflow YAML configs (e.g. config_cli_default.yml)frontends/ui/Next.js / React / TypeScript / Tailwind / KUI web UI frontends/benchmarks/Eval harnesses: freshqa,deepsearch_qa,deepresearch_benchdeploy/Docker Compose and Helm/Kubernetes assets; deploy/.envfor secretsdocs/source/...
Files:
tests/aiq_agent/agents/clarifier/test_agent.pysrc/aiq_agent/agents/clarifier/agent.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/clarifier/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/clarifier/agent.py
🔇 Additional comments (7)
tests/aiq_agent/agents/clarifier/test_agent.py (5)
25-32: LGTM!Also applies to: 47-55
968-1012: LGTM! Assertion correctly matches the trailingSystemMessagescaffolding per the updated implementation.
1213-1264: LGTM! Consecutive-assistant-message invariant and tool-call adjacency are both verified against every mocked LLM call.
1267-1428: LGTM! Skip-branch ordering, HumanMessage persistence, and blank-reply sentinel behavior are all exercised correctly, including under plan approval.
1431-1528: LGTM! Both the prior-turn-tool-call scoping and budget-exhaustion sentinel-interleaving regressions correctly assert against the module-level_adjacent_assistant_pairshelper and theSystemMessage-based guidance check.src/aiq_agent/agents/clarifier/agent.py (2)
606-610: Already flagged: drain pending tool calls before completing.This branch still treats any
AIMessageas a plain clarification turn; if it carriestool_calls, the sentinel + completion leaves unresolved tool calls in history. As per path instructions,src/aiq_agent/agents/**/*: “Review agent changes for research workflow correctness, graph state transitions, prompt/tool contracts, HITL behavior, and failure handling.”Source: Path instructions
500-537: LGTM!Also applies to: 558-572, 681-704, 713-740
A trailing SystemMessage in the forced-search retry is rejected by providers that only accept a leading system message (langchain-anthropic raises on non-consecutive system messages before the request is even sent). Merge FORCE_SEARCH_GUIDANCE into the leading system prompt for the retry instead — it stays system-level scaffolding rather than a fake user turn, and the message list remains valid for every supported wrapper. Test updates: - Assert the retry carries exactly one system message, in the leading position, containing the guidance. - Replace three negative assertions that only scanned HumanMessages (vacuous after the guidance moved to the system prompt) with checks over all messages, and verify the guidance never persists into state.messages via the final graph state. - Harden the pending-tool-call node test to require a dict result instead of silently passing on other shapes. 158 clarifier tests pass; ruff check + format clean. Signed-off-by: Torkian <torkian@mac.com>
Signed-off-by: Torkian <torkian@mac.com>
|
@cdgamarose-nv branch is up to date with |
|
/ok to test bd866f4 |
|
/ok to test 7dcdeef |
3 participants
Closes #234.
Summary
The clarifier agent already had tool-calling bound to a search loop, but weak or non-frontier models routinely skipped the search and asked the user a clarification question immediately. The expected behavior — and what the issue asks for — is to gather context with the available tools first, and only ask the user once a genuine ambiguity remains.
This fixes the bug at the agent level (the issue explicitly says "this should not be overly prompt engineered for one model"), with a small prompt update as a supporting hint.
Changes
agent_node. On the first turn, if the model returns a clarification request without using its bound search tools, the node nudges it once withFORCE_SEARCH_GUIDANCEand re-invokes the LLM inline, returning both the original response and the retry. No extra graph node or state field is needed.iteration == 0(so it never fires after the user has replied) andnot _has_tool_invocations(state.messages)(so once any tool call is in history — e.g. after a successful forced search, even while iteration is still 0 — it never re-nudges). A model that stubbornly refuses to search after the single nudge simply falls through to asking the user; no loop.FORCE_SEARCH_GUIDANCEis sent only in the localretry_messagesand is never returned to state, so it cannot leak intoget_latest_user_queryor other history lookups (which would otherwise surface internal scaffolding in fallback text).prompts/research_clarification.j2Tool Usage section rewritten as "Search first, ask second" so frontier models follow this voluntarily and skip the nudge entirely.How I verified (end-to-end, against a real LLM + web search)
I exercised the full clarifier graph against a real NVIDIA NIM-hosted LLM and a real Tavily web search tool, with model
nvidia/nemotron-3-nano-30b-a3b(the repo default inconfigs/config_cli_default.yml) and the query "Research the latest NVIDIA AI announcements from 2026":developDeterministic across three runs. (The throwaway reproducer script is not included in the PR, per review — these are its results.)
Tests
150 clarifier tests pass. Key additions in
tests/aiq_agent/agents/clarifier/test_agent.py:TestHasToolInvocations— the helper that detects prior tool calls in the message history.TestClarifierForceSearch:test_force_search_fires_when_llm_skips_tools— the bug path: model clarifies → nudge + inline retry → model emits a tool call → completes; user never prompted.test_force_search_skipped_when_no_tools— no behavior change when no tools are configured.test_force_search_fires_at_most_once— no loop if the model stubbornly refuses tools after the nudge.test_force_search_not_triggered_when_llm_searches_first— no extra detour for models that already search.test_force_search_guidance_not_in_state_messages— regression test: the internal guidance must never appear in user-facing fallback text.test_force_search_guidance_not_injected_after_user_reply— regression test: the nudge does not re-fire after the user has answered.test_forced_retry_does_not_emit_consecutive_assistant_messages— regression test for the message-ordering fix below.TestClarifierSkipMessageOrdering— regression tests for the skip-path ordering fix (persisted history + planner sequence underenable_plan_approval).Message-ordering correctness (from review)
Per @cdgamarose-nv's review the implementation was simplified to an inline retry; follow-up review feedback then flagged that returning both the skipped and retried responses produced two consecutive assistant-role messages once the retry carried a tool call, which the OpenAI/Anthropic APIs reject with a 400 (NVIDIA NIM tolerates it). Fixed by returning only the retry response. Auditing the same invariant across the graph surfaced a separate pre-existing instance on the skip-clarification path (skip emitted a completion without persisting the user reply, and graph re-entry appended a duplicate completion) — also fixed, with regression tests for both.
Test plan
pytest tests/aiq_agent/agents/clarifier/— 150/150 passingruff checkandruff formatcleandevelopagainst the repo's default model