From 7b80ed70a38ef5fc74bccb9e89552cedc6c05c6b Mon Sep 17 00:00:00 2001 From: Mateusz Konopelski Date: Tue, 7 Jul 2026 16:47:08 +0200 Subject: [PATCH 1/2] docs(review-backend): flag guards that defend against developer errors instead of runtime input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add review item #14 teaching reviewers to classify every if/else and try/except by what it guards against. A guard is legitimate only when the value can be wrong for reasons outside our code's control (user/API input, network, filesystem, DB availability, third-party payloads, concurrency). A defensive branch on a trusted internal value — an internal constant, an Enum member, a dict key that exists by construction, a field our own code sets, a registry entry — guards against a developer error: it adds dead branches to the hot path and hides the bug. Such invariant violations belong in a unit test, not a runtime branch; let the operation raise naturally (KeyError/AttributeError/TypeError) per Python EAFP, since a swallowed or re-wrapped developer error passes silently past the suite (PEP 20). This is the sibling of item #12 (asserts forbidden outside tests). --- .claude/commands/review-backend.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/commands/review-backend.md b/.claude/commands/review-backend.md index 141c475..243c8e6 100644 --- a/.claude/commands/review-backend.md +++ b/.claude/commands/review-backend.md @@ -102,6 +102,14 @@ Check new code against the mandatory patterns from `CLAUDE.md`: - **No raw collections as public API** — functions returning `dict`/`list` for structured data should return a typed model instead. - **Model with the right paradigm for clarity** — choose the tool that makes the domain intent obvious at the call site: pure functions for stateless transforms, magic methods for natural domain operations (`current + delta` via `__add__` instead of manual field construction), OOP for encapsulating state and invariants. Flag code that uses a weaker paradigm when a stronger one would eliminate boilerplate and make the intent self-evident. +**14. Guards that defend against developer errors instead of runtime input** +Classify every `if/else` and `try/except` by **what it guards against**. A guard is only legitimate when the value it checks can be wrong for reasons **outside our code's control**. This is the sibling of item #12 (`assert` forbidden outside tests) — same philosophy: internal invariants belong in unit tests, not hot-path branches. +- **Decision heuristic:** "Can this value only be wrong if another part of OUR code is wrong? → delete the guard, add a unit test, let it raise. Can it be wrong from input or environment outside our control? → keep the guard and handle it gracefully." +- **Keep** guards on real runtime/external conditions: user/API request bodies, network/HTTP responses, filesystem, DB availability, third-party payloads, parsing external/untrusted data, concurrency races. Validating a request field from a caller SHOULD be guarded and return a clear error. +- **Flag** defensive branches on trusted internal values — an internal constant, an `Enum` member, a dict key that exists by construction, a field our own code sets, a registry entry. These add dead branches to the hot path and hide the bug. Prefer letting the operation raise naturally (`KeyError`/`AttributeError`/`TypeError`) per Python **EAFP** (Easier to Ask Forgiveness than Permission), and cover the invariant with a unit test — unit tests are the sanctioned place to assert internal invariants. A swallowed or re-wrapped developer error violates PEP 20 (Zen of Python): "Errors should never pass silently" and "Explicit is better than implicit" — it slips past the test suite. +- **Concrete example:** an internal table-name registry lookup on an internal constant should be `table = _TABLE[name]` (raises `KeyError`; add a test asserting every collection constant is registered) — NOT `if name not in _TABLE: raise ValueError("register it")`. +- Flag "just in case" guards on trusted internal inputs as **CHANGES REQUIRED** (or a nit if trivial). + ## What NOT to flag - Minor inefficiencies (extra dict copy, redundant log line) From 00a0256559169845c4c5b730b87b0c17e1b9135a Mon Sep 17 00:00:00 2001 From: Mateusz Konopelski Date: Tue, 7 Jul 2026 16:53:33 +0200 Subject: [PATCH 2/2] docs(review-backend): add real DB examples of developer-error guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Turn item #14's single SQLite example into a "Real examples from this repo" list with two more verified cases from the DB layer on this branch: - db_adapter.py's repeated `and "_id" in r` re-check normalizing query rows — the IDatabase.query() contract guarantees `_id`, so the extra check defends against a backend breaking its own contract (developer error); prefer `r["id"] = r["_id"]` and unit-test the invariant. - memory.py `_matches_filter` has no `case _:` default, so an unsupported operator silently matches nothing while firestore.py `_apply_filter` raises — operators are hardcoded literals in our own query code, so a bad one is a developer error the memory backend lets pass silently (PEP 20), diverging from Firestore. Add a raising default and test it. --- .claude/commands/review-backend.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.claude/commands/review-backend.md b/.claude/commands/review-backend.md index 243c8e6..057c27d 100644 --- a/.claude/commands/review-backend.md +++ b/.claude/commands/review-backend.md @@ -107,7 +107,10 @@ Classify every `if/else` and `try/except` by **what it guards against**. A guard - **Decision heuristic:** "Can this value only be wrong if another part of OUR code is wrong? → delete the guard, add a unit test, let it raise. Can it be wrong from input or environment outside our control? → keep the guard and handle it gracefully." - **Keep** guards on real runtime/external conditions: user/API request bodies, network/HTTP responses, filesystem, DB availability, third-party payloads, parsing external/untrusted data, concurrency races. Validating a request field from a caller SHOULD be guarded and return a clear error. - **Flag** defensive branches on trusted internal values — an internal constant, an `Enum` member, a dict key that exists by construction, a field our own code sets, a registry entry. These add dead branches to the hot path and hide the bug. Prefer letting the operation raise naturally (`KeyError`/`AttributeError`/`TypeError`) per Python **EAFP** (Easier to Ask Forgiveness than Permission), and cover the invariant with a unit test — unit tests are the sanctioned place to assert internal invariants. A swallowed or re-wrapped developer error violates PEP 20 (Zen of Python): "Errors should never pass silently" and "Explicit is better than implicit" — it slips past the test suite. -- **Concrete example:** an internal table-name registry lookup on an internal constant should be `table = _TABLE[name]` (raises `KeyError`; add a test asserting every collection constant is registered) — NOT `if name not in _TABLE: raise ValueError("register it")`. +- **Real examples from this repo:** + - An internal table-name registry lookup on an internal constant should be `table = _TABLE[name]` (raises `KeyError`; add a test asserting every collection constant is registered) — NOT `if name not in _TABLE: raise ValueError("register it")`. + - `db_adapter.py` normalizes query rows with `if "id" not in r and "_id" in r: r["id"] = r["_id"]` (repeated at ~L136/L147/L187). The `IDatabase.query()` contract (`interface.py`) guarantees every row has `_id`, and both backends honor it (`firestore.py` L191, `memory.py` L350), so the `and "_id" in r` re-check defends against a backend breaking its own contract — a developer error. Prefer `r["id"] = r["_id"]` (raises `KeyError` if the contract is broken) and cover the "`query()` returns `_id`" invariant with a per-backend unit test. + - `memory.py` `_matches_filter` (L394–L419) is a `match operator:` with no `case _:` default, so an unsupported operator silently matches nothing — while `firestore.py` `_apply_filter` (L298) raises `ValueError` for the same input. Operators are always hardcoded literals in our own query code (e.g. `[("status", "==", "running")]`), so a bad operator is a developer error; the memory backend lets it pass silently (violating PEP 20) and diverges from Firestore. Add a `case _: raise ValueError(...)` so both backends fail the same way, and unit-test the unsupported-operator path. - Flag "just in case" guards on trusted internal inputs as **CHANGES REQUIRED** (or a nit if trivial). ## What NOT to flag