# Codex PR Risk Review

Use this skill when an AI coding agent needs to review a pull request, local git diff, or recently changed code for correctness risks before shipping.

## Outcome

Produce a concise engineering review that leads with actionable findings. Prioritize bugs, regressions, security issues, data loss risks, broken contracts, and missing tests. Avoid style-only comments unless they hide a real behavioral problem.

## Inputs To Collect

- Repository root
- The exact diff or commit range under review
- Project test commands, if discoverable
- Any user-stated acceptance criteria
- Relevant files touched by the diff

## Review Procedure

1. Identify the changed surface area.
   - Run `git status --short`.
   - Run `git diff --stat`.
   - Run `git diff --name-only`.
   - If a base branch is known, compare against it with `git diff BASE...HEAD`.

2. Read the implementation in context.
   - Open changed files and nearby helpers.
   - Search for each changed function, type, route, component, or config key.
   - Check whether call sites expect a contract that the diff changed.

3. Build a risk map.
   - Boundary changes: public APIs, schemas, migrations, auth, payments, file IO, network calls.
   - Control-flow changes: retries, fallbacks, error handling, cancellation, concurrency.
   - Data changes: nullability, ordering, filtering, pagination, time zones, units, precision.
   - Frontend changes: responsive layout, disabled states, keyboard path, loading and error states.
   - Test changes: removed assertions, narrower fixtures, mocks that no longer match runtime behavior.

4. Verify with commands.
   - Run the narrowest relevant tests first.
   - Run type checks or linters when they are part of the project norm.
   - If a command fails, determine whether the failure is caused by the diff or by unrelated environment state.

5. Convert risks into findings.
   - A finding needs a clear bug, a concrete trigger, and a plausible user or system impact.
   - Include file and line references.
   - Explain why the current behavior fails, not just what changed.
   - Do not pad the review with uncertain concerns.

## Output Format

Use this shape:

```text
Findings
- [P1] Short title
  File: path/to/file.ext:123
  Why it matters: ...
  Trigger: ...
  Suggested fix: ...

Open Questions
- ...

Verification
- Command: ...
  Result: pass/fail/not run

Summary
- ...
```

If there are no issues, say so directly:

```text
Findings
- No correctness issues found in the reviewed changes.

Verification
- ...

Residual Risk
- ...
```

## Severity Rubric

- P0: Immediate security exposure, data loss, production outage, or unrecoverable corruption.
- P1: High-impact bug likely to affect normal users or core workflows.
- P2: Real bug with narrower trigger, degraded behavior, or missing coverage around risky logic.
- P3: Low-risk issue, confusing edge case, or maintainability problem that can cause future mistakes.

## High-Yield Checks

### API And Backend

- Does validation happen before side effects?
- Are authorization checks applied to every path, including update and delete operations?
- Are retries idempotent?
- Does pagination preserve ordering and cursor semantics?
- Do database migrations handle existing rows and rollback expectations?
- Are secrets, tokens, or PII logged?

### Frontend

- Does text fit on mobile and desktop?
- Are loading, empty, disabled, and error states reachable and clear?
- Are form submissions protected against double-clicks?
- Do optimistic updates roll back on failure?
- Are server errors surfaced instead of swallowed?
- Are keyboard and screen-reader paths preserved for controls?

### Data And Time

- Are dates timezone-safe?
- Are monetary values handled without floating point surprises?
- Are units explicit?
- Are null, empty string, zero, and missing keys distinct where the domain requires it?
- Are filters applied in the intended order?

### Tests

- Is the changed behavior covered by tests that would fail before the fix?
- Are mocks hiding the integration that can break in production?
- Did the diff delete or weaken assertions?
- Is there at least one negative case for validation/auth logic?

## Anti-Patterns To Avoid

- Do not invent a bug without a reproducible path.
- Do not review as if every code smell is a shipping blocker.
- Do not recommend a rewrite when a local fix solves the risk.
- Do not claim tests passed unless you ran them.
- Do not expose secrets from logs, config, or API responses in the review.

## Fast Mode

When time is limited:

1. Inspect the diff stat and touched files.
2. Search changed identifiers.
3. Check auth, data mutation, and error paths.
4. Run the smallest relevant test command.
5. Return only high-confidence findings.

## Deep Mode

When reviewing a larger change:

1. Build a changed-contract list.
2. Trace each contract through callers and tests.
3. Run narrow tests, then broader suite.
4. Inspect docs, migrations, and generated artifacts.
5. Produce findings only where evidence is strong.

## Example Finding

```text
Findings
- [P1] Deleted projects can still be edited through the update endpoint
  File: src/routes/projects.ts:84
  Why it matters: The list endpoint now filters deleted projects, but updateProject still loads by id without checking deleted_at. A user with an old project URL can continue mutating a project that the UI treats as deleted.
  Trigger: DELETE /projects/:id followed by PATCH /projects/:id with any valid field.
  Suggested fix: Add deleted_at IS NULL to the update lookup or reject updates when deleted_at is set, then add a regression test for the deleted project path.
```