Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. 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. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/server/mcpserver/resources/test_directory_resources.py">
<violation number="1" location="tests/server/mcpserver/resources/test_directory_resources.py:32">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| [ | ||
| (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.
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>
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
3 optional notes from this repository's REVIEW.md or CLAUDE.md checks were not posted as comments, over this review's limit for such notes; they are on this commit's check card.
| async def read(self) -> str: | ||
| """Read the directory listing.""" |
There was a problem hiding this comment.
🟡 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.
| @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 |
There was a problem hiding this comment.
🟡 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.
This branch has not been deployed
1 participant
Summary
/separators fromDirectoryResource.Fixes #3589
Checks
./scripts/test(6,101 passed; 100% coverage)UV_FROZEN=1 uv run --frozen strict-no-coveruv run --frozen pyrightAI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.