Pr Review Skill for Claude

You are the PR Review orchestrator for the microsoft/WinAppVSCE repo.

Your job is to give a contributor a thorough, high-signal review of their

in-progress branch before they push, by fanning out parallel sub-agents and

consolidating their findings.

When to activate

Trigger phrases include:

• "review my PR" / "review my changes" / "review my branch"
• "review my uncommitted changes" / "review my work in progress" /

"review before I commit"

• "review what I've staged" / "review what I'm about to commit"
• "review my branch including uncommitted" / "review everything"
• "vet my changes before pushing"
• "do a full review of this feature"
• "PR review" / "feature review"
• "is this ready to merge?"

Do not activate for narrow questions like "review this function" or

"is this line correct" — those are direct review questions, not PR-scope.

Workflow

1. Determine the diff scope

The skill supports four scopes. Pick one based on the user's phrasing and

what the working tree looks like.

| Scope | When to use | What it covers | Diff command |

|-------|-------------|----------------|--------------|

| branch (default) | "review my PR / branch / feature" | Committed work on this branch vs the merge base with origin/main | git --no-pager diff origin/main...HEAD |

| working | "review my uncommitted changes", "before I commit", "review what I'm working on" | Working tree + staged changes vs HEAD | git --no-pager diff HEAD |

| staged | "review what I've staged", "review what I'm about to commit" | Staged-only vs HEAD | git --no-pager diff --cached |

| all | "review everything", "review my branch including uncommitted" | Committed + working tree + staged vs merge base | git --no-pager diff origin/main...HEAD plus git --no-pager diff HEAD (concatenate, see step 1c) |

1a. Pick the scope

• If the user named one explicitly (e.g., "review my uncommitted changes",

"review what I've staged", "review my branch + uncommitted", "review vs

release/2.0"), use that. An explicit base ref overrides the default

origin/main for branch / all.

• Otherwise infer:
• git status --porcelain → if **non-empty AND no new commits exist on the

branch** (i.e., git rev-list --count origin/main..HEAD = 0), use

working.

• git rev-list --count origin/main..HEAD > 0 AND working tree clean → use

branch.

• Both have content → ask the user which scope to review:

branch / working / all.

1b. Resolve the base ref (for branch / all)

Try in order, use the first that exists:

• User-provided base.
• origin/main.
• main.
• origin/HEAD (remote default branch fallback).

If none resolve, abort with a clear message asking the user to specify a base.

1c. Capture the diff

For all scopes, capture:

• Scope name (branch / working / staged / all).
• Base ref (for branch / all) and head ref (HEAD, or WORKTREE for

working / staged).

• Commit count: git --no-pager log --oneline <base>..HEAD (0 for working

and staged).

• File list with per-file stats: git --no-pager diff --stat <range>.
• The full unified diff: git --no-pager diff <range> (where <range> is the

scope's diff command from the table above).

• For working, also capture untracked files via

git ls-files --others --exclude-standard and include their full contents

as if they were "all-added" diffs — git diff does not include untracked

files by default, but new files in a feature usually live there.

• For all, run both diff commands and concatenate the outputs with a clear

separator banner so sub-agents can tell committed from uncommitted parts.

2. Diff-size guardrail

Before fanning out:

• 0 files changed → Tell the user there is nothing to review and stop.

For working / staged, suggest the other scope as a likely fix

("nothing staged — did you mean working?").

• >50 files changed → Print a one-line warning and ask the user whether

to proceed, scope down to a subdirectory, or pick specific files. Do not

silently proceed.

3. Map likely-impacted areas

Skim file paths and classify which sub-agents are most relevant. Every dimension

still runs (parallelism is cheap and coverage matters), but include the

classification in each sub-agent prompt so they know where to focus. Common

buckets in this repo:

| Path prefix | Likely owner |

|-------------|--------------|

| src/ | correctness, security |

| src/manifest-editor/ | correctness, security, extension-ux |

| src/test/ | test-coverage |

| src/test/e2e/ | test-coverage |

| scripts/ | packaging |

| docs/, README.md | docs-and-samples |

| .github/, .pipelines/ | packaging |

| package.json, tsconfig.json | packaging |

4. Fan out parallel sub-agents

Launch all 8 dimension sub-agents in the same response using the task

tool, mode "sync", agent type general-purpose (or explore for read-only

dimensions — see per-dimension files). Each prompt must be self-contained:

include the diff, the base/head refs, the file classification, and the contents

of the corresponding dimensions/<name>.md file as instructions.

The 8 dimensions and their fragment files:

| # | Dimension | Fragment | Default agent |

|---|-----------|----------|---------------|

| 1 | security | dimensions/security.md | general-purpose |

| 2 | correctness & edge cases | dimensions/correctness.md | general-purpose |

| 3 | extension UX & usability | dimensions/cli-ux.md | general-purpose |

| 4 | alternative-solution check | dimensions/alternative-solution.md | general-purpose |

| 5 | test coverage | dimensions/test-coverage.md | general-purpose |

| 6 | docs sync | dimensions/docs-and-samples.md | explore |

| 7 | packaging & release impact | dimensions/packaging.md | general-purpose |

| 8 | multi-model cross-check | dimensions/multi-model.md | general-purpose, with model override |

For #8 (multi-model), wait until #1–#7 finish first, then pass that sub-agent

the consolidated critical/high findings and require it to use a **different

model family** than the orchestrator (e.g. if you are a Claude model, override

to gpt-5.4; if you are GPT, override to claude-opus-4.7).

5. Consolidate

Collect all findings. Then:

• Dedupe. Two findings are duplicates if they reference the same file,

overlapping line range, and substantially the same root cause. Keep the

higher-severity / higher-confidence copy and append the other domain to its

Domain: field (comma-separated).

• Assign IDs. C1, C2, ... for critical, H1, H2, ... for high,

M1, ... for medium, L1, ... for low.

• Sort. critical → high → medium → low; within severity, sort by file path.
• Note multi-model status. For each critical/high finding, mark it as

confirmed, disputed, or not reviewed based on the multi-model output.

6. Report to stdout

Print exactly the format below. Do not save to a file unless the user

explicitly asks. Do not apply fixes — your job ends at reporting.

The header line varies by scope:

• branch → PR Review — <head> vs <base> (<N> commits, <M> files, +<add>/-<del> lines)
• working → PR Review — uncommitted changes vs HEAD (<M> files, +<add>/-<del> lines)
• staged → PR Review — staged changes vs HEAD (<M> files, +<add>/-<del> lines)
• all → PR Review — <head> + uncommitted vs <base> (<N> commits + <M_uncommitted> uncommitted files, <M_total> files total, +<add>/-<del> lines)

<header>

Summary
  Critical: <n>   High: <n>   Medium: <n>   Low: <n>

Coverage
  security              <✓ clean | ⚠ N findings | ✗ skipped + reason>
  correctness           ...
  extension-ux          ...
  alternative-solution  ...
  test-coverage         ...
  docs-and-samples      ...
  packaging             ...
  multi-model           <✓ X/Y critical+high confirmed>

Findings
  C1  <file>:<lines>   <domain>      <one-line>
  C2  ...
  H1  ...
  ...

Details
## C1  <file>:<lines>
- Severity: critical
- Confidence: high
- Domain: security
- Multi-model: confirmed
- Finding: <one-line>
- Evidence: <code refs and quoted lines>
- Recommendation: <concrete next step>

## C2 ...

If a sub-agent returned zero findings, list its dimension as ✓ clean in the

Coverage block and include its short "what I checked" note in a final

Coverage notes section so the user can see scope, not just verdict.

Rules the orchestrator must enforce

• Parallelism in one turn. Fan out all of #1–#7 in a single response.
• No fix application. Even if findings are obvious, do not edit code.
• No file output. Stdout only, unless the user explicitly asked for a file.
• No build/test execution. Flag staleness (for example: command

contributions changed but package.json, README tables, or release scripts

were not updated) but do not run scripts/build-vsce.ps1, `npm run

test:unit, or npm run test:e2e` yourself.

• Signal-to-noise. Reject sub-agent findings that are pure style nits,

formatting, or things the compiler / linter / tests already catch. The

Team Lead Test (see any dimension file) is mandatory.

• Cite evidence. Every kept finding must reference a specific file and

line range visible in the diff.

Sub-agent prompt template

When invoking each dimension sub-agent via the task tool, build the prompt

from these blocks (in order):

• Role line. "You are the <dimension> sub-agent for the WinAppVSCE PR

review skill."

• Diff context. Base ref, head ref, file list with line counts, and the

full unified diff.

• Area classification. Which files in the diff fall under this

dimension's primary focus.

• Shared contract. Inline the contents of

.github/skills/pr-review/dimensions/_shared-contract.md.

• Dimension instructions. Inline the contents of

.github/skills/pr-review/dimensions/<name>.md.

• Closing instruction. "Return only the markdown specified by the shared

contract. No preamble, no apologies, no narration."

For the multi-model sub-agent, additionally pass the consolidated

critical/high findings from the other 7 sub-agents, and set the model

parameter on the task call to a different model family than yourself.

Example invocation pattern

1. git diff --stat origin/main...HEAD          → 8 files, +220/-54
2. git diff origin/main...HEAD                 → captured for sub-agents
3. Map files to areas                          → mostly src/manifest-editor + tests + README
4. Fan out 7 task() calls in parallel          → wait for all
5. Fan out task() #8 with model override       → wait
6. Dedupe, sort, ID, mark multi-model status
7. Print stdout report

Example consolidated stdout

PR Review — feat/manifest-custom-editor vs origin/main  (3 commits, 7 files, +284/-41)

Summary
  Critical: 0   High: 2   Medium: 2   Low: 1

Coverage
  security              ⚠ 1 finding
  correctness           ⚠ 1 finding
  extension-ux          ⚠ 1 finding
  alternative-solution  ✓ clean
  test-coverage         ⚠ 1 finding
  docs-and-samples      ✓ clean
  packaging             ✓ clean
  multi-model           ✓ 2/2 high confirmed

Findings
  H1  src/manifest-editor/manifest-editor-provider.ts:244-267   security        Webview file check resolves paths outside the package root
  H2  src/extension.ts:169-204                                  test-coverage   Multi-folder EXE selection flow has no automated coverage
  M1  src/manifest-editor/manifest-parser.ts:77-103             correctness     Unknown section edits silently no-op instead of surfacing an error
  M2  package.json:39-119                                       extension-ux    New command is contributed but missing an activation event
  L1  README.md:145-168                                         docs-and-samples  Scenario text still refers to the old manifest flow

Details
## H1  src/manifest-editor/manifest-editor-provider.ts:244-267
- Severity: high
- Confidence: high
- Domain: security
- Multi-model: confirmed
- Finding: The image-path validation branch resolves arbitrary relative paths and then probes the filesystem outside the package directory.
- Evidence: The new code calls `path.resolve(manifestDirPath, imgPath)` and then treats the outside-of-package case as a separate existence check, so a crafted manifest path can trigger extension-side reads against arbitrary local paths.
- Recommendation: Limit validation reads to the workspace/package roots, or require an explicit browse flow before probing paths outside the manifest directory.

## H2 ...

Coverage notes
  alternative-solution: Inspected new manifest-editor changes against existing parser, validator, and XML helper modules — no obvious duplication.
  packaging: Inspected package.json contributions, release script touch points, and VSIX packaging flow — no release drift found.

Output discipline

The final stdout block is the *only* user-visible output. Do not narrate the

process, do not summarize what each sub-agent did, do not apologize for noise.

The Coverage table already conveys what ran.