review-checklist

Review Checklist

Prioritize findings that can break users, expand permissions, hide failures, or make parallel agent runs unsafe.

• API contract safety: are new endpoints backward compatible? Does removing a field break existing callers?
• Silent IAM expansion: any change that quietly adds new IAM permissions is a HIGH finding. It must be explicit and operator-approved.
• Exception handling: bare except: or overly broad except Exception: that discards traceback context is a MEDIUM finding. Prefer typed exceptions with raise ... from e.
• Cleanup safety: cleanup code must be best-effort. Use try/finally; never let cleanup raise and abort the cleanup sequence. also_teardown_controller=False is the established safe default for SkyPilot.
• Narrow exception paths: test coverage for failure paths such as submit failure, auth failure, and cleanup failure is required. Absence is a MEDIUM finding.
• Concurrent run safety: does the code handle parallel Codex runs safely? Check for file ownership assumptions and missing commit-lock patterns.
• Config injection consistency: config should resolve through one precedence-ordered path: explicit arg, then env var, then config file. Inconsistent resolution is a MEDIUM finding.
• Temp file leaks: tempfile.mkdtemp without cleanup on exception paths is a MEDIUM finding. Prefer TemporaryDirectory context manager.
• Version pins: required dependency versions should be asserted at runtime, not just documented.