diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md index c3be39e0..354e1cf4 100644 --- a/.github/skills/pr-review/SKILL.md +++ b/.github/skills/pr-review/SKILL.md @@ -1,13 +1,14 @@ --- 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/edge cases, CLI UX, alternative-solution check, test coverage, docs & samples sync, packaging/release impact, and a multi-model cross-check. Reports a consolidated finding list to stdout. Does NOT apply fixes. +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/edge cases, CLI UX, alternative-solution check, necessity & simplicity, test coverage, docs & samples sync, packaging/release impact, and an independent multi-model cross-check, then validates high/critical findings by building and running the CLI the way a user would. Reports a consolidated finding list to stdout. Does NOT apply fixes unless explicitly asked. infer: true --- You are the **PR Review orchestrator** for the `microsoft/winappcli` 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. +in-progress branch before they push, by fanning out parallel sub-agents, +validating the high-risk findings against a real build, and consolidating the +results. ## When to activate @@ -105,8 +106,8 @@ buckets in this repo: | Path prefix | Likely owner | |-------------|--------------| -| `src/winapp-CLI/WinApp.Cli/Commands/` | CLI UX, correctness, security | -| `src/winapp-CLI/WinApp.Cli/Services/` | correctness, alternative-solution, security | +| `src/winapp-CLI/WinApp.Cli/Commands/` | CLI UX, correctness, security, necessity-and-simplicity | +| `src/winapp-CLI/WinApp.Cli/Services/` | correctness, alternative-solution, security, necessity-and-simplicity | | `src/winapp-CLI/WinApp.Cli.Tests/` | test-coverage | | `src/winapp-npm/` | packaging, CLI UX (npm wrapper) | | `src/winapp-NuGet/` | packaging | @@ -118,29 +119,53 @@ buckets in this repo: ### 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/.md` file as instructions. +Launch the specialist dimension sub-agents (#1–#8) 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/.md` file as +instructions. -The 8 dimensions and their fragment files: +**#5 (necessity & simplicity) is conditional.** Launch it **only when the diff +adds new user-facing surface** — a new command / verb / subcommand, a new public +flag / option / API, or a materially new capability. For bug fixes, refactors, +perf, docs, tests, and dependency / CI / packaging changes, **skip it** and mark +it `n/a (no new surface)` in the Coverage block. A non-feature PR fans out 7 +specialists; a feature PR fans out all 8. + +The 9 dimensions and their fragment files — **8 always-on + 1 conditional** +(necessity & simplicity, feature PRs only): | # | Dimension | Fragment | Default agent | |---|-----------|----------|---------------| | 1 | security | `dimensions/security.md` | general-purpose | -| 2 | correctness & edge cases | `dimensions/correctness.md` | general-purpose | +| 2 | correctness & edge cases (incl. regression) | `dimensions/correctness.md` | general-purpose | | 3 | CLI 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 & samples 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 | necessity & simplicity — *conditional, feature PRs only* | `dimensions/necessity-and-simplicity.md` | general-purpose | +| 6 | test coverage | `dimensions/test-coverage.md` | general-purpose | +| 7 | docs & samples sync | `dimensions/docs-and-samples.md` | explore | +| 8 | packaging & release impact | `dimensions/packaging.md` | general-purpose | +| 9 | multi-model cross-check | `dimensions/multi-model.md` | general-purpose, with `model` override | + +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** +than the orchestrator, chosen among the three co-equal families — GPT, Opus, and +Gemini (if you are Opus, use the latest GPT or Gemini; if GPT, use the latest +Opus or Gemini; if Gemini, use the latest Opus or GPT) — and (b) record which +model family actually ran. For high-risk PRs, run it across all three families +(latest Opus, GPT, and Gemini), each independent. + +**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 +blocked tool, keep the analysis). Mark a dimension `✗ skipped + reason` only if +the retry also fails. ### 5. Consolidate @@ -153,13 +178,61 @@ Collect all findings. Then: 2. **Assign IDs.** `C1, C2, ...` for critical, `H1, H2, ...` for high, `M1, ...` for medium, `L1, ...` for low. 3. **Sort.** critical → high → medium → low; within severity, sort by file path. -4. **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. +4. **Reconcile the multi-model pass.** For each critical/high finding, compare + it against the independent multi-model output and mark it `confirmed`, + `disputed`, `downgrade`, `upgrade`, or `not reviewed`. Record which model + family ran. +5. **Seed validation status.** Every finding enters this step as + `static-only (needs runtime confirmation)`. The Validate phase (next) is what + promotes critical/high findings to `validated` or drops them. + +### 6. Validate findings for real + +Static review misses what only shows up at runtime (empty output files, +physical- vs logical-pixel bugs, dead gates that never fired, cold-cache version +drift). Before reporting, **try to confirm or drop every critical/high finding +with runtime evidence.** This phase deliberately replaces the old "no build/test +execution" rule. + +Do as much of this as the environment allows, and **record what you could not +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. +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 + 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. +3. **Scaffold a throwaway app** in a temp dir to exercise the changed commands + against something real (`winapp init`, package, sign, run). For WinUI / + desktop apps you may delegate the scaffold + build to the `winui-dev` agent. +4. **Exercise each changed command** end-to-end. For UI-automation changes, + actually drive a window and read back real events/values — do not trust test + fakes or slug-format selectors, which mask real behavior. +5. **Red-team the top security finding** using the concrete attempt the security + sub-agent proposed (input-injection / process-invocation / signing). +6. **Resolve each critical/high finding:** + - **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, + sample app): keep `Validation: static-only (needs runtime confirmation)` + and state exactly what was needed. This feeds the opt-in test-setup handoff. + +Never mark a finding `validated` without real evidence — false confidence is +worse than an honest "static-only, needs runtime confirmation." + +### 7. Report to stdout + +Print exactly the format below. **Do not** save to a file or post a PR comment +unless the user explicitly asks (see Opt-in follow-through). **Do not** apply +fixes by default — reporting is where the default run ends. The header line varies by scope: @@ -179,10 +252,13 @@ Coverage correctness ... cli-ux ... alternative-solution ... + necessity-simplicity test-coverage ... docs-and-samples ... packaging ... - multi-model <✓ X/Y critical+high confirmed> + multi-model <✓ X/Y critical+high confirmed · models: opus,gpt> + regression <✓ no change to existing flows | ⚠ N regressions> + validation <✓ K/L critical+high validated at runtime | ⚠ static-only: ...> Findings C1 : @@ -194,10 +270,11 @@ Details ## C1 : - Severity: critical - Confidence: high +- Validation: validated | static-only (needs runtime confirmation) - Domain: security - Multi-model: confirmed - Finding: -- Evidence: +- Evidence: - Recommendation: ## C2 ... @@ -209,18 +286,26 @@ Coverage block and include its short "what I checked" note in a final ## 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 (e.g. `cli-schema.json` not - regenerated, npm `winapp-commands.ts` out of sync) but do not run - `scripts/build-cli.ps1` or test suites yourself — they are slow and the - contributor will run them. +- **Parallelism in one turn.** Fan out the specialists (#1–#8, skipping #5 on + non-feature PRs) in a single response; run #9 (multi-model) after they return. +- **Validate high/critical for real.** Build and run the branch to confirm or + drop critical/high findings (see the Validate phase). This **replaces** the + old "no build/test execution" rule. Still flag staleness you can see + statically (e.g. `cli-schema.json` not regenerated, npm `winapp-commands.ts` + out of sync). Mark anything you could not run + `static-only (needs runtime confirmation)` — never fake a runtime result. +- **No fixes by default.** Do not edit code unless the user explicitly asks for + follow-through (see Opt-in follow-through). Reporting is the default endpoint. +- **Stdout by default.** No file output and no posted PR comment unless the user + explicitly asks. +- **Fault-tolerant fan-out.** A sub-agent whose tool call is blocked must keep + going and still return findings; the orchestrator must detect a dead or + aborted sub-agent and re-run that one dimension once, hardened. - **Signal-to-noise.** Reject sub-agent findings that are pure style nits, formatting, or things the compiler / analyzers / linter 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. +- **Cite evidence.** Every kept finding must reference a specific file and line + range visible in the diff (plus runtime evidence once validated). ## Sub-agent prompt template @@ -240,9 +325,12 @@ from these blocks (in order): 6. **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. +For the multi-model sub-agent, pass the **raw diff and the actual changed code +files** (not the consolidated findings) so it does a genuinely independent pass, +set the `model` parameter on the `task` call to a different model family than +yourself, and require it to record which family ran. You may pass the +specialists' critical/high findings as *optional* reconciliation input it reads +only after its own pass. ## Example invocation pattern @@ -250,49 +338,63 @@ parameter on the `task` call to a different model family than yourself. 1. git diff --stat origin/main...HEAD → 12 files, +340/-87 2. git diff origin/main...HEAD → captured for sub-agents 3. Map files to areas → mostly Services + Commands + 1 doc -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 +4. Fan out 7–8 task() calls in parallel → skip #5 necessity if no new surface; retry any that died +5. Fan out task() #9 (diff+code, model override) → wait, record model family +6. Dedupe, sort, ID, reconcile multi-model, seed validation +7. Validate: build + run the published single-file binary (npm/MSIX only if the change needs it) + run changed cmds → confirm/drop crit+high +8. Print stdout report (opt-in follow-through only if asked) ``` -## Example consolidated stdout +## Example consolidated stdout (feature PR) ``` PR Review — feat/sparse-trustedlaunch vs origin/main (4 commits, 9 files, +312/-58) Summary - Critical: 0 High: 2 Medium: 3 Low: 1 + Critical: 0 High: 2 Medium: 4 Low: 1 Coverage security ⚠ 1 finding correctness ⚠ 1 finding cli-ux ⚠ 1 finding alternative-solution ✓ clean + necessity-simplicity ⚠ 1 finding test-coverage ⚠ 1 finding docs-and-samples ⚠ 2 findings packaging ✓ clean - multi-model ✓ 2/2 high confirmed + multi-model ✓ 2/2 high confirmed · models: gpt + regression ✓ no change to existing flows + validation ⚠ 1/2 high validated at runtime; H2 static-only (no signing cert) Findings H1 src/winapp-CLI/.../SparsePackageService.cs:142-160 security Process.Start with manifest-derived path H2 src/winapp-CLI/.../Commands/CreateExternalCatalogCommand.cs:34-49 test-coverage New command has no unit tests M1 src/winapp-CLI/.../SparsePackageService.cs:88-95 correctness Manifest auto-detect skips Package.appxmanifest M2 src/winapp-CLI/.../Commands/CreateExternalCatalogCommand.cs:18-25 cli-ux --catalog-name has no default; breaks scripted use - M3 docs/usage.md (missing) docs-and-samples New command not documented + M3 src/winapp-CLI/.../Commands/CreateExternalCatalogCommand.cs:1-60 necessity-simplicity New top-level verb overlaps `package`; could be a `--external-catalog` flag + M4 docs/usage.md (missing) docs-and-samples New command not documented L1 .github/plugin/agents/winapp.agent.md:189 docs-and-samples Command list out of order Details ## H1 src/winapp-CLI/WinApp.Cli/Services/SparsePackageService.cs:142-160 - Severity: high - Confidence: high +- Validation: validated - Domain: security - Multi-model: confirmed - Finding: Process.Start invokes makeappx.exe with a path read from the input manifest without validation. -- Evidence: Line 148 builds args via $"makeappx.exe pack /d {manifestPath} ..." where manifestPath comes from XElement.Attribute("Source").Value at line 131. A crafted manifest could inject an additional makeappx flag or shell metacharacters via an unquoted path with spaces. +- Evidence: Line 148 builds args via $"makeappx.exe pack /d {manifestPath} ..." where manifestPath comes from XElement.Attribute("Source").Value at line 131. Validate-phase red-team on the npm-installed CLI (cold cache): a manifest with Source='a b" /p x' made the extra /p flag reach makeappx. - Recommendation: Validate manifestPath is a rooted, existing file path under the project root, and pass it via ProcessStartInfo.ArgumentList rather than concatenating into Arguments. -## H2 ... +## H2 src/winapp-CLI/WinApp.Cli/Commands/CreateExternalCatalogCommand.cs:34-49 +- Severity: high +- Confidence: high +- Validation: static-only (needs runtime confirmation) +- Domain: test-coverage +- Multi-model: confirmed +- Finding: New create-external-catalog command has no unit tests. +- Evidence: Command added at lines 34-49; no matching test in WinApp.Cli.Tests. Could not exercise end-to-end — needs a signing cert to run against a scaffolded package (see Test-setup handoff). +- Recommendation: Add a happy-path + error-path unit test; supply a dev cert to enable a runtime smoke test. Coverage notes alternative-solution: Inspected new sparse-package code paths against @@ -300,8 +402,67 @@ Coverage notes packaging: Inspected version.json, npm wrapper, NuGet targets — no impact. ``` +## Example consolidated stdout (non-feature PR — bug fix) + +A bug fix / refactor adds no new command, flag, or capability, so the +orchestrator **skips** necessity & simplicity and marks it `n/a`: + +``` +PR Review — fix/stream-empty-file vs origin/main (2 commits, 3 files, +41/-12) + +Summary + Critical: 1 High: 0 Medium: 1 Low: 0 + +Coverage + security ✓ clean + correctness ⚠ 1 finding + cli-ux ✓ clean + alternative-solution ✓ clean + necessity-simplicity n/a (no new surface) + test-coverage ⚠ 1 finding + docs-and-samples ✓ clean + packaging ✓ clean + multi-model ✓ 1/1 critical confirmed · models: gemini + regression ✓ fixes a regression; no new behavior change + validation ✓ 1/1 critical validated at runtime + +Findings + C1 src/winapp-CLI/.../StreamService.cs:77-84 correctness Watch writes an empty file when the source never flushes + M1 src/winapp-CLI/.../StreamService.cs:77-84 test-coverage No regression test pins the non-empty-output fix + +Details +## C1 src/winapp-CLI/WinApp.Cli/Services/StreamService.cs:77-84 +- Severity: critical +- Confidence: high +- Validation: validated +- Domain: correctness +- Multi-model: confirmed +- Finding: On a cold cache the streaming watch still closes the output file before the first flush, producing a 0-byte file. +- Evidence: Reproduced on the npm-installed CLI (cold cache): `winapp ... --watch` left out.log empty until the fix at line 80 moved the flush ahead of Dispose. +- Recommendation: Keep the fix; add the regression test from M1 so the empty-file case can't silently return. +``` + +## Opt-in follow-through + +Default behavior is unchanged: **stdout only, no fixes, no posting.** Only when +the user explicitly asks, offer these follow-ups: + +- **Post an AI-labelled PR review comment.** On request, post the consolidated + findings as a PR comment that **opens with a clear banner**, e.g. + `> 🤖 AI-generated review (winappcli pr-review skill) — verify before acting.` + Never post silently and never drop the banner. +- **Emit a test-setup / prerequisites handoff.** Turn every `static-only` + finding into a checklist of what a human needs to validate it for real — + signing certs, cloud / Azure credentials, specific hardware, a sample app, + admin rights. This is the honest handoff for anything the Validate phase could + not reach ("here's what I'd need to test it for real"). +- **Open a follow-up fix PR — only on explicit request.** If (and only if) the + user asks you to fix the findings, make the changes on a **new** branch and + open a separate PR; never push fixes onto the branch under review. + ## 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. +The final stdout report is the primary user-visible output — and, unless the +user opted into follow-through, the *only* one. 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. diff --git a/.github/skills/pr-review/dimensions/_shared-contract.md b/.github/skills/pr-review/dimensions/_shared-contract.md index 88b11d10..81df8cc9 100644 --- a/.github/skills/pr-review/dimensions/_shared-contract.md +++ b/.github/skills/pr-review/dimensions/_shared-contract.md @@ -11,8 +11,8 @@ Start with exactly one line: ``` Where `` 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: ## :- - **Severity**: critical | high | medium | low - **Confidence**: high | medium | low +- **Validation**: static-only (needs runtime confirmation) | validated - **Domain**: - **Finding**: - **Evidence**: @@ -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 | diff --git a/.github/skills/pr-review/dimensions/alternative-solution.md b/.github/skills/pr-review/dimensions/alternative-solution.md index ded6fdcf..b6fd45ff 100644 --- a/.github/skills/pr-review/dimensions/alternative-solution.md +++ b/.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. diff --git a/.github/skills/pr-review/dimensions/correctness.md b/.github/skills/pr-review/dimensions/correctness.md index 211ea092..773d647c 100644 --- a/.github/skills/pr-review/dimensions/correctness.md +++ b/.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). diff --git a/.github/skills/pr-review/dimensions/multi-model.md b/.github/skills/pr-review/dimensions/multi-model.md index 3e1577ee..cfefe93e 100644 --- a/.github/skills/pr-review/dimensions/multi-model.md +++ b/.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: () +``` + +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 ...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: @@ -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 diff --git a/.github/skills/pr-review/dimensions/necessity-and-simplicity.md b/.github/skills/pr-review/dimensions/necessity-and-simplicity.md new file mode 100644 index 00000000..8c1b2c17 --- /dev/null +++ b/.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. diff --git a/.github/skills/pr-review/dimensions/security.md b/.github/skills/pr-review/dimensions/security.md index 327b8f54..26ed45e2 100644 --- a/.github/skills/pr-review/dimensions/security.md +++ b/.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.