pr-review

name	pr-review
description	Use this skill to review pull requests for VoxBento. Covers correctness, security, architecture compliance, and testing.

Skill: PR Review

Use this skill to review pull requests for VoxBento. Covers correctness, security, architecture compliance, and testing.

---

PR Review Checklist

1. Invariant Compliance

• No Vue, React, jQuery, inline <script> blocks.
• No Flask, Socket.IO, aiortc.
• No AudioContext.destination for interpreter mic audio.
• from __future__ import annotations at top of every new/modified Python file.
• New Python code uses portal.* imports (not relative imports or new top-level modules).
• uv.lock only changed if uv sync --python 3.13 --dev was run.
• Role is never trusted from client data in WS handlers.

2. Auth & Security

• All redirects use safe_redirect() — no raw RedirectResponse(url=user_input).
• No open redirect: next_url / next query params validated before use.
• Admin routes have dependencies=[Depends(require_admin)].
• API keys stored encrypted via portal.crypto.encrypt_val; never stored plaintext.
• No secrets logged or returned in API responses.
• JWT tokens use settings.effective_jwt_secret; no hardcoded secrets.
• New form inputs validated before DB write.

3. Database Changes

• Model changes in portal/models.py have a corresponding Alembic migration.
• Migration uses batch_alter_table for column operations on SQLite (see migration 008 as reference).
• New DB columns have appropriate defaults and nullability.
• Relationships that need mediamtx_path use joinedload(DBBooth.event).
• CRUD functions use async with get_session() as session: pattern.

4. Route Changes

• New routes have correct auth dependency.
• Error cases raise HTTPException with appropriate status codes.
• New page routes redirect to login if unauthenticated (using safe_redirect).
• New API routes respect _require_access(credentials, token) if applicable.

5. WebSocket Protocol

• New WS message types have a handler in fastapi_app.py ws_booth loop.
• session.granted_role used, not data['role'].
• New Booth.as_public_dict() fields are intentional (broadcast to all clients).

6. Transcription Changes

• New provider implements TranscriptionProvider ABC.
• New provider added to PROVIDERS dict in worker.py.
• New provider registered in ProviderEnum and ALLOWED_MODELS.
• New API key column follows Fernet encryption pattern.
• Worker lifecycle handles CancelledError and cleans up ffmpeg process.

7. Frontend Changes

• Plain ES modules — no import maps, no build step, no npm.
• node --check static/js/*.js passes.
• No AudioContext.destination for mic audio.
• New UI elements have IDs/data attributes expected by JS (not hardcoded strings).
• WHIP/WHEP URLs constructed from portal.dataset.* — not hardcoded.

8. Tests

• New functionality has at least one test.
• Tests use anyio + pytest.mark.anyio fixture (see conftest.py).
• DB tests use configure('sqlite+aiosqlite:///:memory:') + init_db().
• No test uses production DB URL.
• uv run pytest tests/ -v passes.

9. Documentation

• README.md updated if user-facing behavior changed.
• docs/how-it-works.mdx updated if system design changed.
• Relevant context file in .github/.agents/context/ updated.
• agents.md updated if invariants changed.

---

High-Risk Patterns to Flag

Pattern	Risk	Action
RedirectResponse(url=request.query_params['next'])	Open redirect	Replace with safe_redirect
role = data.get('role') in WS handler	Role injection	Use session.granted_role
session.execute(f"... {user_input} ...")	SQL injection	Use parameterized queries
event.openai_api_key = openai_key (plaintext)	API key exposure	Use encrypt_val
logger.info(f"Key: {api_key}")	Secret leakage	Remove log line
New npm/yarn/vite config	Violates no-build constraint	Remove
New <script> tag in template	Inline script	Move to ES module file

---

Running Validation Locally

uv sync --python 3.13 --dev
uv run pytest tests/ -v
node --check static/js/interpreter-booth.js
node --check static/js/whep-listener.js
uv run alembic upgrade head   # if migration added