chore: simplify — address /simplify review findings#27
Merged
Conversation
…ion sweep Fixes from three-agent review of 01ed305: Correctness - SECURITY.md: delete duplicate "Reporting" section (the sweep added a new one without removing the original). Efficiency - ci.yml + cd.yml: add `cache: pip` + `cache-dependency-path: pyproject.toml` to all setup-python steps. Saves ~30-60s per matrix job on cache hit. - pyproject.toml: move --cov / --cov-fail-under out of pytest addopts; CI now passes the flags explicitly. Local `pytest tests/test_one.py` no longer pays the instrumentation tax or fails on --cov-fail-under from a single-file run. - codeql.yml: gate Autobuild step with `if: matrix.language == 'python'`. The `actions` matrix entry skips the no-op build step. Quality (comment trim — every starter clone reads these) - pyproject.toml: drop multi-line WHAT comments on dev deps + ruff rule list. - cd.yml: drop comments narrating step names and pypa guidance restatements. - ci.yml: drop comment explaining why pytest has no flags (now moot). - codeql.yml: drop "leave for simplicity" non-decision narration. - .github/CODEOWNERS: collapse 3-line preamble to 1 line. Other - .github/PULL_REQUEST_TEMPLATE.md: drop hardcoded `>= 70%` (would drift silently if threshold bumped); reference ci.yml as source of truth. - Delete .python-version. Forcing pyenv 3.13 on every clone hurts UX for developers who only have 3.12 installed; CI matrix is the canonical Python-version source. Skipped from agent findings (false positives or out-of-scope): - Cross-repo CODEOWNERS/templates hoisting (agent itself flagged "do not fix here"). - cd.yml version extraction via __version__ (no real drift; current tomllib approach is fine). - gitleaks fetch-depth shallow optimization (mixed trade-offs). - Merge `licenses` job into `test (3.13)` (loses parallelism). - cd.yml `ci:` re-invocation removal (pre-existing scope). - CHANGELOG entry compression (accurate, not a footgun). Local verification: - bare `pytest` → 17 passed, 0.15s (no cov) - `pytest --cov=...` → 17 passed, 70.64% coverage - ruff/mypy/ruff format → clean
5 tasks
heznpc
added a commit
that referenced
this pull request
May 21, 2026
Adversarial second-pass on commits 01ed305 (PR #26) and ec7045d (PR #27), both reported as "done / tests pass / CI green". Two Critical findings. [C1] CodeQL `analyze (actions)` open alert - Rule: `actions/missing-workflow-permissions` - File: .github/workflows/cd.yml - Message: "Actions job or workflow does not limit the permissions of the GITHUB_TOKEN." - Detected by the CodeQL `actions` language matrix that PR #26 itself added. PR #26 introduced top-level permissions on most workflows but missed cd.yml and setup.yml. The 5 required CI checks didn't include CodeQL, so the alert landed silently after merge. - Fix: add `permissions: contents: read` at workflow top level in cd.yml and setup.yml. Jobs continue to opt into the writes they need (contents:write / id-token:write / attestations:write for publish; issues:write for setup). [C2] `ok()` and `err()` Response Helpers — dead code marketed as feature - server.py:41 and server.py:47 defined ok() / err() helpers. - Zero references anywhere in src/ or tests/ (`grep -rn "\\bok(\\|\\berr("` = only the definitions + the introductory comment). - Coverage report confirmed: server.py:43-44, 49 uncovered. - README.md:32 and README.ko.md:30 marketed these as "Response Helpers — ok() and err() for consistent tool responses" in the "What You Get" list — i.e. a public feature claim with no backing code usage. - The bundled `greet` tool returns plain `str` — the README's marketed pattern doesn't match the bundled example. - FastMCP's idiom is `return value` / `raise on error`; ok/err helpers are a raw-MCP-without-FastMCP relic that actively teach the wrong pattern. - Fix: delete ok() / err() / their introductory comment. Replace the Tools-section banner comment with one line documenting the actual FastMCP idiom. Remove the "Response Helpers" bullet from README.md and README.ko.md. Why this matters beyond cosmetics: a starter is a teaching artifact. False "What You Get" entries silently propagate the wrong pattern to every clone. The dead helpers were also reachable code surface that CodeQL has to scan but no test exercises. Verification - All 7 workflows now have top-level `permissions:` (verified via `awk` scan of column-1 `permissions:` line). - ruff / ruff format / mypy strict / pytest 17/17 all clean. - Coverage 72.12% (improved from 70.64% — uncovered dead code removed). Out-of-scope for this PR (will be raised as Major findings for user decision): - tests/test_tools.py has no `test_greet_registered_on_server` despite test_server_info.py and test_code_review.py both having that pattern. - server_info.py wheel-install fallback path (lines 41-42, 52-56) is 0% covered; test only exercises the dev/source-tree read-pyproject path. - src/my_mcp_server/tools/greet.py defines a `register()` that's never called by server.py — fully dead modular example. - SECURITY.md "branch protection on main" claim is starter-side only; clones inherit nothing. - 4 Dependabot PRs (#28, #29, #30, #31) opened today — #28 is attest-build-provenance v3.2.0 → v4.1.0, making our 6-hour-old SHA pin already one major behind.
4 tasks
heznpc
added a commit
that referenced
this pull request
May 28, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three-agent
/simplifyreview of the modernization sweep (#26 / 01ed305) surfaced concrete issues. This PR fixes the real ones, skips the false-positives.Correctness
SECURITY.md— delete duplicate "Reporting" section. The sweep added a new one without removing the original.Efficiency (Efficiency agent — HIGH/MED)
ci.yml+cd.yml— addcache: pip+cache-dependency-path: pyproject.tomlto everysetup-pythonstep. Free ~30-60s per matrix job.pyproject.toml— move--cov/--cov-fail-underout of pytestaddopts. CI now passes the flags explicitly. Localpytest tests/test_one.pyno longer pays instrumentation tax or fails on--cov-fail-underfrom a single-file run.codeql.yml— gateAutobuildwithif: matrix.language == 'python'. Theactionsmatrix entry skips the no-op build step.Quality (Quality agent — comment cleanup)
pyproject.toml— drop multi-line WHAT comments on dev deps + ruff rule list (names are self-describing:B = bugbear,S = bandit).cd.yml— drop step-name-restating comments + pypa-guidance restatements.ci.yml— drop comment explaining why pytest has no flags (now moot, has flags).codeql.yml— drop "leave for simplicity" non-decision narration..github/CODEOWNERS— 3-line preamble → 1 line..github/PULL_REQUEST_TEMPLATE.md— drop hardcoded>= 70%(would silently drift); referenceci.ymlas source..python-version— forcing pyenv 3.13 on every clone hurts UX. CI matrix is the canonical version source.Skipped (agent findings deemed false-positive or out-of-scope)
cd.ymlversion extraction via__version__(no real drift risk).fetch-depth: 0→ shallow (mixed trade-offs).licensesjob intotest (3.13)(loses parallelism + separation).cd.ymlci:re-invocation removal (pre-existing scope, would need separate consideration).[Unreleased]compression (accurate, not a footgun).Test plan
pytestruns without coverage (17 passed, 0.15s)pytest --cov=my_mcp_server --cov-fail-under=70passes (70.64%)ruff check ./ruff format --check ./mypy src/cleananalyze (python)green;analyze (actions)skips Autobuild