pr-review

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 job list:

• 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 scope.)

Special cases: When any of the following conditions is met, skip this step and continue directly:

• statusCheckRollup is empty (CI has never been triggered)
• statusCheckRollup is non-empty, but none of the required jobs are in the list (indicating that the pr-checks.yml workflow was not triggered at all, such as for a PR that only changes 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 passed(all required jobs satisfy status == COMPLETED && conclusion == SUCCESS，and statusCheckRollup there is nonon-informational job whose conclusion is FAILURE or CANCELLED；codecov/* failure does not affect this judgment)

Continue directly to subsequent steps without prompting.

Case 2 — Some are still running(there exists a status whose QUEUED is IN_PROGRESS orrequired job; non-required jobs still running do not affect this judgment)

Display a warning and ask:

⏳ The following CI jobs have not yet completed: [job list] PR CI has not fully completed; it is recommended to wait before reviewing. Do you still want to continue? (yes/no)

• User selects no → terminate

• User selects yes → continue to subsequent 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（statusCheckRollup there exists inany non-informational job whose conclusion is FAILURE or CANCELLED, not limited to the required list;codecov/* is always excluded)

Display a warning and ask:

❌ The following CI jobs did not pass: [job list and conclusions] PR CI has failures; the review conclusion may be inaccurate. Do you still want to continue? (yes/no)

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

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

  Whether to post a comment on PR #<PR_NUMBER> reminding the author to fix the failed CI jobs? (yes/no)

  • User selects yes → post a CI failure reminder comment (format see the "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 fails and the user chooses not to continue the review but chooses to post a reminder, the comment format:

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

## CI 检查未通过

以下 job 在本次 review 时未通过，请修复：

| Job | 结论 |
|-----|------|
| <失败的 job 名称> | ❌ <FAILURE 或 CANCELLED> |

本次 code review 暂缓，待 CI 全部通过后将重新执行。"

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

Report Enhancement

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

> ⚠️ **CI 状态警告**：以下 job 在 review 时未通过：`<job 名称>`（<结论>）。本报告结论仅供参考，建议修复 CI 后重新 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/aionui-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 Solution reasonableness 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 Chinese.

Review dimensions:

• Solution reasonableness — whether the overall solution correctly solves the problem; whether it introduces unnecessary complexity; whether it is consistent with the project's existing architecture and patterns; whether there is a simpler/more elegant implementation path; whether the solution itself has known defects or design blind spots. Specific evaluation points: whether the solution truly solves the problem described in the PR (rather than solving a different problem); whether it bypasses ready-made mechanisms provided by the framework/library (reinventing the wheel); whether it is consistent with src/process/、src/renderer/, consistent with architectural boundaries such as the IPC bridge; whether unnecessary abstraction layers or over-engineering have been introduced; whether the solution has known edge cases or race conditions that were not considered at the design level
• Correctness — whether the logic is correct and whether edge conditions are handled
• Security — injection, XSS, key leakage, privilege escalation
• Supply chain security — prevent malicious code injection, with focus on: (1) eval()、new Function()、vm.runInNewContext() and other dynamic code execution; (2) suspicious strings encoded in base64/hex or Unicode escape sequences (common backdoor obfuscation techniques); (3) newly added fetch/axios/http/net and other network requests, especially those pointing to external domains or dynamically concatenated URLs (data exfiltration risk); (4) unconventional reading or external transmission of sensitive variables in process.env ; (5) modifying build scripts, postinstall hooks, or implanting additional commands in CI configuration. Mark the above patterns as CRITICAL
• Immutability — whether there is direct mutation of objects/arrays (a key principle of this project)
• Error handling — whether exceptions are silently swallowed and whether error messages are reasonable
• Performance — unnecessary re-renders, large loops, blocking calls
• Code quality — function length, nesting depth, naming clarity
• Leftover console.log — whether debug logs remain in production code
• Database changes — if the PR involves migration files or database schema: (1) whether the migration is correct (field types, constraints, indexes, default values, rollback capability); (2) whether the changes are reasonable and consistent with the PR goal; (3) whether there is a risk of data loss for existing data; (4) whether the migration order and dependencies are correct. Mark incorrect migrations as CRITICAL.
• IPC bridge / preload — if the PR involves src/preload.ts or IPC channel definitions: (1) whether unnecessary Node.js APIs are exposed to the renderer; (2) whether all exposed APIs have input validation; (3) whether the renderer can trigger privileged operations without authorization. Mark exposed insecure APIs as CRITICAL.
• Electron security configuration — if the PR involves electron-builder.yml、entitlements.plist or electron.vite.config.ts Electron configuration: (1) whether sandbox/nodeIntegration/contextIsolation settings have been weakened; (2) whether entitlements grant excessive permissions; (3) whether signing and notarization have been broken. Mark security regressions as CRITICAL.
• Tests — evaluate against the standards of testing skill , and point out any of the following:
  • New functionality has no corresponding test cases
  • Logic was modified but existing related tests were not updated
  • Newly added source files are unexpectedly excluded by vitest.config.ts 's coverage.exclude (i.e., they should count toward coverage but are incorrectly excluded)
  • Existing tests do not comply with the quality rules in testing skill Step 2
  • codecov/patch CI check shows FAILURE (patch coverage below 50%): although codecov.yml sets this check to informational: true(non-blocking for merge), insufficient coverage indicates that the new code added by this change lacks tests and should be pointed out in the review (severity LOW, for the author's reference)
• Testability — whether the changed code can still be tested independently; whether dependencies can be mocked; whether it remains decoupled from existing modules; whether unit tests can run without relying on a complete runtime environment. When coupling is found, distinguish its source:
  • Coupling newly introduced by this change — grade by impact (new functionality should be decoupled from the design stage; list as HIGH; if it prevents tests from running, list as CRITICAL)
  • Pre-existing historical coupling — not a blocker for this PR; recommend tracking it in a separate issue

Only report real existing problems. If there is no problem with the code in a given dimension, just skip it; do not pad with problems just to show that "a careful review is being done". Base it on the actual code: report what exists, and if nothing exists, honestly say the code is clean. The same applies to the solution reasonableness dimension — if the solution itself has no problems, honestly write "the solution is reasonable"; do not nitpick deliberately just to show "depth".

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 标题> (#<PR_NUMBER>)

### 变更概述

[2–3 句话说明这个 PR 改了什么，影响了哪些模块。]

---

### 方案评估

**结论**：✅ 方案合理 / ⚠️ 方案有缺陷 / ❌ 方案根本错误

[2–4 句话说明：方案是否正确解决了目标问题；是否与项目架构一致；有无更优雅的替代方案（如有，简述思路）；方案层面有无设计盲点。]

---

### 问题清单

#### 🔴 CRITICAL — <问题标题>

**文件**：`path/to/file.ts`，第 N 行

**问题代码**：

```ts
// 有问题的代码
```

Problem description: [explain why it is a problem]

Fix suggestion：

// 修复后的代码

---

🟠 HIGH — <problem title>

(same format as above)

---

🟡 MEDIUM — <problem title>

(same format as above)

---

🔵 LOW — <problem title>

(same format as above)

---

Summary

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

Conclusion

[Choose one of the following three:]

• ✅ Approve merge — no blocking problems
• ⚠️ Conditional approval — minor problems exist; can merge after they are addressed
• ❌ Changes required — blocking problems exist and must be resolved first

[one-sentence explanation]

---

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

---

If no issues are found across all dimensions, output:

> ✅ 未发现明显问题，代码质量良好，建议批准合并。

### 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 完成。是否将此报告发布为 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'

1. 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>"

1. 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 conclusion	CONCLUSION
None / LOW only	✅ Approve merge	APPROVED
MEDIUM	⚠️ Conditional approval	CONDITIONAL
HIGH	⚠️ Conditional approval	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 "Conditional approval". 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.