pr-review

name	pr-review
description	PR Code Review (Local): perform a thorough local code review with full project context. Use when: (1) User asks to review a PR, (2) User says "/pr-review", (3) User wants to review code changes before merging.

PR Code Review (Local)

Perform a thorough local code review with full project context - reads source files directly, no API truncation limits.

Announce at start: "I'm using pr-review skill to review the pull request."

Usage

/pr-review [pr_number]

$ARGUMENTS may contain an optional PR number and/or --automation flag.

• Without --automation: interactive mode (prompts for confirmation, comment, cleanup)
• With --automation: non-interactive mode (auto-post comment, auto-delete branch, output machine-readable result)

---

Steps

Step 1 - Determine PR Number

If $ARGUMENTS is non-empty, use it as the PR number.

Otherwise run:

gh pr view --json number -q .number

If this also fails (not on a PR branch), abort with:

No PR number provided and cannot detect one from the current branch. Usage: /pr-review <pr_number>

Also parse --automation from $ARGUMENTS:

AUTOMATION_MODE=false
if echo "$ARGUMENTS" | grep -q -- '--automation'; then
  AUTOMATION_MODE=true
fi

Step 2 - Check CI Status

gh pr view <PR_NUMBER> --json statusCheckRollup \
  --jq '.statusCheckRollup[] | {name: .name, status: .status, conclusion: .conclusion}'

Required jobs to check:

• Code Quality
• Unit Tests (ubuntu-latest)
• Unit Tests (macos-14)
• Unit Tests (windows-2022)
• Coverage Test
• i18n-check

(build-test is an optional job and is not included in the required check list.)

Special cases: Skip this step and proceed directly when either of the following is true:

• statusCheckRollup is empty (CI was never triggered)
• statusCheckRollup is non-empty, but none of the required jobs appear in the list (indicating the pr-checks.yml workflow was not triggered at all - e.g. a PR that only touches docs/md files)

Parsing logic: Handle three cases:

Informational checks exclusion: codecov/patch and codecov/project are configured as informational: true in codecov.yml - they never block merging and must be excluded from all failure checks below. Treat them as non-existent when evaluating CI status.

Case 1 - All passing (all required jobs satisfy status == COMPLETED && conclusion == SUCCESS, and no non-informational job in statusCheckRollup has a conclusion of FAILURE or CANCELLED; codecov/* failures do not affect this determination)

Proceed to the next steps without prompting.

Case 2 - Some still running (at least one required job has status of QUEUED or IN_PROGRESS; non-required jobs still running do not affect this determination)

Display a warning and ask:

⏳ The following CI jobs are not yet complete: [job list] Not all PR CI jobs have finished - it is recommended to wait before reviewing. Continue anyway? (yes/no)

• User selects no → abort

• User selects yes → continue to next steps

• Automation mode: do not prompt. Output signal and stop:

  <!-- automation-result -->
  CONCLUSION: CI_NOT_READY
  IS_CRITICAL_PATH: false
  CRITICAL_PATH_FILES: (none)
  PR_NUMBER: <PR_NUMBER>
  <!-- /automation-result -->
  

  Then exit.

Case 3 - Failures exist (any non-informational job in statusCheckRollup has a conclusion of FAILURE or CANCELLED, not limited to the required job list; codecov/* is always excluded)

Display a warning and ask:

❌ The following CI jobs did not pass: [job list with conclusions] PR CI has failures - review conclusions may be inaccurate. Continue anyway? (yes/no)

• User selects yes → continue, and append a CI status warning at the end of the "Change Summary" section in the final report (see the "Report Enhancement" section for format)

• User selects no → abort the review, then immediately ask:

  Would you like to post a comment on PR #<PR_NUMBER> to notify the author to fix the failing CI jobs? (yes/no)

  • User selects yes → post the CI failure reminder comment (see "CI Failure Reminder Comment" section below), then exit
  • User selects no → exit directly

• Automation mode: do not prompt. Post CI failure comment automatically (same format as "CI Failure Reminder Comment"), then output signal and stop:

  <!-- automation-result -->
  CONCLUSION: CI_FAILED
  IS_CRITICAL_PATH: false
  CRITICAL_PATH_FILES: (none)
  PR_NUMBER: <PR_NUMBER>
  <!-- /automation-result -->
  

  Then exit.

CI Failure Reminder Comment

When CI has failed and the user chooses not to continue the review but opts to post a reminder, use this comment format:

gh pr comment <PR_NUMBER> --body "<!-- pr-review-bot -->

## CI Checks Failed

The following jobs did not pass at the time of this review. Please fix them:

| Job | Conclusion |
|-----|------------|
| <failing job name> | ❌ <FAILURE or CANCELLED> |

Code review is on hold until all CI checks pass, at which point it will be re-run."

(List only the jobs that actually failed; skip any that passed.)

Report Enhancement

When CI has failures but the user chooses to continue, append the following at the end of the "Change Summary" section in the final report:

> ⚠️ **CI Status Warning**: The following jobs did not pass at review time: `<job name>` (<conclusion>). This report's conclusions are for reference only - it is recommended to fix CI and re-review.

---

Step 3 - Create Worktree

Create an isolated worktree for this PR review. The main repo stays on its current branch.

REPO_ROOT=$(git rev-parse --show-toplevel)
PR_NUMBER=<PR_NUMBER>
WORKTREE_DIR="/tmp/wayland-pr-${PR_NUMBER}"

# Clean up any stale worktree from a previous crash
git worktree remove "$WORKTREE_DIR" --force 2>/dev/null || true

# Fetch PR head AND base branch so the three-dot diff is accurate
git fetch origin pull/${PR_NUMBER}/head
BASE_REF=$(gh pr view ${PR_NUMBER} --json baseRefName --jq '.baseRefName')
git fetch origin "$BASE_REF"
git worktree add "$WORKTREE_DIR" FETCH_HEAD --detach

# Symlink node_modules so lint/tsc/test can run in the worktree
ln -s "$REPO_ROOT/node_modules" "$WORKTREE_DIR/node_modules"

Save REPO_ROOT and WORKTREE_DIR for use in subsequent steps. All file reads, lint, and diff commands from this point forward run inside WORKTREE_DIR.

Save the checked-out HEAD info:

cd "$WORKTREE_DIR"
git log --oneline -1

Step 4 - Collect Context (Parallel)

Run the following in parallel:

PR metadata:

gh pr view <PR_NUMBER> --json title,body,author,labels,headRefName,baseRefName,state,createdAt,updatedAt

Full diff (no truncation):

cd "$WORKTREE_DIR"
git diff origin/<baseRefName>...HEAD

Changed file list:

cd "$WORKTREE_DIR"
git diff --name-status origin/<baseRefName>...HEAD

PR discussion comments (excluding bot review comments):

gh pr view <PR_NUMBER> --json comments \
  --jq '[.comments[] | select(.body | startswith("<!-- pr-review-bot -->") | not) | select(.body | startswith("<!-- pr-automation-bot -->") | not) | {author: .author.login, body: .body, createdAt: .createdAt}]'

Save as pr_discussion. Use in Step 7 as supplementary context for the approach soundness evaluation - if participants have explained design decisions or flagged known trade-offs, factor that in. Code is always the authoritative source; comments are context only.

Step 5 - Run Lint on Changed Files

Run oxlint on all changed .ts / .tsx files (skip deleted files):

cd "$WORKTREE_DIR"
bunx oxlint <changed_ts_tsx_files...>

Save the lint output as lint baseline. Use it when reviewing style and code quality in Step 6:

• If a pattern produces no lint warning → it is project-approved; do not flag it as a style issue.
• If a pattern produces a lint warning/error → it is a real violation; report it at the appropriate severity (ERROR → HIGH, WARNING → LOW).
• Do not suggest replacing a lint-clean pattern with an alternative based on general convention alone (e.g. do not suggest spread over Object.assign if no-map-spread is active).

Step 6 - Read Changed File Contents

Use the Read tool to read each changed file from the worktree path ($WORKTREE_DIR/<relative_path>), not from the main repo.

Skip:

• *.lock files
• Images, fonts
• dist/, node_modules/, .cache/
• *.map, *.min.js, *.min.css

Priority order (read highest priority first):

1. src/process/
2. src/process/channels/
3. src/common/
4. src/process/worker/
5. src/renderer/

Also read key interface/type definition files imported by the changed files when they provide important context.

Step 7 - Perform Code Review

Write the code review report in English.

Review dimensions:

• Approach soundness - Does the overall approach correctly solve the problem? Does it introduce unnecessary complexity? Is it consistent with the project's existing architecture and patterns? Is there a simpler or more elegant implementation path? Are there known defects or design blind spots in the approach itself? Specific evaluation points: Does the approach actually solve the problem described in the PR (rather than solving a different problem)? Does it bypass ready-made mechanisms provided by the framework/library (reinventing the wheel)? Is it consistent with architecture boundaries such as src/process/, src/renderer/, and the IPC bridge? Does it introduce unnecessary abstraction layers or over-engineering? Are there known edge cases or race conditions that were not considered at the design level?
• Correctness - Is the logic correct? Are edge cases handled?
• Security - Injection, XSS, key leakage, privilege escalation
• Supply chain security - Guard against malicious code injection; focus on: (1) dynamic code execution via eval(), new Function(), vm.runInNewContext(), etc.; (2) suspicious base64/hex-encoded strings or Unicode escape sequences (common backdoor obfuscation techniques); (3) newly added network requests via fetch/axios/http/net, especially those targeting external domains or dynamically composed URLs (data exfiltration risk); (4) unusual reading or exfiltration of sensitive variables from process.env; (5) modifications to build scripts, postinstall hooks, or CI configuration that inject additional commands. Flag any of these patterns as CRITICAL.
• Immutability - Are objects or arrays being directly mutated? (A key principle of this project)
• Error handling - Are exceptions silently swallowed? Are error messages reasonable?
• Performance - Unnecessary re-renders, large loops, blocking calls
• Code quality - Function length, nesting depth, naming clarity
• Leftover console.log - Are there debug logs remaining in production code?
• Database changes - If the PR touches migration files or database schema: (1) Is the migration correct (field types, constraints, indexes, defaults, rollback safety)? (2) Is the change reasonable and aligned with the PR's goal? (3) Is there a risk of data loss for existing data? (4) Is the migration order and dependency correct? Flag incorrect migrations as CRITICAL.
• IPC bridge / preload - If the PR touches src/preload.ts or IPC channel definitions: (1) Are unnecessary Node.js APIs being exposed to the renderer? (2) Do all exposed APIs have input validation? (3) Can the renderer trigger privileged operations without authorization? Flag exposure of unsafe APIs as CRITICAL.
• Electron security configuration - If the PR touches Electron configuration in electron-builder.yml, entitlements.plist, or electron.vite.config.ts: (1) Are sandbox/nodeIntegration/contextIsolation settings being weakened? (2) Are entitlements over-permissive? (3) Is signing or notarization being broken? Flag security regressions as CRITICAL.
• Tests - Evaluate against the standards of the testing skill. Flag any of the following: (1) New features with no corresponding test cases; (2) Logic changes without updates to existing relevant tests; (3) New source files accidentally excluded by vitest.config.ts's coverage.exclude (i.e. should count toward coverage but are incorrectly excluded); (4) Existing tests that don't meet the quality rules in testing skill Step 2; (5) codecov/patch CI check shows FAILURE (patch coverage below 50%): although codecov.yml sets this check as informational: true (non-blocking), insufficient coverage indicates the new code in this change lacks tests - note this in the review (severity LOW, for the author's reference).
• Testability - Can the changed code still be tested in isolation? Are dependencies mockable? Is it decoupled from existing modules? Can unit tests run without depending on a full runtime environment? When coupling is found, distinguish the source:
  • Coupling newly introduced by this change - rate by impact (new features should be decoupled from the design stage: list as HIGH; causes tests to be unable to run: list as CRITICAL)
  • Pre-existing historical coupling - not a blocker for this PR; recommend opening a separate issue to track

Report only real problems. If a dimension has no issues, skip it - do not manufacture findings to appear thorough. Use the actual code as the authority: report what's there, and honestly say the code is clean when it is. The same applies to approach soundness - if the approach has no problems, write "approach is sound" and move on. Do not nitpick for the sake of appearing rigorous.

For each issue found:

1. Specify file path and line number(s)
2. Quote the problematic code
3. Explain why it is an issue
4. Provide a concrete fix with corrected code

Use the following report template:

---

## Code Review: <PR Title> (#<PR_NUMBER>)

### Change Summary

[2–3 sentences describing what this PR changed and which modules are affected.]

---

### Approach Assessment

**Verdict**: ✅ Approach is sound / ⚠️ Approach has flaws / ❌ Approach is fundamentally wrong

[2–4 sentences covering: does the approach correctly solve the stated problem; is it consistent with the project architecture; is there a more elegant alternative (if so, briefly describe); are there any design blind spots at the approach level.]

---

### Issues

#### 🔴 CRITICAL - <Issue Title>

**File**: `path/to/file.ts`, line N

**Problematic code**:

```ts
// problematic code
```

Explanation: [Why this is a problem]

Suggested fix:

// corrected code

---

🟠 HIGH -

(Same format as above)

---

🟡 MEDIUM -

(Same format as above)

---

🔵 LOW -

(Same format as above)

---

Summary

#	Severity	File	Issue
1	🔴 CRITICAL	file.ts:N	...
2	🟠 HIGH	file.ts:N	...

Conclusion

[Choose one:]

• ✅ Approved - no blocking issues
• ⚠️ Conditionally approved - minor issues; can merge after addressing them
• ❌ Changes required - blocking issues that must be resolved first

[One sentence explaining the rationale]

---

This report was generated by the local pr-review skill with full project context and no truncation limits.

---

If no issues are found across all dimensions, output:

> ✅ No obvious issues found. Code quality is good - recommend approving the merge.

### Step 8 - Ask to Post Comment

Print the complete review report to the terminal.

**Automation mode:** skip the prompt - automatically proceed to post the comment.

**Non-automation mode:** ask the user:
> Review complete. Would you like to post this report as a comment on PR #<PR_NUMBER>? (yes/no)
If the user says **no**, skip posting.

To post:

1. Check for an existing review comment:
```bash
gh pr view <PR_NUMBER> --json comments --jq '.comments[] | select(.body | startswith("<!-- pr-review-bot -->")) | .databaseId'

2. If a previous comment exists, update it:

gh api repos/{owner}/{repo}/issues/comments/<comment_id> -X PATCH -f body="<!-- pr-review-bot -->

<review_report>"

3. If no previous comment exists, create a new one:

gh pr comment <PR_NUMBER> --body "<!-- pr-review-bot -->

<review_report>"

Automation mode only - after posting the comment, output the machine-readable result block:

Map the review conclusion to CONCLUSION value based on the highest severity issue found:

Highest issue severity	Review verdict	CONCLUSION
None / LOW only	✅ Approved	APPROVED
MEDIUM	⚠️ Conditionally approved	CONDITIONAL
HIGH	⚠️ Conditionally approved	CONDITIONAL
CRITICAL	❌ Changes required	REJECTED

Key rule: If all issues are LOW (or there are no issues), emit APPROVED even when the human-facing verdict says "Conditionally approved". pr-fix explicitly skips LOW issues, so triggering a fix session for LOW-only reviews wastes a round with no actionable outcome.

Determine IS_CRITICAL_PATH using the CRITICAL_PATH_PATTERN env var (defined in scripts/pr-automation.conf, passed by daemon at runtime). When a pattern is defined, check and capture matched files:

# CRITICAL_PATH_PATTERN is an env var - set by pr-automation daemon or manually
if [ -n "$CRITICAL_PATH_PATTERN" ]; then
  cd "$WORKTREE_DIR"
  CRITICAL_FILES=$(git diff origin/<baseRefName>...HEAD --name-only | grep -E "$CRITICAL_PATH_PATTERN")
  if [ -n "$CRITICAL_FILES" ]; then
    IS_CRITICAL_PATH=true
  else
    IS_CRITICAL_PATH=false
  fi
else
  IS_CRITICAL_PATH=false
  CRITICAL_FILES=""
fi

Output:

<!-- automation-result -->
CONCLUSION: APPROVED
IS_CRITICAL_PATH: false
CRITICAL_PATH_FILES: (none)
PR_NUMBER: 123
<!-- /automation-result -->

When IS_CRITICAL_PATH is true, list matched files one per line:

<!-- automation-result -->
CONCLUSION: APPROVED
IS_CRITICAL_PATH: true
CRITICAL_PATH_FILES:
- docs/feature/extension-market/agent-hub-requirements.md
- docs/feature/extension-market/research/architecture.md
PR_NUMBER: 456
<!-- /automation-result -->

Step 9 - Cleanup

Remove the worktree. No branch switching needed - the main repo was never touched.

cd "$REPO_ROOT"
git worktree remove "$WORKTREE_DIR" --force 2>/dev/null || true

Both automation and non-automation modes use the same cleanup - no prompt needed since worktree removal has no side effects.