skynet/best-practices
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

distill: best practices from 2026-04-19 cross-project run

Browse Source 
Adds 3 new topic files (ai-parallel-agents, api-integration,
python-patterns) and extends 21 existing topic files with new gotchas
and patterns surfaced from memory across tracked projects. Index
updated accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Paul O'Reilly
2026-04-25 13:41:47 +12:00
parent 8aa400a5d4
commit 22d49b2c9a
24 changed files with 1394 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

287
python-patterns.md Normal file
View File

@@ -0,0 +1,287 @@
# Python Patterns & Gotchas
Patterns, anti-patterns, and gotchas encountered in real Python projects. Covers concurrency, Pydantic, testing, and cross-field validation.
## Non-Reentrant `threading.Lock` Causes Deadlocks
Python's `threading.Lock` is **non-reentrant**: if the same thread tries to acquire a lock it already holds, it blocks forever. This is a common source of deadlocks in services where a method holding a lock calls another method that also acquires the same lock.
**Symptom:** Service hangs indefinitely with no error, CPU at 0%, no log output after the hang point.
**Fix:** Use `threading.RLock` (reentrant lock) for locks that may be acquired by the same thread multiple times — e.g., a `_persist()` method called both directly and from within a method that already holds the lock.
```python
# Bad — deadlocks if update() calls persist() while holding _lock
self._lock = threading.Lock()
def update(self, key, value):
with self._lock:
self._data[key] = value
self._persist() # acquires _lock again → deadlock
# Good — RLock allows re-acquisition by the same thread
self._lock = threading.RLock()
```
Use `Lock` only when you're certain a lock will never be re-acquired by the same thread. When in doubt, use `RLock`.
## Pydantic v2 `extra='ignore'` Silently Drops Unknown Fields
Pydantic v2 models with `model_config = ConfigDict(extra='ignore')` (or the class-level `class Config: extra = 'ignore'`) silently discard any fields not declared in the model. This is often the desired behaviour for API consumers that receive payloads with forward-compatible fields — but it becomes a bug when a required field is misspelled or renamed.
**Symptom:** An expected field is `None` or missing despite being present in the input dict. No validation error is raised.
**Debug pattern:** Temporarily switch to `extra='forbid'` to surface unexpected field names, which often reveals the misspelling or rename.
```python
class TaskPayload(BaseModel):
model_config = ConfigDict(extra='ignore')
task_id: str
prompt: str
# Silently drops 'task_id' if the input has 'taskId' (camelCase)
payload = TaskPayload(**{"taskId": "abc", "prompt": "..."})
print(payload.task_id) # None — no error raised
```
**When to use `extra='ignore'`:** For external API payloads where forward-compatibility matters and the caller may add fields you don't care about. Always document that the model uses `extra='ignore'` so maintainers know unknown fields are dropped.
**Protocol / duplicate-store drift:** The same silent-drop failure mode appears at a different layer when multiple implementations share a `Protocol` or interface (e.g., `InMemoryStore` and `PostgresStore` both implementing a task store). Adding a field to the model isn't enough — each store's ORM-style `_to_row` / `_row_to_task` mappings (and any serialization helpers) must also learn the new field, or the field round-trips as its default/`None` in whichever store was missed. Symptom: value is present in-memory during tests but arrives as default/`None` in production where the other store is used.
**Rule:** After adding a field to a shared model, grep for every implementation of the Protocol and every `_to_row` / `_row_to_task` (or equivalent mapping) pair, and add a round-trip test per store that fails if the field is dropped.
## Pydantic v2: `BaseSettings` Moved to `pydantic-settings`
In Pydantic v2, `BaseSettings` was removed from the `pydantic` package and now lives in a separate `pydantic-settings` package. Code that does `from pydantic import BaseSettings` fails with `ImportError` on fresh installs.
**Fix:** Add `pydantic-settings>=2` as an explicit dependency in `pyproject.toml` whenever using `BaseSettings`. Do not rely on transitive installation via `pydantic` — it is not transitive.
```python
# Pydantic v1
from pydantic import BaseSettings
# Pydantic v2
from pydantic_settings import BaseSettings
```
## Use Routing Callables to Mock subprocess Without Patching Path Strings
Mocking `subprocess.run` (or `subprocess.Popen`) by patching the module path is brittle — the patch target must match exactly how the code imports it, and it breaks when code is refactored. A routing callable pattern is more robust:
```python
# test helper — routes subprocess calls to per-command handlers
def make_subprocess_router(routes: dict):
"""
routes: {command_prefix: mock_result_or_callable}
e.g. {"git clone": CompletedProcess(...), "ssh": lambda cmd, **kw: ...}
"""
def router(cmd, **kwargs):
for prefix, handler in routes.items():
if isinstance(cmd, list) and " ".join(cmd[:len(prefix.split())]) == prefix:
return handler(cmd, **kwargs) if callable(handler) else handler
raise ValueError(f"Unrouted subprocess call: {cmd}")
return router
# Usage in tests
mock_runner = make_subprocess_router({
"git clone": CompletedProcess([], 0, stdout="", stderr=""),
"git push": CompletedProcess([], 0, stdout="", stderr=""),
})
with patch.object(module_under_test, "subprocess_runner", mock_runner):
result = module_under_test.run_task(payload)
```
This avoids hard-coded patch target strings, handles multiple commands cleanly, and makes test setup readable.
## `model_validator` for Cross-Field Validation in Pydantic v2
Use `@model_validator(mode='after')` for validation that depends on multiple fields. Field-level validators (`@field_validator`) only see the single field being validated. Cross-field logic in field validators requires workaround hacks.
```python
from pydantic import BaseModel, model_validator
from typing import Optional
class TaskRuntime(BaseModel):
cli: str = "claude"
model: Optional[str] = None
timeout: int = 1800
@model_validator(mode='after')
def validate_model_for_cli(self) -> 'TaskRuntime':
if self.cli == "openai_compat" and self.model is None:
raise ValueError("model is required when cli='openai_compat'")
return self
```
**`mode='after'` vs `mode='before'`:**
- `mode='after'` — runs after all field validators. `self` is the fully-constructed model instance. Use for cross-field checks.
- `mode='before'` — runs on the raw input dict before field parsing. Use for input normalization (e.g., converting camelCase keys to snake_case).
## TLS 1.3 Post-Handshake Client Auth Requires Explicit `SSLContext`
Python mTLS clients using the convenience form `httpx.AsyncClient(cert=(crt, key))` fail against Go servers (Traefik, gRPC-Go, anything using the Go stdlib `crypto/tls`) because Go sends `CertificateRequest` as a TLS 1.3 **post-handshake** message. Python's `ssl` module ignores post-handshake client auth unless `SSLContext.post_handshake_auth=True` is set explicitly — and the convenience `cert=` argument does not set it.
**Symptom:** Server returns `401` "no client certificate" (or equivalent) even though the cert and key are configured correctly. Capping the negotiation to TLS 1.2 (`ssl.TLSVersion.TLSv1_2`) makes auth work — that's the diagnostic fingerprint.
**Fix:** Build an `SSLContext` explicitly and pass it as `verify=ctx`.
```python
import ssl
import httpx
ctx = ssl.create_default_context()
ctx.post_handshake_auth = True
ctx.load_verify_locations(cafile=ca_path)
ctx.load_cert_chain(certfile=crt_path, keyfile=key_path)
client = httpx.AsyncClient(verify=ctx)
```
**Diagnostic:** If forcing `minimum_version = maximum_version = ssl.TLSVersion.TLSv1_2` makes mTLS authenticate successfully while TLS 1.3 fails, you've hit the post-handshake gap. Don't ship the TLS 1.2 workaround — set `post_handshake_auth=True` instead.
## Recreate `venv` After Renaming or Moving a Project Directory
Python venvs embed absolute paths in pip shims (the shebang line of `.venv/bin/pip`, `.venv/bin/python` symlinks) and in `.pth` files for editable installs. After renaming or moving a project directory the venv looks intact — `python` runs, imports mostly work — but pip and editable installs break in subtle ways (`ModuleNotFoundError` for the local package, pip resolving against the wrong site-packages, etc.).
**Fix:** Recreate the venv. Don't try to patch paths in place.
```bash
rm -rf .venv
python3 -m venv .venv
source .venv/bin/activate
pip install -e ".[dev]"
```
## Jinja2: Prefer `Environment` Whitespace Controls Over `{%- %}` in Code Templates
Using the hyphen-trim form `{%- ... -%}` inside Python (or other code-generating) templates collapses newlines between statements, producing syntactically invalid output — class bodies run together, method defs glue to the previous `return`, etc.
**Fix:** Configure whitespace once on the `Environment` and use plain `{% ... %}` tags inside code templates.
```python
from jinja2 import Environment, FileSystemLoader
env = Environment(
loader=FileSystemLoader("templates"),
trim_blocks=True,
lstrip_blocks=True,
extensions=["jinja2.ext.do"], # if templates use {% do list.append(...) %}
)
```
- `trim_blocks=True` — strip the newline *after* a block tag.
- `lstrip_blocks=True` — strip leading whitespace *before* a block tag on the same line.
- `jinja2.ext.do` — enables `{% do %}` for list/dict mutation. Without it, templates using `{% do %}` fail at render time with a `TemplateSyntaxError`.
Inside code-generating templates, never use `{%-` or `-%}` — rely on the Environment settings.
## Packaging with `pyproject.toml` on Modern Linux
Three common setup failures, each with a one-line fix:
**(a) PEP 668 blocks system pip (Ubuntu 23.04+, Debian 12+, etc.).** Running `pip install` against the system Python raises `error: externally-managed-environment`. Always create a venv as step 1:
```bash
python3 -m venv .venv
source .venv/bin/activate
pip install -e ".[dev]"
```
**(b) setuptools flat-layout auto-discovery fails with multiple top-level dirs.** If your repo has more than one top-level directory (e.g., `mypkg/`, `tests/`, `scripts/`), setuptools' auto-discovery raises `Multiple top-level packages discovered` and refuses to build. Declare the package explicitly:
```toml
[tool.setuptools.packages.find]
include = ["mypkg*"]
```
**(c) `build-backend` typo.** The correct value is `"setuptools.build_meta"` (underscore, not plural). `"setuptools.backends"` / `"setuptools.build_metas"` yield `ModuleNotFoundError: No module named 'setuptools.backends'` at build time.
```toml
[build-system]
requires = ["setuptools>=68"]
build-backend = "setuptools.build_meta"
```
## Bridging `threading.Event` to `asyncio` — Use a Polling Bridge
When an HTTP server or other handler running in a **background thread** needs to signal an `asyncio` controller loop (e.g., webhook handler waking a reconciler), a plain `threading.Event.set()` cannot directly wake `asyncio.wait_for` or `asyncio.sleep` in the event-loop thread. The event loop only wakes for tasks it scheduled.
**Simplest bridge — async loop polls the event during its sleep interval:**
```python
async def reconcile_loop(wake_event: threading.Event, interval: float = 30.0):
while True:
await do_reconcile()
# Poll event every 1s instead of sleeping for the full interval
for _ in range(int(interval)):
if wake_event.is_set():
wake_event.clear()
break
await asyncio.sleep(1)
```
This accepts ~1s of bounded latency in exchange for a trivially correct bridge. Reach for `asyncio.run_coroutine_threadsafe(queue.put(...), loop)` only when your latency budget demands it — it's correct but adds complexity (you must capture the loop reference, handle loop shutdown, and deal with the queue from both sides).
## `dict.get(key, default)` Returns `None` When the Value Is Explicitly `null`
`raw.get("ttl", 300)` returns `None` — **not** `300` — when the key is present but its value is `null` / `None`. The default only applies when the key is *missing*. External APIs commonly return explicit `null` for optional fields (TTL, priority, timestamps, nullable FKs), which then crashes Pydantic validators that expect a non-null type, or propagates `None` through code that assumed the default kicked in.
**Fix — two options:**
```python
# Option 1: falsy-coalesce at the call site (simple, but conflates 0/"" with null)
ttl = raw.get("ttl") or 300
# Option 2: Pydantic before-validator mapping None → default (preferred for models)
from pydantic import BaseModel, field_validator
class Record(BaseModel):
ttl: int = 300
@field_validator("ttl", mode="before")
@classmethod
def _default_null(cls, v):
return 300 if v is None else v
```
Use the `or` form for throwaway scripts where `0` and `""` are not valid values. Use the Pydantic validator for models where you want to preserve legitimate falsy values while still mapping `null` to the default.
## Python Stdlib Sharp Edges: Helper Names, `re.sub`, Extensionless Imports
Three unrelated stdlib traps that all fail silently — no exception, wrong output:
**(1) Never use single-letter function names like `v`, `d`, `f`.** They silently shadow when callers rebind the same name (`v = dict(...)` in a caller's scope, list comprehensions using `v` as the loop variable, etc.). The failure mode is invisible: empty output, zero errors, no traceback. Use `fv()`, `format_value()`, `fmt_dict()` even for one-line helpers.
**(2) `re.sub()` interprets `$` and `\` in replacement strings as backreferences.** `\g<name>`, `\1`, `\\`, and even bare `\` in the replacement are all special — so any replacement containing literal dollar signs, backslashes, or backref-looking sequences is corrupted, and an equality check against the "expected" string fails without explaining why.
```python
# Bad — '$' and '\' in replacement are reinterpreted
re.sub(pattern, replacement_text, source)
# Good — escape replacement string
re.sub(pattern, re.escape(replacement_text), source) # escapes backrefs but not $
# Better for arbitrary text — use search + slicing
m = re.search(pattern, source)
if m:
source = source[:m.start()] + replacement_text + source[m.end():]
```
When the replacement is arbitrary user-supplied or data-derived text, prefer `re.search()` + string slicing over `re.sub()`.
**(3) `importlib.util.spec_from_file_location` returns `None` for extensionless files.** Loading a script like `bin/my-tool` (no `.py`) via `spec_from_file_location` silently yields `None`, and subsequent `module_from_spec(None)` raises a confusing `AttributeError`.
```python
import importlib.util
import sys
from importlib.machinery import SourceFileLoader
loader = SourceFileLoader("my_tool", "/path/to/bin/my-tool")
spec = importlib.util.spec_from_loader("my_tool", loader)
module = importlib.util.module_from_spec(spec)
sys.modules["my_tool"] = module # register BEFORE exec_module
spec.loader.exec_module(module)
```
The `sys.modules` registration must happen before `exec_module` — otherwise any `import my_tool` inside the loaded module creates a second, distinct module object.