Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/servers/resources.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ The same rule applies to anything else JSON-serialisable: a list, a Pydantic mod
`mcp.server.mcpserver.resources` has ready-made `Resource` classes (`TextResource`,
`BinaryResource`, `FileResource`, `HttpResource`, `DirectoryResource`) that you register
with `mcp.add_resource(...)`.
`DirectoryResource` returns sorted relative file paths with `/` separators on every platform.

A client can also **subscribe** to a resource and be notified when it changes; that's the client's half of the story and it lives in **[The Client](../client/index.md)**.

Expand Down
11 changes: 6 additions & 5 deletions src/mcp/server/mcpserver/resources/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,13 @@ class DirectoryResource(Resource):

@pydantic.field_validator("path")
@classmethod
def validate_absolute_path(cls, path: Path) -> Path: # pragma: no cover
def validate_absolute_path(cls, path: Path) -> Path:
"""Ensure path is absolute."""
if not path.is_absolute():
raise ValueError("Path must be absolute")
return path

def list_files(self) -> list[Path]: # pragma: no cover
def list_files(self) -> list[Path]:
"""List files in the directory."""
if not self.path.exists():
raise FileNotFoundError(f"Directory not found: {self.path}")
Expand All @@ -223,8 +223,9 @@ def list_files(self) -> list[Path]: # pragma: no cover
return list(self.path.glob(self.pattern)) if not self.recursive else list(self.path.rglob(self.pattern))
return list(self.path.glob("*")) if not self.recursive else list(self.path.rglob("*"))

async def read(self) -> str: # Always returns JSON string # pragma: no cover
async def read(self) -> str:
"""Read the directory listing."""
Comment on lines +226 to 227

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional), pre-existing: callers of DirectoryResource.read() still get no docstring saying it raises FileNotFoundError or NotADirectoryError, even though this PR now tests and commits to both. AGENTS.md asks public APIs to list exceptions a caller would reasonably catch in a Raises: section; the one-line docstring at types.py:227 (and on list_files at types.py:216) omits them. Fix: add a Raises: section naming FileNotFoundError and NotADirectoryError on read() (and list_files()), which covers the 2 sites listed. Same instruction at 2 sites (src/mcp/server/mcpserver/resources/types.py:227, src/mcp/server/mcpserver/resources/types.py:216). [also at: src/mcp/server/mcpserver/resources/types.py:227 - nit: AGENTS.md asks public APIs that raise catchable exceptions to document them in a Raises: section: the rewritten DirectoryResource.read() (and the un-pragma'd list_files()) raise FileNotFoundError / NotADirectoryError for a missing or non-directory path, which the new tests now pin as behaviour, yet the docstrings stay one-liners with no Raises:.]

Why this was flagged

A server author registers a DirectoryResource whose path may be missing or may be a file; read() at src/mcp/server/mcpserver/resources/types.py:226-231 propagates FileNotFoundError or NotADirectoryError raised in list_files() at types.py:217-220. The diff removes the pragma: no cover markers on both methods and adds tests pinning these exceptions, so they are now deliberate public behaviour, but neither docstring (types.py:216, types.py:227) has a Raises: section. On the base branch the same omission exists, so this is pre-existing; what changes is that the behaviour is now tested and documented as stable without the docstring following. AGENTS.md Code Quality guards against exactly this: a caller reading the API docs cannot know which exceptions to catch.

Verification: pre-existing. The cited instruction exists essentially as quoted in AGENTS.md:51-53. list_files() raises FileNotFoundError and NotADirectoryError at types.py:218 and :220, and read() propagates them through run_sync at :228-230. Yet the docstrings remain one-liners with no Raises: section: types.py:216 and types.py:227. Nothing fails at runtime, only the documentation is incomplete.

files = await anyio.to_thread.run_sync(self.list_files)
file_list = [str(f.relative_to(self.path)) for f in files if f.is_file()]
file_list = await anyio.to_thread.run_sync(
lambda: sorted(f.relative_to(self.path).as_posix() for f in self.list_files() if f.is_file())
)
return json.dumps({"files": file_list}, indent=2)
68 changes: 68 additions & 0 deletions tests/server/mcpserver/resources/test_directory_resources.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import json
from pathlib import Path

import pytest
from pydantic import ValidationError

from mcp.server.mcpserver.resources import DirectoryResource


@pytest.fixture
def directory_with_files(tmp_path: Path) -> Path:
(tmp_path / "z.txt").write_text("z", encoding="utf-8")
(tmp_path / "a.txt").write_text("a", encoding="utf-8")
(tmp_path / "nested").mkdir()
(tmp_path / "nested" / "b.txt").write_text("b", encoding="utf-8")
return tmp_path


@pytest.fixture
def regular_file(tmp_path: Path) -> Path:
path = tmp_path / "file.txt"
path.write_text("content", encoding="utf-8")
return path
Comment on lines +10 to +23

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers reading these tests must jump to file-level fixtures that only one test each uses, against the repo's test-quality bar. directory_with_files (line 10) feeds a single parametrized test and regular_file (line 19) feeds a single test, so neither is genuinely shared. Fix: inline the directory and file setup into the test bodies via tmp_path, keeping a fixture only if a second test in this file comes to share it.

Why this was flagged

AGENTS.md:68 delegates test conventions to .claude/skills/test-quality/SKILL.md, whose Naming & shape section says to inline setup in the test and lift to a file-level fixture only when several tests in that file genuinely share it. In tests/server/mcpserver/resources/test_directory_resources.py:10-16 directory_with_files is consumed only by test_directory_resource_returns_sorted_posix_file_paths (line 35), and regular_file at lines 19-23 only by test_directory_resource_rejects_regular_file (line 56). Nothing fails at runtime; the convention guards readability so the whole observable behaviour of a test fits on one screen without hopping to a fixture. The base branch has no test file for DirectoryResource, so this is new code introduced by the diff.

Verification: nit. SKILL.md:23-25 says "Lift to a file-level fixture only when several tests in that file genuinely share it". In tests/server/mcpserver/resources/test_directory_resources.py, directory_with_files (lines 10-16) is consumed only by test_directory_resource_returns_sorted_posix_file_paths, and regular_file (lines 19-23) only by test_directory_resource_rejects_regular_file.



@pytest.mark.anyio
@pytest.mark.parametrize(
("recursive", "pattern", "expected"),
[
(False, None, ["a.txt", "z.txt"]),
(True, None, ["a.txt", "nested/b.txt", "z.txt"]),
(False, "*.txt", ["a.txt", "z.txt"]),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: These *.txt cases cannot catch regressions that ignore pattern, because every fixture file matches it. Add a non-.txt file and assert that pattern-filtered listings exclude it.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/server/mcpserver/resources/test_directory_resources.py, line 32:

<comment>These `*.txt` cases cannot catch regressions that ignore `pattern`, because every fixture file matches it. Add a non-`.txt` file and assert that pattern-filtered listings exclude it.</comment>

<file context>
@@ -0,0 +1,68 @@
+    [
+        (False, None, ["a.txt", "z.txt"]),
+        (True, None, ["a.txt", "nested/b.txt", "z.txt"]),
+        (False, "*.txt", ["a.txt", "z.txt"]),
+        (True, "*.txt", ["a.txt", "nested/b.txt", "z.txt"]),
+    ],
</file context>

(True, "*.txt", ["a.txt", "nested/b.txt", "z.txt"]),
],
)
async def test_directory_resource_returns_sorted_posix_file_paths(
directory_with_files: Path, recursive: bool, pattern: str | None, expected: list[str]
) -> None:
"""SDK-defined listings contain sorted relative file paths for each glob mode."""
resource = DirectoryResource(
uri="test://directory", name="directory", path=directory_with_files, recursive=recursive, pattern=pattern
)

assert json.loads(await resource.read()) == {"files": expected}


@pytest.mark.anyio
async def test_directory_resource_rejects_missing_directory(tmp_path: Path) -> None:
"""SDK-defined directory reads report a missing path."""
resource = DirectoryResource(uri="test://directory", name="directory", path=tmp_path / "missing")

with pytest.raises(FileNotFoundError):
await resource.read()


@pytest.mark.anyio
async def test_directory_resource_rejects_regular_file(regular_file: Path) -> None:
"""SDK-defined directory reads reject a regular file path."""
resource = DirectoryResource(uri="test://directory", name="directory", path=regular_file)

with pytest.raises(NotADirectoryError):
await resource.read()


def test_directory_resource_requires_absolute_path() -> None:
"""SDK-defined resource validation rejects relative directory paths."""
with pytest.raises(ValidationError):
DirectoryResource(uri="test://directory", name="directory", path=Path("relative"))
Loading