/review-python

Review Python

Python review that covers what ruff and a type checker cannot decide for you — how state is modeled, whether parsed data stays parsed, whether an exception hierarchy tells callers anything, whether async work has an owner, and whether a good primitive is being bypassed.

Run on .py / .pyi files. Complements review-cleancode (SOLID/DRY/smells) — don't repeat it.

Relationship to the toolchain (read first)

Ruff's defaults changed radically in 0.16.0 and most "what ruff covers" intuition predates it:

"Ruff now enables a much larger set of rules by default (413, up from 59)." — https://astral.sh/blog/ruff-v0.16.0

The trap: select replaces the default set; extend-select adds to it. A project with a hand-written select = [...] is usually running fewer rules than an unconfigured ruff check. So you cannot infer coverage from "they configured ruff" in either direction — read the config.

Step 0 is a baseline read, not a code read

Open pyproject.toml (or ruff.toml / setup.cfg / mypy.ini) and record:

Read this to know what to stay quiet about, not to report it. Recommending a lint or type configuration is a project-level action — see §P — and belongs in an --all audit. On a diff review the baseline has one job: stop you raising something a rule already covers.

What this skill checks

Python codebases go wrong the same handful of ways, over and over. So the default review looks only at what the diff introduces — a new dict where a dataclass belongs, a new bare except, a new subprocess without a timeout, a new write that bypasses the repo's atomic writer. Repo-wide properties (is ruff configured with select or extend-select, is mypy strict, does pytest run with --strict-markers) belong to §P, which runs at --all and nowhere else.

Usage

/review-python                  # Review context-related code
/review-python --staged         # Review staged changes
/review-python --unpushed       # Review files changed across all unpushed commits
/review-python --changed        # Review unstaged changes
/review-python --all            # Full codebase audit (parallel agents)
/review-python --multi          # Also get external advisor opinions

Scope

--unpushed derives its range from git rev-list HEAD --not --remotes (oldest unpushed commit's parent → HEAD). If nothing is unpushed, or there is no remote/upstream (or the range walks back to the root commit) so it can't be determined reliably, stop and ask the user to pick another scope. Restrict the resolved file list to Python extensions before reviewing.

Workflow

1. Determine scope (see table) and filter to Python files only.

2. Read the baseline (Step 0 above) — before reading any code.

3. Identify the project shape — CLI, library, backend, or data/script. §5 changes by shape, and a finding that is right for a library is often wrong for a CLI.

4. Read AGENTS.md / CLAUDE.md in the repo root for project conventions.

5. Review each file against §1–§6 — against what the diff introduces. Add §P only at --all. Load references/async.md when the code touches asyncio, threads or a TUI event loop; load references/boundaries.md when it touches subprocess, the filesystem, or parsing external input.

6. Parallelize if scope has >5 files: one sub-agent per category, merge and dedupe.

7. External opinions (if --multi): invoke second-opinion --quick with this prompt:

   Read-only Python review. Assume ruff and mypy already handle mechanical rules — do NOT repeat
   lint-level findings (mutable defaults, naive datetimes, late-binding closures, bare excepts,
   unsorted imports). Focus on DESIGN judgment: is data that was validated at the perimeter
   re-widened to dict[str, Any] so every consumer re-validates or forgets to? Are correlated
   fields modeled as a bag of optionals where a Literal-discriminated union plus match plus
   assert_never belongs? Does the exception hierarchy correspond to distinct caller responses, or
   is it bypassed by raw ValueError? Does every async task have an owner that awaits it, and does
   cancellation propagate? Is a good existing primitive (atomic write, lock, coercion helper)
   bypassed by some call sites? 300 words or less.

   Wait for all external results before proceeding.

8. Classify severity and report, grouped by severity.

1. Data & state modeling

The core value, and usually the root of findings in §2 and §3. A type checker checks that types are consistent; only a human checks that they are right.

• A dict standing in for a domain record. The tell is the same field read as t["x"] in some places and t.get("x") in others — each call site independently guessing at optionality. Second tell: the schema exists only inside one constructor, and other code paths add keys that constructor never mentions, so the shape is genuinely open. Recommend a TypedDict (for a dict you were handed) or a dataclass (for a thing you constructed).
• Validated at the perimeter, then re-widened. A schema validator proves exact shapes and hands back dict[str, Any]; the type system learns nothing, so interior code either re-checks the same invariants in branches that can never execute, or forgets to and trusts a value nobody proved. Both are the same defect. "Push the burden of proof upward as far as possible, but no further… Get your data into the most precise representation you need as quickly as you can." (Parse, don't validate — https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/)
• A declared enum that isn't enforced. A JSON schema with "enum": [...] sent to an external service (an LLM, an API) is a request, not a guarantee. If the response string is written into the domain unchecked, an unexpected value silently matches no branch anywhere and the record vanishes from every rollup with no error. Recommend a Literal on the setter plus one perimeter check.
• Bag-of-optionals where a tagged union belongs. Fields correlated by convention — "claim is non-None exactly when status != "busy"" — force a guard at every use, and the guard usually raises an internal error for a state the code constructs itself. A string like "internal: ..." in a raise message is the author flagging an unrepresentable state. Recommend a Literal discriminator + match + assert_never, which converts the impossible branch from a runtime raise into a type error. "The type checker will allow this call only if it can prove that the code is not reachable." (https://typing.python.org/en/latest/guides/unreachable.html)
• An enum decomposed back into mutually-exclusive booleans. Four independent bools for five legal states is 16 representable states, and the exclusivity gets reassembled by an elif chain in the renderer. Keep the enum and switch on it.
• In-band sentinels — "", -1, or 0 overloaded to mean "none"/"not applicable", so a typo in a magic string is a silent no-op. Especially where a str field carries three values and one of them is emptiness.
• Positional construction from parsed data — Record(*fields) splatting a row of same-typed strings. Reordering the dataclass silently mis-assigns every persisted record and nothing type-checks it. Also flag the asymmetry where the encoder spells all fields out and the decoder splats, since the two halves can drift.
• Tuples with a comment instead of a NamedTuple — list fields whose element shape lives in a trailing comment and is then indexed positionally.
• frozen= as a decision, not a habit. Ask whether the object is ever a dict key or set member: "If eq is true and frozen is false, __hash__() will be set to None, marking it unhashable." And __eq__ without __hash__ (PLW1641, off by default) breaks the invariant that equal objects hash equally.
• Structural conformance faked with alias properties — a class growing a field that is always None, or aliasing another field, so two types can be indexed uniformly. That is a union wanting to be written down.

2. Typing judgment

• Unannotated defs and args: Any are coverage holes. Under non-strict mypy the body of an unannotated function is unchecked entirely. args: Any on a CLI command function switches strict mode off at the busiest boundary in the program — and the cost shows up in the tests, which must fabricate the contract by hand (Namespace(...) objects that nothing verifies). Report the hole, not the missing annotation.
• Any laundering into a Literal. A value from json.loads or a dict[str, Any] satisfies a Literal[...] parameter with no check. Related: a cast(Literal[...], x) that is correct only because an argparse choices= tuple elsewhere happens to match — two unlinked copies of one enumeration.
• @runtime_checkable Protocol isinstance is not validation. "act as simple-minded runtime protocols that check only the presence of given attributes, ignoring their type signatures." Flag code that treats it as a guarantee about the signature.
• Variance: accept wide, return narrow. list[Shape] as a parameter rejects a list[Triangle] because list is invariant; Sequence[Shape] accepts it. Take Iterable/Sequence/Mapping in, return the concrete type. Flag list[X] demanded for read-only iteration.
• Any vs object vs Never — object keeps checking and forces narrowing; Any disables it. Never is the bottom type and is what makes assert_never work.
• Self for fluent APIs, alternative constructors and __enter__; @override (PEP 698) to catch a base-class rename that leaves the subclass orphaned — no ruff rule covers it.
• @final has no runtime effect — flag code relying on it for safety rather than for the checker.

3. Error modeling

• A hierarchy that exists and is bypassed. The highest-value finding in this section. A project defines AppError(exit_code, is_error) with meaningful subclasses, then raises raw ValueError from dozens of sites — sometimes in the same function as a correct raise. Every distinction the hierarchy exists to carry is discarded at the top-level handler.
• An exception that escapes the top-level handler. Check what the entry point actually catches against what the code actually raises. A bare RuntimeError matches neither except AppError nor except (DomainError, ValueError) even when AppError subclasses RuntimeError — subclass relationships run the wrong way for this.
• A catch so broad it flattens the hierarchy. except (FileNotFoundError, OSError, ValueError, RuntimeError) catches the entire domain when the domain's base is RuntimeError, collapses every exit code to 1, and drops a deliberate exit 0.
• Cross-function exception flow. A module's discover() guards json.JSONDecodeError and its parse() doesn't, while the caller catches only OSError. JSONDecodeError and UnicodeDecodeError are both ValueError, not OSError — one corrupt input file kills the whole scan. This needs reading two functions and the caller; no rule reaches it.
• A builtin IO exception used as a domain signal, then caught too broadly — raise FileNotFoundError("no config here") caught by an except FileNotFoundError that also swallows a genuinely missing binary and reports success.
• Result-or-error smuggled in-band — -> RunResult | str where str means failure, or -> tuple[str | None, int | None]. Nothing forces a new caller to check. Recommend a failure dataclass or an exception.
• Computed diagnostics discarded — a skipped counter incremented and never returned, so the user is told "deleting 4" with no sign the 5th was silently retained. F841 stays quiet because += counts as a read.
• A handler that catches but does nothing. Detection of a blind except is BLE001/E722; whether the handler is the bug is yours. Distinguish "genuinely don't care" from "lost the diagnostic". contextlib.suppress spanning ten statements suppresses far more than the one call it was written for — "should be used only to cover very specific errors where silently continuing with program execution is known to be the right thing to do."
• Chaining, only if B904 is not reachable per Step 0.

4. Async, concurrency & boundaries

Load references/async.md when the code touches asyncio, threads or a TUI event loop, and references/boundaries.md for subprocess, filesystem and untrusted input. The two headline items, because they recur everywhere:

• Blocking work reachable from the event loop indirectly. Ruff's ASYNC22x rules are call-site-local: they flag a literal subprocess.run inside an async def, not a named function three frames above one. An async handler that calls something which scans a filesystem, sleeps, and shells out is invisible to them and freezes the UI completely.
• A good primitive that call sites bypass. Find the repo's best existing atomic-write, lock, or coercion helper, then check who doesn't use it. Divergent copies of the same primitive with different safety guarantees — only one of which fsyncs — are a real and common defect.

5. Project shape

A finding that is right for a library is often wrong for a CLI. Establish the shape first.

6. Test quality

Module-level state and import-time side effects are in references/boundaries.md §8 — they belong to the same review pass as the other boundary items. What follows is tests.

• Asserting private state instead of rendered behaviour — app._inbox, app._active_tab. A change that correctly updates the list but fails to render it still passes.
• Fixed-iteration event-loop pumps as a worker wait — for _ in range(5): await pilot.pause(), with the number varying across tests. Every such test is timing-dependent by construction; recommend the framework's real wait (workers.wait_for_complete).
• Monkeypatching private internals, and the double-patch tell — the same name patched in two modules because from X import y creates a second binding. A third importer silently gets the real function and the stub quietly stops covering it, with no failure.
• Tests fabricating an untyped contract — hand-built Namespace(...) objects standing in for a real parser; a downstream cost of §2's args: Any.
• Negative assertions that pin nothing — assert "Metro" not in text passes for every wrong renderer that happens not to print that word. Keep the ones naming a specific thing that would otherwise have happened (assert not path.exists() after a delete, assert secret not in err).

P. Project-level checks — --all only

Repo-wide configuration. It changes rarely, so raising it on a diff review is noise.

• ruff select vs extend-select. select replaces the 413-rule default set, so a project with a hand-written select is usually running fewer rules than an unconfigured ruff check. Diff the prefixes against the defaults and report what's lost — commonly all 10 DTZ (naive datetime), all 10 ASYNC, plus TRY, LOG, G, TC, PIE, PERF. Recommending extend-select fixes the whole category at once. Note that 18 opinionated E/F rules were removed from the 0.16 default set (E401, E402, E701–E703, E711–E714, E721, E731, E741–E743, F403, F405, F406, F722), so a repo selecting bare E/F must keep those listed explicitly to retain them.
• mypy strictness. Plain mypy does not check the bodies of unannotated functions at all. Where a codebase has many partially-annotated defs, stage it: check_untyped_defs, then disallow_incomplete_defs, then strict.
• pytest configuration — --strict-markers, --strict-config, filterwarnings = ["error"], a coverage threshold, and whether asyncio_mode matches how the tests are actually marked.
• Whether the repo has a good primitive that call sites bypass — atomic write, lock, coercion helper. Finding the divergent copies is a whole-repo sweep; a diff review only checks whether this change bypassed it.
• Encoding discipline across all readers, which is only visible in aggregate.

Do NOT flag these

• Anything in §P, on a diff review. Lint and type configuration are project properties; this diff did not change them.

Already mechanical, or wrong. Check Step 0 before suppressing — if the project's config doesn't reach the rule, fold it into the one-time recommendation instead of reporting each occurrence.

• Mutable default arguments (B006), function calls in defaults (B008), mutable class defaults (RUF012) — and the dataclass decorator raises on them anyway.
• Late-binding closures in loops (B023), lru_cache on methods (B019).
• Naive datetimes — all ten DTZ rules are on by default. The best-covered category ruff has.
• Blind except detection (BLE001, E722) — the judgment of whether the handler does anything useful stays yours.
• PEP 695 generic syntax (UP046, UP047), import sorting (I001), modern-syntax rewrites (UP).
• print in a CLI — see §5.
• Demanding Pydantic. A stdlib dataclass plus a perimeter validator is a complete answer; a validation library is a dependency decision, not a review finding.
• Demanding ty. It is 0.0.x with no API stability commitment. pyrefly is production-ready if mypy's speed is a real problem — but mypy --strict is not a finding to begin with.
• Docstring coverage (D) unless the project already enforces it.

Severity

• Critical: crashes, corrupts, or silently loses data at runtime — an unvalidated external value entering the domain where no branch matches, a truncate-in-place write to a file another program depends on, malformed-row skipping combined with a rewrite, an exception that escapes the top-level handler on a reachable path.
• High: will cause bugs as the code evolves — invalid states representable in a load-bearing model, a hierarchy bypassed so exit codes are lost, blocking work on the event loop, a task with no owner, shared mutable state mutated across an await, a missing timeout on the slowest call.
• Medium: dict where a dataclass belongs, in-band result/error unions, coverage holes from unannotated defs, variance, encoding and atomicity inconsistencies, discarded diagnostics.
• Suggestion: NamedTuple over commented tuples, @override, __all__ on a library, naming and ergonomics where the code is awkward rather than buggy.

Output Format

## Python Review: {scope}

### Baseline
{Project shape, and whether `B904` is reachable (it decides whether exception chaining is yours to
review). **At `--all` only:** the §P results. On a diff review, omit this block unless something in
it changes what you reported.}

### Critical (crash / data loss / corruption reachable)
- {file}:{line} — {category}: {description}
  **Why it's not a ruff or mypy finding:** {what judgment this needed}
  **Impact:** {what breaks}
  **Fix:** {model change / boundary validation / real failure path — with code}

### High (design problems that will cause bugs)
- {file}:{line} — {category}: {description}
  **Fix:** {solution}

### Medium (modeling, typing, boundaries)
- {file}:{line} — {category}: {description} — {suggested change}

### Suggestions
- {opportunities}

If --multi was used, append one subsection per advisor that responded (titled with the advisor's name as reported by second-opinion), then a Cross-Model Agreement subsection.

Examples

Validated data re-widened:

/review-python --staged

A 400-line schema validator proves the exact shape of each resource spec, then stores the result as dict[str, Any]. Downstream, provision() re-checks isinstance(spec.get("range"), list) and raises on an unknown type — branches that cannot execute, and which permanently depress the coverage number. Reports High: a TypedDict union keyed on a Literal type field turns the final else into an assert_never type error and deletes five defensive raises. Not a mypy finding: Any conforms to every consumer, so mypy has no discriminator to narrow on and cannot see the branches as unreachable.

Blocking the event loop indirectly:

/review-python --changed

An async worker pushes a progress modal, then calls cmd_roundup(...), which three frames down scans every session directory, calls time.sleep, runs shutil.rmtree, and issues N sequential LLM subprocesses with no timeout=. Reports High — the modal cannot paint and keystrokes are dead until it returns; fix with asyncio.to_thread or thread=True, and add a timeout. ASYNC221 is silent because the subprocess call is not lexically inside the async def.

Hierarchy bypassed:

/review-python

errors.py defines ApplicationError(exit_code, is_error) with UsageError (exit 2) and MissingRecipeError (exit 0). The code raises 61 raw ValueErrors, four of them in a function that raises UsageError correctly on the line above. Reports High — every one renders identically through the generic handler and exits 1, discarding the distinctions the hierarchy exists to carry. TRY002 only catches raise Exception; ValueError is a perfectly ordinary raise.

Troubleshooting

Findings overlap with ruff or mypy

Solution: Check Step 0 — whether the project's config actually reaches that rule. Ruff's defaults are large but select replaces them, so coverage is not inferable from the presence of a config. If the rule runs, drop the finding. If it doesn't, fold it into the one-time baseline recommendation rather than reporting each occurrence.

Can't tell whether a dict should be a dataclass

Solution: Ask where it was born. Handed to you by json.loads, a subprocess, or a wire format and passed straight through → TypedDict describes it honestly. Constructed by your own code, or carrying invariants across function boundaries → a dataclass, validated once at construction. The deciding tell is whether any consumer has to ask "is this key present?" about a field the producer always sets.

Can't tell whether a broad except is wrong

Solution: Read what the handler does, not what it catches. Re-raising as a domain error, or logging with the traceback and continuing a loop that must not die, is fine. Returning None to a caller that cannot distinguish "absent" from "failed" is not. And check the callers — a handler can be correct while the caller's own except clause fails to match what actually propagates.

Notes

• Model first, parse second, annotate third. Most Any and most defensive re-validation dissolve once the underlying data is modeled correctly.
• Respect project conventions in AGENTS.md / CLAUDE.md — a codebase may deliberately use plain dicts at a boundary, or standardize on returning Result-shaped tuples.
• Don't be dogmatic: TypedDict migrations, wrapper types, and exhaustive match all have costs. Recommend them where they prevent real bugs, not everywhere they are possible.
• Test code: patching your own module's private helper in a focused unit test is defensible; patching a name in two places because of import bindings, or asserting on private attributes instead of rendered output, is not. Also flag sleep-based synchronization and real clock, network, or home-directory access in unit tests.
• Sources: the ruff 0.16 release notes and rule documentation (astral.sh); the CPython asyncio, dataclasses, enum, contextlib, subprocess and logging docs; the typing spec and guides at typing.python.org (exhaustiveness, distributing, re-export); PEP 654 and PEP 698; Alexis King, "Parse, don't validate"; Hynek Schlawack on hashes and equality; mypy's command-line reference. ruff and the type checker own the mechanical rules referenced above.