fix(nemo-agents): strip workspace qualifier in eval preflight (#934)

* fix(nemo-agents): strip workspace qualifier in eval preflight

validate_llm_models rejected workspace-qualified default models (e.g.
default/nvidia-nemotron) because the VM route takes workspace as its own
path segment. Strip a leading {workspace}/ before retrieve, mirroring
IGW's OpenAI proxy removeprefix pattern.

Fixes ASTD-271

Signed-off-by: Nathan Walston <nwalston@nvidia.com>

* fix(nemo-agents): guard empty model name after workspace strip

A model_name of exactly {workspace}/ (e.g. default/) normalized to an
empty string, bypassing the pre-strip non-empty check and reaching the
SDK. Re-check non-empty after removeprefix and skip. Adds dedup-across-
normalized-forms and empty-after-strip regression tests (CodeRabbit).

Signed-off-by: Nathan Walston <nwalston@nvidia.com>

* refactor(nemo-agents): use parse_qualified_name for model workspace resolution

Replace the manual removeprefix strip with nemo_platform_plugin.entities.
parse_qualified_name, which splits any workspace qualifier and returns the
target (workspace, name). A qualified model now routes to its own workspace
(cross-workspace correct), not just the path workspace. Dedup and NotFound
tracking move to the resolved (workspace, name) pair. Trims verbose comments
per review.

Signed-off-by: Nathan Walston <nwalston@nvidia.com>

---------

Signed-off-by: Nathan Walston <nwalston@nvidia.com>

Author: walston

plugins/nemo-agents/src/nemo_agents_plugin/utils.py

@@ -16,6 +16,7 @@
 
 import yaml
 from nemo_platform import NeMoPlatform, NotFoundError
+from nemo_platform_plugin.entities import parse_qualified_name
 
 logger = logging.getLogger(__name__)
 
@@ -317,9 +318,11 @@ def validate_llm_models(
     """Pre-flight check that every IGW-routed LLM in *config* exists as a VirtualModel.
 
     Iterates ``config["llms"]`` and, for each block whose ``_type`` is in
-    :data:`_IGW_LLM_TYPES`, calls
-    ``sdk.inference.virtual_models.retrieve(model_name, workspace=workspace)``.
-    Names are deduplicated before lookup so the same model declared under
+    :data:`_IGW_LLM_TYPES`, strips a leading ``{workspace}/`` qualifier from
+    ``model_name`` (mirroring IGW's OpenAI proxy — the VM route takes
+    workspace as its own path segment, so the bare name is what it expects)
+    then calls ``sdk.inference.virtual_models.retrieve(name, workspace=workspace)``.
+    Names are deduplicated *after* stripping so the same model declared under
     multiple LLM keys (e.g. agent + judge) costs one network call.
 
     LLM blocks whose ``model_name`` still contains an unexpanded ``$VAR`` /
@@ -349,11 +352,9 @@ def validate_llm_models(
     if not isinstance(llms, dict):
         return
 
-    # Collect (llm_key, model_name) pairs we should validate, deduped by
-    # model_name so multiple LLMs pointing at the same model only cost one
-    # retrieve call.  We keep the first llm_key we saw for each model_name
-    # so error messages can point at a concrete YAML location.
-    to_check: dict[str, str] = {}
+    # Dedup by resolved (workspace, name) so the same model under multiple LLM
+    # keys costs one retrieve; keep the first llm_key for error locations.
+    to_check: dict[tuple[str, str], str] = {}
     for llm_key, llm_cfg in llms.items():
         if not isinstance(llm_cfg, dict):
             continue
@@ -370,31 +371,33 @@ def validate_llm_models(
                 model_name,
             )
             continue
-        to_check.setdefault(model_name, llm_key)
+        # A model_name may be workspace-qualified ("prod/foo"); the VM route
+        # takes workspace as its own path segment, so resolve it here.
+        target_ws, target_name = parse_qualified_name(model_name, default_workspace=workspace)
+        if not target_name:
+            continue
+        to_check.setdefault((target_ws, target_name), llm_key)
 
     if not to_check:
         return
 
-    missing: list[tuple[str, str]] = []  # (model_name, llm_key)
-    for model_name, llm_key in to_check.items():
+    missing: list[tuple[str, str]] = []  # (qualified_name, llm_key)
+    for (target_ws, target_name), llm_key in to_check.items():
         try:
-            sdk.inference.virtual_models.retrieve(name=model_name, workspace=workspace)
+            sdk.inference.virtual_models.retrieve(name=target_name, workspace=target_ws)
         except NotFoundError:
-            missing.append((model_name, llm_key))
+            missing.append((f"{target_ws}/{target_name}", llm_key))
         except Exception as exc:  # pragma: no cover - defensive soft-fail
-            # Soft-fail: log and continue.  We don't want a transient platform
-            # outage or auth blip to gate the eval/optimize run; the
-            # subprocess will fail with the real error if the model truly
-            # isn't reachable.  ``exc_info`` preserves the traceback so a
-            # debugger looking at the warning has the full chain of context
-            # (which we'd otherwise drop by formatting ``exc`` via ``%s``).
+            # Soft-fail: a transient platform outage or auth blip shouldn't gate
+            # the run; the subprocess surfaces the real error. exc_info keeps the
+            # traceback.
             logger.warning(
-                "Could not validate LLM %r (model_name=%r) against workspace %r: %s. "
+                "Could not validate LLM %r (model=%r) against workspace %r: %s. "
                 "Continuing anyway; the underlying eval/optimize call will surface any "
                 "real error.",
                 llm_key,
-                model_name,
-                workspace,
+                target_name,
+                target_ws,
                 exc,
                 exc_info=exc,
             )

plugins/nemo-agents/tests/unit/test_utils.py

@@ -1473,6 +1473,83 @@ def test_happy_path_dedupes_repeated_model_names(self) -> None:
 
         assert vms.calls == [{"name": "shared-model", "workspace": "default"}]
 
+    def test_strips_workspace_qualifier_before_lookup(self) -> None:
+        """A ``{workspace}/model`` value is stripped to bare ``model`` before retrieve.
+
+        Mirrors IGW's OpenAI proxy. ``nemo setup`` stores the qualified form as
+        the default, so ``${NEMO_DEFAULT_MODEL}`` resolves to e.g.
+        ``default/nvidia-nemotron`` — the VM route takes workspace as a separate
+        path segment, so the bare name is what must reach the SDK.
+        """
+        config = {
+            "llms": {
+                "agent_llm": {"_type": "openai", "model_name": "default/nvidia-nemotron"},
+            }
+        }
+        vms = _RecordingVirtualModels()
+        sdk = _StubSDKWithVirtualModels(vms)
+
+        validate_llm_models(config, workspace="default", sdk=sdk)  # type: ignore[arg-type]
+
+        assert vms.calls == [{"name": "nvidia-nemotron", "workspace": "default"}]
+
+    def test_qualified_name_routes_to_its_own_workspace(self) -> None:
+        """A qualified ``ws/model`` looks up ``model`` in ``ws``, not the path workspace.
+
+        ``parse_qualified_name`` splits on the first ``/``: a qualifier always
+        wins over the fallback workspace, so a cross-workspace reference resolves
+        where it points. Bare names fall back to the path workspace.
+        """
+        config = {
+            "llms": {
+                "bare": {"_type": "openai", "model_name": "plain-model"},
+                "other_ws": {"_type": "openai", "model_name": "prod/foo"},
+            }
+        }
+        vms = _RecordingVirtualModels()
+        sdk = _StubSDKWithVirtualModels(vms)
+
+        validate_llm_models(config, workspace="default", sdk=sdk)  # type: ignore[arg-type]
+
+        calls = sorted((c["workspace"], c["name"]) for c in vms.calls)
+        assert calls == [("default", "plain-model"), ("prod", "foo")]
+
+    def test_qualified_and_bare_forms_dedupe_to_one_lookup(self) -> None:
+        """``default/foo`` and ``foo`` normalize to the same name → one retrieve.
+
+        Dedup happens *after* stripping, so the same model spelled both ways
+        across LLM keys costs a single lookup.
+        """
+        config = {
+            "llms": {
+                "qualified": {"_type": "openai", "model_name": "default/foo"},
+                "bare": {"_type": "openai", "model_name": "foo"},
+            }
+        }
+        vms = _RecordingVirtualModels()
+        sdk = _StubSDKWithVirtualModels(vms)
+
+        validate_llm_models(config, workspace="default", sdk=sdk)  # type: ignore[arg-type]
+
+        assert vms.calls == [{"name": "foo", "workspace": "default"}]
+
+    def test_bare_workspace_prefix_normalizes_to_empty_and_is_skipped(self) -> None:
+        """A ``model_name`` of exactly ``{workspace}/`` strips to '' and is skipped.
+
+        The empty string is not a routable model name; it must not reach the SDK.
+        """
+        config = {
+            "llms": {
+                "empty_after_strip": {"_type": "openai", "model_name": "default/"},
+            }
+        }
+        vms = _RecordingVirtualModels()
+        sdk = _StubSDKWithVirtualModels(vms)
+
+        validate_llm_models(config, workspace="default", sdk=sdk)  # type: ignore[arg-type]
+
+        assert vms.calls == []
+
     def test_distinct_model_names_each_get_one_call(self) -> None:
         config = {
             "llms": {
@@ -1504,7 +1581,7 @@ def test_missing_model_raises_value_error_with_actionable_message(self) -> None:
         message = str(exc_info.value)
         # Names the missing model + the YAML key + the workspace, and points
         # the user at concrete recovery steps.
-        assert "'missing-model'" in message
+        assert "'default/missing-model'" in message
         assert "llms.judge_llm.model_name" in message
         assert "'default'" in message
         assert "NEMO_DEFAULT_MODEL" in message
@@ -1524,8 +1601,8 @@ def test_multiple_missing_models_listed_in_single_error(self) -> None:
             validate_llm_models(config, workspace="default", sdk=sdk)
 
         message = str(exc_info.value)
-        assert "'missing-1'" in message
-        assert "'missing-2'" in message
+        assert "'default/missing-1'" in message
+        assert "'default/missing-2'" in message
 
     def test_non_igw_llm_types_are_skipped(self) -> None:
         """LLMs whose ``_type`` doesn't route through IGW aren't validatable here."""
@@ -1738,4 +1815,4 @@ def test_missing_model_propagates_validate_llm_models_error(self, tmp_path: Path
 
         # Sanity-check the message shape; full message coverage lives in
         # TestValidateLLMModels.
-        assert "'missing-model'" in str(exc_info.value)
+        assert "'default/missing-model'" in str(exc_info.value)