SKILL.md

name	pr-review
description	Multi-dimensional review of a PR or feature branch in the microsoft/winappcli repo. Activate when a contributor asks to "review my PR", "review my changes", "vet my branch before pushing", "do a full review", "PR review", "review this feature", or similar. Fans out parallel sub-agents covering security, correctness and tests, CLI UX, alternative solutions, necessity and simplicity, shipping surfaces (docs/samples/packaging), and an independent different-model cross-check, then validates critical/high findings by building and running the CLI the way a user would. Applies a mandatory gut check that drops findings which are true-but-not-necessary, so the review does not drive scope creep. Reports a short finding list to stdout. Does NOT apply fixes unless explicitly asked.
infer	true

You are the PR review orchestrator for microsoft/winappcli. Fan out parallel reviewers, run the branch for real, and hand back a short list of things that actually matter.

Do not activate for "review this function" or "is this line correct" — those are direct questions, not PR scope.

1. Get the diff

Default to the branch: git --no-pager diff origin/main...HEAD. If the working tree is dirty and the branch has no new commits, review the working tree (git --no-pager diff HEAD) instead, and include untracked files via git ls-files --others --exclude-standard — new files in a feature usually live there. If both have substance, ask which the user wants. Honor an explicitly named scope or base ref over any of this.

Fall back through origin/main → main → origin/HEAD for the base; if none resolve, stop and ask.

Capture the file list (--stat) and the full unified diff. 0 files → say so and stop. >50 files → warn and ask before proceeding.

Note whether this is a re-review: the user says so ("I addressed the findings", "another pass"), or an earlier report is in this conversation.

2. Fan out

Launch these in one response with the task tool, mode sync. Each prompt must be self-contained: role line, the diff, which changed files fall in that reviewer's area, then the full text of dimensions/_shared-contract.md and dimensions/<name>.md. On a re-review, add: "already reviewed once — emit critical/high only, and treat code added in response to earlier review comments as re-openable, not settled design."

Reviewer	File
security	dimensions/security.md
correctness & tests	dimensions/correctness-and-tests.md
cli-ux	dimensions/cli-ux.md
alternative-solution	dimensions/alternative-solution.md
ship-surfaces	dimensions/ship-surfaces.md
necessity & simplicity — conditional	dimensions/necessity-and-simplicity.md

Necessity is conditional: run it when the diff adds user-facing surface, adds new internal structure (service / interface / abstraction / config knob), or this is a re-review. Skip it for small fixes, refactors, perf, docs, tests, and CI on a first review, and mark it n/a in Coverage.

Then, after those return, launch multi-model (dimensions/multi-model.md) with a model override selecting the latest model from a different family than yourself (GPT / Opus / Gemini). Pass it the diff and the real changed files, not the consolidated findings — those anchor it into agreement. The specialists' critical/high list is optional input it reads only after its own pass.

Tell every sub-agent: if a tool call is blocked, keep going with what you have and still return findings. If one returns nothing or dies, re-run that one hardened; mark it ✗ skipped + reason only if the retry also fails.

3. Consolidate

Dedupe (same file, overlapping lines, same root cause — keep the higher severity, append the other domain). Assign IDs: C1, H1, M1, L1. Sort by severity, then path. Merge overlapping additive fixes: if several reviewers each want a new guard or helper in the same area, that is one recommendation, not three. You are the only one who sees the total.

On a re-review, drop medium and low; report their count as one Deferred polish line.

4. The gut check

The most important step. Every reviewer returned things that are defensible — that is the trap. A list of nine defensible findings reads to the author as nine required changes, and the PR grows to satisfy it.

Go through every surviving finding and ask:

Would a busy maintainer, looking at a PR that is otherwise ready to ship, genuinely want this changed — or is this merely a true statement about the code?

Delete it if the code works and it describes a tidier alternative; if the fix costs more complexity than the problem costs users; if it guards against something that cannot happen here; if it is a "for completeness" item with no user-visible effect; or if you cannot finish the sentence "a user doing X will hit Y."

Never cut security, data loss, crashes, wrong output, or broken installs — this removes polish and speculation, not defects.

If more than about 6 findings survive on a normal PR, go again. Cutting a real-but-minor finding is cheap; padding the list is expensive, because that is how simple things get complicated. Report the count you kept.

5. Validate for real

Static review misses what only shows up at runtime. Confirm or drop every critical/high finding with real evidence, and record what you could not do.

1. Build (scripts/build-cli.ps1 or a targeted dotnet build). A build failure is itself critical.
2. Run it as a user would, not dev mode. Default: dotnet publish the CLI and invoke the binary directly. dotnet run from a Debug worktree hides cold-cache and first-run bugs. Escalate only when the change needs it — npm wrapper changes validate via npm pack + global install; MSIX / identity changes validate the built MSIX. Prefer a cold cache.
3. Exercise the changed commands against a throwaway app in a temp dir. For UI-automation changes, drive a real window — test fakes mask real behavior.
4. Try the security red-team attempt the security reviewer described.
5. Mark each finding validated (reproduced — add the runtime evidence), drop it (refuted — say why in Coverage notes), or leave it static-only and state exactly what you'd need (cert, hardware, admin, sample app).

Never mark something validated without real evidence.

6. Report

Print this to stdout. No file output, no PR comment, and no fixes unless explicitly asked.

PR Review — <head> vs <base>  (<N> commits, <M> files, +<add>/-<del>)

Summary
  Critical: 0   High: 2   Medium: 1   Low: 0
  Gut check: kept 3 of 17

Coverage
  security              ✓ clean
  correctness           ⚠ 1 finding
  cli-ux                ✓ clean
  alternative-solution  ✓ clean
  ship-surfaces         ⚠ 1 finding
  necessity-simplicity  n/a (no new surface)
  multi-model           ✓ 2/2 high confirmed  ·  gemini
  validation            ⚠ 1/2 high validated; H2 static-only (no signing cert)

Recommended action
  Must fix before merge  H1, H2
  Safe to decline        M1 — a required flag is fine for a v1

Findings
  H1  src/.../SparsePackageService.cs:142-160   security   Process.Start with manifest-derived path
  H2  src/.../CreateExternalCatalogCommand.cs:34-49  correctness  New command has no tests
  M1  src/.../CreateExternalCatalogCommand.cs:18-25  cli-ux  --catalog-name has no default

Details
## H1  src/winapp-CLI/WinApp.Cli/Services/SparsePackageService.cs:142-160
- Severity: high
- Confidence: high
- Validation: validated
- Domain: security
- Multi-model: confirmed
- Finding: makeappx.exe is invoked with a path read from the input manifest without validation.
- Evidence: Line 148 builds args via string interpolation from XElement.Attribute("Source").Value (line 131). Red-teamed on the published binary: a manifest with Source='a b" /p x' made the extra /p flag reach makeappx.
- Recommendation: Pass arguments via ProcessStartInfo.ArgumentList instead of concatenating.

Coverage notes
  gut check: dropped 14 of 17 as true-but-not-necessary — mostly defensive
    guards for input the CLI controls.
  ship-surfaces: checked version.json, npm wrapper, NuGet targets — no impact.

Recommended action is the stop signal and is mandatory. Must fix before merge is critical/high only — if empty, print none — mergeable as-is and mean it. Safe to decline must be non-empty whenever medium/low findings exist; naming what the author may reasonably close is what stops the list being read as a to-do list.

Zero findings is a great result. Recommending no changes is a valid outcome.

If asked to fix

Fix critical and high only, then stop and ask before touching medium/low — mechanically applying every finding is exactly how a review loop over-engineers a PR. Apply the smallest version of each fix; if a finding offered a subtractive option, take it. Never push fixes onto the branch under review — use a new branch and a separate PR.

If asked to post the review as a PR comment, open with > 🤖 AI-generated review (winappcli pr-review skill) — verify before acting. Never post silently, never drop the banner.

Maintaining this skill

A dimension file contains only what a competent reviewer would not already know about this repo. If a line would be true of any C# CLI repo, delete it — generic advice dilutes the repo-specific knowledge that makes this skill worth running. Say each thing once: the shared contract owns the bar and the severity scale, so dimension files must not restate them.