Instruction file imported from percona/SEP (.github/instructions/python.instructions.md). Copyright stays with the author.

Python — Types, Imports, Style

Pre-commit runs ruff (lint + format), bandit, and addlicense. Don't flag what these catch: import sorting, quoting, line endings, trailing whitespace, ERA001 commented-out code, generic Bandit findings, license headers. Flag manual-judgment items only. Exception — pyproject.toml's extend-exclude list (app/*/migrations/*, payload.py, payload, *_payload, **/payloads/*, data, etc, build, /docs, .github, the framework golden tree) is excluded from ruff lint and format, so neither pre-commit nor CI touches it: there PEP 8 spacing, quote style and import order are a manual obligation and a reviewer finding is the enforcement — match the sibling files. app/*/migrations/* is the live case, since revisions are hand-authored.

Type annotations

• Annotate function parameters, return types, and class-level attributes. Ruff never requires a local-variable annotation (ANN0/ANN2 on, ANN1 off), with one exception that does: an empty collection assigned to a local with no element type (specs = [], by_id = {}, seen = set()) is a finding, so the "prefer no annotation" default stops at = [] / = {} / = set() — there, annotate. Exempt: an empty tuple, and a name already annotated elsewhere in the same function. Loop variables and comprehension targets are never annotated. Don't flag a local annotation by itself; flag only ones that merely restate a type the assignment already makes obvious (e.g. count: int = len(items), name: str = "done"). Leave an annotation alone when it carries type information inference can't: an empty collection (items: list[str] = []), an optional initialized to None (x: Foo | None = None), or an ambiguous/Any return (payload: dict[str, Any] = response.json()).
• Modern syntax only: str | None not Optional[str]; list[str] not List[str]. Target is Python 3.11+.
• Pick the weakest abstract type that captures every operation the body performs. If the body only iterates or does x in xs membership, prefer Iterable[T] over Sequence[T] over list[T]. Use list[T] when the body mutates, indexes, calls len(), or iterates more than once. Use set[T] over UniqueList[T] when order is irrelevant.
• The loosening stops at multi-pass consumption, and that direction has no safety net: Iterable[T] where the body consumes twice type-checks, passes review, and fails only when some caller passes a generator — silently, with no exception. Count consumption sites, not visible for loops: a parameter spread as *xs into two different calls is consumed twice just as surely, and reads far less like repeated iteration. Two tells that the floor is Sequence[T]: the parameter is unpacked or iterated in more than one statement (especially across branches of a conditional), and the sibling parameters on the same signature are already Sequence. A lone Iterable among Sequence siblings is a smell, not a deliberate loosening.
• When two parameters fill the same semantic role (within or across functions), annotate with the same breadth. Asymmetry like list[str] next to Sequence[str] — when neither body mutates — should be flagged.
• An annotation wider than every value that reaches it is the defect, whatever it's spelled: Any where every caller passes Snippet, mocker: object, a bare dict whose keys and values are fixed. Name the real type. object is the same violation as Any and hides better, because it looks deliberate — and ruff can't help either way, since the ANN family only checks whether an annotation is present, so x: object satisfies it exactly as x: Snippet does. Prefer the concrete type the library documents (Dialect, TypeEngine[T], Table, AsyncIterator[T], a TypedDict for known dict shapes); reserve Any for genuinely dynamic input (JSON parsing, user-supplied dicts).
• A closed value space belongs in an enum, not in sibling constants. When a group of module constants enumerates every legal value of one concept and that concept is threaded through signatures, mappings, or validation, make it a StrEnum / IntEnum. The payoff is the annotation: a parameter typed str admits every string in the language, the same parameter typed as the enum admits exactly the values that exist. StrEnum members compare and serialize as their plain string value, so payloads, JSON, DB columns and == against raw strings are unaffected. Signals: the names share a prefix/suffix and are only ever used as alternatives; they're collected into a tuple/set/dict elsewhere (that collection is the value space, spelled twice); a function takes one as a bare str or branches on which it received. Follow AlertSeverity / CheckSeverity / TaskBackendEnum.
• A shape constraint expressible as a regex belongs in the field type, not a validator body. Declare Annotated[str, StringConstraints(pattern=...)] and let Pydantic reject at parse time; reach for a hand-written field_validator only when the check needs what a regex can't carry (cross-field state, a DB lookup, the value's own enum). The difference is totality, not style: a pattern is closed by construction, while a validator body rejects only what its author enumerated. Whitespace is the canonical miss because it's invisible on inspection — a validator splitting "Class.FIELD" on . and checking both halves non-empty accepts "Settings.LOGGING " and "Settings. LOGGING", both well-formed by its own rule and matching nothing downstream. What you give up is aggregation (Pydantic reports one error per entry rather than listing all), which rarely justifies the hand-rolled version.
• NEVER wrap custom field types in str(). StrHttpUrl, NonEmptyStr, LowercaseStr, URIPath, UTCDatetime (and the other subclasses in app/core/utils/fields.py) are already typed — the rule applies to any of them, not just these five.
• Bare generic containers erase the element contract — dict/list/set/tuple/frozenset with no type args is [Any]. Supply concrete args; only genuinely heterogeneous content may use dict[str, Any] / list[Any].
• type[X] bound must be the tightest common base of the expected callers; bare type (= type[object]) is always wrong. When the base can't be imported at runtime, annotate under TYPE_CHECKING.
• Callable return signatures: a factory returning a callable with a fixed signature spells the parameter list — Callable[[str, TaskAPI], Awaitable[Task]], not Callable[..., Awaitable[Task]]. Reserve ... for genuinely varying signatures.
• Over-nullable fields: an X | None field whose after-validator unconditionally fills it from a non-optional sibling is a defaulting artifact — move the default to a before-validator and type the field non-optional. Keep | None only when None is a legitimate post-construction value.
• Wire format ≠ field type: a response/wrapper model field projecting from a canonical sibling (BaseSQLModel, an upstream response) mirrors the source's field type (UTCDatetime, NonEmptyStr, an Enum subclass), not the JSON wire type (str, list[str]).

Imports & suppressions

• Top-level imports only. Inline imports inside function bodies are acceptable for exactly four concrete, verified reasons, each with a one-line marker comment adjacent to the import (not in the docstring, not only in the PR description):
  • A circular import. The named chain must close back to the module the comment sits in. # circular-import: config imports registry on an inline import inside policy.py leaves the reader unable to reconstruct why importing config here closes anything — write the whole path: # circular import: config imports registry imports policy (this module). If you can't state the chain end to end, you haven't verified the cycle.
  • A forbidden cross-app edge — sidecar/ only. Deferral is no longer a remedy under app/: tests/app/sep/test_import_boundary.py::test_no_module_defers_an_import_into_another_app_package rejects a cross-app import deferred into a function body anywhere in the app/ tree, marker or not. Deferring never removed the edge — in the image that strips the package it only relocates the ModuleNotFoundError from import time into a lifespan or a request. Remove the edge, don't move it: derive what you need from the activation list through the existing seam (collect_app_owned_settings_classes() for app-owned settings classes, the app registry for anything else) so the module names no app package at all. There is no allowlist, by design. The guard walks app/**/*.py only, so under sidecar/ a deferred cross-app import is still sanctioned, is not a circular import, and takes # import-boundary: <reason>; writing # circular-import: for it is a false citation. That marker may sit on a comment line above a contiguous import block, covering every import in it.
  • An import that resolves settings. Importing a settings module is its Settings() construction running, so the import raises on a profile that doesn't validate. A caller whose job is to report that failure rather than die of it defers the import into the try that handles it, marked # import-time-settings: <reason>. The test is that hoisting would move the failure earlier than the handler that exists for it — not that the import is slow or the module large. If nothing in the function catches the resolution error, the marker is a false citation. In-repo case: sidecar/grafana_service_account.py, run before supervisord, where PID 1 dying takes the container with it.
  • An optional third-party driver in a standalone payload. A payload that ships to a host and runs there imports the driver for the backend it was dispatched against; a module-scope import pymysql makes the whole payload fail to load on hosts carrying only the drivers they need. Defer into the branch that uses it, marked # optional-dependency: <backend>. Not defensiveness, and there is no cycle to name — app/tasks/connectivity/payload.py defers four such drivers, each marked.
• A deferred import must not be called at module scope, and it belongs inside the guarded region that handles its failure, not merely at the top of the function body. (Under app/ this no longer arises for cross-app edges — deferral is retired there — but it binds on every # import-time-settings: / # optional-dependency: deferral and on sidecar/.) A deferred import can raise ModuleNotFoundError — in the image where the package is stripped it will, that being the case the deferral exists for. At the top of the body it raises ahead of the function's own guards, bypassing the early return that declines the work, the branch that records a terminal failure, and the enclosing except; the deferral then converts an import-time failure into a worse runtime one instead of a handled one. Generally: position a statement that can raise relative to the destination function's guards, not relative to its first line.
• No new # noqa: or # type: ignore: unless the PR explains why fixing the root cause is impossible. The accepted-suppression registry is exactly four entries covering seven codes, and a noqa is exempt only when its code is listed and its site meets the condition: ARG001/ARG002/ARG003/ARG004 where the signature is imposed from outside and the body genuinely has no use for the parameter (a parent-class or protocol override, a FastAPI exception handler's request, a lifespan callback's app) — keep the imposed name, never rename to _param; BLE001 on a deliberate classified catch-all whose docstring promises totality and re-raises nothing (not a licence to widen a narrow except); TRY004 on a Pydantic validator raising ValueError so the framework surfaces 422 rather than 500 (app/core/utils/fields.py carries the file-wide form); S603 on a subprocess call whose argv is a list of repo-internal, already-validated values, never shell=True — pair it with # nosec B603 where bandit also scans the file. Deliberately off the registry, because the repo has a better answer: F401 on a side-effect-only model import (register it in the track's env.py instead) or a dep re-export in a route module (override the canonical callable), and SLF001 against a function whose docstring enumerates its callers (widen the docstring or factor a public wrapper). Treat an author-less # TODO with # noqa: TD002, TD003 as a standards bypass — name an owner. Required TODO format: # TODO(<owner>): <description>; don't add a # <ticket-key> line — Jira keys never go in comments (see below), so suppressing the missing-link rule (TD003) is the accepted path and the ticket reference lives in the PR.
• per-file-ignores: a [tool.ruff.lint.per-file-ignores] entry keyed on a bare filename ("contract_suite.py") is a basename glob matching every file of that name in the tree — anchor it to the full repo-relative path, or use an inline # noqa for specific lines. exclude / extend-exclude removes the file from all linting and formatting — never a bare basename there.

Style

• Boolean trap: process_data(data, True) is opaque. Require keyword validate=True.
• Sentinel anti-pattern: don't write result = None; try/except; if result is None — use try/except/else.
• Single-use locals: inline unless the name documents non-obvious intent, the branch body is ≥10 lines, or the expression has a side effect.
• Merge boolean-branch returns: two if cond: return X with the same X collapse to if a or b: return X, unless branches have side effects.
• Redundant construction: when the same constructor call returns the same value on every path, extract it once at the top.
• Collapse trivial guards: ≤2 cheap preconditions guarding a single-statement body → one combined conditional over a chain of if not x: return guards. Likewise a single-condition guard whose body is one bare continue/return/raise followed by the happy path — fold it into a conditional wrapping that block, so the skip-marker disappears and the happy path stops living at the tail. The collapse is the rule; the resulting polarity is an artifact of the input — whether the surviving condition reads if X: or if not X: depends on how the original guard was spelled and carries no weight of its own, so don't reject a collapse for landing on the negative form. x not in y and x is not y are single comparison operators, not negations, and never cost a collapse. Carve-outs: the body is multi-statement and deeply nested, the guard has side effects, there are ≥3 preconditions, or the skipped branch needs its own explicit return value.
• Numeric literals: drop trailing zeros on floats — 0.1 not 0.10.
• Empty modules: if a refactor empties a file, delete it.
• Comments: flag NEW inline comments that don't document a non-obvious invariant or workaround; a verbose-but-justified comment is a tighten signal (condense to the core why), not a delete. Never put a Jira key (SEP-\d+) or fixes #N in an inline comment (any syntax — Python #, Jinja {# #}, HTML, JS) — that rationale belongs in the PR/ticket.
• Every cross-site citation in a comment must resolve against the source — any symbol name, module path, import chain, or file reference naming code the reader can't see from this line. Resolve each before the comment lands: grep the name, open the module, walk the chain. Two failure shapes, both of which pass any review that reads the comment for plausibility rather than resolving it: a wrong name (# create_annotation returns early when API_KEY is unset where the function is create_pmm_annotation — the behavioural claim being correct is what makes it costly, since a reader who verifies the behaviour trusts the name too and then greps for a symbol that doesn't exist), and a truncated chain. Re-resolve when you touch either end.
• A comment asserting an out-of-band obligation must have that obligation already met. A carve-out documented as "a table added to this set must have a tracked follow-up" or "remove once upstream fixes X" asserts the obligation holds for every entry already present — satisfy it before the comment lands. An invariant unenforced from its first entry is the weakest starting position for one that depends entirely on discipline, and the next editor reads the unmet precedent as permission.
• No magic HTTP integers: status.HTTP_404_NOT_FOUND, not 404 — imported from fastapi, not starlette (from fastapi import status).
• Custom exceptions: raise HTTPNotFoundException / HTTPConflictException / HTTPForbiddenException, not fastapi.HTTPException.
• Pydantic / SQLModel class body order: model_config first, then __table_args__ on table=True models (table-level config governs the class as a whole and its Index / UniqueConstraint entries name columns, so it must precede the fields it constrains), then fields, then @field_validator / @model_validator, then methods (@computed_field properties before regular methods). model_config placed below the fields hides it from a reader scanning the top of the class. Ordering has no runtime effect — a violation is a consistency finding, not a bug.
• Raw-string prefix: use r / rb only when the body contains a literal backslash escape; an r prefix on a string with no backslash is misleading noise.
• Branches that restate their own condition: when the two branches of a conditional expression differ only in a value the condition already determines, the conditional collapses to that value. return (masked, False) if masked is not None else (None, True) → return masked, masked is None. This covers tuples, dict literals, and any expression built branch-by-branch. Two common instances: literal-True ternary True if cond else X → cond or X, False if cond else X → not cond and X (only when cond is already boolean-typed — comparison, isinstance, membership).
• None-first ternary: value if x is not None else None → None if x is None else value (don't introduce a not just to satisfy this). Unlike the collapse rules above this is a genuine ordering rule — the reader should see the trivial base case first — so here the polarity is the point.
• Text-mode open() pins encoding="utf-8". Any open() / Path.open() in text mode passes it explicitly; without it Python uses the host's locale.getpreferredencoding(), so the same code round-trips differently on a differently-configured host. Binary mode ("rb"/"wb") is exempt. json.dump does not exempt the writer — its ensure_ascii=True default makes the body pure ASCII, which reads like the encoding can't matter, but the argument is about the reader: where the read side decodes as UTF-8 and maps UnicodeDecodeError to a benign value, a locale mismatch degrades into silent data loss rather than a visible error. Pin it on both sides.
• A bare string is not a comment. A string expression used to explain why the surrounding code does what it does is a comment — write #. The one legitimate bare string in that position is the PEP 258 attribute docstring: a string documenting the name just assigned, which Sphinx autodoc reads and which this codebase uses deliberately (app/core/utils/fields.py documents every exported type alias that way). Keep those; don't convert them to comments, and don't add a bare string that follows any other statement.
• __all__ placement: in the module header region, before the first def / class — never at the end of the file. Whether it sits directly below the docstring or below the import block is not standardized; match the file you're editing and never churn a module just to switch sides.
• Stub ellipsis: a Protocol method / @overload / abstract stub that already has a docstring must not also carry a trailing ... — the docstring is the body, and the method is implicitly abstract either way. A bare ... stays correct when there is no docstring. Whether ruff's PIE790 reaches stub contexts is not established, so treat this as a reviewer convention rather than assuming a linter catches it.
• Buffer scanning: flag a Python for-loop scanning a byte/string buffer when a C-level builtin (.count(), .find(), .split(), .translate()) expresses the same operation — relevant on potentially large buffers (log chunks, file reads, upload bodies).