Improve pr-review skill: real validation, necessity/simplicity, and independent multi-model#613
Draft
nmetulev wants to merge 7 commits into
Draft
Improve pr-review skill: real validation, necessity/simplicity, and independent multi-model#613nmetulev wants to merge 7 commits into
nmetulev wants to merge 7 commits into
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>
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
1 participant
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/.