Python code review standards

# Python code review standards

Follow these standards when writing or changing code in this repository. Code review flags changes that break them.

## Prevent Swallowed Exceptions in Python

Applies to `**/*.py`.

Review Python exception handling for failures that are hidden from the caller or lose their original cause.

Flag changes that:
- Catch `Exception` or `BaseException`, or use `contextlib.suppress(Exception)`, and then continue without re-raising in code whose caller needs to know the operation failed.
- Catch broadly and return a default such as `None`, `False`, `0`, `[]`, or `{}` that the caller cannot tell apart from a real result.
- Wrap a large `try` block in `except Exception`, so programming errors such as `KeyError`, `AttributeError`, or `TypeError` get the same handling as the expected failure.
- Raise a new exception inside an `except` block without `from err` (or `from None` when hiding the cause is deliberate), so the traceback reports the original as a separate failure during handling.
- Log a caught exception with only `str(e)` and then continue, which drops the traceback; use `logger.exception()` or `exc_info=True`.
- Catch `BaseException` or `asyncio.CancelledError` without re-raising, which breaks task cancellation and clean shutdown.
- Use `return`, `break`, or `continue` inside a `finally` block, which discards any exception in flight.

Do not flag:
- Handlers for a specific expected exception, such as `FileNotFoundError`, `KeyError`, or a `ValueError` from parsing, that handle it deliberately.
- Top-level boundaries such as a worker loop, request middleware, or CLI entry point that catch `Exception`, log it with the traceback, and report the failure.
- Best-effort side effects, such as metrics or cache writes, that log the failure and whose outcome the caller does not use.

## Prevent Resource Leaks with Context Managers in Python

Applies to `**/*.py`.

Review Python code that opens files, sockets, connections, locks, subprocesses, or temporary files for resources that are not released on every path.

Flag changes that:
- Call `open()`, `socket.socket()`, `zipfile.ZipFile()`, `tarfile.open()`, or a database `connect()` without a `with` block or a `try`/`finally` that closes it.
- Release a resource with a plain `.close()` call placed after code that can raise, so an exception skips it.
- Treat `with sqlite3.connect(...) as conn` or a psycopg2 `with conn:` block as if it closed the connection; both only commit or roll back, so wrap the connection in `contextlib.closing()` or close it explicitly.
- Call `lock.acquire()` or `semaphore.acquire()` without a guaranteed release; use `with lock:`.
- Create files or directories with `tempfile.mkstemp()`, `tempfile.mkdtemp()`, or `NamedTemporaryFile(delete=False)` without removing them in a `finally`, or leave the `mkstemp()` file descriptor open.
- Return a lazy object tied to a resource opened in a `with` block, such as `csv.reader(f)`, a generator over `f`, or a cursor, which fails once the block has closed the resource.
- Start `subprocess.Popen` without `wait()`, `communicate()`, or a `with` block, or create a `requests.Session` or `httpx.Client` per call without closing it.

Do not flag:
- Resources whose ownership passes to an object or caller that closes them, such as a class with `close()` or `__exit__`, a pytest fixture that yields and closes, or a framework-managed connection.
- Process-lifetime clients or pools created once at startup.
- `Path.read_text()`, `Path.write_bytes()`, and similar helpers that open and close the file themselves.

## Require Timeouts on Python Network Calls

Applies to `**/*.py`.

Review Python code on production paths that calls HTTP APIs, cloud SDKs, databases, caches, or other network services for calls that can wait without a bound.

Flag changes that:
- Call `requests.get`, `requests.post`, `requests.request`, or a `requests.Session` method without a `timeout` argument; requests has no default timeout.
- Pass `timeout=None` to a request, or build an `httpx.Client` or `httpx.AsyncClient` with `timeout=None`, which disables the limit.
- Call `urllib.request.urlopen`, `smtplib.SMTP`, `ftplib.FTP`, or `socket.create_connection` without a `timeout`, which uses the global socket default of no timeout unless the application calls `socket.setdefaulttimeout()`.
- Create a `boto3` client or resource for request handlers or workers without a `botocore.config.Config` that sets `connect_timeout`, `read_timeout`, and `retries`; the timeouts default to 60 seconds per attempt.
- Open PostgreSQL, MySQL, or Redis connections without connect and query timeouts, such as a libpq `connect_timeout` and server-side `statement_timeout`, or redis-py `socket_timeout` and `socket_connect_timeout`.
- Retry network calls in a loop with no cap on attempts or total time.
- Await network work that has no timeout of its own without `asyncio.timeout()` or `asyncio.wait_for()`.

Do not flag:
- Calls through a shared session, client factory, or wrapper that already sets timeouts.
- Scripts, notebooks, one-off management commands, and tests with mocked transports.
- Streaming downloads, websockets, or long-poll calls that set an explicit, deliberately long read timeout.

## Prevent Blocking Calls in Async Python Code

Applies to `**/*.py`.

Review `async def` functions, including asyncio tasks and async web handlers such as FastAPI or Starlette endpoints, for calls that block the event loop.

Flag changes that:
- Call `time.sleep()` inside a coroutine instead of `await asyncio.sleep()`.
- Make HTTP calls with `requests`, `urllib.request`, or a sync `httpx.Client` inside a coroutine instead of `httpx.AsyncClient`, `aiohttp`, or another async client.
- Use a sync database or cache driver inside a coroutine, such as `psycopg2`, `sqlite3`, `pymysql`, a sync SQLAlchemy `Session`, or `redis.Redis`, instead of an async driver or `asyncio.to_thread()`.
- Run CPU-heavy work inline, such as password hashing, image processing, large JSON or CSV parsing, or compression; offload it with `asyncio.to_thread()` when the library releases the GIL, or with a `ProcessPoolExecutor` through `loop.run_in_executor()`.
- Call `subprocess.run()`, `os.system()`, or blocking reads and writes of large files inside a coroutine; use `asyncio.create_subprocess_exec()` or offload the work.
- Block on thread primitives inside a coroutine, such as `threading.Lock.acquire()`, `queue.Queue.get()`, or `concurrent.futures.Future.result()`.
- Change a FastAPI endpoint or dependency from `def` to `async def` while it still calls blocking libraries.

Do not flag:
- Plain `def` FastAPI endpoints and dependencies, or sync Starlette endpoints, which run in a threadpool.
- Blocking calls wrapped in `asyncio.to_thread()`, `loop.run_in_executor()`, `anyio.to_thread.run_sync()`, or `run_in_threadpool()`.
- Small, fast operations such as reading a small config file at startup, logging, or parsing a small JSON payload.

## Block Unsafe Deserialization and Shell Injection in Python

Applies to `**/*.py`.

Review Python code that parses serialized data or runs commands for paths where untrusted input can execute code.

Flag changes that:
- Call `pickle.load`, `pickle.loads`, `marshal.loads`, `shelve.open`, `dill.loads`, `joblib.load`, or `pandas.read_pickle` on data from requests, uploads, message queues, shared caches, or storage that other parties can write to.
- Call `torch.load` with `weights_only=False`, the default before PyTorch 2.6, or `numpy.load(..., allow_pickle=True)` on files the service did not produce.
- Parse YAML from outside the codebase with `yaml.unsafe_load`, or with `yaml.load` and `Loader=yaml.Loader` or `yaml.UnsafeLoader`; use `yaml.safe_load`.
- Run `subprocess` with `shell=True`, `os.system`, or `os.popen` on a command string built from input with f-strings, `%`, `.format()`, or concatenation.
- Pass user input as a positional argument to tools such as `git`, `curl`, `tar`, or `ssh` without validating it or adding a `--` separator, which allows option injection without a shell.
- Call `eval`, `exec`, or `compile` on input, including variants that strip `__builtins__`; use `json.loads` or `ast.literal_eval` for data.

Do not flag:
- Pickle or joblib files the same service writes and reads from storage no other party can write to, such as a local cache directory.
- `subprocess` calls that pass a list of arguments built from constants or validated values, or `shell=True` with a constant string.
- Test fixtures, build scripts, and developer tooling that only read files from the repository.

## Flag Security Vulnerabilities

Scan for security flaws: hardcoded secrets, SQL injection, XSS, weak auth, exposed data, poor crypto, unsafe deserialization, insecure HTTP calls in production, and more. Ensure all network requests use HTTPS/TLS. Identify attack vectors and security violations to maintain robust defenses.

## Prevent Sensitive Data in Logs

Logging, error reporting, analytics events, and audit trails must not expose sensitive data. Flag code that logs PII, emails, passwords, API keys, auth tokens, session identifiers, payment data, or raw request/response bodies without redaction. Error serialization must preserve enough context to debug while sanitizing secrets and user data.

## Require API Auth, Validation, and Tenant Scoping

Review API handlers and controller endpoints for missing authentication, authorization, tenant scoping, and request validation.

Flag new or modified endpoints that:
- Accept request bodies without runtime validation, such as Zod schemas, DTOs, or framework-native validators.
- Trust user-supplied identifiers for tenant, organization, or account access instead of deriving access from the authenticated session.
- Return sensitive internal errors or use inconsistent HTTP status codes for 400, 401, 403, 404, and 500 cases.
- Mix reads and writes in a way that makes side effects hard to reason about.
- Add public or high-volume endpoints without rate limiting or abuse controls.

## Prevent Database Query Performance Regressions

Flag database access patterns that can degrade production performance, including N+1 queries, unbounded list endpoints, missing pagination, full-table scans, missing indexes on selective filters, per-row writes in loops, and queries that load more columns or rows than needed. Prefer bounded reads, batching, selective indexes, and query shapes that scale with table size.

## Enforce Pragmatic Test Coverage

Be reasonable when evaluating tests. Flag changes when important behavior is untested, but do not require tests for every export, type, config, metadata file, or mechanical change.

Flag PRs that:
- Change core business logic without covering the main success and failure paths.
- Add UI behavior without testing the user-visible interaction or state change.
- Add tests that only assert rendering, existence, snapshots, or that a function returns any value.
- Skip, mute, or weaken existing tests without a clear reason.
- Mock so much of the system that the test no longer validates the behavior the PR claims to change.

## Enforce Strict Maintainability Standards

Review changes for structural maintainability regressions. Behavior-correct code can still violate this rule if it makes the codebase harder to understand, extend, or decompose.

Be ambitious about structural simplification. Look for reframings that delete whole branches, helpers, modes, conditionals, or layers instead of adding more of them. Prefer solutions that remove complexity rather than rearrange it.

Flag PRs that:
- Push a file from under 1,000 lines to over 1,000 without a strong reason or clear organization. Prefer extracting helpers, subcomponents, modules, or local abstractions first.
- Add ad-hoc conditionals, scattered special cases, one-off booleans, nullable modes, or flags into already busy shared flows instead of a dedicated abstraction, helper, state machine, policy object, or separate module.
- Introduce thin wrappers, identity abstractions, or pass-through helpers that add indirection without reducing complexity.
- Use brittle, ad-hoc, or "magic" mechanisms that hide simple data-shape assumptions.
- Rely on unnecessary casts, any, unknown, or optional parameters when a clearer type boundary or explicit model would simplify control flow.
- Use silent fallbacks to paper over unclear invariants instead of making the boundary explicit.
- Duplicate existing helpers or put logic in the wrong layer when a canonical utility or module already owns the concept.
- Leak feature-specific logic into general-purpose or shared paths.
- Copy-paste logic instead of extracting a shared helper or pure function.
- Handle narrow edge cases in the middle of an already busy function.
- Refactor in ways that move complexity around without reducing the number of concepts a reader must hold in their head.
- Add "temporary" branching that is likely to become permanent debt.
- Serialize obviously independent work or leave related updates in a non-atomic shape when a cleaner structure is available.

Prioritize findings in this order:
1. Structural code-quality regressions
2. Missed opportunities for dramatic simplification
3. Spaghetti or branching complexity increases
4. Boundary, abstraction, or type-contract problems
5. File-size and decomposition concerns
6. Modularity and legibility concerns

Do not flag cosmetic nits when a larger structural issue is present. Prefer a smaller number of high-conviction findings over a long list of minor notes. Only report violations on changed lines with clear evidence from the diff.

## Flag AI Slop and Fabricated Changes

Catch AI-generated changes that look finished but aren't, such as placeholder or fabricated behavior, claims the code doesn't back up, and references that don't resolve.
Be conservative. Flag only obvious, evidence-backed low-quality AI output in the diff.

Flag changes that:
- Ship placeholder, mock, demo, fake-data, temporary debug, or filler text to production paths without a clear reason.
- Make claims in docs, comments, config, or PR text that contradict the implementation. Distinguish claims about current behavior or verification from documented plans or explicit statements that a capability is not supported.
- Reference files, commands, routes, functions, env vars, or config keys that do not resolve in their intended environment. Check the documented setup, including aliases, generated definitions, and externally supplied inputs.
- Add duplicated, patchwork, or overengineered code where a smaller cohesive solution would clearly work.
- Add narrating comments that restate obvious code instead of explaining intent or constraints.
- Change consequential behavior without meaningful verification, add tests that bypass the behavior they claim to cover, or weaken assertions to permit a regression. For coverage concerns, identify a realistic failure the verification would miss; consider existing shared tests and relevant type or static checks.

Do not flag:
- Generated files, fixtures, snapshots, migrations, vendored code, or mechanical renames unless one of the rules above clearly applies.
- Explicitly labeled scaffolding with concrete follow-up guidance, provided unfinished behavior is isolated from active production use. A TODO does not excuse fabricated results in a live path.
- Well-scoped AI-assisted code that is cohesive and tested.