---
name: code-review
description: >-
  Fast multi-perspective code review for regorus. Use for everyday code reviews.
  Reviews from 3 perspectives with calibrated severity and noise filtering.
allowed-tools: shell
---

# Code Review Skill

## What You're Protecting

A bug in regorus can mean `allow` when the answer should be `deny`.
Review this diff to find bugs that matter at that severity level.

Key constraints (details in copilot-instructions.md):
- **Undefined ≠ false** — silent wrong policy results
- **Panics across FFI** → permanent engine poisoning (process-wide)
- **9 binding targets** → any API change has 9x blast radius
- **Dual execution paths** — interpreter and RVM must agree
- **`enforce_limit()`** required in accumulation loops

**Do not** run cargo, clippy, tests, or build commands. Diff-review only.

## Step 1: Get the Diff

```bash
# Primary: use gh pr diff (works in cloud agent + any PR context).
# Fallback: git merge-base for local non-PR usage.
if gh pr diff --name-only >/dev/null 2>&1; then
  echo "---STAT---"
  gh pr diff --name-only
  echo "---DIFF---"
  gh pr diff
else
  BASE=$(git merge-base upstream/main HEAD 2>/dev/null \
      || git merge-base origin/main HEAD 2>/dev/null \
      || git merge-base main HEAD 2>/dev/null)
  echo "Reviewing changes since: $BASE"
  git diff "$BASE"..HEAD --stat
  git diff "$BASE"..HEAD
fi
```

If the diff is empty, stop and report: "No changes found to review."

## Step 2: Triage and Inventory

Classify the diff before reviewing:
- **Trivial/mechanical**: renames, formatting, comments, dep version bumps, generated code
  → Report "No material issues found" unless something catches your eye. Skip Step 3.
- **Targeted change**: ≤300 changed lines in a focused area → Review with relevant perspectives.
- **Large/cross-cutting**: >300 lines or multiple subsystems → Review all perspectives.

**Quick inventory:** List every changed function/struct/pub item (one line each).
At the end of Step 3, confirm you examined each one.

## Step 3: Review — Three Passes

**Your goal is breadth.** Cover the entire diff, don't fixate on one area.
Report anything suspicious even if you're only 60% sure — better to include a
Low finding than miss a Medium.

### Pass 1: Line-by-line correctness

Walk through every changed line. For each, ask:
- What was the author's intent? Does the code achieve it for ALL inputs?
- What happens with: empty, null, zero, max-size, wrong-type, nested, Undefined?
- What happens on Windows? With non-ASCII? With empty string vs absent?
- If output must follow a standard (SARIF, URI, JSON Schema): are all MUST
  requirements met? Reserved chars escaped? Required fields present?
- What does the most common real-world input to this function look like?
  Does the code handle that correctly? What about the second and third most
  common patterns?

For suspicious code paths, trace a concrete value through them:
```
input = <concrete example>
→ after line N: variable = <concrete value>
→ after line M: result = <concrete value>
→ expected: <what it should be>
```
Concrete traces strengthen Critical/High findings but are NOT required to
report a finding. If something looks wrong, report it — even at Medium/Low
confidence.

Use `view` to read surrounding context for anything suspicious.

### Pass 2: System-level consequences

Step back from individual lines:
- Does this new API freeze anything via semver? (pub fields, pub types, pub mods
  without feature gates)
- Could a caller misuse this API in a way the author didn't anticipate?
- Resource consumption: is anything proportional to untrusted input without bounds?
- Error handling: are errors propagated or silently swallowed? Appropriate types?
- Does this interact badly with existing features? (feature flags, no_std, `arc`,
  dual interpreter/RVM paths)
- If touching `src/engine.rs`, `src/lib.rs`, or `bindings/`: do all 9 targets handle it?
- If touching `Cargo.toml` or `#[cfg(feature)]`: feature gate correctness, no_std?

### Pass 3: What's missing

Scan the diff stat one final time:
- Are there files or functions you haven't examined closely? Look now.
- For each new public function: what happens with every `Value` variant?
  (Null, Bool, Number, String, Array, Set, Object, Undefined)
- What test cases would you write? Are the obvious ones present?
- What does the code assume about inputs that isn't validated?
- If control flow uses `break` in nested loops — does it exit the right level?

### Edge-Case Exploration

For each significant new function or data transformation:

1. **Boundary inputs**: empty collections, zero/max integers, single vs many,
   deeply nested
2. **Type mismatches**: expected object with fields → gets string/array/Undefined?
   Silent default? Error? Wrong output passed downstream?
3. **Platform variance**: Unix assumptions? (path separators, encoding, locale).
   Wrong output on Windows?
4. **Composition**: How does this interact with other modules? Could a valid
   combination produce unexpected behavior?
5. **Specification conformance**: If output follows a standard, are all MUST/SHOULD
   met? Reserved chars escaped? Required fields always present?

Only report edge cases with concrete example input → wrong output.

## Step 4: Design Considerations

Skip if the diff is trivial/mechanical or <50 changed lines.

Otherwise, briefly assess (2-3 sentences each, only if relevant):
- Is there a fundamentally simpler way to achieve the same goal?
- Does this duplicate existing infrastructure that could be reused?
- Are there tradeoffs the author may not have considered?

Only suggest alternatives you can concretely describe with clear benefit.

## Step 5: Report

### Findings (sorted by severity)

For each finding:
- **Severity**: Critical / High / Medium / Low
- **Confidence**: High / Medium / Low
- **Perspective**: which perspective found it
- **Location**: file:line
- **Issue**: one-sentence summary
- **Trace**: concrete input → concrete intermediate values → concrete wrong output
  (strengthens Critical/High but not required for Medium/Low)
- **Evidence**: the specific code (max 5 lines) and why it's wrong
- **Suggestion**: concrete fix (include code snippet when possible)

**Confidence guide:**
- **High**: you have a concrete trace showing wrong output
- **Medium**: pattern match + plausible scenario but no full trace
- **Low**: suspicious but cannot fully demonstrate the issue

**Severity calibration — lean toward reporting, not filtering.**
A separate review step can always downgrade. If you're unsure between two
severity levels, pick the higher one.

- **Critical**: Wrong policy result (allow/deny), panic reachable from FFI, security bypass.
  Every Critical MUST include: who triggers it, what specific input, why guards fail.
  If you can't construct a trigger path, downgrade to High.
- **High**: Panic in non-FFI path, unbounded resource usage, API break, data loss/corruption
- **Medium**: Logic error with limited blast radius, silent wrong output for edge-case inputs,
  missing bound on trusted path, design issue with concrete consequence
- **Low**: Minor inefficiency with measurable impact, missing validation, documentation gap

**Do NOT report:**
- Style preferences (naming, formatting) with no functional impact
- Anything the compiler or ~53 deny lints would catch
- "Consider using X" without explaining what goes wrong if you don't

**0 findings is valid** — do not manufacture findings without evidence.

**Calibration examples:**

Good finding:
> HIGH | src/eval.rs:42 | `items[idx]` where `idx` comes from untrusted input
> via `parse_array()` at line 38. No bounds check between parse and use.
> **Fix:** `items.get(idx).ok_or_else(|| anyhow!("index out of bounds"))?`

Bad finding (reject):
> "This unwrap could panic" — without verifying the value isn't guaranteed
> `Some` by construction. Check first.

Bad finding (reject):
> "Consider using a more descriptive variable name."

### Design Notes

Observations from Step 4 (if applicable).

### Coverage Check

Confirm: every function/struct from your inventory was examined in at least
one pass. If any were skipped, note them and briefly assess.

### Summary

X findings (N critical, N high, N medium, N low). One sentence overall assessment.

### Output

After generating the report above, write the COMPLETE report to `/tmp/code-review-report.md`
using the `create` tool or shell. This ensures the full report is preserved even if
display output is truncated.