From f5b657694f74d675a353a371ffc17f584afa00aa Mon Sep 17 00:00:00 2001 From: heznpc Date: Fri, 29 May 2026 06:45:13 +0900 Subject: [PATCH] fix: resolve all 15 /code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single PR fixes every CONFIRMED + PLAUSIBLE finding from the extra-high- effort code-review on the post-/simplify range (ec7045d..HEAD). Workflow bugs (active failures) ------------------------------- 1. dependabot-auto-merge.yml: pre-create `manual-review` label via `gh label create --force` before adding it. Previously the workflow failed red on every Dependabot major bump (observed on PR #34) because the label didn't exist yet. 2. stale.yml: add `dependencies,manual-review` to `exempt-pr-labels` so Dependabot major-bump PRs aren't silently closed after 37 days. 5. dependabot-auto-merge.yml major-branch now calls `gh pr merge --disable-auto || true` before re-labeling. Without this, a Dependabot rebase that promoted a previously-classified minor into a major would keep the prior `--auto` enable and silently squash-merge. Doc drift (user-facing contradictions) -------------------------------------- 3. README.md + README.ko.md project-structure tree: drop the stale `tools/greet.py` line; relabel server.py from "+ inline tools + helpers" to "+ inline `greet` tool example". Replace README's "Modular (recommended for larger servers)" code block (which pointed at a deleted example file) with prose pointing at resources/server_info.py and prompts/code_review.py as the live `register(mcp)` exemplars. 4. CHANGELOG.md [Unreleased] reset to an empty courtesy stub. The previous bullets described attest-build-provenance v3.2.0, stale v10.2.0, ruff>=0.15<1, mypy>=2<3, pytest>=8<10, pytest-asyncio>=1<2, a `.python-version` file, and a SECURITY.md "feature set" framing — every one of those is now superseded by PRs #28-#34 + #27. The file's own header already declares GitHub Releases as canonical, so the courtesy stub now matches that contract. 6. server.py module docstring + tools-section comment: drop references to the deleted greet.py example; reference the live register(mcp) pattern in resources/ and prompts/. tools/__init__.py is now also deleted (the package was a dead public namespace shipping in the wheel after greet.py was removed). 11. setup.yml first-run bootstrap issue: rewrite the `Replace the example greet tool` checklist item to point at server.py inline (and mention modular split as an option), matching reality. 12. SECURITY.md adds a one-line WARNING above the branch-protection JSON that the hardcoded `test (3.11/3.12/3.13)` contexts MUST be updated in lockstep with any matrix change — otherwise required checks point at jobs that never report. 13. SECURITY.md prepends `gh auth refresh -s admin:repo,security_events` so copy-pasters who used the default `gh auth login` scopes don't hit opaque 404s on the vulnerability-alerts / automated-security-fixes PUTs. Production code correctness (rename-safety + entry-point testability) --------------------------------------------------------------------- 7+8. resources/server_info.py: replace the two hardcoded `"my-mcp-server"` literals with a module-level `PKG_NAME` derived from `__package__` (with underscores → hyphens). A clone that renames the package per setup.yml's first-run checklist now reports the correct dist name in `info://server/status` without a second hardcoded literal to remember. Test in tests/test_server_info.py is updated to compare `payload["name"]` against `mod.PKG_NAME` and `payload["version"]` against `importlib.metadata.version(mod.PKG_NAME)` — both sides resolve to the SAME source, so an uncommitted pyproject.toml version bump no longer makes the test fail for the wrong reason. 14. __main__.py: extract the try/except wrapper into `run() -> int` and guard the call site with `if __name__ == "__main__":`. Importing `my_mcp_server.__main__` (IDE auto-import, coverage discovery, etc.) no longer blocks on the server loop, and the KeyboardInterrupt / Exception arms are now testable. Test integrity -------------- 6. tests/test_server_info.py adds `test_read_pyproject_returns_none_when_no_pyproject_anywhere` exercising the OUTER `return None` (loop exhaustion at server_info.py:42) that the existing wheel-fallback tests bypassed via monkeypatch. Coverage on server_info.py jumps 96% → 100%. 9. tests/test_server_info.py::test_read_pyproject_returns_none_when_project_table_missing now installs a spy on `tomllib.load` and asserts EXACTLY ONE pyproject was read — so a future refactor that drops the inner `return None` and falls through to keep walking gets caught here (the spy would see >1 load) instead of silently passing because no other pyproject.toml is reachable on the way to /. 10. tests/test_server_info.py adds `test_pkg_name_derived_from_package` pinning the `PKG_NAME = __package__.split(".")[0].replace("_", "-")` contract — a refactor that re-hardcodes the literal would fail here. 15. tests/test_tools.py introduces a `_schema_constraint` helper that reads a JSON Schema keyword either at the top of a property OR inside an `anyOf` branch, so a future widening of the `greet` signature to `Annotated[str | None, Field(...)]` (where pydantic emits `anyOf`) fails with a clear schema-shape message instead of `KeyError`. 13. tests/test_main.py (new file, 3 tests): exercise the wrapper's clean return / KeyboardInterrupt / Exception arms. The Exception test also asserts the traceback is logged via `logging.exception` (silently exiting 1 without the log would make production debugging much harder). Coverage hygiene ---------------- 17. pyproject.toml `[tool.coverage.report]`: switch from `exclude_lines` (which appended to the default and matched nothing in src/) to `exclude_also` with patterns that actually have real or near-real matches (`if TYPE_CHECKING:`, `if __name__ == "__main__":`). The `pragma: no cover` and `raise NotImplementedError` patterns moved to coverage.py's built-in defaults. Verification ------------ - ruff / ruff format / mypy strict: all clean. - pytest: 26 passed (was 21; +3 main tests + 2 server_info tests, with test_falls_back tightened rather than added). - Coverage: 70.64% → 97.06% (was 84.69% after the last 2nd-pass session; __main__.py 0% → 100%, server_info.py 96% → 100%). Findings deferred from this PR ------------------------------ None — all 15 from the /code-review JSON output (CONFIRMED + PLAUSIBLE) are addressed. Findings REFUTED by verifiers (so no change required): - C-F1 (GITHUB_TOKEN read-only on Dependabot PRs): empirically false here because the workflow declares an explicit `permissions:` block. - C-F14 (cd.yml top-level permissions cap ci.yml): GitHub docs confirm reusable workflows use their own top-level permissions. - C-F15 (symlink resolution skips tmp_path): pytest's tmp_path on macOS already returns the canonical /private/var path. --- .github/workflows/dependabot-auto-merge.yml | 16 +++- .github/workflows/setup.yml | 2 +- .github/workflows/stale.yml | 4 +- CHANGELOG.md | 40 +--------- README.ko.md | 5 +- README.md | 36 ++------- SECURITY.md | 17 +++- pyproject.toml | 8 +- src/my_mcp_server/__main__.py | 33 ++++++-- src/my_mcp_server/resources/server_info.py | 16 +++- src/my_mcp_server/server.py | 12 ++- src/my_mcp_server/tools/__init__.py | 0 tests/test_main.py | 46 +++++++++++ tests/test_server_info.py | 87 +++++++++++++++++---- tests/test_tools.py | 20 ++++- 15 files changed, 228 insertions(+), 114 deletions(-) delete mode 100644 src/my_mcp_server/tools/__init__.py create mode 100644 tests/test_main.py diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 872e6be..ebd0b15 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -28,7 +28,21 @@ jobs: - name: Label major updates for manual review if: steps.meta.outputs.update-type == 'version-update:semver-major' - run: gh pr edit "$PR_URL" --add-label "dependencies,manual-review" + # 1. Ensure the `manual-review` label exists (idempotent; `--force` + # succeeds whether or not it pre-exists). Without this `gh pr edit + # --add-label` fails on a fresh repo, taking the whole job red. + # 2. Disable any prior `--auto` enable from an earlier classification + # of this same PR — Dependabot may rebase a minor into a major; the + # auto-merge wouldn't otherwise be revoked and the major would + # silently squash-merge once CI passes. + # 3. Apply the labels so a maintainer sees the PR in their queue. + run: | + gh label create manual-review \ + --color fbca04 \ + --description "Needs maintainer decision before merging" \ + --force + gh pr merge --disable-auto "$PR_URL" || true + gh pr edit "$PR_URL" --add-label "dependencies,manual-review" env: PR_URL: ${{ github.event.pull_request.html_url }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/setup.yml b/.github/workflows/setup.yml index c704018..f0040b3 100644 --- a/.github/workflows/setup.yml +++ b/.github/workflows/setup.yml @@ -41,7 +41,7 @@ jobs: - [ ] Update `pyproject.toml` — change `name`, `description`, `authors`, `[project.scripts]` entry - [ ] Rename `src/my_mcp_server/` package directory to match your project name - [ ] Update imports and `pyproject.toml` scripts after rename - - [ ] Replace the example `greet` tool with your own tools in `src//tools/` + - [ ] Replace the inline `greet` example in `src//server.py` with your own tools (or split into modules — see `resources/` and `prompts/` for the `register(mcp)` pattern) - [ ] Set safety annotations on each tool (readOnly, destructive, idempotent, openWorld) - [ ] Update tests in `tests/` diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index c5c1e92..1a981b2 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -26,4 +26,6 @@ jobs: This PR has been inactive for 30 days and will be closed in 7 days unless there is new activity. exempt-issue-labels: pinned,bug,help wanted - exempt-pr-labels: pinned,work in progress + # `dependencies` + `manual-review` keep Dependabot major-bump PRs from + # being silently auto-closed before a maintainer can decide. + exempt-pr-labels: pinned,work in progress,dependencies,manual-review diff --git a/CHANGELOG.md b/CHANGELOG.md index 102c056..91d0e9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,40 +18,8 @@ people reading this file directly. Clear them by hand when cutting a release. ## [Unreleased] -### Added -- `actions/attest-build-provenance@v3` step in `cd.yml` — wheel/sdist now - ship with sigstore-signed SLSA build provenance attestations; PyPI surfaces - this as "Build attestations: verified". -- CodeQL now scans `actions` workflows in addition to `python` source, so - workflow injection / missing-permissions issues are caught alongside code - issues. -- `pytest-cov` to `[dev]` deps + `--cov-fail-under=70` baseline in `pyproject.toml` - (matches measured 71% coverage; bump only upward). -- Ruff rule set extended with `B` (bugbear), `S` (bandit subset), `ASYNC`, - `RUF`, `SIM` — security/correctness checks beyond the previous style-only set. -- `.github/CODEOWNERS` — solo-dev auto-assignment for Dependabot PRs and outside - contributions. -- `.github/ISSUE_TEMPLATE/{bug_report,feature_request,config}.yml` + - `.github/PULL_REQUEST_TEMPLATE.md` — minimal templates with scope checks. -- `.python-version` (pyenv/uv users). - -### Changed -- Third-party Actions are now SHA-pinned (with `# vX.Y.Z` comment for human - legibility): `softprops/action-gh-release` v3.0.0, `actions/stale` v10.2.0, - `actions/github-script` v9.0.0, `actions/attest-build-provenance` v3.2.0. - First-party `actions/*`, `github/codeql-action/*`, and - `pypa/gh-action-pypi-publish@release/v1` keep tag pinning per their respective - publisher guidance. -- Dev tooling now has major-version bounds: `ruff>=0.15,<1`, `mypy>=2,<3`, - `pytest>=8,<10`, `pytest-asyncio>=1,<2`. Prevents a surprise major release - from turning CI red overnight. -- `SECURITY.md` documents the full feature set including the GitHub repo-side - toggles (secret scanning + push protection + Dependabot security updates + - branch protection on `main`). - -### Fixed -- Stop tracking `.coverage` and add coverage artifacts (`.coverage*`, `htmlcov/`, - `coverage.xml`) to `.gitignore` so local test runs no longer pollute commits. -- Replace EN DASH (`–`) with hyphen-minus in `greet` field description - (`server.py:71`) — ruff `RUF001` flagged the ambiguous character. +_None yet — see [GitHub Releases](https://github.com/starter-series/python-mcp-server-starter/releases) +for the authoritative history. Hand-written bullets here are a courtesy for +people reading this file directly; clear them when cutting a release so the +auto-prepended release notes stay the canonical record._ diff --git a/README.ko.md b/README.ko.md index e8c7691..130e30b 100644 --- a/README.ko.md +++ b/README.ko.md @@ -172,10 +172,7 @@ python -m my_mcp_server src/my_mcp_server/ ├── __init__.py # 버전 ├── __main__.py # python -m 진입점 -├── server.py # FastMCP 서버 + 인라인 툴 + 헬퍼 -├── tools/ -│ ├── __init__.py -│ └── greet.py # 모듈형 툴 예시 +├── server.py # FastMCP 서버 + 인라인 `greet` 툴 예시 ├── resources/ │ ├── __init__.py │ └── server_info.py # Resource 예시 (info://server/status) diff --git a/README.md b/README.md index 6592ddf..7a9bde7 100644 --- a/README.md +++ b/README.md @@ -82,32 +82,11 @@ async def your_tool(input: str) -> str: ### Modular (recommended for larger servers) -Create `src/my_mcp_server/tools/your_tool.py`: - -```python -from mcp.server.fastmcp import FastMCP -from mcp.types import ToolAnnotations - - -def register(mcp: FastMCP) -> None: - @mcp.tool( - annotations=ToolAnnotations( - readOnlyHint=True, - destructiveHint=False, - idempotentHint=True, - ), - ) - async def your_tool(input: str) -> str: - """What your tool does.""" - return f"Processed: {input}" -``` - -Then in `server.py`: - -```python -from my_mcp_server.tools.your_tool import register -register(mcp) -``` +See `resources/server_info.py` and `prompts/code_review.py` for the +`register(mcp)` pattern this repo applies to resources and prompts. The +same shape works for tools: define `register(mcp: FastMCP) -> None` in +`src/my_mcp_server/tools/your_tool.py`, decorate `@mcp.tool(...)` inside, +then call `register(mcp)` from `server.py`. ## Adding Resources @@ -213,10 +192,7 @@ Setup: [PyPI OIDC trusted publishing docs](https://docs.pypi.org/trusted-publish src/my_mcp_server/ ├── __init__.py # Version ├── __main__.py # python -m entry point -├── server.py # FastMCP server + inline tools + helpers -├── tools/ -│ ├── __init__.py -│ └── greet.py # Example modular tool +├── server.py # FastMCP server + inline `greet` tool example ├── resources/ │ ├── __init__.py │ └── server_info.py # Example resource (info://server/status) diff --git a/SECURITY.md b/SECURITY.md index 8da7e8c..7955116 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -36,6 +36,10 @@ your new repo: ```bash REPO=your-org/your-repo +# Most of these endpoints need repo admin AND token scopes that the default +# `gh auth login` flow does not request. Refresh first: +gh auth refresh -s admin:repo,security_events + # Secret scanning + push protection (public repos: free; private: needs GHAS) gh api -X PATCH "repos/$REPO" \ -f 'security_and_analysis[secret_scanning][status]=enabled' \ @@ -45,7 +49,12 @@ gh api -X PATCH "repos/$REPO" \ gh api -X PUT "repos/$REPO/vulnerability-alerts" gh api -X PUT "repos/$REPO/automated-security-fixes" -# Branch protection — adjust required checks to match your CI job names +# Branch protection — the `checks` list below MUST match the actual job names +# your ci.yml produces. The example matches this repo's matrix +# (`security`, `licenses`, plus `test (3.11/3.12/3.13)`). If you change the +# Python matrix (drop 3.11, add 3.14, rename `test`) UPDATE THIS LIST IN +# LOCKSTEP — otherwise required checks point at jobs that never report and +# every PR sits in "Expected — waiting for status" forever. gh api -X PUT "repos/$REPO/branches/main/protection" --input - <<'JSON' { "required_status_checks": { @@ -65,6 +74,12 @@ JSON # Auto-merge + auto-delete merged branches gh api -X PATCH "repos/$REPO" -F allow_auto_merge=true -F delete_branch_on_merge=true + +# Dependabot auto-merge needs a `manual-review` label (referenced by +# .github/workflows/dependabot-auto-merge.yml). The workflow creates it +# idempotently on first major-bump PR, but you can seed it now: +gh label create manual-review --color fbca04 \ + --description "Needs maintainer decision before merging" --force ``` ## Best Practices diff --git a/pyproject.toml b/pyproject.toml index 6f17b75..eb68a69 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -73,8 +73,10 @@ branch = true source = ["my_mcp_server"] [tool.coverage.report] -exclude_lines = [ - "pragma: no cover", +# Defaults already cover `pragma: no cover`. Add patterns here when you +# introduce code that should NOT count against coverage — e.g. +# `if TYPE_CHECKING:` blocks, `raise NotImplementedError` stubs. +exclude_also = [ + "if TYPE_CHECKING:", "if __name__ == .__main__.:", - "raise NotImplementedError", ] diff --git a/src/my_mcp_server/__main__.py b/src/my_mcp_server/__main__.py index 3a27a02..8cc68f4 100644 --- a/src/my_mcp_server/__main__.py +++ b/src/my_mcp_server/__main__.py @@ -1,14 +1,31 @@ -"""Allow running as ``python -m my_mcp_server``.""" +"""Allow running as ``python -m my_mcp_server``. + +The wrap-and-exit logic lives in ``run()`` so it can be tested directly +(``import my_mcp_server.__main__`` no longer blocks on the server loop). +""" import logging import sys from my_mcp_server.server import main -try: - main() -except KeyboardInterrupt: - sys.exit(0) -except Exception: - logging.exception("Fatal error running MCP server") - sys.exit(1) + +def run() -> int: + """Run ``server.main()`` and translate termination into a process exit code. + + Returns: + ``0`` on clean return or Ctrl-C, ``1`` on any other exception + (logged via ``logging.exception`` so the traceback hits stderr). + """ + try: + main() + except KeyboardInterrupt: + return 0 + except Exception: + logging.exception("Fatal error running MCP server") + return 1 + return 0 + + +if __name__ == "__main__": + sys.exit(run()) diff --git a/src/my_mcp_server/resources/server_info.py b/src/my_mcp_server/resources/server_info.py index a2a235d..1b1a4d8 100644 --- a/src/my_mcp_server/resources/server_info.py +++ b/src/my_mcp_server/resources/server_info.py @@ -22,6 +22,14 @@ DESCRIPTION = "Server metadata: name, version, Python runtime, and platform." MIME_TYPE = "application/json" +# Convention: PyPI distribution name = import package name with underscores +# rewritten as hyphens. Derived from `__package__` so a clone that renames +# `src/my_mcp_server/` (per setup.yml's first-run checklist) doesn't have to +# update a second hardcoded literal — the wheel-install fallback below picks +# the new name up automatically. +PKG_NAME = (__package__ or "my_mcp_server").split(".")[0].replace("_", "-") +FALLBACK_VERSION = "0.0.0" + def _read_pyproject() -> dict[str, str] | None: """Walk up from this file to locate pyproject.toml and parse it. @@ -45,15 +53,15 @@ def _read_pyproject() -> dict[str, str] | None: def _server_metadata() -> dict[str, object]: project = _read_pyproject() if project is not None: - pkg_name = str(project.get("name", "my-mcp-server")) - pkg_version = str(project.get("version", "0.0.0")) + pkg_name = str(project.get("name", PKG_NAME)) + pkg_version = str(project.get("version", FALLBACK_VERSION)) else: # Fallback for wheel installs where pyproject.toml isn't shipped. - pkg_name = "my-mcp-server" + pkg_name = PKG_NAME try: pkg_version = version(pkg_name) except PackageNotFoundError: - pkg_version = "0.0.0" + pkg_version = FALLBACK_VERSION return { "name": pkg_name, diff --git a/src/my_mcp_server/server.py b/src/my_mcp_server/server.py index d0b956f..1340dfe 100644 --- a/src/my_mcp_server/server.py +++ b/src/my_mcp_server/server.py @@ -1,7 +1,7 @@ """MCP server entry point. -Registers tools, resources, and prompts via FastMCP. -Add your own tools in the tools/ directory following the greet.py pattern. +Registers tools, resources, and prompts via FastMCP. Add your own tools +inline below the existing `greet` example, or split them into modules. """ import logging @@ -68,11 +68,9 @@ async def greet( return f"Hello, {name}!" -# To add more tools, either decorate inline above or create a module under -# tools/ exposing a `register(mcp)` function and call it here, e.g.: -# -# from my_mcp_server.tools.your_tool import register -# register(mcp) +# To add more tools, either decorate inline above or split them into modules +# (see resources/server_info.py and prompts/code_review.py for the `register(mcp)` +# pattern this repo applies to resources and prompts). # --------------------------------------------------------------------------- diff --git a/src/my_mcp_server/tools/__init__.py b/src/my_mcp_server/tools/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..13a25d4 --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,46 @@ +"""Tests for the ``python -m my_mcp_server`` entry-point wrapper. + +These exercise ``__main__.run()`` directly — the body of __main__.py is +guarded by ``if __name__ == "__main__":`` so importing the module no +longer blocks on the server loop, which is what makes these tests possible. +""" + +from __future__ import annotations + +import logging + +from my_mcp_server import __main__ as entry + + +def test_run_returns_zero_on_clean_exit(monkeypatch) -> None: + """main() returns normally → run() returns 0.""" + monkeypatch.setattr(entry, "main", lambda: None) + assert entry.run() == 0 + + +def test_run_returns_zero_on_keyboard_interrupt(monkeypatch) -> None: + """Ctrl-C during main() → run() exits cleanly with 0.""" + + def raise_keyboard_interrupt() -> None: + raise KeyboardInterrupt() + + monkeypatch.setattr(entry, "main", raise_keyboard_interrupt) + assert entry.run() == 0 + + +def test_run_returns_one_and_logs_on_unhandled_exception(monkeypatch, caplog) -> None: + """Any other exception → run() returns 1 AND logs the traceback. Both + halves are part of the contract: silently exiting 1 without the log + would make production debugging much harder.""" + + def boom() -> None: + raise RuntimeError("simulated server crash") + + monkeypatch.setattr(entry, "main", boom) + + with caplog.at_level(logging.ERROR): + rc = entry.run() + + assert rc == 1 + assert "Fatal error running MCP server" in caplog.text + assert "simulated server crash" in caplog.text diff --git a/tests/test_server_info.py b/tests/test_server_info.py index 7a29f5b..1acd75c 100644 --- a/tests/test_server_info.py +++ b/tests/test_server_info.py @@ -69,54 +69,111 @@ async def test_registered_on_server() -> None: # When the package is installed from a wheel, pyproject.toml is NOT shipped, # so `_read_pyproject()` returns None and `_server_metadata()` must fall back # to `importlib.metadata.version()`. The tests above all exercise the source -# tree (where pyproject.toml IS reachable) — without these two cases, the -# production install path is 0% covered. +# tree (where pyproject.toml IS reachable). async def test_falls_back_to_importlib_metadata_when_pyproject_missing( monkeypatch, ) -> None: - """Wheel install: _read_pyproject() returns None, version comes from metadata.""" + """Wheel install: _read_pyproject() returns None, both name and version + come from importlib.metadata (both sides of the equality below resolve + against the SAME source, not pyproject-on-disk — so a maintainer who + bumps pyproject.toml's version without re-running ``pip install -e .`` + won't see a misleading failure here).""" + from importlib.metadata import version as imp_version + from my_mcp_server.resources import server_info as mod monkeypatch.setattr(mod, "_read_pyproject", lambda: None) payload = json.loads(await mod.server_info()) - # Package is installed editable in tests; importlib.metadata can find it. - project = _pyproject_project() - assert payload["name"] == "my-mcp-server" - assert payload["version"] == project["version"] + # `mod.PKG_NAME` is derived from `__package__` — survives a clone rename + # without re-hardcoding the literal in the test. + assert payload["name"] == mod.PKG_NAME + assert payload["version"] == imp_version(mod.PKG_NAME) async def test_falls_back_to_zero_version_when_package_not_installed( monkeypatch, ) -> None: """Edge of the edge: pyproject missing AND importlib.metadata can't find - the dist. Should return '0.0.0' instead of raising PackageNotFoundError.""" + the dist. Should return FALLBACK_VERSION instead of raising + PackageNotFoundError.""" from my_mcp_server.resources import server_info as mod - def raise_not_found(_name: str) -> str: + def raise_not_found(*_args: object, **_kw: object) -> str: + # Tolerant signature in case `version()` ever gets called with kwargs. raise mod.PackageNotFoundError("simulated wheel-install-without-metadata") monkeypatch.setattr(mod, "_read_pyproject", lambda: None) monkeypatch.setattr(mod, "version", raise_not_found) payload = json.loads(await mod.server_info()) - assert payload["version"] == "0.0.0" + assert payload["version"] == mod.FALLBACK_VERSION def test_read_pyproject_returns_none_when_project_table_missing(tmp_path, monkeypatch) -> None: - """Walk-up finds a pyproject.toml with no [project] table — exit path - at line 41 (the `return None` inside the `is_file` branch).""" + """Walk-up finds a pyproject.toml with no [project] table — must exit at + the inner `return None` (NOT continue walking — that's the contract). + + Strengthened with a tomllib.load spy so a future refactor that drops the + explicit inner return and falls through to keep walking gets caught here + (the spy would see >1 load) instead of silently passing because no other + pyproject.toml is reachable on the way to /. + """ + from my_mcp_server.resources import server_info as mod + + pkg = tmp_path / "pkg" + pkg.mkdir() + fake_pyproject = tmp_path / "pyproject.toml" + fake_pyproject.write_text("[build-system]\nrequires = []\n") + fake_file = pkg / "server_info.py" + fake_file.write_text("") + + loaded_paths: list[str] = [] + original_load = mod.tomllib.load + + def spy_load(fh): # type: ignore[no-untyped-def] + loaded_paths.append(getattr(fh, "name", "")) + return original_load(fh) + + monkeypatch.setattr(mod.tomllib, "load", spy_load) + monkeypatch.setattr(mod, "__file__", str(fake_file)) + + assert mod._read_pyproject() is None + assert loaded_paths == [str(fake_pyproject)], ( + f"_read_pyproject must stop at the first pyproject.toml encountered; " + f"saw {len(loaded_paths)} load(s): {loaded_paths}" + ) + + +def test_read_pyproject_returns_none_when_no_pyproject_anywhere(tmp_path, monkeypatch) -> None: + """Walk-up exhausts the parent chain without finding any pyproject.toml — + exercises the outer `return None` after the loop (server_info.py:42). + + Without this test the loop-exhaustion path is 0% covered: the + "fallback" tests above bypass the walk entirely by monkeypatching + `_read_pyproject` itself. + """ from my_mcp_server.resources import server_info as mod - # Build a fake directory tree: tmp_path/pkg/server_info.py with a - # pyproject.toml at tmp_path that lacks [project]. pkg = tmp_path / "pkg" pkg.mkdir() - (tmp_path / "pyproject.toml").write_text("[build-system]\nrequires = []\n") fake_file = pkg / "server_info.py" fake_file.write_text("") + # Deliberately no pyproject.toml anywhere in tmp_path or its ancestors + # (system tmp dirs don't contain one), so the walk reaches root. monkeypatch.setattr(mod, "__file__", str(fake_file)) assert mod._read_pyproject() is None + + +def test_pkg_name_derived_from_package() -> None: + """PKG_NAME is the dist-name form (hyphens) of the import package name — + pins the rename-safety contract documented in server_info.py.""" + from my_mcp_server.resources import server_info as mod + + assert mod.PKG_NAME == "my-mcp-server" + # Sanity: derivation tracks __package__, not a stray literal somewhere. + root_import = (mod.__package__ or "").split(".")[0] + assert root_import.replace("_", "-") == mod.PKG_NAME diff --git a/tests/test_tools.py b/tests/test_tools.py index 4cb136b..f87db19 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -52,6 +52,20 @@ async def test_greet_accepts_boundary_lengths(): # JSON Schema generation (the actual MCP wire contract) is never exercised. +def _schema_constraint(prop: dict, key: str) -> object: + """Read a JSON Schema keyword that may sit at the top of a property OR + inside an ``anyOf`` branch (which is how pydantic emits constraints once + a field gains a union type, e.g. ``Annotated[str | None, Field(...)]``). + Returns the first match; raises a clear AssertionError if neither shape + contains the key.""" + if key in prop: + return prop[key] + for sub in prop.get("anyOf", []): + if key in sub: + return sub[key] + raise AssertionError(f"{key!r} not found in property schema {prop!r}") + + async def test_greet_registered_on_server(): """The greet tool is wired into the server and its JSON Schema reflects the Annotated[..., Field(min_length, max_length)] constraints.""" @@ -63,7 +77,7 @@ async def test_greet_registered_on_server(): schema = by_name["greet"].inputSchema name_prop = schema["properties"]["name"] - assert name_prop["type"] == "string" - assert name_prop["minLength"] == 1 - assert name_prop["maxLength"] == 200 + assert _schema_constraint(name_prop, "type") == "string" + assert _schema_constraint(name_prop, "minLength") == 1 + assert _schema_constraint(name_prop, "maxLength") == 200 assert "name" in schema["required"]