Repository navigation
Stabilize directory resource listings #3680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Why this was flaggedAGENTS.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 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, |
||
|
|
||
|
|
||
| @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"]), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: These Prompt for AI agents |
||
| (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")) | ||
There was a problem hiding this comment.
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 aRaises: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 aRaises:section: the rewrittenDirectoryResource.read()(and the un-pragma'dlist_files()) raiseFileNotFoundError/NotADirectoryErrorfor a missing or non-directory path, which the new tests now pin as behaviour, yet the docstrings stay one-liners with noRaises:.]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, andread()propagates them throughrun_syncat :228-230. Yet the docstrings remain one-liners with noRaises:section: types.py:216 and types.py:227. Nothing fails at runtime, only the documentation is incomplete.