docs(review-backend): flag guards that defend against developer errors#27
Merged
Merged
Conversation
…s instead of runtime input 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).
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.
awrobel-gd
approved these changes
Jul 7, 2026
awrobel-gd
left a comment
awrobel-gd
left a comment
Contributor
There was a problem hiding this comment.
Approving but still think this change is verbose. Maybe it can be compressed
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
2 participants
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.
What
Adds one new review item (#14) to
.claude/commands/review-backend.md, under the 🟡 HIGH — CLAUDE.md coding pattern adherence section, right after item #13.Why
Reviewers and coding assistants need a crisp rule for classifying every
if/elseandtry/exceptby what it guards against:Enummember, a dict key that exists by construction, a field our own code sets, a registry entry) — the value can only be wrong if some other part of OUR code is wrong. A defensive branch here adds dead branches to the hot path and hides the bug. Delete it, let the operation raise naturally (KeyError/AttributeError/TypeError), and cover the invariant with a unit test.The item ships a one-line decision heuristic, a concrete example grounded in the SQLite table-name registry (
table = _TABLE[name], notif name not in _TABLE: raise ...), and cites Python EAFP and PEP 20 ("Errors should never pass silently", "Explicit is better than implicit").It is explicitly framed as the sibling of existing item #12 (asserts forbidden outside tests) so the two rules read as one philosophy: internal invariants belong in unit tests, not hot-path branches.
Scope
Documentation only — one file, no application code touched.