Review Checklist

How to run a Python idiom review end to end, grade severity consistently, avoid false positives, and write comments that get acted on. Pairs with scripts/review_idioms.py.

#Contents

1. Before you start
2. Running the scanner
3. Manual pass checklist
4. Grading severity
5. Rule reference and false positives
6. Writing review comments
7. Rewriting safely
8. Reporting
9. CI integration

#Before you start

Answer these before judging any line:

#Running the scanner

python scripts/review_idioms.py path/to/file.py                 # full review with rewrites
python scripts/review_idioms.py src/ --min-severity medium      # triage a whole package
python scripts/review_idioms.py src/ --json > review.json       # feed into another tool
python scripts/review_idioms.py src/ --select PY036,PY037,PY038,PY039,PY040,PY041   # production-risk sweep
git diff --name-only main -- '*.py' | xargs python scripts/review_idioms.py   # only changed files

JSON shape:

{
  "files": 1,
  "summary": {"high": 1, "medium": 0, "low": 2},
  "findings": [
    {"path": "app.py", "line": 14, "col": 1, "rule": "PY036", "name": "hardcoded-secret",
     "severity": "high", "message": "...", "source": "OPENAI_API_KEY = \"sk-...\"", "fix": "..."}
  ],
  "errors": []
}

The scanner is a first pass. Treat each finding as a question ("is this really a problem here?"), not a verdict.

#Manual pass checklist

Go through the code once more with this list; most items cannot be detected statically.

#Correctness

• Every rewrite candidate is behaviour-equivalent (see Rewriting safely)
• Functions return one type; no None-or-list mixes; errors raise, not return strings
• No shared mutable state: module-level lists/dicts mutated by functions, class attributes used as instance state
• Float comparisons use tolerances (math.isclose, np.allclose)
• Integer division and off-by-one in slicing, chunk overlap, pagination
• dict.get(k, default) vs stored None values
• Iterators consumed twice (generators, zip, map, file handles)
• Late-binding closures in loops (lambda: i captures the variable, not the value)

#Errors

• No bare except:; broad except Exception only at boundaries (request handler, worker loop) and always logged
• raise ... from err when translating exceptions
• try blocks contain only the call that can fail
• Cleanup in finally or a context manager, never only on the happy path

#Resources

• Files, sockets, sessions, DB connections, locks use with / async with
• Text I/O passes encoding="utf-8"
• Clients are created once and reused; closed on shutdown

#Async and concurrency

• No blocking calls in async def (including sync SDK methods and CPU-heavy work)
• Every coroutine is awaited; created tasks are kept or owned by a TaskGroup
• Concurrency is bounded (Semaphore) and matched to rate limits
• Timeouts on every network call and an overall deadline per request

#AI specifics

• No secrets in code, notebooks, test fixtures or default args
• Retries bounded, only on retryable errors, with jittered backoff; not stacked on top of SDK retries unknowingly
• Model output is parsed and validated (pydantic / JSON schema), never evaled
• Prompts are templates with named fields; untrusted text is delimited
• Token counting uses the tokenizer, not len(str)
• Prompts/completions/PII not logged at INFO
• numpy/torch truthiness not used on arrays/tensors

#Readability

• Names say what, not how (relevant_chunks, not lst2)
• Functions do one thing; flag arguments split into two functions
• Comments explain why; no commented-out code
• Type hints on public functions

#Grading severity

Adjust by context:

• Raise by one level when the code is on a hot path, handles user input, or runs unattended (workers, cron jobs).
• Lower by one level in throwaway scripts, notebooks, or tests (except secrets: always high).
• A low finding repeated 50 times is still low; mention the pattern once with a count.

#Rule reference and false positives

Suppress a reviewed exception inline: do_thing() # noqa: PY034 - CLI output.

#Writing review comments

A good comment has four parts: where, what, why it matters here, concrete fix.

L14 [high] PY036 hardcoded key
The OpenAI key is committed in source; anyone with repo access (and any bot scanning
GitHub) can use it. Please revoke it and read it from the environment:

    api_key = os.environ["OPENAI_API_KEY"]

L30 [high] PY037 time.sleep inside async def
fetch_context() runs on the event loop; time.sleep(1) pauses every request in this
worker for a second. Use `await asyncio.sleep(1)`.

L24, L41, L57 [low] PY012 range(len(...)) (3 places)
Iterating directly reads better and avoids index bugs:

    for text in texts: ...
    for i, text in enumerate(texts): ...   # if you need i

Tone and form:

• Lead with the impact, not the rule name.
• Be specific about the fix; include the code.
• Group repeated low findings into one comment.
• Mark optional suggestions as "nit:" or "optional:"; do not block a merge on them.
• Ask a question when intent is unclear ("Is swallowing this deliberate? If so, a comment would help").
• Praise good patterns briefly when they are non-obvious (a correct TaskGroup, a clean retry policy).

#Rewriting safely

Equivalence traps to check before suggesting a rewrite:

After rewriting: re-run the scanner, run the tests, and spot-check edge inputs (empty list, None, one item, unicode, very large input).

#Reporting

## Summary
Solid structure, but two production risks: a committed API key and blocking calls in
async handlers. Assumed Python 3.11.

## Findings
| # | Line | Rule  | Severity | Issue                         | Fix                          |
|---|------|-------|----------|-------------------------------|------------------------------|
| 1 | 14   | PY036 | high     | OpenAI key in source          | env var + rotate             |
| 2 | 30   | PY037 | high     | time.sleep in async def       | await asyncio.sleep          |
| 3 | 22   | PY039 | medium   | requests.post without timeout | timeout=(5, 60)              |
| 4 | 46   | PY018 | low      | += string building in loop    | "\n".join(...)               |

## Rewritten code
...

## Behaviour changes
- Requests now raise after 60 s instead of hanging.

## Not changed
- print() in main(): CLI output, intended.

## Next steps
- Add `python scripts/review_idioms.py src --strict` to CI.
- Test for the retry path (mock a 429 then 200).

#CI integration

GitHub Actions step:

- name: Idiom review
  run: python skills/programming/python/python-idiomatic-code-reviewer/scripts/review_idioms.py src --strict --min-severity medium

pre-commit local hook:

repos:
  - repo: local
    hooks:
      - id: review-idioms
        name: review idioms
        entry: python tools/review_idioms.py --strict
        language: system
        types: [python]

Rollout on a legacy codebase:

1. Start with --select on the high rules only; fix or # noqa existing hits.
2. Add medium rules once high is clean.
3. Keep low rules advisory (no --strict), or run them only on changed files.