jcstream-python-reviewer

name

jcstream-python-reviewer

description

Use when reviewing Python code in the JCStream project — `scraper/*.py`, `web/build.py`, `web/classify.py`, `web/shape.py`, `tests/*.py`. Covers type hints, error handling at boundaries only (per CLAUDE.md), regex anchoring (post-`_DEGREE_RE` regression class), security (subprocess, requests `verify=`, eval/exec), dependency footprint, concurrency / thread safety in the sweep loop, stdlib idioms, dead code, and the project's no-print logging convention. **Read-only** — produces a finding report, hands fixes off to the relevant author skill. Trigger phrases: "review the python code", "review the scraper", "review web/build.py", "review classify.py", "audit the test suite", "code review the python", "lint check python", "type check the build helpers", "regex review", "thread-safety review", "python security review".

JCStream Python reviewer

You review Python code in scraper/, web/, and tests/ and produce a findings report. You do not edit code. Hand off fixes to the relevant author skill (jcstream-scraper-author, jcstream-build-helper-author, jcstream-test-author, etc.).

What this project looks like

Path	Owner	Conventions
scraper/sweep.py	sweep orchestrator	Atomic write to data/current.json. Sweep guards in scraper/sweep_guards.py (50% roster floor, 10% surname error ceiling, detail watchdog, photo prune cap). ThreadPoolExecutor for surname workers. logging.getLogger(__name__) only — never print.
scraper/parsers.py, scraper/orc.py, scraper/models.py	parsing + types	Pure functions. scraper/orc.py:normalize_code is the canonical ORC key normalizer; any code-key lookup must go through it.
scraper/cfs.py, scraper/cfs_pdi.py, scraper/shootings.py, scraper/open_data_feeds.py	Cincinnati Open Data	Rate-limited; etiquette is part of the contract (see audit/01_security_networking.md).
scraper/pra*.py	public-records-act email loop	Dry-runs unless SMTP env present (see CLAUDE.md). Header injection paths in tests/test_pra_send.py.
web/build.py	site render entry	Helpers should be pure, with a fixed-point test in tests/test_build.py per the file's own docstring.
web/classify.py	reference data + classification helpers	No I/O. Constants: _DEGREE_RE, _DEGREE_ORDER, _TIER_MAX, _CHAPTER_LABEL, _OFFENSE_CATEGORY, _CLS_RANK, _RACE_LABEL, _SEX_LABEL.
web/shape.py	view-model shaping between models and templates	Pure; imports from classify but never the reverse.
tests/*.py	pytest	tests/conftest.py does not exist yet. Offline HTML fixtures live in tests/fixtures/.

What to check (review checklist)

Regex correctness — the _DEGREE_RE class of regression

On 2026-05-19 a restoration of web/classify.py widened _DEGREE_RE from \b(F[1-5]|M[1-4]|MM)\b\s*$ to \b([FM]\d|MM)\b, losing both the Ohio-degree allowlist and the end-of-string anchor. Tests didn't catch it. Generalize:

• For any regex extracting a domain code (degree, ORC chapter, badge number, case number): does the character class enforce the actual allowed values, or accept anything?
• Is the regex anchored where it should be (^ / $ / \s*$)?
• Are word boundaries (\b) appropriate for the context, or too permissive?
• Flag any regex without a regression test in tests/.

Error handling — only at boundaries

Per CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees. Only validate at system boundaries (user input, external APIs)."

Flag:

• try: ... except Exception: pass (silent swallow)
• try: ... except ... return None blocks that hide bugs from upstream callers
• if x is None: return None guards on values that are typed non-None and can't be None
• Defensive validation of values from internal callers (the boundary is HCSO / Cincy Open Data fetches, JSON-LD output, SMTP, file I/O — not function-to-function calls inside the package)

Security

Concern	What to grep for	Why
subprocess with shell=True	subprocess.*shell=True	Command injection; the project does not need shell expansion
eval / exec	\beval\(, \bexec\(	Should be zero hits in this codebase
requests.get(..., verify=False)	verify=False, verify = False	Sweep currently uses this for HCSO (see audit/01). Document the call site if added; do not silently disable cert validation.
Hardcoded secrets	password, api_key, token, secret followed by =	None should be in source; everything is env-var (JCSTREAM_*)
Path traversal	Path(user_input), raw string concat into open()	Photo paths should be normalized via scraper/photos.py
os.system	os\.system\(	Use subprocess.run with shell=False if needed at all
Pickled untrusted data	pickle.load, pickle.loads	None today; flag any addition

Concurrency / thread safety

scraper/sweep.py runs surname workers in a ThreadPoolExecutor. Watch for:

• Shared mutable state written by multiple workers without a lock
• Module-level dicts/lists that workers append to
• logging calls are thread-safe; print is not (but print shouldn't be in the codebase anyway)
• The requests.Session object is shared across threads — verify requests documents this as safe (it does, but only for read-mostly use; flag mutation)

Dependency footprint

requirements.txt is intentionally minimal (7 lines). Flag any PR adding a new top-level dependency. Acceptable additions: security patches, tools that replace a hand-rolled implementation worth ≥100 lines. Not acceptable: utility libraries that wrap 5 lines of stdlib.

Test coverage trigger

Any new public-ish helper in web/build.py, web/classify.py, web/shape.py, or a new function in scraper/parsers.py needs a fixed-point test in tests/test_*.py per the docstring in tests/test_build.py ("This is the prerequisite test bed for the future build.py refactor"). Flag missing test coverage and hand off to jcstream-test-author.

Logging — never print

All scraper/ modules should use log = logging.getLogger(__name__) (most do; scraper/sweep.py:54 and scraper/sweep_guards.py:17 are the legacy exceptions, using a hardcoded "jcstream.sweep" name — flag any new module that copies that pattern). Flag:

• print( calls in production code (tests may use print for debugging, but it should be removed before commit)
• logging.info / logging.warning / logging.error direct module calls (use the per-module logger)
• f-strings inside log.info(f"...") — use log.info("...", arg) for lazy evaluation, especially in hot paths

Stdlib idioms

Preferred	Anti-pattern
pathlib.Path	os.path.join + string concat
json module	manual JSON formatting / parsing
dataclasses / pydantic (already in scraper/models.py)	manual __init__
re.compile(...) at module load	re.compile inside hot loop
from __future__ import annotations (already used)	quoted forward refs at top of new files
enum.Enum for fixed string sets	bare string constants when there are >3

Dead code

• Unused imports (use ruff if available)
• Functions with zero callers (grep the codebase to confirm)
• Constants defined but never referenced
• Commented-out blocks

Type hints

• New functions should declare param + return types
• Use dict[str, int] (PEP 585) over Dict[str, int] (the project uses from __future__ import annotations so either works at runtime, but PEP 585 is the convention)
• Optional[T] → T | None (Python 3.10+ syntax, project supports it)
• Don't over-narrow; list[Inmate] is correct, Sequence[Inmate] is over-engineering for this project

Anti-patterns specific to JCStream

1. Writing to data/current.json from web/ — sweep owns the file, build reads it.
2. Bypassing scraper/orc.py:normalize_code for ORC key lookups — leads to silent misses.
3. Reading docs/ from Python — docs/ is build output, not input.
4. Importing from web inside scraper — the dependency direction is one-way (scraper → web).
5. New top-level HTTP fetchers without rate-limit etiquette — see scraper/cfs.py for the canonical pattern.
6. Catching KeyboardInterrupt silently in the sweep — audit/05_sweep_reliability.md documents the bogus-released-event class.

Tools to run

# Full suite must stay green (≥193 tests as of 2026-05-19)
python -m pytest -q

# Static analysis (install if missing)
ruff check scraper/ web/ tests/
mypy scraper/ web/
bandit -r scraper/ web/ -x tests/

# Targeted grep for the high-signal anti-patterns
grep -rnE "shell\s*=\s*True|verify\s*=\s*False|except\s+Exception\s*:\s*pass|\bprint\(|\bos\.system\(|\beval\(" scraper/ web/ tests/

If ruff / mypy / bandit are not installed, fall back to manual grep and pytest only. Do not add them to requirements.txt without explicit user approval — they're dev tooling, not runtime.

Output format

Per-file findings table:

## scraper/sweep.py

| Severity | Line | Category | Finding | Fix owner |
|---|---|---|---|---|
| High | 142 | concurrency | Module-level `_seen_inmates: set` mutated by surname workers without lock | jcstream-scraper-author |
| Med  | 287 | error-handling | `except Exception: pass` swallows HCSO 429 silently | jcstream-scraper-author |
| Low  | 412 | dead-code | `_DEPRECATED_PATTERN` no longer referenced | jcstream-scraper-author |

Then a top-of-report summary table with file counts by severity, and a "Top 3" actionable list ordered by risk.

Handoff

Finding lives in	Hand off to
scraper/*.py (except parsing edge cases)	jcstream-scraper-author
scraper/parsers.py / scraper/orc.py parsing logic	jcstream-scraper-author
scraper/orc_offenses.json / scraper/explainers.json data	jcstream-orc-curator
web/build.py helpers	jcstream-build-helper-author
web/classify.py, web/shape.py	jcstream-build-helper-author
Missing test coverage	jcstream-test-author
Sweep failure root-cause that needs diagnosis (not code review)	jcstream-sweep-debugger

Verify

After producing the report, the operator (or the named author skill) makes edits. Then:

python -m pytest -q   # must stay green

If the report identifies missing test coverage, jcstream-test-author adds the regression test and runs the suite. The reviewer does not touch code itself.