diff --git a/docs/code-review.md b/docs/code-review.md new file mode 100644 index 0000000..3d301aa --- /dev/null +++ b/docs/code-review.md @@ -0,0 +1,61 @@ +# Code review standards + +The reviewer's contract for this repo — and a reusable checklist for any +Python team. + +## Layer 0: machines argue about style, humans argue about design + +These run in CI; a human review comment about anything they cover is wasted: + +| Tool | Standard it enforces | +|---|---| +| `ruff check` + `ruff format` | PEP 8, import order, bugbear/simplify/pyupgrade rule packs — each rule documented at [docs.astral.sh/ruff/rules](https://docs.astral.sh/ruff/rules/) | +| `mypy --strict` | PEP 484 typing, no untyped defs, no implicit Any | +| `pytest` + coverage | behavior, not just "it imports" | +| `python -m design_patterns.readme_table --check` | docs can't drift from code | + +Worth adding for security-sensitive work: `bandit` (SAST) and `pip-audit` +(dependency CVEs). Note: bandit flags every `assert` (B101) — in pytest +tests that's idiomatic, not a finding. + +## Layer 1: the written standards behind the tools + +- **PEP 8** (style) · **PEP 257** (docstrings) · **PEP 20** (design sensibility) +- **Google Python Style Guide** — the most common team-level extension +- This repo's own bar: [CLAUDE.md](../CLAUDE.md) (unit template, frontmatter + schema) and [verdicts.md](verdicts.md) + +## Layer 2: what human reviewers actually check + +Severity-ordered — block on CRITICAL/HIGH, note MEDIUM: + +**CRITICAL** +- Injection: user input reaching `eval`/`exec`, SQL strings, `subprocess` with `shell=True` +- Unsafe deserialization: `pickle.loads`/`yaml.load` on data crossing a trust boundary +- Secrets in code + +**HIGH** +- `assert` as a runtime guard (vanishes under `python -O`) +- Mutable default arguments; shared mutable module state +- Swallowed exceptions (`except: pass`), or `except Exception` hiding real errors +- Resources without context managers; missing cleanup on the error path +- Thread-safety claims the code doesn't earn (unguarded lazy init, shared caches) +- Unbounded recursion/loops on user-controlled input + +**MEDIUM** +- Work at import time (I/O, big computation) — see `patterns/python/global_object` +- `isinstance` traps (`bool` passes `int` checks), `is` vs `==` on sentinels +- API honesty: docstrings/comments that promise more than the code delivers +- A design pattern where a language feature suffices — check the catalog's verdict first + +## Reference sources for reviewers (MCP) + +- **This repo's own MCP server** — `claude mcp add design-patterns -- uv run --directory python-design-patterns-mcp`. `recommend_pattern` answers "should this be a Singleton?" with python-patterns.guide's verdicts and caveats; `get_pattern` serves the reference implementation to compare against. +- **Context7 MCP** — current library/framework docs, for "is this the right API usage?" questions. +- **python-patterns.guide** — the prose authority behind this catalog's verdicts. + +## Review etiquette + +- Cite the rule or the file, not taste ("B008: mutable default" beats "I don't like this"). +- One approval pass = one severity sweep top-down; don't drip-feed. +- The author of a change never approves it. diff --git a/docs/index.md b/docs/index.md index 8cfb22f..208cff9 100644 --- a/docs/index.md +++ b/docs/index.md @@ -3,4 +3,5 @@ - [How to read this repo](how-to-read-this-repo.md) — the unit anatomy and where to start - [Verdicts](verdicts.md) — what ✅ / ⚠️ / 🔄 mean, and who decides - [MCP server](mcp.md) — connect agents to the catalog +- [Code review standards](code-review.md) — the reviewer's contract and severity checklist - [Contributing](contributing.md) — adding or improving a pattern unit diff --git a/patterns/behavioral/interpreter/real_world.py b/patterns/behavioral/interpreter/real_world.py index f288305..21b9490 100644 --- a/patterns/behavioral/interpreter/real_world.py +++ b/patterns/behavioral/interpreter/real_world.py @@ -18,18 +18,31 @@ } -def safe_eval(formula: str) -> float: - """Evaluate arithmetic like '2 * (3 + 4)'; reject everything else.""" - return _walk(ast.parse(formula, mode="eval").body) +#: Deeper than any human formula; shallower than the recursion limit, so a +#: hostile input gets a clean ValueError instead of a RecursionError crash. +MAX_DEPTH = 50 -def _walk(node: ast.expr) -> float: - if isinstance(node, ast.Constant) and isinstance(node.value, int | float): +def safe_eval(formula: str) -> float: + """Evaluate arithmetic like '2 * (3 + 4)'; reject everything else.""" + return _walk(ast.parse(formula, mode="eval").body, depth=0) + + +def _walk(node: ast.expr, depth: int) -> float: + if depth > MAX_DEPTH: + raise ValueError("expression too deeply nested") + if ( + isinstance(node, ast.Constant) + and isinstance(node.value, int | float) + and not isinstance(node.value, bool) + # bool subclasses int, and a *safe* evaluator should not quietly + # compute True + 1 -- so it is excluded explicitly. + ): return float(node.value) if isinstance(node, ast.BinOp) and type(node.op) in _BINOPS: - return _BINOPS[type(node.op)](_walk(node.left), _walk(node.right)) + return _BINOPS[type(node.op)](_walk(node.left, depth + 1), _walk(node.right, depth + 1)) if isinstance(node, ast.UnaryOp) and isinstance(node.op, ast.USub): - return -_walk(node.operand) + return -_walk(node.operand, depth + 1) raise ValueError(f"disallowed syntax: {ast.dump(node)[:40]}") diff --git a/patterns/behavioral/interpreter/tests/test_interpreter.py b/patterns/behavioral/interpreter/tests/test_interpreter.py index deb7f77..762f951 100644 --- a/patterns/behavioral/interpreter/tests/test_interpreter.py +++ b/patterns/behavioral/interpreter/tests/test_interpreter.py @@ -34,3 +34,13 @@ def test_attack_is_rejected_not_executed(self) -> None: def test_names_are_rejected(self) -> None: with pytest.raises(ValueError): real_world.safe_eval("x + 1") + + def test_bool_constants_are_rejected(self) -> None: + # bool subclasses int; a safe evaluator must not compute True + 1. + with pytest.raises(ValueError, match="disallowed"): + real_world.safe_eval("True + 1") + + def test_hostile_nesting_gets_a_clean_error_not_a_crash(self) -> None: + bomb = "1" + " + 1" * 200 # deeper than MAX_DEPTH + with pytest.raises(ValueError, match="deeply nested"): + real_world.safe_eval(bomb) diff --git a/patterns/behavioral/memento/README.md b/patterns/behavioral/memento/README.md index e1da153..423cf61 100644 --- a/patterns/behavioral/memento/README.md +++ b/patterns/behavioral/memento/README.md @@ -8,6 +8,7 @@ symptoms: ["undo", "checkpoint and rollback", "save game", "restore previous sta verdict: use-with-care caveats: - "Immutable state makes the pattern nearly free: a snapshot is just keeping the old object. Design the state to be frozen and mementos fall out." + - "pickle.loads executes code while deserializing — only unpickle snapshots your own process produced; use JSON for anything crossing a trust boundary." - "Deep-copying big mutable graphs per keystroke is the naive cost; snapshot the smallest state that matters." stdlib_sightings: [copy.deepcopy, pickle.dumps, dataclasses.replace] --- diff --git a/patterns/behavioral/memento/real_world.py b/patterns/behavioral/memento/real_world.py index c17c7c0..740780a 100644 --- a/patterns/behavioral/memento/real_world.py +++ b/patterns/behavioral/memento/real_world.py @@ -2,6 +2,11 @@ dumps() produces an opaque snapshot; loads() restores an equivalent object -- checkpoint/rollback for anything picklable. + +SECURITY: ``pickle.loads`` executes code during deserialization. Only ever +unpickle snapshots your own process produced and stored somewhere untrusted +input cannot reach (CWE-502). For snapshots that cross a trust boundary, +serialize explicit state as JSON instead. """ from __future__ import annotations @@ -21,6 +26,7 @@ def checkpoint(game: Game) -> bytes: def rollback(snapshot: bytes) -> Game: + # Safe ONLY because `snapshot` came from checkpoint() in this process. restored = pickle.loads(snapshot) assert isinstance(restored, Game) return restored diff --git a/patterns/creational/singleton/pythonic.py b/patterns/creational/singleton/pythonic.py index 8764022..0d2d01b 100644 --- a/patterns/creational/singleton/pythonic.py +++ b/patterns/creational/singleton/pythonic.py @@ -30,7 +30,12 @@ def log(self, message: str) -> None: def get_logger() -> Logger: - """Build the shared instance on first call, then keep handing it back.""" + """Build the shared instance on first call, then keep handing it back. + + Not thread-safe: two threads racing the first call can each build a + Logger (one wins the slot). Harmless for a cheap object; guard with a + threading.Lock if construction has side effects. + """ global _lazy_instance if _lazy_instance is None: _lazy_instance = Logger() diff --git a/patterns/structural/facade/pythonic.py b/patterns/structural/facade/pythonic.py index ac99167..465ab01 100644 --- a/patterns/structural/facade/pythonic.py +++ b/patterns/structural/facade/pythonic.py @@ -81,6 +81,9 @@ def place_order( except PermissionError: warehouse.release(sku, quantity) # the step copy-paste always forgets raise + # Honest boundary: a crash below this line leaves the charge captured. + # Real systems make charge/label/notify a saga (compensate on failure) + # or an idempotent retry -- the facade pattern doesn't solve that part. label = shipping.create_label(sku, address) notifier.confirm(address, txn, label) return OrderResult(transaction_id=txn, shipping_label=label) diff --git a/patterns/structural/flyweight/pythonic.py b/patterns/structural/flyweight/pythonic.py index 0e2e55b..d97caf0 100644 --- a/patterns/structural/flyweight/pythonic.py +++ b/patterns/structural/flyweight/pythonic.py @@ -18,7 +18,11 @@ def get_card(rank: str, suit: str) -> tuple[str, str]: class Card: - """The __new__ form: ``Card('9', '♥') is Card('9', '♥')``.""" + """The __new__ form: ``Card('9', '♥') is Card('9', '♥')``. + + The pool is unbounded and unsynchronized: fine for a fixed domain like + 52 cards, wrong for unbounded user-supplied keys or racing threads. + """ _pool: ClassVar[dict[tuple[str, str], Card]] = {} diff --git a/src/design_patterns_mcp/sandbox.py b/src/design_patterns_mcp/sandbox.py index a997abd..1e69529 100644 --- a/src/design_patterns_mcp/sandbox.py +++ b/src/design_patterns_mcp/sandbox.py @@ -33,6 +33,8 @@ def run_example(catalog: Catalog, pattern_id: str, variant: str) -> RunResult: if variant not in variants: raise KeyError(f"{pattern_id} has no variant {variant!r} (has: {sorted(variants)})") path = variants[variant] # resolved by the catalog, never by the caller + if not path.is_file(): # a real check, not an assert: survives python -O + raise FileNotFoundError(f"catalog names {path} but it does not exist") repo_root = pattern.path.parents[2] module = f"patterns.{pattern.group}.{pattern.slug}.{variant}" @@ -54,8 +56,13 @@ def run_example(catalog: Catalog, pattern_id: str, variant: str) -> RunResult: ) except subprocess.TimeoutExpired as exc: out = exc.stdout.decode() if isinstance(exc.stdout, bytes) else (exc.stdout or "") - return RunResult(exit_code=-1, stdout=out[:MAX_OUTPUT_BYTES], stderr="", timed_out=True) - assert path.is_file() + err = exc.stderr.decode() if isinstance(exc.stderr, bytes) else (exc.stderr or "") + return RunResult( + exit_code=-1, + stdout=out[:MAX_OUTPUT_BYTES], + stderr=err[:MAX_OUTPUT_BYTES], + timed_out=True, + ) return RunResult( exit_code=completed.returncode, stdout=completed.stdout[:MAX_OUTPUT_BYTES], diff --git a/src/design_patterns_mcp/server.py b/src/design_patterns_mcp/server.py index 29a2077..1cab21f 100644 --- a/src/design_patterns_mcp/server.py +++ b/src/design_patterns_mcp/server.py @@ -7,6 +7,7 @@ from __future__ import annotations import argparse +from functools import lru_cache from typing import Any from mcp.server import MCPServer @@ -15,8 +16,18 @@ from design_patterns_mcp.sandbox import run_example as _run_example from design_patterns_mcp.search import SearchIndex -_catalog: Catalog = load_catalog() -_index = SearchIndex(_catalog) + +# Lazy initialization (see patterns/python/global_object): importing this +# module must not do disk I/O; the catalog loads on first use, once. +@lru_cache(maxsize=1) +def get_catalog() -> Catalog: + return load_catalog() + + +@lru_cache(maxsize=1) +def get_index() -> SearchIndex: + return SearchIndex(get_catalog()) + mcp = MCPServer( "python-design-patterns", @@ -63,7 +74,7 @@ def list_patterns(group: str | None = None, verdict: str | None = None) -> list[ """List catalog patterns, optionally filtered by group (creational, structural, behavioral, python, principle, modern) or verdict (pythonic, use-with-care, prefer-alternative).""" - patterns = _catalog.patterns + patterns = get_catalog().patterns if group is not None: patterns = tuple(p for p in patterns if p.group == group) if verdict is not None: @@ -77,9 +88,9 @@ def get_pattern(pattern_id: str, variant: str | None = None) -> dict[str, Any]: (e.g. 'structural/decorator'). variant: 'naive', 'pythonic', 'real_world', or 'all' to include example source code.""" try: - pattern = _catalog.get(pattern_id) + pattern = get_catalog().get(pattern_id) except KeyError: - known = ", ".join(_catalog.ids()) + known = ", ".join(get_catalog().ids()) raise ValueError(f"unknown pattern {pattern_id!r}; known ids: {known}") from None return _detail(pattern, variant) @@ -88,14 +99,14 @@ def get_pattern(pattern_id: str, variant: str | None = None) -> dict[str, Any]: def search_patterns(query: str, limit: int = 5) -> list[dict[str, Any]]: """Full-text search across pattern names, aliases, problems, symptoms, and prose. Returns the best matches with scores.""" - return [{**_summary(h.pattern), "score": h.score} for h in _index.search(query, limit)] + return [{**_summary(h.pattern), "score": h.score} for h in get_index().search(query, limit)] @mcp.tool() def run_example(pattern_id: str, variant: str) -> dict[str, Any]: """Execute one of a pattern's vendored example files ('naive', 'pythonic', 'real_world') in a sandboxed subprocess and return its real output.""" - result = _run_example(_catalog, pattern_id, variant) + result = _run_example(get_catalog(), pattern_id, variant) return { "exit_code": result.exit_code, "stdout": result.stdout, @@ -110,7 +121,7 @@ def recommend_pattern(problem_statement: str, limit: int = 3) -> list[dict[str, each with its caveats and verdict attached. A 'prefer-alternative' verdict means the pythonic variant shows what to write instead.""" recommendations = [] - for hit in _index.search(problem_statement, limit): + for hit in get_index().search(problem_statement, limit): p = hit.pattern rec = { **_summary(p), @@ -130,19 +141,19 @@ def recommend_pattern(problem_statement: str, limit: int = 3) -> list[dict[str, @mcp.resource("catalog://index") def catalog_index() -> str: """The whole catalog as JSON: every pattern's metadata and variants.""" - return _catalog.to_json() + return get_catalog().to_json() @mcp.resource("pattern://{group}/{slug}") def pattern_doc(group: str, slug: str) -> str: """One pattern's README prose.""" - return _catalog.get(f"{group}/{slug}").prose + return get_catalog().get(f"{group}/{slug}").prose @mcp.resource("pattern://{group}/{slug}/{variant}") def pattern_source(group: str, slug: str, variant: str) -> str: """One pattern's example source (naive | pythonic | real_world).""" - pattern = _catalog.get(f"{group}/{slug}") + pattern = get_catalog().get(f"{group}/{slug}") variants = pattern.variants() if variant not in variants: raise KeyError(f"{pattern.id} has no variant {variant!r}") @@ -152,7 +163,7 @@ def pattern_source(group: str, slug: str, variant: str) -> str: @mcp.prompt() def refactor_toward(pattern_id: str, code: str) -> str: """Ask for a refactor of the given code toward one catalog pattern.""" - pattern = _catalog.get(pattern_id) + pattern = get_catalog().get(pattern_id) caveats = "\n".join(f"- {c}" for c in pattern.caveats) return ( f"Refactor the following code toward the {pattern.name} pattern " @@ -165,7 +176,7 @@ def refactor_toward(pattern_id: str, code: str) -> str: @mcp.prompt() def explain_pattern(pattern_id: str, audience: str = "an intermediate Python developer") -> str: """Ask for an explanation of one pattern, tuned to an audience.""" - pattern = _catalog.get(pattern_id) + pattern = get_catalog().get(pattern_id) return ( f"Explain the {pattern.name} pattern to {audience}. Problem it solves: " f"{pattern.problem} Use the catalog's naive-vs-pythonic contrast, state "