Code Review Quality Lens
Review quality issues from 6 perspectives that automated tools miss.
Check 1: Dead Code Detection
# Unused imports
uv run ruff check core/ --select F401
# Unused variables
uv run ruff check core/ --select F841
# Residual TODO/FIXME/HACK
grep -rn "TODO\|FIXME\|HACK\|XXX" core/ --include="*.py"
# Empty function bodies (stubs with only pass)
grep -rn "def.*:" core/ --include="*.py" -A1 | grep -B1 "^\s*pass$"
Check 2: Exception Handling Anti-patterns
# Empty except blocks (swallowing exceptions)
grep -rn "except.*:" core/ --include="*.py" -A1 | grep -B1 "^\s*pass$"
# Catch-all Exception (overly broad except)
grep -rn "except Exception" core/ --include="*.py" | grep -v "# noqa"
# Bare except (except without type)
grep -rn "except:" core/ --include="*.py"
Rules:
- Empty except is a discovery candidate: inspect expected failure, suppression
and the upper caller. Follow the existing
error conventions;
do not add duplicate logging, lose cancellation or manufacture empty success.
- except Exception: Must state legitimate justification
- Bare except: Prohibited — specify at minimum the Exception type
Check 3: Resource Leak Detection
# File handles (without with statement)
grep -rn "open(" core/ --include="*.py" | grep -v "with " | grep -v "# noqa"
# subprocess handles not collected
grep -rn "subprocess\.Popen" core/ --include="*.py" | grep -v "with "
# Temporary files not cleaned up
grep -rn "tempfile\.\|NamedTemporaryFile\|mktemp" core/ --include="*.py"
Trace the creator, ownership transfer, borrower and lifetime owner. Use a
context manager, finally or the established async shutdown owner as appropriate;
verify constructor failure, cancellation and partial teardown. Preserve failed
resources when the owning lifetime permits retry, and still attempt sibling cleanup. Follow
construction/lifecycle
and error conventions,
rather than requiring a with statement for every shared client.
Check 4: Thread Safety
# Global mutable state
grep -rn "^[A-Z_]*\s*=\s*\[\|^[A-Z_]*\s*=\s*{" core/ --include="*.py" | grep -v "frozenset\|tuple\|Final"
# Shared state modification without Lock
grep -rn "threading\.\|asyncio\.\|concurrent\." core/ --include="*.py"
# ContextVar usage patterns
grep -rn "ContextVar\|contextvars" core/ --include="*.py"
GEODE context:
core.wiring composes services through constructors/configuration or typed
contexts such as ToolContext; a service-locator ContextVar is not the DI contract.- Request-local
ContextVar use does not prove mutable values are isolated or
that context propagates across every thread/task boundary. Trace the actual
producer, handoff, mutation, and reset paths.
- For a module-level dict/list, establish whether concurrent mutation exists
and which owner serializes it before recommending a lock or immutable type.
See architecture boundaries.
Check 5: SOLID Principles
| Principle | Discovery candidates, not violations | Detection |
|---|
| SRP | 500+ line files, classes with 5+ responsibilities | wc -l core/**/*.py | sort -rn | head |
| OCP | if/elif chains with 10+ branches | grep -rn "elif" count |
| LSP | NotImplementedError in subclasses | grep -rn "NotImplementedError" |
| ISP | Protocol with 10+ methods | Trace actual Protocol consumers |
| DIP | Import bypassing a declared package/capability boundary | Check the current import-linter contracts |
These source/size heuristics need caller and failure evidence. Concrete-class
imports are valid when no declared boundary or substitution contract is bypassed.
Use the field guide before
deleting code/tests or introducing an abstraction.
Check 6: Performance
# N+1 pattern (I/O inside loops)
grep -rn "for.*in.*:" core/ --include="*.py" -A5 | grep "\.get\|\.fetch\|\.call\|\.execute"
# Unnecessary list creation (generator possible)
grep -rn "\[.*for.*in.*\]" core/ --include="*.py" | grep -v "test"
# Redundant computation (repeated calls to same function)
# (requires manual review)