Dev/mahikaw/agentic enhancements

[mahikaw]

Description

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[greptile-apps]

Greptile Summary

This PR makes three agentic enhancements: sets a default LLM model (nvidia/llama-3.3-nemotron-super-49b-v1.5) so --agentic works out-of-the-box without --agentic-llm-model; adds a NEMO_RETRIEVER_AGENTIC_LLM_MODEL env-var override; and re-hydrates full RetrievalHit metadata (text, source, page_number, …) into the agentic output so it matches what the dense path returns.

• Retrieval knob forwarding: candidate_k, page_dedup, and content_types are now threaded through AgenticRetrievalConfig and applied on every ReAct retrieval hop, mirroring the dense path's options.
• Hit cache: _hit_cache is introduced in AgenticRetriever to capture full hits at the retrieval boundary before the agent reduces them to doc_ids, then re-hydrate the output.
• Logging change: quiet_capture() is removed for the agentic path and replaced with logging.basicConfig(force=True) so ReAct step progress is surfaced on stderr.

Confidence Score: 3/5

The agentic output enrichment logic is well-structured and test-covered, but a behavioural inconsistency in content_types handling between the dense and agentic paths could cause a silent runtime failure.

The content_types normalization is missing from the agentic workflow — the dense path converts Sequence[str] to a comma-joined string before calling Retriever.query, but the agentic path passes the raw list through. Any programmatic caller passing content_types=["text", "image"] will see different behaviour or a runtime failure on the agentic branch.

workflow.py (content_types normalization gap) and agentic.py (candidate_k falsy check, _hit_cache.clear without lock) deserve the most attention before merge.

Important Files Changed

Filename	Overview
nemo_retriever/src/nemo_retriever/query/workflow.py	Passes content_types directly from QueryRetrievalOptions to AgenticRetrievalConfig without the sequence-to-string normalization that the non-agentic resolve_query_plan path applies; enriches agentic output with re-hydrated hit metadata.
nemo_retriever/src/nemo_retriever/query/agentic.py	Adds candidate_k/page_dedup/content_types forwarding to per-hop retrieval and introduces _hit_cache for metadata re-hydration; the cache clear is not lock-protected and candidate_k=0 is silently ignored.
nemo_retriever/src/nemo_retriever/cli/query/app.py	Removes quiet_capture() around agentic retrieval and adds logging.basicConfig(force=True) to surface ReAct progress; force=True aggressively replaces root handlers and the missing explicit stream= is a latent test fragility.
nemo_retriever/src/nemo_retriever/query/options.py	Adds DEFAULT_AGENTIC_LLM_MODEL constant and changes llm_model default in QueryAgenticOptions from None to the 49B Nemotron model; clean change.
nemo_retriever/src/nemo_retriever/cli/query/options.py	Adds NEMO_RETRIEVER_AGENTIC_LLM_MODEL envvar support and updated help text for --agentic-llm-model; straightforward and correct.
nemo_retriever/tests/test_agentic_eval.py	Adds tests for hit re-hydration (hit column assertions) and for retrieval knob forwarding (candidate_k/page_dedup/content_types); FakeRetriever extended with query_calls tracking; good coverage of happy path.
nemo_retriever/tests/test_root_query_cli.py	Replaces the 'requires --agentic-llm-model' test with a test asserting the default 49B model is used when the flag is omitted; correct for the new behavior.

Prompt To Fix All With AI

Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
nemo_retriever/src/nemo_retriever/query/workflow.py:168-170
**`content_types` sequence not normalized for agentic path**

The non-agentic path in `resolve_query_plan()` explicitly converts a `Sequence[str]` to a comma-joined string before passing it to `Retriever.query`. The agentic path passes `request.retrieval.content_types` raw to `AgenticRetrievalConfig`, which then forwards it directly to `self._retriever.query(content_types=...)`. If a caller passes `content_types=["text", "image"]` programmatically, the agentic path will hand a `list` to `Retriever.query` while the dense path would produce `"text,image"`, meaning the same option behaves differently across the two paths and may fail at runtime in the agentic case.

### Issue 2 of 4
nemo_retriever/src/nemo_retriever/query/agentic.py:286
**`candidate_k=0` silently treated as `None`**

`if self._cfg.candidate_k else None` is a falsy check. `candidate_k=0` is falsy in Python, so it would be silently ignored and treated as `None` rather than triggering the floor calculation. An integer check is safer here and makes the intent explicit.

```suggestion
        candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k is not None else None
```

### Issue 3 of 4
nemo_retriever/src/nemo_retriever/query/agentic.py:215
**`_hit_cache.clear()` called without holding `self._lock`**

`self._hit_cache` is shared across concurrent `_retrieve_for_agent` workers (which hold `self._lock` when writing). If `retrieve()` is ever called on the same `AgenticRetriever` instance from two threads simultaneously, the `clear()` here races with `setdefault()` calls from workers of the first invocation. The current usage pattern (new instance per call in `agentic_query_documents`) is safe, but the absence of a lock on `clear()` leaves the class undocumented as "not safe for concurrent `retrieve()` calls" — a footgun for future users who hold onto an `AgenticRetriever` and call it concurrently.

### Issue 4 of 4
nemo_retriever/src/nemo_retriever/cli/query/app.py:251-255
**`logging.basicConfig(force=True)` without explicit `stream` parameter**

`force=True` replaces all existing handlers on the root logger. When this CLI runs under Typer's `CliRunner` (which defaults to `mix_stderr=True`), log lines emitted to stderr appear in `result.output` and break any `json.loads(result.output)` assertions. Passing `stream=sys.stderr` explicitly makes the intent clear and is safer if `sys.stderr` is swapped by the test runner.

```suggestion
            logging.basicConfig(
                level=logging.INFO,
                format="%(asctime)s %(levelname)s %(name)s: %(message)s",
                stream=sys.stderr,
                force=True,
            )
```

_{Reviews (1): Last reviewed commit: "Reuse agentic LLM default in CLI help" | Re-trigger Greptile}

[greptile-apps]

content_types sequence not normalized for agentic path

The non-agentic path in resolve_query_plan() explicitly converts a Sequence[str] to a comma-joined string before passing it to Retriever.query. The agentic path passes request.retrieval.content_types raw to AgenticRetrievalConfig, which then forwards it directly to self._retriever.query(content_types=...). If a caller passes content_types=["text", "image"] programmatically, the agentic path will hand a list to Retriever.query while the dense path would produce "text,image", meaning the same option behaves differently across the two paths and may fail at runtime in the agentic case.

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/workflow.py
Line: 168-170

Comment:
**`content_types` sequence not normalized for agentic path**

The non-agentic path in `resolve_query_plan()` explicitly converts a `Sequence[str]` to a comma-joined string before passing it to `Retriever.query`. The agentic path passes `request.retrieval.content_types` raw to `AgenticRetrievalConfig`, which then forwards it directly to `self._retriever.query(content_types=...)`. If a caller passes `content_types=["text", "image"]` programmatically, the agentic path will hand a `list` to `Retriever.query` while the dense path would produce `"text,image"`, meaning the same option behaves differently across the two paths and may fail at runtime in the agentic case.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[greptile-apps]

candidate_k=0 silently treated as None

if self._cfg.candidate_k else None is a falsy check. candidate_k=0 is falsy in Python, so it would be silently ignored and treated as None rather than triggering the floor calculation. An integer check is safer here and makes the intent explicit.

Suggested change

	candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k else None
	candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k is not None else None

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/agentic.py
Line: 286

Comment:
**`candidate_k=0` silently treated as `None`**

`if self._cfg.candidate_k else None` is a falsy check. `candidate_k=0` is falsy in Python, so it would be silently ignored and treated as `None` rather than triggering the floor calculation. An integer check is safer here and makes the intent explicit.

```suggestion
        candidate_k = max(int(self._cfg.candidate_k), int(top_k)) if self._cfg.candidate_k is not None else None
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

_hit_cache.clear() called without holding self._lock

self._hit_cache is shared across concurrent _retrieve_for_agent workers (which hold self._lock when writing). If retrieve() is ever called on the same AgenticRetriever instance from two threads simultaneously, the clear() here races with setdefault() calls from workers of the first invocation. The current usage pattern (new instance per call in agentic_query_documents) is safe, but the absence of a lock on clear() leaves the class undocumented as "not safe for concurrent retrieve() calls" — a footgun for future users who hold onto an AgenticRetriever and call it concurrently.

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/query/agentic.py
Line: 215

Comment:
**`_hit_cache.clear()` called without holding `self._lock`**

`self._hit_cache` is shared across concurrent `_retrieve_for_agent` workers (which hold `self._lock` when writing). If `retrieve()` is ever called on the same `AgenticRetriever` instance from two threads simultaneously, the `clear()` here races with `setdefault()` calls from workers of the first invocation. The current usage pattern (new instance per call in `agentic_query_documents`) is safe, but the absence of a lock on `clear()` leaves the class undocumented as "not safe for concurrent `retrieve()` calls" — a footgun for future users who hold onto an `AgenticRetriever` and call it concurrently.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logging.basicConfig(force=True) without explicit stream parameter

force=True replaces all existing handlers on the root logger. When this CLI runs under Typer's CliRunner (which defaults to mix_stderr=True), log lines emitted to stderr appear in result.output and break any json.loads(result.output) assertions. Passing stream=sys.stderr explicitly makes the intent clear and is safer if sys.stderr is swapped by the test runner.

Suggested change

	logging.basicConfig(
	level=logging.INFO,
	format="%(asctime)s %(levelname)s %(name)s: %(message)s",
	force=True,
	)
	logging.basicConfig(
	level=logging.INFO,
	format="%(asctime)s %(levelname)s %(name)s: %(message)s",
	stream=sys.stderr,
	force=True,
	)

Prompt To Fix With AI

This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/cli/query/app.py
Line: 251-255

Comment:
**`logging.basicConfig(force=True)` without explicit `stream` parameter**

`force=True` replaces all existing handlers on the root logger. When this CLI runs under Typer's `CliRunner` (which defaults to `mix_stderr=True`), log lines emitted to stderr appear in `result.output` and break any `json.loads(result.output)` assertions. Passing `stream=sys.stderr` explicitly makes the intent clear and is safer if `sys.stderr` is swapped by the test runner.

```suggestion
            logging.basicConfig(
                level=logging.INFO,
                format="%(asctime)s %(levelname)s %(name)s: %(message)s",
                stream=sys.stderr,
                force=True,
            )
```

How can I resolve this? If you propose a fix, please make it concise.