review

어떤 스킬인가요?

Review a local change, branch, or pull request for production readiness, tests, performance, security, or merge-prep quality. Use when the user wants a finish readiness review, a PR code review, to publish a review to a PR, /code-review, or when another skill needs a production-readiness or test-quality pass.

설치 방법

프로젝트 폴더에서 아래 명령어를 실행하고, 설치할 에이전트를 선택하세요.

설치 명령을 직접 실행해야 적용됩니다. 지원 에이전트와 필요한 권한·라이선스는 제작자의 안내를 확인하세요.

스킬 원문 파일

interface:
  display_name: "Review"
  short_description: "Run a focused multi-lens review, optionally publishing a PR COMMENT review."
  default_prompt: "Use $review to review this change. For a PR, first ask whether to publish the review. Apply the finish baseline and automatically load only relevant tests, Ruby performance, and security lenses; do not ask me to choose a template."

# Conventional Comments

Mandatory format for every new inline review comment posted by `publish`. Use it for thread replies when the label clarifies the reply; concise conversational replies are allowed.

## Schema

```text
<label> (<decoration>, <decoration>): <subject>

<discussion>
```

Decorations and discussion are optional. When there are no decorations:

```text
<label>: <subject>

<discussion>
```

## Formatting rules

- Use a single lowercase label.
- Put optional lowercase decorations in parentheses, separated by comma + space.
- Keep the subject concise and actionable; it is the main message.
- Put supporting evidence, impact, reasoning, and next steps in the discussion after one blank line.
- Do not wrap the prefix in Markdown emphasis; preserve machine readability.
- Validate the first line against:

```text
^[a-z][a-z-]*( \([a-z-]+(, [a-z-]+)*\))?: .+
```

- Reject any comment that uses a `blocking` decoration.

## Approved labels

| Label        | Use when                                                                                                                |
| ------------ | ----------------------------------------------------------------------------------------------------------------------- |
| `issue`      | Concrete bug, regression, security/privacy problem, contract mismatch, or test gap that can let incorrect behavior pass |
| `suggestion` | Concrete improvement not required for correctness                                                                       |
| `question`   | Missing context whose answer can materially change the review conclusion                                                |
| `note`       | Verified context or constraint that helps the author but requires no action                                             |
| `nitpick`    | Very small polish; normally dropped by publish triage                                                                   |
| `praise`     | Specific recognition for an outstanding, elegant, unusually safe, or especially maintainable change                     |

## Approved decorations

| Decoration     | Use when                                                             |
| -------------- | -------------------------------------------------------------------- |
| `security`     | Authz, privacy, tenancy, secrets, or other security-relevant concern |
| `test`         | Missing or insufficient behavioral coverage                          |
| `performance`  | Material runtime or query-cost concern                               |
| `non-blocking` | Optional suggestion/nitpick intentionally safe to defer              |

Do **not** use `blocking`. Importance is communicated through finding severity and discussion while publishing a GitHub `COMMENT` review.

Severity (Critical / Important / Nice-to-Have) and Conventional Comments labels remain separate: severity controls whether the ledger publishes the finding; the label communicates what kind of comment it is.

## Examples

Undecorated issue:

```text
issue: unknown status silently returns the full register

Unrecognized `status` values fall through to `all`, so a client typo or meta-key mixup becomes an unfiltered listing. Prefer `422` for unknown values; only blank status should mean “no filter.”
```

Multiple decorations:

```text
issue (security, test): scope the lookup to the authorized tenant

The unscoped lookup accepts any record ID before policy evaluation, creating an IDOR path for authenticated users. Resolve the record through the policy scope and add a cross-tenant request spec.
```

Suggestion with non-blocking decoration:

```text
suggestion (non-blocking): extract the repeated permission lookup

This is duplicated across the two policies. A shared helper could reduce drift, but it does not need to hold up this PR.
```

Earned praise:

```text
praise: keeping the register read-only makes the trust boundary exceptionally clear

Separating support reads from the existing v3 mutation path avoids a second deletion entry point and makes the authorization model much easier to audit.
```

Reject:

```text
issue (blocking): fix the metric undercount
```

## Human touch

- Look actively for 0–2 genuinely outstanding choices: elegant boundary design, unusually strong tests, careful compatibility, clear failure handling, data minimization, or a simplification that removes risk.
- Publish `praise:` only when specific and earned. Explain what was done well and why it matters.
- Never use generic “looks good,” “nice work,” or praise as a buffer before criticism.
- Keep tone direct, collaborative, and written to a teammate. Prefer “This can…” / “Could we…” / “Please…” over robotic verdict language.
- Do not weaken concrete findings with excessive hedging. Human does not mean vague.
- Never manufacture praise to satisfy a quota. Zero praise comments is valid when nothing is exceptional.

## Thread replies

Use Conventional Comments when a reply introduces or sharpens a finding:

```text
issue (test): cover completed requests in the inventory metric

Agree this is a real regression — restore inventory `team_data_finished.count` to “any `team_data_finished_at` present.” Keeping `required_action:ready_for_archival` exclusive of completed is fine. Please add a completed-row example so the undercount cannot land again.
```

Do not force a label onto a simple disposition such as “Addressed in abc1234,” and do not post a bare “agree.”

# Finish

Production-readiness baseline for a local change, branch, commit range, or pull request. Findings report only — no boy-scout edits.

After scope prep in `SKILL.md`, continue here. For a pull request, review the PR patch and surrounding code at the recorded head SHA; never substitute the local working tree or local `HEAD`.

## Workflow

1. Establish scope and assumptions

- Capture explicit assumptions and missing context.

2. Review in priority order

- Production readiness
- Industry-standard patterns
- Maintainability and ownership transfer
- Risk identification and due diligence
- Compliance posture clarity

3. Apply the review workflow

- Assess architecture, boundaries, responsibilities.
- Evaluate code quality, failure modes, edge cases.
- Validate config, logging, security, and ops concerns.
- Enumerate risks, debt, limitations, and compliance gaps.

4. Use the autonomous review loop until convergence or blocked

- Scan → Evaluate → Decide → Document → Re-check.
- Stop only when Critical is empty and Important has owners or rationale.

## Output format (required)

**Findings**

- Categorize as Critical / Important / Nice-to-Have.
- Each finding includes impact and recommended action.

**Non-Goals**

- List explicit exclusions and intentionally unaddressed areas.

**Confidence & Uncertainty**

- Separate known facts from inferred or unverified items.

**Compliance & Risk Posture**

- What would pass review.
- What would be flagged.
- Minimum viable remediation or compensating controls.

**Executive Summary**

- Production readiness: Yes / No / Conditional.
- Top risks.
- Immediate actions.

## Guardrails

- Do not add features or redesign unless current design creates material risk.
- Prefer proven, conventional solutions.
- Optimize for clarity over novelty.
- Treat undocumented behavior as a defect.
- If deviating from standards, write explicit justification.

# GitHub state machine (`publish`)

Pending-review and thread mutations for the `review` skill **`publish`** execution. Ordinary review publishing uses `event: COMMENT` only. Never submit `REQUEST_CHANGES` or `APPROVE` from this skill.

For resolve/reply helpers owned by the `pull-request` skill, see that skill’s `reference/gh-api.md`. End-to-end PR review + publish lives here.

## Identify the PR

```bash
gh pr view --json number,url,headRefName,baseRefName,headRefOid,title
# or
./scripts/pr-context.sh --publish <pr-url-or-number>
```

## Fetch pending review for the current user

Use the `pending_reviews` array from `pr-context.sh --publish`. It is already filtered to `gh api user --jq .login` and fails closed rather than returning truncated review state.

GitHub allows **one** pending review per user per PR.

## State machine

```text
No pending review
  → POST /repos/{owner}/{repo}/pulls/{pr}/reviews
    with commit_id, body, comments[], event: "COMMENT"
  → verify review + comment URLs

Pending review exists
  → snapshot review node id + draft comments
  → deletePullRequestReviewComment for each draft classified drop
  → addPullRequestReviewThread for each new publish-inline finding
  → POST .../reviews/{databaseId}/events  { "event": "COMMENT", "body": "..." }
  → verify; confirm pending queue empty

Useful replies on existing threads (after main review succeeds)
  → addPullRequestReviewThreadReply
```

## Create and submit in one shot (no pending)

```json
{
  "commit_id": "HEAD_SHA",
  "event": "COMMENT",
  "body": "## Review findings\n\n...",
  "comments": [
    {
      "path": "app/models/example.rb",
      "line": 42,
      "side": "RIGHT",
      "body": "issue (test): subject\n\nDiscussion..."
    }
  ]
}
```

```bash
gh api repos/OWNER/REPO/pulls/PR/reviews --input review.json
```

**Payload guard:** abort if `event` is anything other than `COMMENT`.

## Delete an individual draft comment

Prefer per-comment delete over discarding the whole pending review.

```bash
gh api graphql -f query='
mutation($id:ID!) {
  deletePullRequestReviewComment(input:{id:$id}) {
    clientMutationId
  }
}' -F id=COMMENT_NODE_ID
```

## Append a thread to the pending review

Use the pending review **GraphQL node ID** (`id`), not the REST `databaseId`:

```bash
gh api graphql -f query='
mutation($reviewId:ID!, $path:String!, $line:Int!, $body:String!) {
  addPullRequestReviewThread(input:{
    pullRequestReviewId:$reviewId,
    path:$path,
    line:$line,
    side:RIGHT,
    body:$body
  }) {
    thread { id comments(first:1){ nodes { url body } } }
  }
}' -F reviewId=REVIEW_NODE_ID -F path='path/to/file.rb' -F line=123 -F body=$'issue: ...'
```

## Submit a pending review as COMMENT

```bash
gh api repos/OWNER/REPO/pulls/PR/reviews/REVIEW_DATABASE_ID/events \
  --input - <<'EOF'
{ "event": "COMMENT", "body": "## Review findings\n\n..." }
EOF
```

## Reply on an existing thread

```bash
gh api graphql -f query='
mutation($threadId:ID!, $body:String!) {
  addPullRequestReviewThreadReply(input:{
    pullRequestReviewThreadId:$threadId,
    body:$body
  }) {
    comment { id url body }
  }
}' -F threadId=THREAD_ID -F body=$'issue (test): ...'
```

## Last-resort: delete entire pending review

Only when per-comment reconcile is impossible. Snapshot the preserved ledger first, then recreate and verify before submit.

```bash
gh api graphql -f query='
mutation($id:ID!) {
  deletePullRequestReview(input:{pullRequestReviewId:$id}) {
    pullRequestReview { id state }
  }
}' -F id=REVIEW_NODE_ID
```

## 422 recovery

| Error                                                        | Cause                    | Recovery                                                                              |
| ------------------------------------------------------------ | ------------------------ | ------------------------------------------------------------------------------------- |
| `422 User can only have one pending review per pull request` | Pending already exists   | Append via `addPullRequestReviewThread`; do not create a second pending review        |
| Wrong review ID in GraphQL                                   | Used REST numeric ID     | Use GraphQL `id` field from pending reviews query                                     |
| Partial failure mid-reconcile                                | Network / mutation error | Re-fetch pending comments and threads; resume from actual state; never blindly replay |

## Verify

```bash
gh api repos/OWNER/REPO/pulls/PR/reviews/REVIEW_ID/comments \
  --jq '.[] | {path, line, html_url, body: .body[0:120]}'
```

Confirm:

- Review `state` is `COMMENTED` (not `CHANGES_REQUESTED` / `APPROVED`)
- No PENDING review remains for the current user (unless draft-only was requested)
- Every discussion URL is returned to the user

## Scenarios to exercise mentally before mutating

1. No pending review → one-shot COMMENT create
2. Pending with keep + drop drafts → per-comment delete, append, submit COMMENT
3. Duplicate bot thread → `reply-existing` instead of new inline
4. Head SHA changed between ledger and mutate → abort, refresh, re-anchor
5. Partial API failure → inspect state, resume without duplicates

# Perf (Ruby)

Ruby performance lens for changed code, branches, commit ranges, or pull requests.

Review the actual changed code before making recommendations. Use scope prep in `SKILL.md`; for a pull request, use the PR patch and surrounding code at its recorded head SHA rather than a local default-branch diff.

Focus on findings, not generic advice. For each finding, cite file and line, explain the performance impact, and give concrete improvement options.

When checking Ruby core APIs or standard-library behavior, verify them instead of relying on memory alone. Use Dash MCP lookups against the installed Ruby docset first, then Context7 if needed, and only then official API docs or primary sources. Prioritize verifying `Enumerable`, `Enumerator::Lazy`, `Set`, `Hash`, `Array`, and `Data.define` semantics when they materially affect the recommendation.

## Review Workflow

1. Establish scope.

- Identify the target diff or files under review.
- Distinguish committed branch changes from unrelated local edits.
- Read the relevant code before forming conclusions. Gather enough local context to understand call sites, surrounding helpers, data flow, and whether the path is plausibly hot.
- If the user asked for a review, do not start editing unless they explicitly ask for fixes.

2. Find hot paths and repeated work.

- Look for nested loops, repeated regex scans, repeated parsing, repeated allocations, repeated object construction, repeated sorting, and repeated conversions between arrays/hashes/sets.
- Trace the same data through the pipeline. Flag cases where one stage computes facts and a later stage recomputes them.
- Prefer architectural rearrangements when they remove entire passes, repeated normalization, or repeated derivation of the same facts.

3. Analyze data structures and big-O.

- Check membership tests, deduplication, grouping, set operations, lookup tables, queue/stack behavior, and accidental quadratic scans hidden inside helpers.
- Prefer `Set` or `Hash` over repeated `Array#include?`, `Array#any?`, or linear scans when the collection is reused.
- Call out big-O explicitly when it matters, especially `O(n^2)` or hidden `O(n*m)` patterns.
- Check assignment patterns that duplicate memory without need: extra `dup`, `clone`, `to_a`, `transform_values`, `merge`, `flatten`, `compact`, `sort`, or `group_by` results that are immediately re-walked or partially discarded.
- In modern Ruby, treat `Set` as a strong default for repeated membership and dedupe work; do not recommend it when order, duplicates, or tiny one-off collections make arrays simpler and cheaper.

4. Analyze Enumerable usage.

- Prefer single-pass transforms with `each_with_object`, `filter_map`, `to_h`, `sum`, `tally`, or a targeted accumulator over long pipelines that materialize multiple intermediate arrays.
- Avoid replacing a clear single pass with a denser chain unless it actually removes work.
- Flag `map.select`, `select.first`, `group_by.values`, `flat_map.uniq`, `sort_by.first`, and similar chains when a single accumulator or early-exit loop would do less work.
- Check whether laziness or streaming would help, but do not recommend `Enumerator::Lazy` unless it avoids real materialization costs on a meaningful path.
- Distinguish readability wins from real performance wins.

5. Consider object-model changes.

- Look for ad-hoc hashes, positional arrays, or multi-value tuples that force repeated recomputation, repeated unpacking, or unclear contracts.
- Consider `Data.define` as the primary immutable-holder recommendation when a hot path repeatedly passes around derived facts.
- Good candidates: normalized strings, tokenized values, parsed numeric facts, scoring inputs, grouped aggregates, memo payloads passed between phases.
- Do not recommend `Data.define` if a plain local variable, block local, or tiny private helper is sufficient.

6. Validate with evidence.

- If local benchmarks, focused specs, or profiling output exist, use them.
- Do not report assumptions or unverified findings. If a concern depends on missing context, read more code until the claim is supportable or move it to open questions.
- If no evidence exists and the optimization is non-obvious, recommend a minimal benchmark or profiling shape.
- Prefer validation that isolates the suspected hot path: `Benchmark.ips` or `Benchmark.bmbm` for CPU-bound code, allocation-sensitive checks for memory churn, and request/query inspection when a Rails path is involved.

## Performance Heuristics

Prioritize findings in this order:

1. Remove repeated passes over the same data.
2. Replace the wrong data structure.
3. Hoist invariant work out of loops.
4. Cache or memoize expensive pure computations.
5. Reduce allocation churn from intermediate arrays/strings/hashes.
6. Improve constant factors only after the above.

## Ruby-Specific Review Points

- Check for accidental `O(n^2)` loops from repeated `include?`, `find`, `detect`, `index`, or `delete` against arrays inside iteration.
- Check for repeated string building, slicing, interpolation, symbolization, JSON parsing, time parsing, and numeric coercion in inner loops.
- Check for `map { ... }.compact`, `select { ... }.map`, `group_by { ... }.transform_values`, `sort_by { ... }.first`, and similar pipelines that can collapse into one pass or early exit.
- Check for repeated `dup`, `clone`, `merge`, `to_h`, `to_a`, or `flatten` that inflate memory assignment and allocation pressure.
- Check whether `Hash.new(0)`, `Hash` lookup tables, or `Set` membership would replace repeated scans more cheaply.
- Check whether `Data.define` would let the code compute immutable facts once and carry them across phases instead of recomputing or repeatedly unpacking hashes.
- Check for `group_by.values.filter_map.max_by` style pipelines that can become one accumulator pass.
- Check whether `Enumerable` chains allocate arrays where lazy or single-pass accumulation would be better.
- Check whether `Struct` or ad-hoc hashes are being used for immutable facts that would read more clearly as `Data.define`.
- Check whether memoization keys are stable and cheaper than recomputing.
- Check whether a branch introduced cleaner method boundaries but accidentally duplicated work across methods or classes.
- For Rails code, briefly check query count, eager-loading boundaries, repeated relation materialization, and per-record Ruby work before focusing on micro-optimizations.

## Architectural Rearrangement Patterns

Use these when they materially reduce work:

- Candidate/facts/ranking pipeline:
  collect items once, derive immutable facts once, rank/select from those facts.
- Normalize once at the boundary:
  normalize strings, parse tokens, coerce numbers/times, and derive lookup keys once before grouping or membership checks.
- Carry facts forward:
  pass precomputed facts into downstream phases instead of rediscovering them.
- Replace broad cleanup with earlier filtering:
  suppress bad candidates before expensive derivation, sorting, or rendering.
- Build indexes before matching:
  precompute `Hash` or `Set` indexes once, then resolve relationships or membership checks against them.
- Collapse multi-pass aggregation:
  accumulate counts, best candidates, or grouped results in one pass instead of `group_by` followed by several traversals.

## Output Format

Start with findings ordered by severity.
When contributing to a generic review, use Critical / Important / Nice-to-Have and fold findings into the single `finish` report.

For each finding include:

- severity
- file and line
- evidence and supporting context from the code
- why it is slow or risky
- explicit big-O or constant-factor note when useful
- concrete improvement options
- short suggested rewrite when the optimization is straightforward

Then include:

- `Explicit optimization opportunities`
- `Open questions or uncertainty`
- `Benchmark suggestions`

## Review Style

- Be specific and direct.
- Do the reading needed to support each claim. Do not infer hot paths, repeated work, or API behavior without verifying them from the code and, when relevant, the docs.
- Prefer "this loop does an `Array#include?` lookup for every row, turning the pass into `O(n^2)`" over "could be optimized."
- Do not praise code unless it clarifies a tradeoff.
- If no material issues are found, say so explicitly and list residual low-confidence areas.

# Publish

End-to-end PR review publish: retrieve → fresh multi-lens review → reconcile drafts → submit a friendly GitHub `COMMENT` review.

Load references progressively, not all up front:

1. Use [`finish.md`](finish.md) as the baseline and load only the additional lenses selected by `SKILL.md`.
2. Load [`conventional-comments.md`](conventional-comments.md) when drafting bodies.
3. Load [`github-state.md`](github-state.md) immediately before reconciling or publishing GitHub state.

## Hard rules

- When submitting, always use `event: "COMMENT"`.
- Never submit `REQUEST_CHANGES` or `APPROVE`, even for Critical findings.
- Reject any generated payload whose event is `REQUEST_CHANGES` or `APPROVE`.
- Always build a **fresh** finding ledger against the PR head. Existing pending drafts are input to reconcile, not the final source of truth.
- Never alter another reviewer’s comments or resolve their threads.
- Never publish against a stale head SHA or a guessed line.
- Do not change application code in this branch.

## Workflow

```text
retrieve → inspect → fresh ledger → reconcile drafts/threads → recheck SHA
  → (no pending) create COMMENT review
  → (pending) delete dropped own drafts → append kept/new → submit COMMENT
  → useful thread replies → verify URLs
```

### 1. Retrieve one coherent snapshot

Use the coherent publish snapshot captured during `SKILL.md` scope prep. If this execution was entered after prep, resolve the PR from URL, number, or current branch and run the skill-local helper once:

```bash
./scripts/pr-context.sh --publish <pr-url-or-number>
```

Capture at minimum: owner/repo/number/url, title/body, base/head, head SHA, commits, changed files, checks summary, current user login, current user’s pending review + draft comments, unresolved review threads.

Read the PR patch and surrounding code from the **PR head**, not the local dirty tree. Record the head SHA every finding was verified against.
Read `AGENTS.md` from the target repository when present and apply its project-specific guidance.

### 2. Review findings-first

- Apply the `finish` baseline and every additional lens selected from the scoped diff.
- Load repo-specific language/framework skills when `AGENTS.md` routes to them.
- Prioritize correctness, regressions, security/privacy, data integrity, operational behavior, and missing tests.
- Do not post style preferences, speculative concerns, or implementation alternatives without material risk.
- Include CI failures only after proving they are caused by the PR.
- Actively look for 0–2 earned `praise` opportunities (see human touch in `conventional-comments.md`).

### 3. Build a fresh finding ledger

Each candidate records:

| Field      | Values                                                            |
| ---------- | ----------------------------------------------------------------- |
| Severity   | Critical / Important / Nice-to-Have                               |
| Confidence | High / Medium / Low + concrete impact                             |
| Evidence   | path + RIGHT-side diff line on verified head SHA                  |
| Coverage   | new thread / current-user draft / existing human or bot thread    |
| Action     | `publish-inline` / `reply-existing` / `review-body-only` / `drop` |
| Wording    | Conventional Comments body                                        |

Triage:

- Publish Critical and Important findings with high confidence.
- Publish Nice-to-Have only when materially useful and concise.
- Drop decline, soft, optional, speculative, duplicate, and already-answered drafts.
- Prefer a substantive reply on an existing thread over a duplicate inline comment.
- Do not post a bare “agree.” Reply only when adding verified evidence, impact, or a precise remediation distinction.
- Validate every inline comment and every labeled thread reply against the Conventional Comments regex before posting.

### 4. Reconcile the current user’s draft review safely

- Snapshot the pending review ID, body, and every draft comment before mutations.
- Preserve verified draft comments; remove only comments classified `drop`.
- Prefer deleting individual draft comments. Do not delete the whole pending review merely for convenience.
- If GitHub requires deleting/recreating the pending review, reconstruct the complete preserved ledger and verify it before submission.

### 5. Guard against stale publishing

Immediately before the first mutation, fetch the head SHA again:

- If unchanged, publish.
- If changed, stop mutation, refresh the diff, re-anchor affected comments, and revalidate findings.

### 6. Publish one friendly review

Follow [`github-state.md`](github-state.md):

- **No pending review:** create the review and inline comments in one REST request with `event: "COMMENT"`.
- **Pending review exists:** remove discarded own drafts, append verified new comments, set the final body when submitting, submit with `event: "COMMENT"`.
- Post existing-thread replies only after the main review succeeds.
- If a partial write fails, inspect actual GitHub state before retrying to avoid duplicates.

If the user asked only for GitHub-pending drafts, stop after creating/appending PENDING — do not submit.

### 7. Verify and report

- Fetch the submitted review and its inline comments through the API.
- Confirm no pending review remains after publish (unless draft-only was requested).
- Return the review URL and direct URLs for every new inline comment/reply.
- Explicitly state that no code was changed.

## Review body style

- Start with `## Review findings`.
- Short outcome sentence (e.g. “I found a few important issues worth addressing.”).
- Severity-ordered findings with impact — do not paste full inline comments.
- Short “What looks solid” when useful; mention one standout design choice when praise is earned.
- Do not say “Request changes” or “blocking review.”
- Conventional Comments are for inline comments and thread replies only.

End with a short, natural handoff:

- Invite re-request of review when ready: “When this is ready, feel free to re-request my review.”
- If ambiguity/trade-offs would be faster live, offer one low-pressure route with one word (`sync`, `huddle`, or `chat`) — topic-specific, not canned.
- Do not stack “sync/huddle/meet” or invite a meeting when fixes are straightforward.

Example closing:

```text
The read-only support boundary and staged completion flow are thoughtfully separated. When the points above are ready, feel free to re-request my review. If the historical completion semantics would be easier to settle live, ping me for a quick huddle.
```

If no valid findings remain: publish a concise COMMENT summary only when the user explicitly asked to publish; otherwise return “no findings” without creating review noise.

## Completion checklist

- [ ] Fresh ledger verified on recorded head SHA
- [ ] Drafts reconciled (kept / dropped / added)
- [ ] Every posted body matches Conventional Comments
- [ ] Submit event is `COMMENT` only, or explicit draft-only state remains PENDING
- [ ] Review + discussion URLs reported
- [ ] No application code changed

# Quality

Merge-prep execution: audit touched files and neighbors for duplication, boundary violations, and test gaps; plan stacked commits; execute behavior-preserving boy-scout refactors and targeted tests; verify merge readiness via repo-native gates.

Every pass answers: **what got worse or duplicated while shipping, and what tests prove it still works?**

This execution is **merge-prep**, not feature delivery: post-shipping hardening plus final verification on the current git branch.

**Phase 0 supersedes scope prep** (`SKILL.md`) — do not run `scripts/compare_default_branch.sh` before Phase 0.

## Phase 0 — Repo bootstrap (read-only)

Run before Phase 1. Output a one-line **gate recipe** for Phase 4.

1. Read `AGENTS.md`, `CONTRIBUTING.md`, `.cursor/rules`, and CI workflow files if present.
2. **Discover quality gates** (first match wins):
   - `Makefile` targets (`lint`, `lintfix`, `test`, `ready`, `check`, `ci`)
   - Package manifests: `package.json`, `Cargo.toml`, `pyproject.toml`, `Rakefile`, `go.mod` scripts
   - CI job commands (`.github/workflows/`, etc.)
3. **Discover test stack**: Jest, Vitest, pytest, `cargo test`, `go test`, etc. — and whether coverage is configured.
4. **Diff base**: `git merge-base HEAD main` (fallback: `master`, then `origin/HEAD`).

Example gate recipes: `pnpm lintfix && pnpm test`, `make lintfix && make ready`, `cargo clippy && cargo test`.

Resolve `<srcRoot>`, `<testRoot>`, and globs from repo layout during this bootstrap.

### Gate discovery examples

| Repo signal                            | Typical gate recipe                         |
| -------------------------------------- | ------------------------------------------- |
| `Makefile` with `ready`                | `make lintfix && make ready`                |
| `package.json` `scripts.lint` + `test` | `pnpm lint && pnpm test`                    |
| Rust + clippy in CI                    | `cargo clippy -- -D warnings && cargo test` |
| Python + ruff                          | `ruff check . && pytest`                    |

Prefer version-manager wrappers (`mise exec`, `nix develop`, `direnv`) only when the project documents them.

## Phase 1 — Audit (read-only)

1. **Establish diff scope**: `git diff --stat <base>..HEAD`.
2. **Inventory touched modules** — group by area (feature folder, package, crate, module).
3. **Verify with grep/read** — no speculation. Check project rules from bootstrap, plus generic smells:
   - **SRP**: files exceeding project threshold (from `AGENTS.md` or default 400 LOC)
   - **DRY**: duplicate handlers, hooks, factories, or cache helpers in the same area
   - **Stability**: callback deps on unstable inline config objects; per-item factories in hot paths
   - **Boundaries**: presentation layer importing service/data layer directly (pattern varies by repo)
   - **Correctness hotspots**: project-documented invariants (auth, money, offline sync, etc.)
4. **Test coverage** (only if tooling exists): list 0%-coverage touched files; note test-file ratio in touched areas.
5. **Prioritize findings**: P0 (correctness/sync), P1 (large untested logic), P2 (DRY/KISS).

**Output**: findings table + recommended commit themes (not implementation yet).

Scope expansion follows the **neighbor escalation gate** below.

### Audit commands

```bash
# Diff base (try main, then master, then origin/HEAD)
BASE="$(git merge-base HEAD main 2>/dev/null || git merge-base HEAD master 2>/dev/null || git merge-base HEAD origin/HEAD)"
git diff --stat "${BASE}"..HEAD

# Largest touched source files (adjust globs to stack)
git diff --name-only "${BASE}"..HEAD -- '<srcRoot>/**/*.ts' '<srcRoot>/**/*.tsx' \
  | xargs wc -l 2>/dev/null | sort -rn | head -20

# Boundary violations (adjust import pattern to repo conventions)
rg "from ['\"].*\\.(service|repository|dao)['\"]" <presentationGlob> --glob '*.{ts,tsx,py,rb}'

# Suppression drift
rg "eslint-disable|# noqa|allow\\(" <srcRoot> --glob '!**/__tests__/**' '!**/test/**'

# Coverage (only when configured — use discovered test command)
# npm/pnpm: npm run test:coverage
# cargo: cargo llvm-cov or tarpaulin per project docs
```

### Generic smell categories

| Category            | What to look for                                                        |
| ------------------- | ----------------------------------------------------------------------- |
| God file            | Single module > project LOC threshold doing orchestration + logic + I/O |
| DRY                 | Third+ copy of same mutation/queue/cache pattern in one area            |
| Stability           | Inline config objects in hook/callback dependency arrays                |
| Layer breach        | UI/views importing service or persistence layer directly                |
| Hot path            | Per-row data fetch, inline object creation in list renderers            |
| Untested pure logic | Helpers with branching and no unit tests                                |

Honor additional invariants documented in `AGENTS.md` (money parsing, auth, offline sync, etc.).

### Neighbor escalation gate

**Default in-scope**: files in `git diff --name-only <base>..HEAD`.

**Auto-neighbor** (no ask): include without escalation when a file:

- Directly imports a touched module
- Duplicates the same pattern in the same area (e.g. third queue cache, fourth mutation hook)
- Is one import hop away as orchestrator (sync coordinator ↔ queue applier)

**Escalate before including**: use AskQuestion or explicit user confirm before expanding scope to:

- Cross-module service decomposition (large facade or god service)
- Files more than two import hops from the diff
- New abstractions not listed in audit findings

Do not expand into unrelated areas.

## Phase 2 — Plan

Use CreatePlan unless user says "just do it".

Plan must include:

- **Commit stack** (dependency order; single commit when diff is small or user says "just do it")
- **Per commit**: files, boy-scout items, targeted tests (name test files)
- **Explicit out of scope** (defer list to prevent creep)
- **Success criteria**: gate recipe green; coverage threshold changes only in a separate final `chore(test)` commit

KISS rules for plans:

- Prefer **extract pure utils → unit test → thin consumer** over new abstractions
- One factory per repeated pattern (mutations, queues, cache helpers)
- No backward-compat shims unless user requires them
- Apply project agent rules and stack-specific skills when bootstrap detects them

### Commit stack ordering

1. Extract pure utils + tests
2. Refactor consumers to use utils
3. Unify factories (mutations, queues, caches)
4. Slim UI/models
5. Test hygiene + gate recipe
6. **`chore(test): raise coverage thresholds + verify`** — last, separate from refactors (skip if no coverage tooling)

### Commit message template

```
refactor(scope): one-line why

Optional body: what test locks the behavior.
```

Types: `refactor`, `test`, `chore` — avoid `feat` in quality loops.

### Test targeting rules

| Layer                                       | Test type                                           |
| ------------------------------------------- | --------------------------------------------------- |
| Pure utils (`buildX`, `applyY`, `resolveZ`) | Unit, no mocks                                      |
| Stateful hooks / use-cases                  | Framework test utils + mocked dependencies          |
| Read/API paths                              | Integration tests with HTTP/DB fixtures per project |
| Screens / pages                             | Mock providers; honor project bans on heavy forms   |

Read `AGENTS.md` and existing test patterns before adding screen or integration tests. Do not mount surfaces the project explicitly bans from golden-path tests.

## Phase 3 — Execute

Default: **stacked commits** on current feature branch when audit yields ≥2 independent themes.

Per commit:

1. Smallest behavior-preserving refactor first (extract utils)
2. Tests that lock behavior before deleting duplication
3. Run lintfix equivalent on touched paths before commit
4. HEREDOC commit message: `type(scope): why` (one sentence why)

**Failure policy**: if gates fail on commit _k_ of _N_, fix forward on the branch. When the fix belongs to an earlier stack commit, use `git commit --fixup=<target-sha>` (user may `git rebase -i --autosquash <base>` later).

**Fixup commits** (mid-stack failure):

```bash
git commit --fixup=<earlier-commit-sha>
# later: git rebase -i --autosquash <base>
```

Otherwise fix forward with a normal commit on the branch.

**Delegation**: for stacks of ≥3 commits, prefer a background `Task` subagent with the full commit list and branch name; parent verifies final gates.

**Coverage thresholds**: raise or adjust coverage config only in a dedicated final `chore(test): …` commit after all refactor commits are green — never bundled with refactor commits. Skip if repo has no coverage tooling.

**Do not**:

- Expand into unrelated areas without escalation gate
- Lower coverage thresholds without user approval
- Mount heavy integration surfaces in tests when project docs ban it (read from `AGENTS.md`)
- Add coverage for dev/static screens unless asked

## Phase 4 — Verify

Run the discovered **gate recipe** from Phase 0.

If coverage thresholds fail: add **targeted** tests on the failing path in a separate `chore(test)` commit — do not lower thresholds without user approval.

## After verify

If P0/P1 findings remain, **re-invoke** this skill on the branch with branch **`quality`**. Do not run a baked-in second pass in the same session.

## Related skills

- `refactor-type-driven` — type/domain refactors; quality may _follow_ that work
- `css-cleaner` — CSS/token DRY only; defer CSS-wide cleanup there
- Stack-specific skills (e.g. React Native perf) — consult when bootstrap detects that stack

# Security

Security and compliance-focused review for changes that may impact confidentiality, integrity, availability, privacy, auditability, or regulatory controls.

After scope prep in `SKILL.md`, continue here.

## Purpose

Review code and configuration changes for security and compliance risk with findings-first output.
Focus on practical risk reduction, auditable evidence, and concise reporting.

Use this lens when a change may affect:

- authentication or authorization boundaries
- tenant isolation or sensitive data handling
- externally reachable APIs, webhooks, or integrations
- privileged operations, auditability, or operational resilience
- regulated workflows or evidence relevant to certification controls

When another project guide exists, follow it in addition to this reference.
If no project-specific guidance exists, this reference is self-sufficient.

## Reusable Review Scope

Run this review when at least one is true:

- the change touches authn, authz, sessions, secrets, or permissions
- the change affects data flow for confidential, regulated, or multi-tenant data
- the change adds or alters an external interface, webhook, background worker, or third-party integration
- the change alters logging, audit trails, export/report behavior, or resilience controls
- the change could plausibly affect compliance posture or incident evidence quality

If none apply, do not force this lens into the review.

## Threat Context

Before analysis, identify:

- actor classes: external user, authenticated user, admin, internal staff, third-party system
- asset sensitivity: public, internal, confidential, regulated
- exposure surface: public API, internal API, background job, admin UI, integration boundary

If context is incomplete, state assumptions explicitly and narrow claims accordingly.

## Review Prep

Build evidence before writing findings:

1. Inspect the diff and the surrounding code.
2. Map changed assets, trust boundaries, and entry points.
3. Check nearby tests, validation logic, policies, and logging behavior when present.
4. Run targeted verification commands when they materially affect confidence.
5. If evidence is missing, lower confidence and say so plainly.

Do not invent exploitability, framework applicability, or verification evidence.

## Method

1. Identify abuse paths using STRIDE-style thinking:
   - spoofing
   - tampering
   - repudiation
   - information disclosure
   - denial of service
   - privilege escalation
2. Evaluate control coverage:
   - least privilege and deny-by-default authorization
   - input validation and safe query construction
   - data minimization and sensitive data redaction
   - auditability and incident forensics readiness
   - resilience boundaries such as retries, timeouts, and rate limits
3. Produce severity-ranked findings with minimal, testable remediation.

## Control Mapping

Map findings to control intent for auditability. This is not legal advice or a certification statement.

Frameworks available for mapping when applicable:

- `ISO 27001`
- `BSI C5`
- `NIS2`
- `DORA`
- `KRITIS`
- `EU MDR`
- `IEC 62304`
- `ISO 13485`

Use concise control tags only when the finding clearly maps to the framework, for example:

- `ISO27001-AccessControl`
- `ISO27001-LoggingMonitoring`
- `BSI-C5-IAM`
- `BSI-C5-DataProtection`
- `NIS2-IncidentHandling`
- `DORA-ICTRisk`
- `KRITIS-Resilience`
- `EU-MDR-Traceability`
- `IEC62304-ChangeControl`
- `ISO13485-DesignControl`

## Applicability Rules

Make an explicit applicability decision per framework.
Only assess a framework when the change touches its control surface and there is enough evidence to say something meaningful.

Default heuristics:

- `ISO 27001` and `BSI C5`: usually applicable for security-relevant backend, infrastructure, and data-handling changes
- `NIS2`, `DORA`, `KRITIS`: applicable when the change affects resilience, incident handling, service dependencies, operational continuity, or critical operations
- `EU MDR`, `IEC 62304`, `ISO 13485`: applicable when the change affects patient safety, clinical logic, software lifecycle controls, medical data integrity, design controls, or regulated traceability

If evidence is insufficient:

- mark `Assessed: No`
- give a short reason
- do not speculate

If the repository context indicates a non-EU regulatory scope, say so explicitly and avoid framework-specific claims beyond the evidence.

## Severity Model

- `Critical`: likely exploit leading to data breach, auth bypass, major integrity loss, or regulated-data compromise
- `Important`: meaningful exploitation path, material control gap, or bounded weakness requiring remediation
- `Nice-to-Have`: hardening or defense-in-depth improvement without a concrete current failure

Severity anchors:

- `Critical`: cross-tenant exposure, auth bypass, secrets leakage, integrity compromise of regulated data
- `Important`: tenant-bound privilege escalation, PII leakage in logs or exports, replayable webhook causing state corruption, ambiguous tenant boundary enforcement

## Output Format

Keep output efficient and precise.
Prefer short findings over exhaustive prose.
Only include sections that add evidence or change the decision.

For each finding, include:

- `Severity`
- `Category`
- `Evidence` (file path + line)
- `Abuse path`
- `Risk`
- `Recommended fix`
- `Control tags` when applicable
- `Confidence`

If no findings:

- State `No Critical or Important findings detected`
- List residual risks or verification gaps in 1 to 3 bullets

Always include a short `Assumptions / Gaps` section when any context, exploitability, or applicability decision depends on incomplete evidence:

- list missing context, skipped verification, or bounded claims
- state `None` when not needed

Always include a short `Compliance coverage summary`:

- one line per relevant framework
- format: `Framework | Assessed: Yes/No | Reason or controls checked`
- include `Not assessed in this review` when a framework is out of scope for the change

Do not produce long control essays.
Do not restate the same evidence in multiple sections.

## Review Lenses

Assess against practical control intent:

- access control and least privilege
- secure development and change management
- logging and monitoring for sensitive operations
- data protection and privacy-by-default
- incident response readiness through traceability and audit evidence

## Optional Project Overlay

If the repository provides project-specific review guidance, incorporate it after the core review.
Useful overlays include:

- framework-specific concerns such as Rails policy patterns
- queue or worker guarantees such as idempotency and retry safety
- webhook signature and replay protections
- tenant-boundary rules for exports, reports, and admin tooling
- local validation commands or review gates

If no overlay exists, do not block the review.

## Compact Output Path

Use compact format only when all are true:

- the change touches `<=2` files
- no authn or authz boundary changes
- no secrets handling or credential flow changes
- no sensitive data-flow or tenant-boundary changes
- no privileged operation, audit trail, or sensitive logging/export changes
- no external integration, webhook, or resilience behavior changes

Compact format still requires:

- threat context
- `No Critical or Important findings detected` if applicable
- `Assumptions / Gaps`
- compliance coverage summary

## Reporting Discipline

- findings first, ordered by severity
- use precise file references
- keep remediation minimal, actionable, and testable
- prefer explicit assumptions over broad claims
- optimize for signal density, not completeness theater

## Operational Guardrails

Before writing findings:

1. Establish the review target explicitly: branch, PR, commit range, or named files.
2. Review the actual diff first, then inspect surrounding code needed to support each claim.
3. Distinguish committed changes from unrelated local workspace edits.
4. Check whether tests, policy code, validation, logging, rate limiting, retry behavior, and configuration changed together.
5. Run targeted verification commands when they materially change confidence, and report when they were not run.

Operational rules:

- Do not edit code unless the user explicitly asks for fixes after the review.
- Do not report exploitability, framework applicability, or control coverage without code evidence.
- If a claim depends on missing deployment, infrastructure, or product context, downgrade confidence and move the gap into `Assumptions / Gaps`.

# Tests

Review changed test files and spec diffs for over-mocking, hidden regressions, contract gaps, assertion weakness, fixture dishonesty, flaky test seams, and brittle architecture.

After scope prep in `SKILL.md`, continue here.

## Workflow

1. Identify the changed test files and the production files they exercise.
2. Read the implementation diffs before judging the tests. Do not assess mocking in isolation from the behavior under test.
3. Classify the role of each test before critiquing it:
   - Characterization test: preserves existing behavior while code is being understood or refactored.
   - Unit test: isolates one decision or transformation but should still assert observable behavior.
   - Boundary or integration test: proves wiring across persistence, HTTP, jobs, CLI, serialization, adapters, or framework glue.
   - Regression test: proves a previously observed bug cannot recur.
4. Check whether each changed test still exercises the public contract, or whether it mostly asserts internal call choreography.
5. Run a coverage-shape pass for risky behavior:
   - Happy path.
   - Error or fallback path.
   - `nil`, empty, malformed, or missing input.
   - Security, authorization, or validation boundary.
   - Cross-component or transport boundary.
   - State transition before and after the action.
6. Distinguish useful isolation from harmful mocking:
   - Useful isolation removes nondeterminism or expensive integration while preserving the contract.
   - Harmful mocking reproduces production behavior inside the test, manually drives private callbacks, or asserts internal sequencing more than outcomes.
7. Prefer findings that connect a weak test seam to a concrete bug risk in the current code.
8. Suggest architectural changes only when they would make tests more behavior-focused, more discriminating, or less duplicative.

## What To Flag

- Tests that manually invoke captured callbacks, private hooks, or internal helper interactions instead of driving the public API.
- Stubs that reimplement real collaborator behavior such as budgeting, retries, parsing, mapping, or state transitions.
- Expectations that assert a method was called without asserting the resulting externally visible behavior.
- New specs that only cover the happy path while implementation changed error handling, fallback logic, or security checks.
- Global stubs on broad services when a narrower fake or injected collaborator would preserve more behavior.
- Snapshot or golden tests that lock in output shape but do not prove semantics.
- Assertion dilution: many coarse assertions, no discriminating assertion that would fail on the likely bug.
- Fixtures or factories that create impossible states, bypass validation, or hide real setup constraints.
- Tests made unrealistically deterministic by freezing time, randomness, ordering, or concurrency without preserving the real contract.
- Tests that became tightly coupled to sequencing or exact implementation structure, making refactors noisy without increasing confidence.

## What To Prefer

- One or two strong end-to-end examples through the public entrypoint for each risky behavior change.
- Small fakes that model collaborator boundaries better than mocks full of `have_received` assertions.
- Assertions on returned values, persisted state, emitted output, serialized payloads, or raised domain errors.
- Tests that cover both the intended path and the most plausible regression path introduced by the change.
- Regression examples that clearly encode the bug trigger, not just the final output.
- Boundary tests that prove adapters and serializers match the real contract at least once.

## When Mocking Is Correct

Mock boundaries that are slow, nondeterministic, hard to trigger, or owned by external systems, such as payment providers, third-party APIs, clocks, random generators, and infrastructure clients.

Do not mock the behavior the application owns unless the test still proves the observable contract through a narrow seam.

## Review Prompts

Ask these questions while reviewing:

- What bug would this test catch?
- What bug introduced in this diff would still pass?
- What production behavior is being recreated inside the spec?
- Is this asserting the result, or only the choreography?
- Is the chosen seam the narrowest public seam available?
- Would a small fake preserve more behavior than this mock setup?

## Review Output

Report findings first, ordered by severity.
When contributing to a generic review, use Critical / Important / Nice-to-Have and fold findings into the single `finish` report.

For each finding, include:

- File and line reference.
- What the test is doing.
- Why that hides a bug or weakens confidence.
- What stronger test shape or architectural seam would improve it.

If no important defects are found, say so explicitly and mention any residual testing gaps.

## Architectural Guidance

Recommend refactors like these when they materially improve test precision:

- Extract policy, validation, parsing, or decision logic into a collaborator that can be exercised with a small fake instead of callback capture.
- Inject fetchers, clocks, clients, random sources, queues, or parsers so tests avoid global service stubs.
- Replace broad doubles with value objects or in-memory fakes where the contract is simple and owned locally.
- Push pure mapping or branching logic into functions that can be tested without transport or framework setup.
- Add contract tests for adapters that are otherwise heavily mocked.
- Move branching logic behind a narrow interface so specs can assert domain behavior instead of transport details.

Do not recommend architecture changes just to reduce mocking stylistically. Tie each suggestion to clearer contracts, fewer duplicated test behaviors, better mutation resistance, or better regression detection.

## Heuristics

- If a test would still pass after deleting the important production branch, it is probably too mocked.
- If the spec duplicates the same algorithm or state transition as production, it is probably asserting the implementation twice.
- If a mock expectation can be replaced with an assertion on the returned result, prefer the result.
- If the test needs many collaborator expectations to prove one outcome, the seam is probably wrong.
- If the fixture state could never happen in production, the test is probably teaching the wrong lesson.
- If a snapshot failure would be hard to interpret, the assertion is probably too coarse.

#!/usr/bin/env bash
set -euo pipefail

if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
  echo "Not inside a git repository." >&2
  exit 1
fi

current_branch=$(git rev-parse --abbrev-ref HEAD)

resolve_base_ref() {
  local branch="$1"

  if git show-ref --verify --quiet "refs/remotes/origin/${branch}"; then
    printf "origin/%s" "${branch}"
  else
    printf "%s" "${branch}"
  fi
}

# Determine default branch (prefer origin/HEAD, then remote show, then main/master)
default_branch=""
if git symbolic-ref --quiet refs/remotes/origin/HEAD >/dev/null 2>&1; then
  default_branch=$(git symbolic-ref --quiet refs/remotes/origin/HEAD | sed 's@^refs/remotes/origin/@@')
elif git remote show origin >/dev/null 2>&1; then
  default_branch=$(git remote show origin | awk '/HEAD branch/ {print $NF; exit}')
fi

if [[ -z "${default_branch}" ]]; then
  if git show-ref --verify --quiet refs/heads/main; then
    default_branch="main"
  elif git show-ref --verify --quiet refs/heads/master; then
    default_branch="master"
  else
    echo "Could not determine default branch (no origin/HEAD, main, or master)." >&2
    exit 1
  fi
fi

base_ref=$(resolve_base_ref "${default_branch}")

printf "Current branch: %s\n" "${current_branch}"
printf "Default branch: %s\n" "${default_branch}"
printf "Base ref: %s\n\n" "${base_ref}"

# Fetch only if needed and origin exists. Silence fetch errors in restricted environments.
if git remote get-url origin >/dev/null 2>&1; then
  git fetch -q origin "${default_branch}" >/dev/null 2>&1 || true
fi

base_ref=$(resolve_base_ref "${default_branch}")

printf "Commits ahead/behind (base...HEAD):\n"
git rev-list --left-right --count "${base_ref}...HEAD"

printf "\nDiffstat (base...HEAD):\n"
git --no-pager diff --stat "${base_ref}...HEAD"

printf "\nChanged files (name-status, base...HEAD):\n"
git --no-pager diff --name-status "${base_ref}...HEAD"

#!/usr/bin/env bash
# Read-only PR snapshot for the review skill.
# Usage: pr-context.sh [--publish] <pr-url-or-number>
# Default output is context-cheap finish metadata. --publish adds complete
# current-user draft and unresolved-thread state.
set -euo pipefail

MODE="finish"
if [[ "${1:-}" == "--publish" ]]; then
  MODE="publish"
  shift
fi

if [[ $# -ne 1 ]]; then
  echo "Usage: $0 [--publish] <pr-url-or-number>" >&2
  exit 2
fi

if ! command -v gh >/dev/null 2>&1; then
  echo "gh is required" >&2
  exit 1
fi

if ! command -v jq >/dev/null 2>&1; then
  echo "jq is required" >&2
  exit 1
fi

INPUT="$1"
OWNER=""
REPO=""
NUMBER=""

if [[ "$INPUT" =~ github\.com/([^/]+)/([^/]+)/pull/([0-9]+) ]]; then
  OWNER="${BASH_REMATCH[1]}"
  REPO="${BASH_REMATCH[2]}"
  NUMBER="${BASH_REMATCH[3]}"
elif [[ "$INPUT" =~ ^[0-9]+$ ]]; then
  NUMBER="$INPUT"
  if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
    echo "Numeric PR requires a git repo with github remote, or pass a full PR URL." >&2
    exit 1
  fi
  remote_url=$(git remote get-url origin 2>/dev/null || true)
  if [[ "$remote_url" =~ github\.com[:/]([^/]+)/([^/.]+)(\.git)?$ ]]; then
    OWNER="${BASH_REMATCH[1]}"
    REPO="${BASH_REMATCH[2]}"
  else
    echo "Could not parse owner/repo from origin remote. Pass a full PR URL." >&2
    exit 1
  fi
else
  echo "Unrecognized PR reference: $INPUT" >&2
  exit 2
fi

pr_json=$(gh pr view "$NUMBER" --repo "${OWNER}/${REPO}" --json \
  number,url,title,body,baseRefName,headRefName,headRefOid,author,commits,files,statusCheckRollup,reviewDecision,state)

if [[ "$MODE" == "finish" ]]; then
  jq -n \
    --arg owner "$OWNER" \
    --arg repo "$REPO" \
    --argjson pr "$pr_json" \
    '{
      owner: $owner,
      repo: $repo,
      pr: {
        number: $pr["number"],
        url: $pr["url"],
        title: $pr["title"],
        body: $pr["body"],
        state: $pr["state"],
        baseRefName: $pr["baseRefName"],
        headRefName: $pr["headRefName"],
        headRefOid: $pr["headRefOid"],
        author: $pr["author"]["login"],
        reviewDecision: $pr["reviewDecision"],
        commits: [($pr["commits"] // [])[]? | .["oid"]],
        files: [($pr["files"] // [])[]? | {path: .["path"], additions: .["additions"], deletions: .["deletions"]}],
        checks: [
          ($pr["statusCheckRollup"] // [])[]?
          | {name: (.["name"] // .["context"] // "unknown"), state: (.["state"] // .["conclusion"] // "unknown")}
        ]
      }
    }'
  exit 0
fi

me=$(gh api user --jq .login)

# shellcheck disable=SC2016 # GraphQL variables must remain literal.
graphql=$(gh api graphql -f query='
query($owner:String!, $repo:String!, $number:Int!) {
  repository(owner:$owner, name:$repo) {
    pullRequest(number:$number) {
      id
      headRefOid
      reviews(first:100, states:[PENDING]) {
        pageInfo { hasNextPage }
        nodes {
          id
          databaseId
          author { login }
          body
          comments(first:100) {
            pageInfo { hasNextPage }
            nodes { id databaseId path line originalLine body }
          }
        }
      }
      reviewThreads(first:100) {
        pageInfo { hasNextPage }
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          comments(first:100) {
            pageInfo { hasNextPage }
            nodes { author { login } body url }
          }
        }
      }
    }
  }
}' -F owner="$OWNER" -F repo="$REPO" -F number="$NUMBER")

if ! jq -e '
  .data.repository.pullRequest as $pr
  | ($pr.reviews.pageInfo.hasNextPage == false)
    and ($pr.reviewThreads.pageInfo.hasNextPage == false)
    and all($pr.reviews.nodes[]?; .comments.pageInfo.hasNextPage == false)
    and all($pr.reviewThreads.nodes[]?; .comments.pageInfo.hasNextPage == false)
' >/dev/null <<<"$graphql"; then
  echo "PR review state exceeds one GraphQL page; refusing an incomplete snapshot." >&2
  exit 1
fi

pr_head=$(jq -r '.headRefOid' <<<"$pr_json")
state_head=$(jq -r '.data.repository.pullRequest.headRefOid' <<<"$graphql")
if [[ "$pr_head" != "$state_head" ]]; then
  echo "PR head changed while capturing review state; retry for a coherent snapshot." >&2
  exit 1
fi

jq -n \
  --arg owner "$OWNER" \
  --arg repo "$REPO" \
  --arg me "$me" \
  --argjson pr "$pr_json" \
  --argjson gql "$graphql" \
  '
  ($gql.data.repository.pullRequest) as $p
  | {
      owner: $owner,
      repo: $repo,
      me: $me,
      pr: {
        number: $pr["number"],
        url: $pr["url"],
        title: $pr["title"],
        body: $pr["body"],
        state: $pr["state"],
        baseRefName: $pr["baseRefName"],
        headRefName: $pr["headRefName"],
        headRefOid: $pr["headRefOid"],
        author: $pr["author"]["login"],
        reviewDecision: $pr["reviewDecision"],
        commits: [($pr["commits"] // [])[]? | .["oid"]],
        files: [($pr["files"] // [])[]? | {path: .["path"], additions: .["additions"], deletions: .["deletions"]}],
        checks: [
          ($pr["statusCheckRollup"] // [])[]?
          | {name: (.["name"] // .["context"] // "unknown"), state: (.["state"] // .["conclusion"] // "unknown")}
        ]
      },
      pending_reviews: [
        ($p["reviews"]["nodes"] // [])[]?
        | select(.["author"]["login"] == $me)
        | {
            id: .["id"],
            databaseId: .["databaseId"],
            body: .["body"],
            comments: [
              (.["comments"]["nodes"] // [])[]?
              | {id: .["id"], databaseId: .["databaseId"], path: .["path"], line: .["line"], originalLine: .["originalLine"], body: .["body"]}
            ]
          }
      ],
      unresolved_threads: [
        ($p["reviewThreads"]["nodes"] // [])[]?
        | select(.["isResolved"] == false)
        | {
            id: .["id"],
            path: .["path"],
            line: .["line"],
            isOutdated: .["isOutdated"],
            comments: [
              (.["comments"]["nodes"] // [])[]?
              | {author: .["author"]["login"], body: .["body"], url: .["url"]}
            ]
          }
      ]
    }
  '

---
name: review
description: >
  Review a local change, branch, or pull request for production readiness, tests,
  performance, security, or merge-prep quality. Use when the user wants a finish
  readiness review, a PR code review, to publish a review to a PR, /code-review,
  or when another skill needs a production-readiness or test-quality pass.
---

# Review

Findings-first analysis of a working tree, branch, commit range, or pull request. A generic review runs the production-readiness baseline plus only the specialized lenses the diff warrants. It never asks the user to choose a review template.

## Choose execution

For a review associated with a pull request, the first step is to ask exactly:

> Publish review on PR?

Skip the question when the user already answered it, explicitly asked for a read-only/draft findings report, or the target has no pull request. The answer selects execution, not review lenses:

| Execution  | Use when                                                                                       |
| ---------- | ---------------------------------------------------------------------------------------------- |
| `findings` | Read-only review; default for non-PR targets or when the user declines publishing              |
| `publish`  | Review a PR end to end, reconcile drafts, and submit a friendly GitHub `COMMENT` review        |
| `quality`  | Explicit merge-prep execution: audit, boy-scout refactors, tests, and repo gates; changes code |

Routing rules:

- “Review and publish/post/ship the review” → `publish`; do not ask again.
- “Post these findings” with an already-verified list → stop and use the `pull-request` skill `comment` branch.
- “Draft review” means read-only findings unless the user explicitly asks for GitHub-pending review comments. End-to-end review drafts use `publish` and stop before submission; a supplied, already-verified ledger uses the `pull-request` skill `comment` branch even when it should remain pending.
- Never infer `quality` from “review.” It requires explicit permission to change code.

## Select review lenses

After scope prep, load references progressively:

| Lens       | Load when                                                                                                              |
| ---------- | ---------------------------------------------------------------------------------------------------------------------- |
| `finish`   | Baseline for every generic findings or publish review                                                                  |
| `tests`    | Tests changed, behavior changed without convincing coverage, or the user explicitly asks for test quality              |
| `perf`     | Ruby code changes a plausibly hot path, query/allocation behavior, or the user explicitly asks for Ruby performance    |
| `security` | Authn/authz, tenancy, sensitive data, external inputs, secrets, privileged operations, resilience, or explicit request |

For an explicitly focused tests, performance, or security review, load only that lens unless another lens is necessary to verify a concrete finding. Never ask “which review?” when the target is known.

Produce one report, not one report per lens. Generic findings use the `finish` output structure; specialized references contribute checks and finding-specific evidence. Fold security assumptions into **Confidence & Uncertainty** and its compliance summary into **Compliance & Risk Posture**. Publish uses the ledger and review-body structure in `publish.md`. A focused review may use its lens-specific output.

## Scope prep

Resolve bundled scripts relative to this installed skill directory.

- **Pull request:** skip local/default-branch comparison. Run `scripts/pr-context.sh <pr-url-or-number>` for findings or `scripts/pr-context.sh --publish <pr-url-or-number>` for publish, fetch the PR patch, and inspect surrounding code at the recorded head SHA.
- **Local branch/change:** identify repo root and default branch, then run `scripts/compare_default_branch.sh`. If unavailable, compare `HEAD` with `origin/<default>` directly.
- **Every target:** read `AGENTS.md` when present, distinguish unrelated dirty changes, and summarize scope plus high-risk areas before selecting lenses.
- **`quality`:** follow its Phase 0 instead of this prep.

## References

- Baseline findings → [`reference/finish.md`](reference/finish.md)
- Tests lens → [`reference/tests.md`](reference/tests.md)
- Ruby performance lens → [`reference/perf.md`](reference/perf.md)
- Security/compliance lens → [`reference/security.md`](reference/security.md)
- Merge-prep execution → [`reference/quality.md`](reference/quality.md)
- PR publish orchestration → [`reference/publish.md`](reference/publish.md); at drafting load [`reference/conventional-comments.md`](reference/conventional-comments.md), and immediately before mutation load [`reference/github-state.md`](reference/github-state.md)

## Handoff

- End-to-end PR review + publish stays in this skill.
- Posting an already-verified ledger continues with the `pull-request` skill `comment` branch.
- Findings execution never posts.

## Completion criteria

| Execution  | Done when                                                                                                                                                               |
| ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `findings` | Every selected lens applied; Critical empty or owned; Important owned/rationale; readiness Yes/No/Conditional; no GitHub writes                                         |
| `quality`  | Audit table produced; commit stack executed (or explicit empty); gates green; P0/P1 fixed or listed for re-invoke                                                       |
| `publish`  | Fresh multi-lens ledger verified on PR head SHA; drafts reconciled; submitted as `COMMENT` or left PENDING when draft-only was explicit; URLs reported; no code changed |