fix: do not forward the unknown-model sentinel as a per-request model override

[knQzx]

Description

Closes #2110

when a model can't be resolved, the ReAct agent builder receives the sentinel model="unknown-model" and forwards it straight through as a per-request configurable.model_name override, so every downstream LLM/NIM call is told to use a model literally named unknown-model and the provider returns 404. this is a regression from #2036, which introduced the sentinel but didn't guard against forwarding it

the fix treats the unknown-model sentinel the same as an unset model - the per-request model override is only applied when a real model name is present

reproduce: build a ReAct agent whose model resolves to the unknown-model sentinel (an unresolved model reference), then run it. on develop the request carries configurable: {model_name: "unknown-model"} and the call 404s; with this change no model override is attached and the configured default is used. the added test_build_lc_config_ignores_unknown_model_sentinel asserts the sentinel is not forwarded

By Submitting this PR I confirm:

• I am familiar with the Contributing Guidelines
• We require that all contributors "sign-off" on their commits - this commit is signed off
• New tests cover these changes
• The documentation is up to date with these changes

Summary by CodeRabbit

• Bug Fixes
  • Prevented unspecified or unknown model values from being incorrectly applied as per-request model overrides.
  • Preserved validation and support checks when a valid model override is provided.
• Tests
  • Added a regression test to ensure unknown/missing model values do not add per-request override configuration, while valid model strings still do.

[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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 53d0546b-1a24-45f1-bdd0-e36a73f2697f

📥 Commits

Reviewing files that changed from the base of the PR and between f74768e and b23575c.

📒 Files selected for processing (2)

• packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py
• packages/nvidia_nat_langchain/tests/agent/test_react.py

🚧 Files skipped from review as they are similar to previous changes (2)

• packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py
• packages/nvidia_nat_langchain/tests/agent/test_react.py

---

Walkthrough

The React agent adds an unknown-model sentinel and prevents it or None from becoming LangChain model overrides. Tests cover sentinel, absent, and valid model values.

Changes

Model override handling

Layer / File(s)	Summary
Filter unspecified model overrides packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py, packages/nvidia_nat_langchain/tests/agent/test_react.py	_build_lc_config ignores the unknown-model sentinel and None, while valid model names still populate the LangChain configurable override. Regression tests cover all three cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change and uses an imperative form.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py (1)

46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the sentinel comment and definition with the upstream contract.

ChatRequest.model defaults to None; only the plain-string converter injects "unknown-model". This comment is misleading, and the sentinel literal is duplicated across packages. Prefer a shared constant or update the comment to document the converter behavior.

🤖 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
`@packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py`
around lines 46 - 48, The _UNKNOWN_MODEL_SENTINEL definition and comment should
reflect the upstream contract: ChatRequest.model defaults to None, while only
the plain-string converter supplies "unknown-model". Reuse the existing shared
sentinel constant if available; otherwise update the comment to describe the
converter behavior without implying ChatRequest.model uses it by default.

packages/nvidia_nat_langchain/tests/agent/test_react.py (1)

1809-1811: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Cover the unsupported-backend sentinel path.

Add an assertion using supports_override=False; this ensures the sentinel is ignored before the backend capability check and can never raise an unsupported-override error.

Proposed test addition

     assert "configurable" not in _build_lc_config(5, sentinel_model)
+    assert "configurable" not in _build_lc_config(5, sentinel_model, supports_override=False)
     assert "configurable" not in _build_lc_config(5, None)

🤖 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 `@packages/nvidia_nat_langchain/tests/agent/test_react.py` around lines 1809 -
1811, Extend the sentinel-model assertions in the test covering _build_lc_config
to pass supports_override=False, verifying the sentinel is ignored before
backend capability validation and does not raise an unsupported-override error.
Keep the existing assertions for the default behavior and None model unchanged.

🤖 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 `@packages/nvidia_nat_langchain/tests/agent/test_react.py`:
- Around line 1802-1803: Update the regression test docstring in the test
covering the `unknown-model` sentinel so its opening descriptive sentence is
concise, ends with a period on the same line, and uses backticks around code
entities such as `unknown-model` and `ChatRequest`.

---

Nitpick comments:
In
`@packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py`:
- Around line 46-48: The _UNKNOWN_MODEL_SENTINEL definition and comment should
reflect the upstream contract: ChatRequest.model defaults to None, while only
the plain-string converter supplies "unknown-model". Reuse the existing shared
sentinel constant if available; otherwise update the comment to describe the
converter behavior without implying ChatRequest.model uses it by default.

In `@packages/nvidia_nat_langchain/tests/agent/test_react.py`:
- Around line 1809-1811: Extend the sentinel-model assertions in the test
covering _build_lc_config to pass supports_override=False, verifying the
sentinel is ignored before backend capability validation and does not raise an
unsupported-override error. Keep the existing assertions for the default
behavior and None model unchanged.

🪄 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: CHILL

Plan: Enterprise

Run ID: 696a05f2-208f-4ea5-a669-d496745ff658

📥 Commits

Reviewing files that changed from the base of the PR and between baf355b and f74768e.

📒 Files selected for processing (2)

• packages/nvidia_nat_langchain/src/nat/plugins/langchain/agent/react_agent/register.py
• packages/nvidia_nat_langchain/tests/agent/test_react.py

[chuenchen309]

I verified this independently against develop (2618705). The bug is real and the fix is in the right place; two notes below, one of which I think is worth acting on.

The chain, link by link. _string_to_nat_chat_request is registered on GlobalTypeConverter and hardcodes the sentinel:

def _string_to_nat_chat_request(data: str) -> ChatRequest:
    return ChatRequest.from_string(data, model="unknown-model")   # api_server.py:954-955

_response_fn converts through exactly that path (register.py:169) and hands message.model to _build_lc_config (register.py:184). The remaining question was whether the sentinel survives validation, and it does — _MODEL_NAME_RE accepts it:

>>> _MODEL_NAME_RE.match("unknown-model")
<re.Match object; span=(0, 13), match='unknown-model'>

So on develop, any plain-string input to a ReAct agent attaches configurable: {"model_name": "unknown-model"}. Confirmed.

Fixing inside _build_lc_config rather than at the call sites matters more than it looks. There is a third caller outside this file: register_per_user_agent.py:89 imports _build_lc_config and passes message.model the same way. That import and call arrived in #2036 alongside the sentinel, so per_user_react_agent has the identical defect. A guard placed at the two call sites in register.py would have left it broken; this patch covers all three. Might be worth a line in the description, since the diff touching only react_agent/register.py makes it look narrower than it is.

The one thing I'd change: the sentinel now has two independent definitions. _UNKNOWN_MODEL_SENTINEL is declared in react_agent/register.py, but the value originates in api_server.py, where it appears as a bare literal in six places (lines 375, 401, 430, 455, 489, 955). If any of those is ever changed, this guard keeps comparing against the old string and silently stops matching — the failure mode is the original 404, with no test catching it, because the test derives its input from the converter and would follow the new value.

This repo already has the pattern for the fix: STDOUT_ENDPOINT_SENTINEL in nat/utils/telemetry/config.py:82, referenced from handler.py:230. Exporting something like UNKNOWN_MODEL_SENTINEL from api_server.py and importing it here would make the coupling explicit and let the six literals collapse onto it later.

Minor, and I think intentional: with the sentinel short-circuiting the whole block, it no longer reaches the supports_override check either. That is the correct behaviour — the user did not request a model, so raising "the configured inference backend does not support per-request model selection" at them would be wrong — but it does mean a backend without override support now silently accepts string input that previously raised. Worth stating explicitly rather than leaving as a side effect of the guard's placement.

AI-assisted review. The code paths above were read on develop at 2618705 and the regex match was executed; I did not build the package or run the test suite, so the pass/fail claims in the PR description are not independently confirmed here.