fix: resolve all 15 /code-review findings (workflow bugs, doc drift, test integrity, rename-safety) by heznpc · Pull Request #36 · starter-series/python-mcp-server-starter

heznpc · 2026-05-28T21:45:47Z

Single PR addressing every CONFIRMED + PLAUSIBLE finding from the extra-high-effort /code-review on ec7045d..HEAD. Three REFUTED findings dropped.

Workflow bugs (active failures, fix-first)

#	File	Fix
1	`dependabot-auto-merge.yml`	Pre-create `manual-review` label via `gh label create --force`. Previously failed red on every Dependabot major bump (PR #34 confirmed).
2	`stale.yml`	Add `dependencies,manual-review` to `exempt-pr-labels` — Dependabot major-bump PRs no longer auto-closed after 37 days.
5	`dependabot-auto-merge.yml`	Major branch now `gh pr merge --disable-auto` before re-labeling — a Dependabot rebase that re-classifies minor→major no longer keeps the prior `--auto` enable.

Doc drift

#	Surface	Fix
3	`README.md` + `README.ko.md`	Drop stale `tools/greet.py` from structure trees; replace stale "Modular (recommended for larger servers)" code block 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 a courtesy stub matching the file's own "GitHub Releases is canonical" framing. All bullets were historically false after PRs #28-#34 + #27.
6	`server.py` docstring + tools-section comment	Reference the live register-pattern in `resources/`/`prompts/`; delete dead `tools/__init__.py`.
11	`setup.yml` first-run issue	Rewrite "Replace the example `greet` tool" checklist item to match reality (greet now lives inline in `server.py`).
12	`SECURITY.md`	One-line WARNING above the branch-protection JSON: hardcoded `test (3.11/3.12/3.13)` contexts MUST be updated in lockstep with matrix changes.
13	`SECURITY.md`	Prepend `gh auth refresh -s admin:repo,security_events` so copy-pasters don't hit opaque 404s on `vulnerability-alerts` / `automated-security-fixes` PUTs.

Production code (rename-safety + entry-point testability)

#	File	Fix
7+8	`resources/server_info.py`	Replace two hardcoded `"my-mcp-server"` literals with `PKG_NAME = __package__.split(".")[0].replace("_", "-")`. Clone renames per setup.yml now flow through to the wheel-install fallback.
14	`__main__.py`	Extract `run() -> int` and guard the call site with `if __name__ == "__main__":`. Importing `my_mcp_server.__main__` no longer blocks on the server loop, and the KeyboardInterrupt/Exception arms become testable.

Test integrity

#	File	Fix
6	`tests/test_server_info.py`	Add `test_read_pyproject_returns_none_when_no_pyproject_anywhere` exercising the OUTER `return None` (loop exhaustion at `server_info.py:42`) the previous wheel-fallback tests bypassed via monkeypatch.
9	`tests/test_server_info.py`	`test_read_pyproject_returns_none_when_project_table_missing` installs a spy on `tomllib.load` and asserts EXACTLY ONE pyproject was read — a future refactor that drops the inner `return None` and keeps walking now fails here instead of silently passing.
7+8	`tests/test_server_info.py`	`test_falls_back_to_importlib_metadata_when_pyproject_missing` compares both sides against `importlib.metadata` (same source). Uncommitted version bumps no longer cause misleading failures.
10	`tests/test_server_info.py`	Add `test_pkg_name_derived_from_package` pinning the rename-safety contract.
15	`tests/test_tools.py`	`_schema_constraint` helper reads a JSON Schema keyword from top-of-property OR `anyOf` branch — a future widening to `Annotated[str
13	`tests/test_main.py` (NEW)	3 tests for `run()`'s clean / KeyboardInterrupt / Exception arms. Exception test also asserts traceback is logged.
17	`pyproject.toml`	Switch `exclude_lines` (which appended to defaults and matched nothing) to `exclude_also` with patterns that actually match (`if TYPE_CHECKING:`, `if __name__ == "__main__":`).

Verification

ruff check . / ruff format --check / mypy src/ → clean
pytest → 26 passed (was 21; +5 net new)
coverage: 70.64% → 97.06% (__main__.py 0%→100%, server_info.py 96%→100%)

Findings REFUTED (no change required)

C-F1 (GITHUB_TOKEN read-only on Dependabot PRs): empirically false here — explicit permissions: block on dependabot-auto-merge.yml grants writes; PR chore(deps): bump the actions group across 1 directory with 3 updates #34's failure was the manual-review label, not the token.
C-F14 (cd.yml top-level perms 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.

Test plan

All 26 tests pass locally with --cov-fail-under=70 (actual 97.06%)
ruff check . + ruff format --check . + mypy src/ all clean
CI matrix 3.11/3.12/3.13 green
CodeQL analyze (python) + analyze (actions) green

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.

… a git repository') (#37) PR #36 added `gh label create` + `gh pr merge --disable-auto` + `gh pr edit` to the major-update branch but the workflow has no `actions/checkout` step. Without a `.git` directory in the workdir, `gh` errors out with `fatal: not a git repository (or any of the parent directories): .git` before any subcommand runs. Observed on PR #35's auto-merge run (logged 2026-05-28T21:47:56.2681559Z). Two fixes: - Set `GH_REPO: ${{ github.repository }}` on the env block of BOTH steps (`gh` consults this env var to resolve the target repo when no git context is present). - Leaves the minor/patch branch with the same shielding even though its current single command (`gh pr merge --auto --squash`) already accepts a URL argument; defensive symmetry against future additions. No source code changes; workflow-only fix.

heznpc enabled auto-merge (squash) May 28, 2026 21:45

heznpc merged commit 43d1efe into main May 28, 2026
8 checks passed

heznpc deleted the fix/code-review-15-findings-2026-05-21 branch May 28, 2026 21:46

heznpc mentioned this pull request May 28, 2026

fix: dependabot-auto-merge needs GH_REPO env (no checkout step) #37

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve all 15 /code-review findings (workflow bugs, doc drift, test integrity, rename-safety)#36

fix: resolve all 15 /code-review findings (workflow bugs, doc drift, test integrity, rename-safety)#36
heznpc merged 1 commit into
mainfrom
fix/code-review-15-findings-2026-05-21

heznpc commented May 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

heznpc commented May 28, 2026

Workflow bugs (active failures, fix-first)

Doc drift

Production code (rename-safety + entry-point testability)

Test integrity

Verification

Findings REFUTED (no change required)

Test plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant