diff --git a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/README.md b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/README.md index c55abd10..cf4cd352 100644 --- a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/README.md +++ b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/README.md @@ -10,6 +10,8 @@ adapters in Harbor tasks. Harbor options select the model, harness, skills, MCP servers, tool policy, and telemetry; `FabricAgent` translates them into one typed `FabricConfig` for the task run. +Harbor's `skills_dir` is a task-side collection containing `/SKILL.md` directories. The task runner expands its immediate entries into individual NeMo Fabric skill paths in name order, preserving explicit `config.skills.paths` and avoiding duplicate paths. An empty collection adds no skills. Missing collections and entries without a regular `SKILL.md` file fail before harness execution; skill contents are still validated by the selected adapter. Collection paths are resolved inside the task environment, relative to `fabric_config_base_dir` when not absolute, never through the host filesystem. Use matching NeMo Fabric versions on the host and in the task environment for this transport contract. + Refer to the [Harbor example](../../../../../../../examples/harbor/README.md) for runnable SWE-Bench commands, configuration variations, reward checks, and Relay artifacts. diff --git a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/fabric_agent.py b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/fabric_agent.py index 292c8567..a5e5cf16 100644 --- a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/fabric_agent.py +++ b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/fabric_agent.py @@ -512,6 +512,7 @@ def _build_spec(self, instruction: str) -> FabricRunPayload: return FabricRunPayload( config=config, config_base_dir=self._environment_config_base_dir, + skills_dir=self.skills_dir, request=self._build_request(instruction), environment_env_names=tuple(self.fabric_environment_env), ) @@ -531,7 +532,6 @@ def _build_config(self) -> FabricConfig: enabled_tools=self.fabric_enabled_tools, telemetry=self.fabric_telemetry, model_name=self.model_name, - skills_dir=self.skills_dir, mcp_servers=tuple( HarborMcpServer.model_validate(server.model_dump(mode="python")) for server in self.mcp_servers @@ -623,7 +623,6 @@ def build_harbor_config( enabled_tools: list[str] | None = None, telemetry: Literal["none", "relay"] = "none", model_name: str | None = None, - skills_dir: str | Path | None = None, mcp_servers: tuple[HarborMcpServer, ...] = (), discovery_paths: tuple[str | Path, ...] = (), ) -> FabricConfig: @@ -713,8 +712,6 @@ def build_harbor_config( url=cast(str, server.url), exposure="harness_native", ) - if skills_dir is not None: - config.add_skill_path(skills_dir) if telemetry == "relay": relay_output = f"{artifact_root}/relay" config.enable_relay( diff --git a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/models.py b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/models.py index 1cea2c60..3bb252ea 100644 --- a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/models.py +++ b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/models.py @@ -49,6 +49,7 @@ class FabricRunPayload(BaseModel): config: FabricConfig config_base_dir: PurePosixPath logs_dir: PurePosixPath = PurePosixPath("/logs/agent") + skills_dir: PurePosixPath | None = None request: RunRequest environment_env_names: tuple[str, ...] = () diff --git a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/runner.py b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/runner.py index b630a0e9..7c4172f0 100644 --- a/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/runner.py +++ b/sdk/python/nemo-fabric-runtime/src/nemo_fabric/integrations/harbor/runner.py @@ -12,6 +12,7 @@ from pathlib import Path from nemo_fabric import Fabric +from nemo_fabric import FabricConfigError from nemo_fabric import FabricError from nemo_fabric import RunResult from nemo_fabric.integrations.harbor.models import FabricRunPayload @@ -26,6 +27,41 @@ async def run(payload: FabricRunPayload) -> RunResult: if name not in os.environ: raise ValueError(f"Harbor runner environment variable {name} is not set") config.environment.env[name] = os.environ[name] + if payload.skills_dir is not None: + base_dir = Path(payload.config_base_dir).resolve() + root = Path(payload.skills_dir) + if not root.is_absolute(): + root = base_dir / root + if not root.is_dir(): + raise FabricConfigError( + f"Harbor skills collection must be an existing task-side directory: {root}", + stage="configuration", + code="harbor_skills_invalid", + ) + skills = sorted(root.iterdir()) + for skill in skills: + skill_file = skill / "SKILL.md" + if ( + not skill.is_dir() + or skill_file.is_symlink() + or not skill_file.is_file() + ): + raise FabricConfigError( + "Harbor skills collection entries must be directories " + f"containing a regular SKILL.md file: {skill}", + stage="configuration", + code="harbor_skills_invalid", + ) + existing_paths = ( + {(base_dir / path).resolve() for path in config.skills.paths} + if config.skills is not None + else set() + ) + for skill in skills: + resolved = skill.resolve() + if resolved not in existing_paths: + config.add_skill_path(skill) + existing_paths.add(resolved) result = await Fabric().run( config, base_dir=payload.config_base_dir, diff --git a/tests/integrations/test_harbor_runner.py b/tests/integrations/test_harbor_runner.py index ce359ccc..b6e953d9 100644 --- a/tests/integrations/test_harbor_runner.py +++ b/tests/integrations/test_harbor_runner.py @@ -52,7 +52,7 @@ def load_codex_adapter(): return adapter -def test_harbor_builder_constructs_complete_config_from_harbor_inputs(tmp_path): +def test_harbor_builder_constructs_complete_config_from_harbor_inputs(): from nemo_fabric.integrations.harbor.fabric_agent import build_harbor_config from nemo_fabric.integrations.harbor.models import HarborMcpServer @@ -60,7 +60,6 @@ def test_harbor_builder_constructs_complete_config_from_harbor_inputs(tmp_path): adapter_id="demo.fabric.smoke", workspace="/testbed", model_name="openai/gpt-5.4", - skills_dir=tmp_path / "skills", mcp_servers=( HarborMcpServer( name="remote", @@ -85,8 +84,7 @@ def test_harbor_builder_constructs_complete_config_from_harbor_inputs(tmp_path): assert config.mcp.servers["local"].url == "mcp-server" assert config.mcp.servers["local"].args == ["--stdio"] assert "args" not in config.mcp.servers["local"].extra_fields - assert config.skills is not None - assert config.skills.paths == [str(tmp_path / "skills")] + assert config.skills is None assert ( json.loads(json.dumps(config.to_mapping()))["metadata"]["name"] == "harbor-smoke" @@ -135,6 +133,7 @@ def test_harbor_transport_models_validate_mcp_targets(): "config", "config_base_dir", "logs_dir", + "skills_dir", "request", "environment_env_names", } @@ -552,7 +551,6 @@ def test_swebench_matrix_translates_harbor_inputs_to_typed_config(tmp_path: Path workspace="/testbed", telemetry="relay", model_name="nvidia/nemotron-3-nano-omni-30b-a3b-reasoning", - skills_dir="/harbor/skills", mcp_servers=tuple( HarborMcpServer.model_validate(server.model_dump(mode="python")) for server in load_mcp_servers(SWEBENCH_MCP_CONFIG) @@ -586,8 +584,7 @@ def test_swebench_matrix_translates_harbor_inputs_to_typed_config(tmp_path: Path assert ( relay.models["default"].model == "nvidia/nemotron-3-nano-omni-30b-a3b-reasoning" ) - assert relay.skills is not None - assert relay.skills.paths == ["/harbor/skills"] + assert relay.skills is None assert relay.mcp is not None assert set(relay.mcp.servers) == {"fabric-repo-inspector"} assert relay.mcp.servers["fabric-repo-inspector"].args == [ diff --git a/tests/integrations/test_harbor_skills.py b/tests/integrations/test_harbor_skills.py new file mode 100644 index 00000000..0bd4638f --- /dev/null +++ b/tests/integrations/test_harbor_skills.py @@ -0,0 +1,327 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Task-side Harbor skill translation, independent of adapter selection.""" + +from __future__ import annotations + +import json +import os +import shutil +import subprocess +import sys +from pathlib import Path +from pathlib import PurePosixPath +from unittest.mock import AsyncMock, MagicMock + +import pytest +from nemo_fabric import Fabric, FabricConfigError, RunResult +from nemo_fabric.integrations.harbor import runner +from nemo_fabric.integrations.harbor.fabric_agent import FabricAgent +from nemo_fabric.integrations.harbor.models import FabricRunPayload + +pytestmark = pytest.mark.usefixtures("requires_harbor") + + +@pytest.fixture(name="skill_collection") +def skill_collection_fixture( + tmp_path: Path, default_skill: Path, alternate_skill: Path +): + root = tmp_path / "uploaded-skills" + # Deliberately create entries in reverse order to exercise stable ordering. + shutil.copytree(default_skill, root / "default") + shutil.copytree(alternate_skill, root / "alternate") + return root + + +@pytest.fixture(name="mock_fabric") +def mock_fabric_fixture(monkeypatch: pytest.MonkeyPatch): + mock_fabric = MagicMock(spec=Fabric) + mock_fabric.run = AsyncMock(return_value=MagicMock(spec=RunResult)) + monkeypatch.setattr(runner, "Fabric", MagicMock(return_value=mock_fabric)) + monkeypatch.setattr(runner, "publish_telemetry_evidence", MagicMock()) + return mock_fabric + + +@pytest.fixture(name="skill_payload") +def skill_payload_fixture(tmp_path: Path, skill_collection: Path): + payload = FabricAgent( + logs_dir=tmp_path / "logs", + fabric_adapter_id="acme.skills", + skills_dir=str(skill_collection), + )._build_spec("Use the uploaded skills.") + # Harbor's host-side task paths are POSIX, even on a Windows host. Simulate + # task-side filesystem access using this host's native temporary directory. + payload.config_base_dir = PurePosixPath(tmp_path.as_posix()) + return payload + + +@pytest.mark.parametrize( + "adapter_id", ["nvidia.fabric.openhands", "nvidia.fabric.claude"] +) +async def test_harbor_collection_is_accepted_by_individual_path_adapters( + tmp_path: Path, skill_collection: Path, mock_fabric, adapter_id: str +): + from nemo_fabric_adapter_contract.models import AgentConfig, RuntimeContext + from nemo_fabric_adapters.claude import adapter as claude + from nemo_fabric_adapters.openhands import adapter as openhands + + payload = FabricAgent( + logs_dir=tmp_path / "logs", + fabric_adapter_id=adapter_id, + skills_dir=str(skill_collection), + )._build_spec("Use both skills.") + payload.config_base_dir = PurePosixPath(tmp_path.as_posix()) + + async def validate(config, *, base_dir, request): + adapter_config = AgentConfig.from_mapping( + {"skills": config.skills.to_mapping()} + ) + expected = [skill_collection / "alternate", skill_collection / "default"] + if adapter_id == "nvidia.fabric.openhands": + assert openhands._skill_paths(adapter_config, str(base_dir)) == expected + else: + plugins = claude._stage_skill_plugin( + adapter_config, + RuntimeContext.from_mapping( + { + "runtime_id": "harbor-skills", + "invocation_id": "skill-invocation", + "request_id": "skill-request", + "environment": { + "environment_id": "task", + "provider": "local", + "control_location": "in_env_control", + "workspace": str(tmp_path), + "ownership": "caller_owned", + }, + "artifacts": {"root": str(tmp_path)}, + } + ), + str(base_dir), + ) + staged = Path(plugins[0]["path"]) / "skills" + assert sorted(path.name for path in staged.iterdir()) == [ + "alternate", + "default", + ] + return MagicMock(spec=RunResult) + + mock_fabric.run.side_effect = validate + await runner.run(payload) + mock_fabric.run.assert_awaited_once() + + +def test_host_transports_task_path_without_reading_host_filesystem(tmp_path: Path): + payload = FabricAgent( + logs_dir=tmp_path, + fabric_adapter_id="acme.skills", + skills_dir="/harbor/task-only-skills", + )._build_spec("Use skills.") + assert str(payload.skills_dir) == "/harbor/task-only-skills" + assert payload.config.skills is None + + +@pytest.mark.parametrize("names", [[], ["default"], ["alternate", "default"]]) +async def test_empty_single_multiple_collections( + skill_payload, skill_collection: Path, mock_fabric, names: list[str] +): + for child in skill_collection.iterdir(): + if child.name not in names: + shutil.rmtree(child) + await runner.run(skill_payload) + config = mock_fabric.run.call_args.args[0] + assert (config.skills.paths if config.skills else []) == [ + str(skill_collection / name) for name in names + ] + assert skill_payload.config.skills is None + + +async def test_explicit_skills_are_preserved_without_duplicates( + skill_payload, skill_collection: Path, mock_fabric, default_skill: Path +): + skill_payload.config.add_skill_path(default_skill) + skill_payload.config.add_skill_path(skill_collection / "default") + original = skill_payload.config.to_mapping() + await runner.run(skill_payload) + await runner.run(skill_payload) + for call in mock_fabric.run.call_args_list: + assert call.args[0].skills.paths == [ + str(default_skill), + str(skill_collection / "default"), + str(skill_collection / "alternate"), + ] + assert skill_payload.config.to_mapping() == original + + +async def test_relative_collection_is_resolved_against_task_base_dir( + skill_payload, skill_collection: Path, mock_fabric +): + skill_payload.skills_dir = Path(skill_collection.name) + transported = FabricRunPayload.model_validate_json(skill_payload.model_dump_json()) + await runner.run(transported) + assert mock_fabric.run.call_args.args[0].skills.paths == [ + str(skill_collection / "alternate"), + str(skill_collection / "default"), + ] + + +async def test_relative_explicit_skill_overlapping_collection_is_not_added_twice( + skill_payload, skill_collection: Path, mock_fabric +): + relative_skill = Path(skill_collection.name) / "default" + skill_payload.config.add_skill_path(relative_skill) + skill_payload.skills_dir = Path(skill_collection.name) + original = skill_payload.config.to_mapping() + await runner.run(skill_payload) + config = mock_fabric.run.call_args.args[0] + assert config.skills.paths == [ + str(relative_skill), + str(skill_collection / "alternate"), + ] + assert skill_payload.config.to_mapping() == original + + +def test_cli_preserves_malformed_skill_path_diagnostic( + tmp_path: Path, skill_payload, skill_collection: Path +): + offending_path = skill_collection / "default" + (offending_path / "SKILL.md").unlink() + spec = tmp_path / "spec.json" + result = tmp_path / "result.json" + spec.write_text(skill_payload.model_dump_json(), encoding="utf-8") + completed = subprocess.run( + [ + sys.executable, + "-m", + "nemo_fabric.integrations.harbor.runner", + "--spec", + str(spec), + "--result", + str(result), + ], + capture_output=True, + text=True, + timeout=60, + ) + assert completed.returncode == 1 + diagnostic = json.loads(result.read_text())["runner_error"] + assert str(offending_path) in diagnostic["message"] + assert diagnostic["code"] == "harbor_skills_invalid" + + +@pytest.mark.parametrize( + "malformation", + [ + "missing", + "file", + "missing-skill", + "skill-is-dir", + "skill-is-symlink", + "loose-file", + ], +) +async def test_malformed_collection_fails_before_harness_execution( + skill_payload, skill_collection: Path, mock_fabric, malformation: str +): + offending_path = skill_collection + if malformation == "missing": + shutil.rmtree(skill_collection) + elif malformation == "file": + shutil.rmtree(skill_collection) + skill_collection.write_text("not a collection", encoding="utf-8") + elif malformation == "missing-skill": + (skill_collection / "default" / "SKILL.md").unlink() + offending_path = skill_collection / "default" + elif malformation == "skill-is-dir": + path = skill_collection / "default" / "SKILL.md" + path.unlink() + path.mkdir() + offending_path = skill_collection / "default" + elif malformation == "skill-is-symlink": + path = skill_collection / "default" / "SKILL.md" + path.unlink() + path.symlink_to(skill_collection / "alternate" / "SKILL.md") + assert path.is_file() + offending_path = skill_collection / "default" + else: + (skill_collection / "SKILL.md").write_text( + "not a child skill", encoding="utf-8" + ) + offending_path = skill_collection / "SKILL.md" + with pytest.raises(FabricConfigError, match="Harbor skills collection") as error: + await runner.run(skill_payload) + assert str(offending_path) in str(error.value) + mock_fabric.run.assert_not_awaited() + + +async def test_no_collection_leaves_explicit_fabric_skills_unchanged( + skill_payload, mock_fabric, default_skill: Path +): + skill_payload.skills_dir = None + skill_payload.config.add_skill_path(default_skill) + await runner.run(skill_payload) + assert mock_fabric.run.call_args.args[0].skills.paths == [str(default_skill)] + + +@pytest.mark.skipif( + sys.platform == "win32", reason="mock Claude CLI requires a POSIX executable" +) +@pytest.mark.parametrize("relative_overlap", [False, True]) +def test_task_runner_cli_stages_collection_through_real_claude_sdk( + tmp_path: Path, skill_collection: Path, repo_root: Path, relative_overlap: bool +): + logs = tmp_path / "logs" + artifacts = tmp_path / "artifacts" + agent = FabricAgent( + logs_dir=logs, + fabric_adapter_id="nvidia.fabric.claude", + model_name="anthropic/claude-test-model", + fabric_python=sys.executable, + fabric_workspace=str(tmp_path), + skills_dir=str(skill_collection), + fabric_harness_settings={"permission_mode": "dontAsk", "setting_sources": []}, + fabric_environment_env={ + "FABRIC_TEST_CLAUDE_CLI_PATH": str( + repo_root / "tests/fixtures/claude/mock-claude-cli.py" + ), + "CLAUDE_AGENT_SDK_SKIP_VERSION_CHECK": "1", + "MOCK_CLAUDE_CLI_LOG": str(tmp_path / "cli-args.jsonl"), + }, + ) + payload = agent._build_spec("Use both skills.") + if relative_overlap: + payload.config_base_dir = PurePosixPath(tmp_path.as_posix()) + payload.skills_dir = Path(skill_collection.name) + payload.config.add_skill_path(Path(skill_collection.name) / "default") + payload.config.runtime.artifacts = artifacts + payload.config.environment.artifacts = artifacts + payload.logs_dir = logs + spec_path = tmp_path / "spec.json" + result_path = tmp_path / "result.json" + spec_path.write_text(payload.model_dump_json(), encoding="utf-8") + completed = subprocess.run( + [ + sys.executable, + "-m", + "nemo_fabric.integrations.harbor.runner", + "--spec", + str(spec_path), + "--result", + str(result_path), + ], + cwd=tmp_path, + capture_output=True, + text=True, + timeout=60, + env={**os.environ, **agent._runner_env}, + ) + assert completed.returncode == 0, completed.stdout + completed.stderr + result = RunResult.from_mapping(json.loads(result_path.read_text(encoding="utf-8"))) + assert result.status == "succeeded", result.to_mapping() + arguments = json.loads((tmp_path / "cli-args.jsonl").read_text().splitlines()[0]) + plugin = Path(arguments[arguments.index("--plugin-dir") + 1]) / "skills" + for name in ("alternate", "default"): + assert (plugin / name / "SKILL.md").read_bytes() == ( + skill_collection / name / "SKILL.md" + ).read_bytes() diff --git a/tests/python/test_harbor_integration.py b/tests/python/test_harbor_integration.py index 71352e38..bcec80a8 100644 --- a/tests/python/test_harbor_integration.py +++ b/tests/python/test_harbor_integration.py @@ -145,14 +145,14 @@ async def test_harbor_integration(tmp_path: Path): assert spec["config"]["environment"]["workspace"] == "/testbed" assert spec["config"]["models"]["default"]["provider"] == "nvidia" assert spec["config"]["models"]["default"]["model"] == "nvidia/test-model" - assert spec["config"]["skills"]["paths"] == ["/opt/fabric-demo/skills"] + assert spec["config"]["skills"] is None + assert spec["skills_dir"] == "/opt/fabric-demo/skills" assert spec["config"]["mcp"]["servers"]["github"] == { "transport": "streamable-http", "url": "https://mcp.example.test", "exposure": "harness_native", } assert "model_name" not in spec - assert "skills_dir" not in spec assert "mcp_servers" not in spec fabric_commands = [