pr-review

What this skill does

# PR Review

**PR Link/Number**: $ARGUMENTS

> **Instructions**: Execute each task in the order given to conduct a thorough code review. Post the review back to the PR on the resolved primary remote (see "Provider Resolution" below).
> **Important**: The future is now—any improvements or "future" recommendations must be addressed **immediately**.

---

## Provider Resolution

Don't assume GitHub. The PR API to use is determined by the project's `.aiwg/aiwg.config` `remotes.primary` (#994):

```ts
import { readAiwgConfig, resolveRemotes, resolveRemoteProvider } from 'aiwg/config';

const cfg = await readAiwgConfig(projectDir);
const resolved = resolveRemotes(cfg?.remotes);     // primary defaults to "origin"
const url = exec(`git remote get-url ${resolved.primary}`).trim();
const host = resolveRemoteProvider(url);           // 'github' | 'gitea' | 'gitlab' | 'unknown'
```

- `host === 'github'` → `gh pr view`, `gh pr review`, `gh api`
- `host === 'gitea'`  → Gitea MCP `pull_request_*` tools or `tea` CLI
- `host === 'gitlab'` → `glab` CLI
- `host === 'unknown'` → ask the operator which provider to use; don't guess

When the operator passes a full PR URL (`<pr_link_or_number>`), parse the host out of the URL and prefer that — the URL is the operator's explicit choice.

---

## Delivery Policy Resolution (#995)

When approving and merging, consult `.aiwg/aiwg.config` `delivery` via `resolveDelivery()`. The resolved policy controls **how** the merge happens:

| Field | Effect on this skill |
|-------|----------------------|
| `merge_style` (default `rebase-merge`) | Pass through to the PR API on merge. Allowed: `rebase-merge` / `squash` / `merge` / `fast-forward-only` |
| `delete_branch_on_merge` (default `true`) | Pass `delete_branch: true` to the merge call so the feature branch is cleaned up |
| `require_ci_green` (default `true`) | Block approval until CI is green on the PR's head SHA. Don't approve a red PR even if the diff looks fine. |
| `require_signed_commits` (default `false`) | When `true`, reject the PR if any commit on the head ref is unsigned |

When no `delivery` block is configured, `resolveDelivery(undefined)` returns the conservative defaults — same behavior this skill exhibits today.

---

## Arguments

- `<pr_link_or_number>` - PR URL or PR number on the resolved primary remote (required)
- `--interactive` - Prompt for confirmation before posting review
- `--guidance "text"` - Additional review guidance or focus areas
- `--regression-gate` - Run regression check before merge approval (default: true)
- `--allow-regression` - Allow merge even if regressions detected (requires justification)

**Default behavior**: Regression gate is enabled by default. To disable: `--regression-gate=false`

---

## Task 1: Product Manager Review

**Objective**: Assess from a product management perspective, focusing on:
- **Business Value**: Does this PR clearly advance our core product goals and deliver immediate ROI?
- **User Experience**: Is the change intuitive and delightful for users right now? If not, make fixes immediately.
- **Strategic Alignment**: Does the PR align with current (and long-term, i.e., now) strategic objectives?

**Action**: Provide clear directives on how to ensure maximum user and business impact. All "future" suggestions must be implemented now.

---

## Task 2: Developer Review

**Objective**: Evaluate the code thoroughly from a senior lead engineer perspective:
1. **Code Quality & Maintainability**: Is the code structured for readability and easy maintenance? If not, refactor now.
2. **Performance & Scalability**: Will these changes operate efficiently at scale? If not, optimize immediately.
3. **Best Practices & Standards**: Note any deviation from coding standards and correct it now.

**Action**: Leave a concise yet complete review comment, ensuring all improvements happen immediately—no deferrals.

---

## Task 3: Quality Engineer Review

**Objective**: Verify the overall quality, testing strategy, and reliability of the solution:
1. **Test Coverage**: Are there sufficient tests (unit, integration, E2E)? If not, add them now.
2. **Potential Bugs & Edge Cases**: Have all edge cases been considered? If not, address them immediately.
3. **Regression Risk**: Confirm changes don't undermine existing functionality. If risk is identified, mitigate now with additional checks or tests.

**Action**: Provide a detailed QA assessment, insisting any "future" improvements be completed right away.

---

## Task 4: Security Engineer Review

**Objective**: Ensure robust security practices and compliance:
1. **Vulnerabilities**: Check for common security flaws (SQL injection, XSS, authentication bypass, etc.). Fix immediately if found.
2. **Sensitive Data**: Confirm no secrets, credentials, or PII are exposed. Remove immediately if found.
3. **Security Best Practices**: Verify adherence to OWASP guidelines and security standards. Address gaps now.

**Action**: Provide security assessment with immediate remediation for any findings. No deferring security issues.

---

## Task 5: Regression Detection Gate

**Objective**: Detect behavioral regressions introduced by this PR

**When**: Runs automatically before merge approval (unless `--regression-gate=false`)

**Process**:

1. **Identify base branch** from PR metadata
2. **Determine scope** using changed files
3. **Execute regression check**:
   ```bash
   /regression-check \
     --baseline <base-branch> \
     --scope changed-files \
     --format summary
   ```
4. **Analyze results**:
   - **No regressions**: Proceed to merge approval
   - **Minor regressions**: Flag for review, document in comment
   - **Critical regressions**: BLOCK MERGE (unless `--allow-regression`)

**Output**: Regression analysis posted as PR comment

### Regression Gate Behavior

| Regression Severity | Default Action | With `--allow-regression` |
|---------------------|----------------|---------------------------|
| **None** | Approve merge | Approve merge |
| **Minor** (warnings) | Approve with note | Approve with note |
| **Major** (behavior change) | Request changes | Approve with documented risk |
| **Critical** (test failures) | BLOCK merge | Approve with justification required |

### Example: Regression Detected (Blocking)

```markdown
## 🚨 Regression Gate: BLOCKED

**Baseline**: main (commit abc123)
**Changed Files**: 5 files
**Tests Affected**: 12 tests

### Critical Regressions

1. **test/unit/auth/login.test.ts::validateCredentials**
   - Status: PASS → FAIL
   - Error: "Expected 200, received 401"
   - Impact: Breaks user authentication
   - **Action**: FIX REQUIRED BEFORE MERGE

2. **test/integration/api/payments.test.ts::processPayment**
   - Status: PASS → FAIL
   - Error: "Transaction timeout"
   - Impact: Payment processing broken
   - **Action**: FIX REQUIRED BEFORE MERGE

### Verdict

❌ **MERGE BLOCKED** - Critical regressions must be resolved.

To override (not recommended): Use `--allow-regression` and provide justification in PR description.

See full report: [regression-2026-01-25T15-30-00Z.md]
```

### Example: Regression Detected (Warning)

```markdown
## ⚠️ Regression Gate: WARNING

**Baseline**: main (commit abc123)
**Changed Files**: 3 files
**Tests Affected**: 8 tests

### Behavior Changes Detected

1. **test/integration/api/users.test.ts::createUser**
   - Performance: 150ms → 450ms (+200%)
   - Severity: MAJOR
   - **Action**: INVESTIGATE before merge

### Verdict

⚠️ **MERGE WITH CAUTION** - Behavior changes detected but not blocking.

Recommendation: Review performance regression and document if intentional.

See full report: [regression-2026-01-25T15-30-00Z.md]
```

### Example: No Regressions

```markdown
## ✅ Regression Gate: PASSED

**Baseline**: main (commit abc123)
**Changed Files**: 2 files
**Tests Affected**: 5 tests
**Result**: All tests passing, no behavioral changes detected

### Verdict

✅ **REGRESSION CHECK PASSED** - Safe to merge from regression perspective.
```

**In