Add spec-review skill: independent, multi-model design/spec review

.github/skills/README.md

@@ -15,6 +15,7 @@ repo-specific developer tasks like reviewing a PR before push.
 | Skill | Purpose |
 |-------|---------|
 | [`pr-review/`](pr-review/SKILL.md) | Multi-dimensional review of a PR / feature branch diff (security, correctness, CLI UX, alternative solutions, tests, docs/samples, packaging, multi-model cross-check). Reports findings to stdout; does not apply fixes. |
+| [`spec-review/`](spec-review/SKILL.md) | Pre-code, decision-oriented review of a spec or proposed feature against the real codebase (necessity & scope, approach & alternatives, feasibility vs reality, risks/unknowns/edge cases, DX & user impact, multi-model cross-check). Reports a proceed / proceed-with-changes / reconsider recommendation to stdout; does not change code or the spec. |
 
 ## Conventions
 

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

@@ -0,0 +1,164 @@
+# Shared output contract
+
+Every dimension sub-agent must follow this output contract.
+
+This is a **spec / design review**, not a code review. You are evaluating a
+proposal *before* it is built. Your value comes from **independent research
+against reality** — the actual `microsoft/winappcli` codebase, the Windows SDK
+tools, Windows APIs, the build flow, and the wider ecosystem — **not** from
+restating or trusting the spec's own claims. Verify load-bearing statements
+yourself, and for anything *mechanical* **prefer a cheap experiment over a
+code-read**: invoke the real tool and inspect its output, build a throwaway
+project in a temp directory, or test a command's real behavior. You do not
+implement the feature or touch the repo/spec — but running cheap, scoped
+experiments in temp directories to confirm how things actually work is expected.
+
+## Header line
+
+Start with exactly one line:
+
+```
+# <dimension name>: <N> findings
+```
+
+Where `<dimension name>` is one of: `necessity-and-scope`,
+`approach-and-alternatives`, `feasibility-vs-reality`,
+`risks-unknowns-edge-cases`, `dx-and-user-impact`, `multi-model`.
+
+## Bottom line
+
+Immediately after the header, emit exactly one line:
+
+```
+Bottom line: <one-sentence assessment for this dimension>
+```
+
+This is required **even when you have zero findings** — a design review's job is
+to reach a judgment, and "the approach is the simplest reasonable one; no better
+alternative found" is a complete, valuable result. Do not manufacture a finding
+just to fill space (see *No quotas* below).
+
+## Per-finding block
+
+Each finding is a level-2 heading anchored to the **part of the spec** it is
+about, followed by labeled bullets:
+
+```markdown
+## <spec section / heading / quoted claim>
+- **Severity**: critical | high | medium | low
+- **Confidence**: high | medium | low
+- **Domain**: <dimension name>
+- **Finding**: <one-line statement of the concern>
+- **Evidence**: <what your INDEPENDENT research found. Prefer an experiment you
+  ran ("invoked <tool> on a throwaway input and its output was …", "built a
+  throwaway project in a temp dir and observed …"); otherwise cite real files as
+  `path:line`, authoritative vendor docs, repo patterns, or ecosystem facts.
+  Do NOT cite the spec as evidence for itself.>
+- **Recommendation**: <concrete next step — e.g. descope, stage, use existing
+  helper X, prototype Y first, adopt alternative Z, answer question Q>
+```
+
+Notes:
+
+- The anchor identifies the spec claim/section under review (e.g.
+  `§Approach — "shell out to makeappx with --foo"`). Keep it short.
+- **Evidence must come from reality, not the spec**, and for *mechanical* claims
+  (tool output, API/command behavior, file/artifact format, build step) a cheap
+  **experiment you ran** is the strongest evidence — reach for it before a
+  code-read. Evidence hierarchy: **experiment > authoritative vendor docs >
+  code-read > spec assertion (never sufficient alone).** If you could not verify
+  a load-bearing claim, that is itself a finding (mark `Confidence: low` or
+  `medium` and say what you could not confirm) — see `feasibility-vs-reality`.
+- Experiments are expected but must stay **cheap, scoped, and confined to temp
+  directories** — never modify the repo working tree or the spec.
+- Emit discontiguous concerns as separate findings.
+
+## Open questions
+
+After the findings, include a section listing questions the spec does **not**
+answer that must be resolved **before** implementation starts:
+
+```markdown
+## Open questions
+- <a decision or unknown that blocks or materially shapes the build>
+- <e.g., "Does Windows App SDK expose an API for X, or is a P/Invoke required?">
+```
+
+Omit the bullets if there are genuinely none. Do not pad.
+
+## Trailing "what I checked" note
+
+After the open questions, include:
+
+```markdown
+## What I checked
+- <one bullet per area you independently researched, e.g. "Read
+  MsixService.cs pack path to confirm makeappx invocation">
+- <e.g., "Grepped Commands/ for an existing `store` subcommand">
+- <e.g., "Checked Windows App SDK docs for a Share Target API">
+```
+
+This appears in the orchestrator's `Coverage notes` section so the reader can
+see the depth of research behind the verdict — not just the verdict.
+
+## The Team Lead Test (mandatory signal-to-noise gate)
+
+Before emitting a finding, ask: *"Would a senior maintainer of this repo raise
+this in a design review, or wave it off as noise?"* If you would wave it off,
+do not emit it.
+
+Specifically, **drop**:
+
+- Bikeshedding on naming, wording, or formatting of the spec itself.
+- Restatements of what the spec says without an independent judgment.
+- Speculative hypotheticals not grounded in the actual code, APIs, or a real
+  usage scenario.
+- "Consider also supporting X" scope-creep suggestions (the point of this review
+  is usually *less* speculative generality, not more).
+- Concerns that only matter after a decision the spec explicitly defers.
+
+**Keep**:
+
+- The feature not fitting winapp's mission, or duplicating existing capability.
+- Load-bearing assumptions that are refuted, unproven, or hand-wavy.
+- A materially simpler / safer / more idiomatic approach that exists.
+- Real risks: compat/migration breakage, missing edge cases, release blockers.
+- CLI UX / API incoherence users will trip over, or breaking changes.
+- Unanswered questions that genuinely block implementation.
+
+## No quotas — a clean result is a valid result
+
+There is **no expectation of finding problems.** If, after genuine independent
+research, the design is sound on your dimension, say so in the `Bottom line`,
+list what you checked, and emit zero findings. **Never manufacture a concern to
+have something to report.** A confident, well-researched "proceed" is exactly as
+valuable as a well-researched objection. Fabricated or padded findings actively
+harm the review by burying the real signal.
+
+## Severity guide
+
+Severity here measures **how much this should affect the go / no-go decision**,
+not code-level blast radius.
+
+| Severity | Meaning |
+|----------|---------|
+| critical | Fundamental flaw: the feature shouldn't exist as scoped, a core assumption is false, or the approach cannot work. Blocks proceeding until resolved. |
+| high     | Significant concern that should change the design *before* implementation (wrong approach, big risk, breaking change, materially better alternative). |
+| medium   | Worth addressing but not a blocker; can be resolved during implementation, with a note. |
+| low      | Minor suggestion; only emit if concrete and actionable. |
+
+## Confidence guide
+
+Confidence reflects how well your **independent research** grounds the finding,
+following the evidence hierarchy (**experiment > authoritative docs > code-read >
+spec assertion**).
+
+- **high**: Proven empirically (you ran an experiment and observed the result) or
+  verified directly against real code / authoritative vendor docs — you can cite
+  exactly what you saw.
+- **medium**: Partially verified; a code-read or docs plus some inference from
+  repo/ecosystem context, without an experiment to close it.
+- **low**: Plausible concern you could not fully confirm. Say what you could not
+  verify and what experiment would close it. (An unverifiable-but-load-bearing
+  spec assumption is worth surfacing at low/medium confidence rather than
+  dropping.)

.github/skills/spec-review/dimensions/approach-and-alternatives.md

@@ -0,0 +1,79 @@
+# Approach & alternatives review
+
+You are the **approach-and-alternatives** sub-agent for the
+`microsoft/winappcli` spec-review skill. Your question: **is the proposed
+approach sound and the simplest reasonable one — or is there a better path
+already available in this repo or the ecosystem?** Apply the shared output
+contract in `_shared-contract.md`. Set `Domain: approach-and-alternatives` on
+every finding.
+
+**Independent research is mandatory.** Do not restate or grade the spec's
+approach on its own terms. Grep the repo, read the relevant code, and draw on
+Windows / ecosystem knowledge to find concrete alternatives before you conclude.
+
+## What to evaluate
+
+- **Soundness.** Will the proposed approach actually achieve the goal? Are the
+  mechanics coherent end-to-end?
+- **Simplicity.** Is this the simplest approach that works, or is it more complex
+  than the problem warrants (new service where a helper would do, new abstraction
+  with one caller, a bespoke mechanism where a standard one exists)?
+- **Idiomatic fit.** Does it match how this repo already solves similar problems?
+
+## In-repo patterns and helpers to weigh as alternatives
+
+Before endorsing a new mechanism, check whether one of these already covers it:
+
+- **Manifest read/write → `AppxManifestDocument`.** New appxmanifest handling
+  should extend it, not add raw `XDocument`/regex parsing.
+- **Manifest discovery → `ManifestHelper` /
+  `MsixService.FindManifestInDirectory`** (documented precedence order).
+- **PE / MRT / PRI → `PeHelper`, `MrtAssetHelper`, `PriService`.**
+- **UI selectors → `SelectorService`.**
+- **CLI parser config → `WinAppParserConfiguration.Default`.**
+- **Service shape** (from the repo's architecture guide):
+  | Pattern | When |
+  |---------|------|
+  | Interface + DI service | stateful, needs deps |
+  | Static helper | pure functions |
+  | Data document | wraps a file/data format |
+  | Partial class | splitting a large tightly-coupled service |
+  Flag a proposal that picks the wrong shape (e.g., a stateless 3-line helper as
+  a DI service, or a stateful thing as a static).
+- **Build orchestration → `scripts/build-cli.ps1`** is canonical; flag new build
+  steps that bypass or duplicate it.
+
+## Ecosystem alternatives to weigh
+
+- An existing **Windows SDK tool** (`makeappx`, `signtool`, `makepri`,
+  `cppwinrt`, etc.) or **Windows App SDK API** that already does the work — so
+  winapp should wrap it thinly rather than reimplement it.
+- A standard OS mechanism, an established NuGet package, or a documented MSIX /
+  manifest feature that the spec reinvents.
+- Conversely, note when a spec reaches for a heavy external dependency where a
+  few lines against an existing tool would do.
+
+## How to report alternatives
+
+- Name each concrete alternative, with **tradeoffs** (what it costs, what it
+  saves). Vague "consider a library" without naming one is noise — drop it.
+- If a materially simpler/safer alternative exists, make it a finding and state
+  it plainly; the orchestrator surfaces the single best alternative in the
+  report. If the proposed approach *is* the simplest reasonable one, say so in
+  the `Bottom line` and emit no findings.
+
+## What to drop
+
+- "Could be more functional / use more LINQ" style refactors with no concrete
+  in-repo callable.
+- Wholesale "rewrite it differently" suggestions that aren't clearly simpler.
+- Premature-abstraction complaints already covered by `necessity-and-scope`
+  (coordinate: scope-of-feature → necessity; shape-of-solution → here).
+
+## Severity guide for this dimension
+
+- Proposed approach won't achieve the goal, or a materially simpler/safer
+  alternative clearly exists → high.
+- A better-fitting in-repo pattern/helper is being reinvented, or the wrong
+  service shape is chosen → medium.
+- Minor "could reuse helper X" with marginal benefit → low.

.github/skills/spec-review/dimensions/dx-and-user-impact.md

@@ -0,0 +1,65 @@
+# DX & user-impact review
+
+You are the **dx-and-user-impact** sub-agent for the `microsoft/winappcli`
+spec-review skill. Your question: **is the proposed CLI UX / API coherent with
+existing winapp conventions, free of surprise breaking changes, and
+understandable to users?** Apply the shared output contract in
+`_shared-contract.md`. Set `Domain: dx-and-user-impact` on every finding.
+
+Verify conventions against the real CLI, not your assumptions — skim
+`docs/cli-schema.json` and `src/winapp-CLI/WinApp.Cli/Commands/` to see how
+existing commands and options actually look before judging the proposal.
+
+## Conventions to check the proposal against
+
+- **Option naming.** kebab-case (`--use-defaults`); `--no-<flag>` for negations;
+  `-a` / `-w` short forms reserved for app/window targeting. Honor existing
+  aliases (e.g. `--use-defaults` ≡ `--no-prompt`) where applicable.
+- **Sane defaults.** The common case should work with minimal flags (e.g.
+  `--manifest` auto-detects; dev `--cert-password` defaults to `password`).
+  Flag new **required** options that could reasonably have a default.
+- **Subcommand placement.** New verbs should slot under the right parent
+  (`ui`, `manifest`, `cert`, `node`, `tool`, `store`) and be discoverable via
+  `--help`, rather than adding an inconsistent new top-level command.
+- **Non-interactive / CI support.** Any new prompt must be skippable
+  (`--use-defaults` / non-interactive) with a sensible default, so scripted and
+  CI use isn't blocked.
+- **Output & logging discipline.** `--json` → machine-readable only (no log
+  lines mixed in); `--verbose` / `--quiet` semantics respected; tabular output
+  aligned.
+- **Exit codes.** Non-zero on user-actionable failure; no silent success after
+  an error.
+- **Coherent mental model.** Will a user predict what the command does from its
+  name and options? Is it consistent with the verbs/nouns winapp already uses?
+  Is the feature discoverable (help text, docs, guides)?
+
+## Breaking changes & cross-surface impact
+
+- **Breaking changes** are high-impact: a renamed/removed command or option, a
+  changed default, or changed output shape will break existing scripts and the
+  downstream **npm wrapper**, **NuGet MSBuild targets**, and **VS Code
+  extension**. Flag these prominently and ask whether the break is justified /
+  has a migration path.
+- **Cross-surface parity.** A new top-level CLI command usually needs to flow
+  through the npm wrapper and be considered for the NuGet targets and VSC
+  command palette. Flag a design that would silently diverge across surfaces.
+
+## What to drop
+
+- Bikeshedding an option name with no real UX impact.
+- Color/emoji/spacing preferences.
+- Anything a user would simply learn from `--help`.
+- Doc-completeness concerns (there's no code yet); focus on whether the *design*
+  is coherent and non-breaking, not on missing docs.
+
+## Severity guide for this dimension
+
+- Unjustified breaking change to an existing command/option/default → high.
+- New required option that blocks scripted/CI use, or a genuinely confusing
+  mental model → high.
+- Convention violation on a new public command/option users will notice, or
+  cross-surface divergence → medium.
+- Minor UX polish with a concrete recommendation → low.
+
+If the proposed UX is coherent, conventional, and non-breaking, say so in the
+`Bottom line` and emit zero findings.