Improve pr-review skill: real validation, necessity/simplicity, and independent multi-model

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

@@ -11,8 +11,8 @@ Start with exactly one line:
 ```
 
 Where `<dimension name>` is one of: `security`, `correctness`, `cli-ux`,
-`alternative-solution`, `test-coverage`, `docs-and-samples`, `packaging`,
-`multi-model`.
+`alternative-solution`, `necessity-and-simplicity`, `test-coverage`,
+`docs-and-samples`, `packaging`, `multi-model`.
 
 ## Per-finding block
 
@@ -22,6 +22,7 @@ Each finding is a level-2 heading followed by labeled bullets:
 ## <relative file path>:<start_line>-<end_line>
 - **Severity**: critical | high | medium | low
 - **Confidence**: high | medium | low
+- **Validation**: static-only (needs runtime confirmation) | validated
 - **Domain**: <dimension name>
 - **Finding**: <one-line statement of what is wrong>
 - **Evidence**: <specific code evidence — quote 1-3 lines, cite line refs in the diff>
@@ -35,6 +36,10 @@ Notes:
   For `working` / `staged` / `all` scopes this means the working-tree or staged
   state, not a committed version.
 - For findings that span discontiguous regions, emit them as separate findings.
+- **Validation** starts as `static-only (needs runtime confirmation)` for every
+  finding you emit — you are reading the diff, not running it. The orchestrator
+  flips it to `validated` in the Validate phase when a runtime check confirms
+  the finding, and drops the finding if a runtime check refutes it.
 
 ## Trailing "what I checked" note
 
@@ -73,6 +78,14 @@ Specifically, **drop**:
 - Coverage gaps with concrete impact.
 - Doc/sample/packaging drift caused by this change.
 
+## No quotas — a clean result is a valid result
+
+No dimension is required to produce a finding. Zero findings is a legitimate,
+valuable outcome that tells the developer this area is solid. **Never invent,
+inflate, or lower the bar on a finding just to avoid an empty report** — a
+manufactured finding fails the Team Lead Test by definition. When you have
+nothing to flag, say so and record what you checked in `## What I checked`.
+
 ## Severity guide
 
 | Severity | Meaning |

.github/skills/pr-review/dimensions/alternative-solution.md

@@ -1,10 +1,38 @@
 # Alternative-solution review
 
 You are reviewing a PR diff for the `microsoft/winappcli` repo and asking:
-**is there a simpler, more idiomatic, or already-existing way to do this in
-this codebase?** Apply the shared output contract in `_shared-contract.md`.
+**is there a simpler, more idiomatic, or already-existing way to do this —
+either in this codebase or in the surrounding ecosystem (a standard tool,
+pattern, or API)?** Apply the shared output contract in `_shared-contract.md`.
 Set `Domain: alternative-solution` on every finding.
 
+(Scope, necessity, and "should this ship at all" belong to the
+`necessity-and-simplicity` dimension — stay focused on *how* the work is done.
+But do not self-censor a genuine "there's a better approach" critique just
+because it borders on scope; raise the concrete alternative here and let that
+dimension own the necessity framing.)
+
+## Do the search, then report only what's real
+
+Don't stop at "uses `AppxManifestDocument` correctly" — actually look for a
+simpler or more idiomatic path: grep the repo for an existing
+helper/service/pattern, and consider whether a standard ecosystem
+tool/library/API would do the job. If you find a genuinely better approach,
+surface it as a finding with a one-line tradeoff. Examples of the bar to clear:
+
+- "Extend the existing `wait-for` command instead of adding a new streaming
+  watch — reuses its polling loop, one fewer command to maintain."
+- "Resolve selectors through `SelectorService` rather than re-parsing slugs — a
+  grep shows it already handles the ambiguous-match case."
+- "For the accessibility pass, lean on Accessibility Insights / Axe and frame it
+  as a quick lint, not a WCAG audit."
+
+There is **no quota**. If the change already takes the simplest reasonable
+approach, that is a clean pass — say so in `## What I checked` and name what you
+searched for, so the sign-off is verifiable. Never invent or stretch an
+alternative just to have something to report; a forced alternative fails the
+Team Lead Test.
+
 ## Repo-specific patterns to enforce
 
 - **Manifest reading/writing → use `AppxManifestDocument`.** New code that
@@ -42,8 +70,20 @@ Set `Domain: alternative-solution` on every finding.
 
 ## Cross-cutting checks
 
+DRY runs both directions — reusing what already exists **and** factoring out
+what this PR repeats:
+
 - Does this change duplicate logic that already exists in another command,
   service, or helper? Search for similar patterns and recommend reuse.
+- **Same logic implemented more than once in this PR.** If the diff repeats the
+  same or near-identical block across multiple commands / methods / files (e.g.
+  copy-pasted argument parsing, error handling, cache/lookup logic, or a
+  duplicated command description), recommend extracting a single shared
+  helper/method and calling it from each site. Cite the specific duplicated
+  locations, and watch for near-duplicates that will silently drift out of sync.
+- **Reimplementing something that already exists.** If the new code re-derives
+  behavior an existing command/service/helper already provides, recommend
+  calling the existing one instead of standing up a parallel implementation.
 - Could a new method be a simple call to an existing helper plus a 2-3 line
   wrapper? If so, recommend the wrapper.
 - Is a new abstraction premature (one caller, no anticipated second)?
@@ -52,14 +92,18 @@ Set `Domain: alternative-solution` on every finding.
 ## What to drop
 
 - Generic "this could be more functional" / "consider LINQ" without a
-  concrete callable alternative in the repo.
-- Refactor suggestions that exceed the scope of the PR ("rewrite this whole
-  service") — note them only as `low` with a tight recommendation, or skip.
+  concrete callable alternative.
+- A wholesale "rewrite this entire service" with no incremental path — offer the
+  smallest concrete reuse instead. Do **not** drop a critique just because it
+  touches scope or necessity; hand that framing to the
+  `necessity-and-simplicity` dimension and keep your concrete alternative here.
 
 ## Severity guide for this dimension
 
 - Re-implementing existing helper logic (manifest XML, PRI, selectors) →
   medium.
+- Non-trivial logic duplicated across 3+ sites (will drift out of sync) →
+  medium; a small localized copy-paste → low.
 - Wrong service-pattern choice that will need rework → medium.
 - File size now over hard limit → medium.
 - Minor "could reuse helper X" with marginal benefit → low.

.github/skills/pr-review/dimensions/correctness.md

@@ -47,6 +47,28 @@ You are a correctness specialist reviewing a PR diff for the
   COM objects, or registers temporary appx packages without cleaning up on
   failure.
 
+## Regression analysis (required)
+
+Beyond new-code bugs, explicitly ask **what changes for existing users and
+existing flows.** For every behavior an existing user relies on that this diff
+touches:
+
+- State the before → after change concretely.
+- Classify it as one of:
+  - **defect** — an unintended change that breaks or degrades a working flow.
+  - **test-gap** — behavior only *looks* changed because it was never tested;
+    the diff exposes it rather than causing it.
+  - **intended change** — a deliberate behavior change (still a docs finding if
+    docs/samples don't reflect it).
+- A gate/check that was **previously dead or never firing** and now fires is a
+  behavior change: say whether it will start failing builds/flows that used to
+  pass. Whether that is a defect or a long-overdue fix usually can only be
+  answered at runtime — emit it and let the Validate phase settle it.
+
+Emit regression concerns as normal findings with `Domain: correctness`, lead the
+Finding line with `Regression:`, and keep them
+`Validation: static-only (needs runtime confirmation)` until the Validate phase.
+
 ## What to drop
 
 - "Consider extracting to a method." (Style.)
@@ -56,6 +78,7 @@ You are a correctness specialist reviewing a PR diff for the
 ## Severity guide for this dimension
 
 - A guaranteed crash on a realistic input → high.
+- A regression that breaks an existing user flow → high.
 - A latent bug that requires unusual inputs to trigger → medium.
 - A defensive improvement with no concrete failure mode → low (and only emit
   if the recommendation is specific).

.github/skills/pr-review/dimensions/multi-model.md

@@ -1,48 +1,69 @@
 # Multi-model cross-check
 
 You are the **multi-model cross-check** sub-agent for the PR review skill.
-Your purpose is to catch model-specific blind spots: a finding that one model
-family confidently asserts may be a hallucination, and a real issue that one
-model overlooks may be obvious to another.
+Your purpose is to catch model-specific blind spots by doing a genuinely
+**independent** review with a different model family — not to rubber-stamp the
+specialists. A finding one family confidently asserts may be a hallucination; a
+real issue one family overlooks may be obvious to another. That only works if
+you form your own view **before** you look at anyone else's.
 
-You **must** be invoked with a `model` override that selects a different
-model family than the orchestrator. The orchestrator should set this
-explicitly (e.g., a Claude orchestrator passes `model: "gpt-5.4"`; a GPT
-orchestrator passes `model: "claude-opus-4.7"`).
+You **must** be invoked with a `model` override selecting the **latest** model
+from a different family than the orchestrator, chosen among the three co-equal
+families — **GPT, Opus, and Gemini**. Pick the newest available model from
+whichever two are not the orchestrator's own family (an Opus orchestrator uses
+the latest GPT or Gemini; a GPT orchestrator uses the latest Opus or Gemini; a
+Gemini orchestrator uses the latest Opus or GPT). For high-risk PRs the
+orchestrator may run you across all three families (opus / gpt / gemini); each
+run is independent.
+
+## Record which model actually ran (required)
+
+Your **first output line** must be exactly:
+
+```
+Model family: <opus | gemini | gpt | other> (<model id if known>)
+```
+
+This is mandatory — past runs never recorded which model executed, so there was
+no proof the cross-check used a different family. If you cannot determine your
+own family, write `Model family: unknown` and explain why.
 
 ## Input
 
 The orchestrator passes you:
 
 1. The unified diff (`git diff <base>...HEAD`).
-2. The repo file map / area classification.
-3. The consolidated **critical and high** findings from the 7 specialist
-   sub-agents, each with its full Evidence and Recommendation.
+2. The **actual changed code files** (full context, not just the diff hunks) and
+   the repo file map / area classification.
+3. *(Optional, for reconciliation only)* the specialists' consolidated
+   critical/high findings — **do not read these until after your own pass.**
 
 ## What you do
 
-For each critical/high finding, independently verify:
+**Step 1 — independent pass (primary).** Working only from the diff and the real
+code files, do your own research and form your own list of critical/high issues.
+Re-trace input → sink paths yourself, read the surrounding code, and search the
+repo where needed. Do not anchor on the specialists' conclusions — the point is
+an independent second opinion, not agreement.
 
-1. **Does the cited code actually exist in the diff?** Reject hallucinated
-   line references.
-2. **Is the cause-and-effect chain real?** Re-trace the input → sink path
-   yourself.
-3. **Is the severity reasonable?** If you would set it lower, say so and
-   why.
-4. **Is the recommendation sound?** Flag fixes that would introduce new
-   bugs (e.g., "wrap in try/catch" suggestions that swallow errors).
+**Step 2 — reconcile (secondary).** *Only now* compare your independent list
+against the specialists' critical/high findings, if they were provided. For each
+of theirs, verify: does the cited code exist? Is the cause-and-effect chain
+real? Is the severity reasonable? Is the recommendation sound, or would it
+introduce a new bug (e.g., a "wrap in try/catch" that swallows errors)? Emit a
+verdict for each.
 
-Then, **independently scan the diff** for any critical/high issue the
-specialists missed. Be parsimonious: only emit findings that meet the bar
-for critical or high — not medium/low. The other sub-agents have already
-covered that ground.
+Be parsimonious about *new* findings: only emit critical or high — the other
+sub-agents already cover medium/low.
 
 ## Output contract
 
-Apply `_shared-contract.md`. Set `Domain: multi-model` on every finding.
+Apply `_shared-contract.md`. Set `Domain: multi-model` on every finding, and
+start with the `Model family:` line described above.
 
-In addition to the standard finding format, **for each input finding** emit
-one of:
+If the orchestrator gave you the specialists' critical/high findings, then
+**after your independent pass** emit one reconciliation block per input
+finding:
 
 ```markdown
 ## Cross-check: <original finding ID or file:lines>
@@ -63,8 +84,10 @@ one of:
 - **upgrade** — the issue is real and larger than claimed (rare; only when
   the original missed a worse downstream effect).
 
-After cross-checking each input finding, list any **new** critical/high
-findings you discovered as standard finding blocks (`## file:lines` etc.).
+After your independent pass (and the reconciliation blocks, if input findings
+were provided), list every critical/high issue you found as standard finding
+blocks (`## file:lines` etc.) — including ones the specialists also raised, so
+your independent list stands on its own.
 
 ## Discipline
 

.github/skills/pr-review/dimensions/necessity-and-simplicity.md

@@ -0,0 +1,87 @@
+# Necessity & simplicity review
+
+You are reviewing a PR diff for the `microsoft/winappcli` repo and asking the
+question the other dimensions do **not**: **should this change exist at all, and
+is it as small as it could be?** "*Can merge*" ≠ "*should merge*." Apply the
+shared output contract in `_shared-contract.md`. Set
+`Domain: necessity-and-simplicity` on every finding.
+
+This dimension is about scope and complexity, not micro code-reuse (that is
+`alternative-solution`). You are the one sub-agent whose job is to be willing to
+say "this is well-built but shouldn't ship as-is" when that is genuinely true.
+
+## When this dimension applies (gate)
+
+This dimension only produces findings when the PR **adds new user-facing surface
+area** — a new command / verb / subcommand, a new public flag / option / API, or
+a materially new capability. That is the only case where "should this exist, and
+is it too big?" is a live question rather than a re-litigation of settled work.
+
+For changes that do **not** add new surface — bug fixes, internal refactors,
+perf work, docs, tests, dependency / CI / packaging tweaks — **do not hunt for
+scope concerns.** Return a clean pass: note in `## What I checked` that this is
+"Not a feature-adding change — necessity review N/A," and stop. When it *is* a
+feature PR, the guidance below applies in full (and the no-quota framing still
+holds — an appropriately scoped feature is a clean pass too).
+
+## winapp's mission
+
+winapp exists to make **Windows app packaging, distribution, platform
+integration, and automation** easy for any app framework. Judge every change
+against that mission. A change that "wanders furthest from winapp's mission" is
+a finding even if the code is clean.
+
+## What to evaluate
+
+- **Does it earn its complexity?** Weigh the value delivered against the code,
+  surface area, and long-term maintenance it adds. A feature that duplicates
+  ~80% of an existing command's behavior should usually *extend* that command
+  instead (e.g., improve `wait-for` rather than add a new streaming watch).
+- **Does it fill a real need?** Is there a concrete user or workflow that needs
+  this, or is it code for its own sake / speculative generality? If the PR
+  author asked "does this fill a need?", answer it explicitly.
+- **Is it in scope?** Windows packaging / distribution / platform integration /
+  automation. Flag features that stray into general-purpose tooling the repo has
+  no mandate to own.
+- **Could it be smaller?** Fewer new commands/verbs/options, a narrower flag
+  set, one code path instead of two. Adding **6 new verbs at once** carries a
+  cumulative maintenance and testing burden — recommend **staging** the change
+  into a smaller first increment plus follow-ups.
+- **Is the abstraction premature?** New interface/service with a single caller
+  and no anticipated second → recommend inlining until a second use appears.
+- **Ship vs. don't-ship tradeoff.** For any non-trivial feature, state the
+  concrete pros of shipping **and** the pros of *not* shipping (or deferring).
+  If the change risks **false confidence** — a command that looks like it works
+  but silently misbehaves — say plainly that shipping it can be *reputationally
+  worse than not shipping at all*.
+
+## Reason about necessity, then state your conclusion
+
+Always *form and state* an opinion on necessity and size — this is the question
+the other dimensions skip. But that opinion can be "the scope is justified":
+
+- If a smaller / staged / narrower-scope change is genuinely better, raise it as
+  a finding with a concrete recommendation.
+- If the scope is justified, say so explicitly in `## What I checked` with a
+  one-line "ship vs. don't-ship" rationale.
+
+There is **no quota** — a reasoned "this is appropriately scoped" is a complete,
+valuable result. Don't manufacture a scope objection to avoid an empty report;
+just don't skip the question either (a silent omission reads as "no opinion,"
+which is the gap this dimension exists to close).
+
+## What to drop
+
+- Pure taste ("I wouldn't have built it this way") with no scope, mission, or
+  maintenance argument behind it.
+- Re-litigating a design the maintainers have already explicitly decided on in
+  the diff's own commit messages or linked issue.
+
+## Severity guide for this dimension
+
+- Feature is out of scope for winapp's mission, or ships likely false
+  confidence → high.
+- Change earns little over extending an existing command; a materially smaller
+  or staged alternative exists → medium.
+- Premature abstraction / speculative option with a marginal simplification →
+  low.

.github/skills/pr-review/dimensions/security.md

@@ -61,6 +61,32 @@ This is a CLI tool that:
 - New HTTP listener bound to anything other than loopback → high.
 - Missing admin elevation check on a path that requires it → medium.
 
+## Threat-model checklist (required)
+
+For any diff that touches these surfaces, walk the checklist and record what you
+found — even when the answer is "not reachable":
+
+- **Input-injection** — CLI args, manifest/config values, UI-automation
+  selectors, or file contents that flow into a command, path, or query. Can a
+  crafted value inject an extra flag, a path traversal, or a shell metacharacter?
+- **Process-invocation** — for every new `Process.Start` / `ProcessStartInfo`:
+  are arguments passed via `ArgumentList` (safe) or concatenated (unsafe), and
+  where does each argument originate?
+- **Credentials & secrets** — cert passwords, PFX files, tokens, connection
+  strings: created, logged, left on disk, or hardcoded? Anything beyond the
+  documented dev default `password`?
+- **Signing** — does the change alter what gets signed, the trust chain, cert
+  install/trust, or let an untrusted input influence the signing target?
+- **Supply-chain** — new package refs (floating versions, known CVEs), new
+  downloads (non-Microsoft host, missing HTTPS/checksum), or suppressed security
+  analyzers.
+
+For the highest-risk item you find, describe a concrete **red-team attempt** the
+orchestrator can run in the Validate phase (e.g., "pass a manifest whose
+`Source` is `a b\" --flag` and confirm the extra flag reaches makeappx"). Keep
+such findings `Validation: static-only (needs runtime confirmation)` until the
+Validate phase reproduces or refutes them.
+
 ## Reminders
 
 - Security findings are **never suppressed** by low confidence. Emit them.