fix: review-pass findings + reviewer standards doc

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 <repo> 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.

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

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]}")
 
 

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)

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]
 ---

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

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()

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)

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]] = {}
 

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],

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 "