Improve pr-review skill: real validation, necessity/simplicity, and independent multi-model#613
Merged
Merged
Conversation
…ndependent multi-model
Evidence-driven improvements grounded in 5 real past pr-review sessions:
- New 'Validate findings for real' phase: build, install the CLI as a user
would (npm pack/install, MSIX, global tool — not dotnet run), scaffold a
throwaway app, exercise each changed command, and confirm/drop every
critical/high finding with runtime evidence. Replaces the old
'No build/test execution' rule. Findings are tagged validated vs static-only.
- New necessity-and-simplicity dimension ('can merge' != 'should merge';
earns-its-complexity; fills-a-need; smaller/staged/in-scope) registered as
dimension #5.
- Strengthened alternative-solution: require >=1 concrete named alternative
(in-repo helper or ecosystem tool) with a tradeoff, backed by a quick search;
softened the guidance that suppressed necessity critiques.
- Reworked multi-model: independent pass from raw diff + code (not the
consolidated findings), record the model family that ran, 2-3 families for
high-risk PRs.
- Added regression analysis (correctness) and a threat-model checklist +
red-team (security); tag static-only findings 'needs runtime confirmation'.
- Added multi-PR/multi-target mode and sub-agent fault-tolerance (from the real
'Policy hook failed' incident).
- Added opt-in follow-through (AI-labelled PR comment, test-setup handoff,
follow-up fix PR only on request); default stays stdout-only/no-fix.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A quota (must produce >=1 finding) pressures models to manufacture findings — the exact anti-pattern this skill prevents. Remove the quota from the alternative-solution and necessity-and-simplicity dimensions and add a shared 'No quotas — a clean result is a valid result' rule, while keeping the requirement to do genuine search/reasoning and to surface anything real. - alternative-solution.md: 'Required output: name a concrete alternative' -> 'Do the search, then report only what's real' (no quota; clean pass is valid). - necessity-and-simplicity.md: 'Output requirement' -> 'Reason about necessity, then state your conclusion' (a reasoned 'appropriately scoped' is complete); softened the intro's 'allowed - required - to say' phrasing. - _shared-contract.md: added 'No quotas — a clean result is a valid result' section after the Team Lead Test. Left legitimate code-under-review checks untouched (exit codes, test-coverage 'at least one test', the multi-model model-family record, orchestrator never-abort resilience). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Cross-cutting checks now flag same-logic-implemented-multiple-times-in-this-PR (factor out into one shared helper), not just reuse of code that already exists elsewhere. Grounded in the ui-input-verbs review's copy-paste description leaks across the 5 new verbs. Added a matching severity line. Stays a flag-when-real check, consistent with the no-quota framing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pinned model IDs (gpt-5.4, claude-opus-4.7) go stale. The two multi-model override examples now say 'the latest model from a different family' (GPT / Opus / Gemini). Bare family-name references elsewhere stay. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Make the necessity & simplicity dimension conditional so it does not re-litigate settled decisions on bug fixes, refactors, docs, tests, and CI/packaging changes. It now only runs when the diff adds new user-facing surface (a new command/verb/flag/API/capability); otherwise the orchestrator skips it and marks it n/a (no new surface) in the Coverage block. Adds an up-front gate to the dimension file, wires the conditional fan-out and counts in SKILL.md (8 always-on + 1 conditional), and adds a non-feature worked example alongside the feature one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…family Validate phase now defaults to the published single-file binary (dotnet publish) on a clean PATH, escalating to npm pack or MSIX only when the diff touches the npm wrapper or MSIX packaging/install/identity. Multi-model cross-check now treats GPT, Opus, and Gemini as three co-equal families with balanced pick-from-a-different-family examples (including a Gemini orchestrator), keeping the high-risk run-all-three guidance. No pinned model versions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Over-scoped for a single-PR review skill. The skill reviews one PR/branch/diff per run; a user wanting several can invoke it per PR. Deletes the whole Multi-PR / multi-target section and all its references. The generic dimension-scoped fault-tolerance rule (retry a dead dimension sub-agent once, else mark skipped + reason) is kept, no longer tied to per-PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 1545 passed, 1 skipped out of 1546 tests in 440.4s (-45.9s vs. baseline) Test Coverage❌ 18.1% line coverage, 37.3% branch coverage · ✅ no change vs. baseline CLI Startup Time45ms median (x64, Updated 2026-07-10 21:46:37 UTC · commit |
nmetulev
added a commit
that referenced
this pull request
Jul 10, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nmetulev
added a commit
that referenced
this pull request
Jul 13, 2026
) ## What & why Adds a new **developer skill** under `.github/skills/spec-review/` that reviews a **design or spec _before_ any code is written** — answering *"should we build this, and is the approach right?"* This is the **pre-code companion to the `pr-review` skill**. `pr-review` reviews code that already exists and deliberately avoids the "should this exist" debate; `spec-review` is the opposite — it evaluates a *proposal* and makes the necessity + approach questions its whole point. The motivating need: contributors want to validate a design with multiple sub-agents that use **different model families (Opus / GPT / Gemini)** and, crucially, get their information from their **own research against reality — not by blindly trusting the spec**. That independent-research + multi-model-family emphasis is the heart of this skill. > Scope note: this PR **only adds files under `.github/skills/spec-review/`**. It does **not** touch the `pr-review` skill (owned by #613). The `.github/skills/README.md` index and the top-level README skills blurb are intentionally left for a follow-up to avoid conflicts with #613. ## How it works Given a spec (usually a markdown file path) or a described feature, the orchestrator captures it, maps the **real** codebase areas it touches, establishes model-family diversity, fans out parallel sub-agents, and consolidates into a **decision-oriented recommendation** on stdout: **proceed / proceed-with-changes / reconsider**, plus top risks, the single best alternative, open questions to resolve before implementation, and per-dimension coverage. It does **not** write code or edit the spec. ## Dimensions (kept lean — 6, not a sprawling set) 1. **necessity-and-scope** — should this exist? fits winapp's mission? real need vs speculative generality? could it be smaller / staged? 2. **approach-and-alternatives** — is this the simplest reasonable approach? names concrete in-repo (`AppxManifestDocument`, `ManifestHelper`, existing services) and ecosystem (SDK tools, Windows App SDK APIs) alternatives with tradeoffs. 3. **feasibility-vs-reality** — do the spec's load-bearing **assumptions** actually match how the code / Windows APIs / build work today? The key anti-"trust the spec" dimension; each assumption is independently verified and labelled verified / unverified / false. 4. **risks-unknowns-edge-cases** — what's underspecified, compat/migration concerns, and what needs a prototype/spike before committing. 5. **dx-and-user-impact** — is the proposed CLI UX / API coherent with winapp conventions? breaking changes? comprehensible? 6. **multi-model** — an independent pass from a **different model family** than the orchestrator (latest Opus / GPT / Gemini, **no pinned versions**), doing its own research, disputing weak conclusions, and recording which family ran. ## Reuses pr-review's proven patterns - **Parallel sub-agent fan-out** and a self-contained sub-agent prompt template. - Its **own** adapted `dimensions/_shared-contract.md` (retargeted from "diff" to "spec") keeping the **Team Lead Test** signal-to-noise gate and the **severity/confidence** guides. - The **no-quota principle** — a well-researched *"the approach is sound, proceed"* is a complete, valuable result; findings are never manufactured to have something to say. - The **latest-per-family multi-model** approach — select the newest model in each family at run time rather than pinning version numbers, and record which families ran. ## Files ``` .github/skills/spec-review/ SKILL.md (orchestrator; name: spec-review, infer: true) dimensions/_shared-contract.md dimensions/necessity-and-scope.md dimensions/approach-and-alternatives.md dimensions/feasibility-vs-reality.md dimensions/risks-unknowns-edge-cases.md dimensions/dx-and-user-impact.md dimensions/multi-model.md ``` These are hand-written developer skills (not the shipped `winapp` plugin skills), so there is nothing to auto-generate or build. Opening as **draft** for review. --------- Co-authored-by: Nikola Metulev <711864+nmetulev@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Expands the pr-review skill toward evidence-driven runtime validation, necessity analysis, and independent multi-model review.
Changes:
- Adds runtime validation and regression/security checks.
- Adds conditional necessity-and-simplicity review.
- Revises multi-model orchestration, reporting, resilience, and follow-through.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
SKILL.md |
Updates orchestration, validation, reporting, and follow-through. |
dimensions/security.md |
Adds threat-model and red-team guidance. |
dimensions/necessity-and-simplicity.md |
Introduces feature necessity and scope review. |
dimensions/multi-model.md |
Defines independent cross-model review. |
dimensions/correctness.md |
Adds regression classification. |
dimensions/alternative-solution.md |
Strengthens concrete alternative research. |
dimensions/_shared-contract.md |
Adds validation metadata and no-quota guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+201
to
+204
| 2. **Run the CLI as a *user* would — not dev mode.** Do **not** validate via | ||
| `dotnet run` or a Debug worktree; that hides cold-cache and first-run bugs | ||
| real users hit. **Default:** build the **published single-file binary** | ||
| (`dotnet publish` the CLI) and invoke it directly on a clean PATH — that is |
| do** rather than silently skipping it: | ||
|
|
||
| 1. **Build the branch** (`scripts/build-cli.ps1`, or a targeted `dotnet build`). | ||
| A build failure is itself a critical finding. |
Comment on lines
+205
to
+209
| enough for most changes. **Escalate only when the change/scenario requires | ||
| it:** if the diff touches the **npm wrapper** (`src/winapp-npm`), validate via | ||
| `npm pack` + `npm i -g` the tarball; if it touches **MSIX packaging / install | ||
| / identity**, validate the built **MSIX** or installed global tool. Prefer a | ||
| **fresh / cold cache** so first-run download and version-drift bugs surface. |
Comment on lines
+39
to
+42
| - **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. |
Comment on lines
+219
to
+223
| - **validated** — you reproduced the problem (or confirmed a fix): set | ||
| `Validation: validated` and add the runtime evidence to its Evidence. | ||
| - **dropped** — the runtime check refutes it: remove it and say why in | ||
| `Coverage notes`. | ||
| - **static-only** — you could not run it (missing creds, hardware, admin, |
Comment on lines
+150
to
+154
| For #9 (multi-model), wait until #1–#8 finish first. Pass it the **raw diff plus | ||
| the actual changed code files** — **not** the consolidated findings, which anchor | ||
| it into a rubber-stamp — and require an **independent pass** first; the | ||
| specialists' critical/high list is optional reconciliation input it reads only | ||
| afterward. Require it to (a) use the **latest model from a different family** |
Comment on lines
+161
to
+165
| **Sub-agent fault-tolerance.** Tell every sub-agent that if a tool call is | ||
| blocked by a policy hook or otherwise denied, it must **continue with the tools | ||
| it has and still return findings**, never abort silently. After the fan-out, if | ||
| a sub-agent returned nothing, errored, or died (e.g. a "Policy hook failed" | ||
| abort), **detect it and re-run that one dimension once, hardened** (drop the |
Comment on lines
+72
to
+74
| - **Process-invocation** — for every new `Process.Start` / `ProcessStartInfo`: | ||
| are arguments passed via `ArgumentList` (safe) or concatenated (unsafe), and | ||
| where does each argument originate? |
Comment on lines
+50
to
+51
| - **Is the abstraction premature?** New interface/service with a single caller | ||
| and no anticipated second → recommend inlining until a second use appears. |
Comment on lines
+63
to
+66
| - 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Evidence-driven improvements to the hand-written
pr-reviewskill(
.github/skills/pr-review/). Grounded in 5 real past pr-review runs minedfrom the local session store — in every one the user had to separately ask for
real testing, simplicity/necessity, or a genuine second-model opinion, and the
skill's own rules ("No build/test execution", "no fixes", "stdout only") fought
that intent. Concrete signals these changes address:
fix/pester5-count) — confidence in the missingexit 1onlycame from running Pester 5.7.1, not from reading the diff; it also surfaced a
GH Actions vs Azure DevOps
$ErrorActionPreferencedivergence and a"will this regress?" question a static pass couldn't answer.
completely empty output file mid-run, and the user explicitly asked whether
a new verb "earns its complexity over just improving
wait-for" ("can merge" ≠"should merge"). Turn 1 also hit a "Policy hook failed" sub-agent abort.
zt/549) — the JsProvider cold-cache version drift wasinvisible to unit tests and only showed up for a freshly-installed user; found
via E2E + red-team, not diff review.
This is a DRAFT for review — no behavior in the repo build/tests changes
(the skill is hand-written and not in any auto-generation pipeline).
The 9 changes
build the branch, install/run the CLI as a user would (npm pack+install,
built MSIX, or global tool — explicitly not
dotnet run/dev mode), scaffolda throwaway app (may delegate to
winui-dev), exercise each changed command,then confirm or drop each critical/high finding with runtime evidence and
record what couldn't be validated. Replaces the old "No build/test execution"
rule. Every finding is tagged validated vs static-only.
necessity-and-simplicitydimension (Added C# SDK. #5): does it earn itscomplexity? in scope for winapp's mission? smaller / fewer commands / staged?
pros & cons of shipping vs not. Uses the real "can merge ≠ should merge",
"earns its complexity", and "fills a need" framings. Registered everywhere.
alternative-solution: require ≥1 concrete namedalternative (in-repo helper or ecosystem tool/pattern/API) with a tradeoff,
backed by a quick search — and softened the guidance that suppressed
out-of-scope/necessity critiques.
multi-model: independent pass from the raw diff + actual codefiles (not the consolidated findings, which anchored it into a rubber-stamp),
records which model family ran, and supports 2–3 families (opus/gemini/gpt)
for high-risk PRs.
correctness(defect vs test-gap vs intended change); a threat-modelchecklist + red-team in
security(input-injection / process-invocation /credentials-secrets / signing / supply-chain); static-only findings tagged
"needs runtime confirmation".
fault-tolerance — a blocked/policy tool call must not abort the sub-agent,
and the orchestrator retries a dead one (from the real "Policy hook failed"
incident).
prerequisites handoff checklist, and (only on explicit request) a follow-up
fix PR. Default stays stdout-only / no-fix.
Coverage block gains the new dimension, a regression line, and a
validation line.
examples all reflect 9 dimensions + the Validate phase.
Files
SKILL.md— new Validate phase, multi-PR mode, fault-tolerance, opt-infollow-through, updated rules/report/coverage/examples, renumbered to 9 dims.
dimensions/necessity-and-simplicity.md— new.dimensions/alternative-solution.md,multi-model.md,correctness.md,security.md,_shared-contract.md— updated per above.All changes are confined to
.github/skills/pr-review/.