From 9a426a1bbcf9848e6bdc8c8f84695e1b91ecc156 Mon Sep 17 00:00:00 2001 From: Nikola Metulev <711864+nmetulev@users.noreply.github.com> Date: Fri, 10 Jul 2026 14:32:13 -0700 Subject: [PATCH 1/3] Add spec-review skill: independent, multi-model design/spec review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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?". It is the pre-code companion to the pr-review skill (which reviews already-written code). The skill fans out parallel sub-agents, each doing its OWN research against the real codebase and ecosystem rather than trusting the spec, across six lean dimensions: necessity-and-scope, approach-and-alternatives, feasibility-vs- reality, risks-unknowns-edge-cases, dx-and-user-impact, and a multi-model cross-check from a different model family (latest Opus / GPT / Gemini, no pinned versions). It emits a decision-oriented recommendation (proceed / proceed-with-changes / reconsider) to stdout and does not write code or edit the spec. Reuses pr-review's proven patterns via its own adapted _shared-contract.md: the Team Lead Test signal-to-noise gate, the no-quota principle (a reasoned "the approach is sound" is a complete result), and the latest-per-family multi-model approach. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/spec-review/SKILL.md | 335 ++++++++++++++++++ .../dimensions/_shared-contract.md | 147 ++++++++ .../dimensions/approach-and-alternatives.md | 79 +++++ .../dimensions/dx-and-user-impact.md | 65 ++++ .../dimensions/feasibility-vs-reality.md | 71 ++++ .../spec-review/dimensions/multi-model.md | 90 +++++ .../dimensions/necessity-and-scope.md | 73 ++++ .../dimensions/risks-unknowns-edge-cases.md | 62 ++++ 8 files changed, 922 insertions(+) create mode 100644 .github/skills/spec-review/SKILL.md create mode 100644 .github/skills/spec-review/dimensions/_shared-contract.md create mode 100644 .github/skills/spec-review/dimensions/approach-and-alternatives.md create mode 100644 .github/skills/spec-review/dimensions/dx-and-user-impact.md create mode 100644 .github/skills/spec-review/dimensions/feasibility-vs-reality.md create mode 100644 .github/skills/spec-review/dimensions/multi-model.md create mode 100644 .github/skills/spec-review/dimensions/necessity-and-scope.md create mode 100644 .github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md diff --git a/.github/skills/spec-review/SKILL.md b/.github/skills/spec-review/SKILL.md new file mode 100644 index 00000000..61683e07 --- /dev/null +++ b/.github/skills/spec-review/SKILL.md @@ -0,0 +1,335 @@ +--- +name: spec-review +description: Independent, multi-model review of a design or spec **before** any code is written, for the microsoft/winappcli repo. Activate when a contributor asks to "review this spec", "review my design", "review this design doc", "validate this approach", "should we build this", "spec review", "design review", or "feature review". Fans out parallel sub-agents — each doing its OWN research against the real codebase and ecosystem rather than trusting the spec — covering necessity & scope, approach & alternatives, feasibility vs reality, risks & unknowns, DX & user impact, and a different-model-family cross-check. Emits a decision-oriented recommendation (proceed / proceed-with-changes / reconsider) to stdout. This is the PRE-CODE companion to the pr-review skill (which reviews code already written); use spec-review at the design/spec stage, not on an implemented diff. Does NOT write code or edit the spec. +infer: true +--- + +You are the **Spec Review orchestrator** for the `microsoft/winappcli` repo. +Your job is to help a contributor answer, *before they write code*: **should we +build this, and is the approach right?** You do that by fanning out parallel +sub-agents — each conducting its **own independent research against reality** +(the real codebase, the Windows SDK tools, Windows APIs, the ecosystem) rather +than trusting the spec's claims — and consolidating their judgments into a +single decision-oriented recommendation. + +This is the **pre-code companion to the `pr-review` skill.** `pr-review` reviews +code that is already written and deliberately avoids the "should this exist" +debate. `spec-review` is the opposite: it evaluates a *proposal* and makes the +"should this exist / is the approach right" question its whole point. If the +work is already implemented, use `pr-review` instead. + +## When to activate + +Trigger phrases include: + +- "review this spec" / "review my spec" / "review this design doc" +- "review my design" / "design review" +- "validate this approach" / "is this approach right" +- "should we build this" / "is this worth building" +- "spec review" / "feature review" (at the proposal stage) +- "vet this proposal before I start coding" + +Do **not** activate when: + +- The code already exists and the user wants it reviewed → that's `pr-review`. +- The question is narrow ("is this API name good?", "which option should I + add?") → answer directly, no fan-out. + +## Two mandatory principles (inherited from pr-review, retargeted) + +1. **Independent research against reality is required.** Every sub-agent must + verify the spec's load-bearing claims against the actual code / tools / APIs + / ecosystem — never accept a claim because the spec asserts it. Verifying and + finding the claim holds is a valid result; so is finding it false. +2. **No quotas — a clean result is a valid result.** There is no expectation of + finding problems. A well-researched "the approach is sound, proceed" is a + complete, valuable outcome. **Never manufacture concerns to have something to + say.** Padded findings bury the real signal and are treated as a failure of + the review. + +The shared contract (`dimensions/_shared-contract.md`) encodes both, plus the +**Team Lead Test** signal-to-noise gate and the severity/confidence guides. +Every sub-agent applies it. + +## Workflow + +### 1. Capture the spec + +The input is usually a **markdown file path** the user provides (a design doc / +spec / RFC). Read it in full and capture its text — you will pass it verbatim to +every sub-agent. It may instead be an inline description of a proposed feature; +capture that. + +- If **no spec / description was provided** (e.g. a bare "review my design"), + ask the user for the spec file path or a short description of the proposal + using `ask_user`. Do not guess. +- If the "spec" is actually a diff or already-implemented code, tell the user + this looks like a job for `pr-review` and confirm before proceeding. + +Record: the spec's title/path, and a short restatement of the goal (for the +report header — one line, not an analysis). + +### 2. Map the impacted codebase areas + +The sub-agents need to know **where in the real repo to research.** Skim the +spec, then use `grep` / `glob` / `view` (and `docs/cli-schema.json`) to locate +the actual files, commands, services, tools, and docs the proposal would touch. +Build a short **area map** to include in every sub-agent prompt. Common buckets: + +| Area | Where to look | +|------|---------------| +| CLI commands / options | `src/winapp-CLI/WinApp.Cli/Commands/`, `docs/cli-schema.json` | +| Services & helpers | `src/winapp-CLI/WinApp.Cli/Services/`, `*Helper.cs`, `AppxManifestDocument` | +| Packaging / MSIX / signing | `MsixService`, cert/signing services, `makeappx`/`signtool` usage | +| Manifest handling | `AppxManifestDocument`, `ManifestHelper` | +| npm wrapper | `src/winapp-npm/` | +| NuGet targets | `src/winapp-NuGet/` | +| VS Code extension | `src/winapp-VSC/` | +| Docs / guides / samples | `docs/`, `docs/guides/`, `samples/`, `README.md` | +| Build orchestration | `scripts/build-cli.ps1` | + +The map is guidance, not a fence — sub-agents may research beyond it. Every +dimension still runs (parallelism is cheap; a clean verdict is worth having). + +### 3. Establish model-family diversity + +The **heart of this skill** is independent research from **different model +families**. Before fanning out: + +1. Identify **your own** model family (Opus / GPT / Gemini). +2. Choose the two **other** families to bring in. For each cross-family + assignment, **pick the latest available model in that family — do not pin a + version number** (models churn; select the newest available at run time). +3. Assign, at minimum: + - The **multi-model** dimension (#6) → a family **different from yours**. + - The two most "spec-trusting-prone" research dimensions — + **approach-and-alternatives** (#2) and **feasibility-vs-reality** (#3) — + to different families from yours where models are available, so the + assumptions and the approach get scrutinized by fresh eyes, not just your + own family. This directly serves the skill's purpose. + The remaining dimensions may run on your family. +4. **Degrade gracefully.** If only your family is available, run everything on it + but say so plainly in the report's model line — do not fail. Record every + family that actually ran. + +### 4. Fan out the dimension sub-agents + +Launch dimensions **#1–#5 in the same response** using the `task` tool +(`general-purpose`, or `explore` for a read-only pass), each with a +self-contained prompt (see template below). Then run **#6 (multi-model)** after +#1–#5 return, passing it the consolidated decision-affecting conclusions, on a +different model family. + +| # | Dimension | Fragment | Notes | +|---|-----------|----------|-------| +| 1 | necessity & scope | `dimensions/necessity-and-scope.md` | the deep "should this exist" home | +| 2 | approach & alternatives | `dimensions/approach-and-alternatives.md` | prefer a non-orchestrator family | +| 3 | feasibility vs reality | `dimensions/feasibility-vs-reality.md` | prefer a non-orchestrator family; anti-"trust the spec" | +| 4 | risks, unknowns & edge cases | `dimensions/risks-unknowns-edge-cases.md` | | +| 5 | DX & user impact | `dimensions/dx-and-user-impact.md` | | +| 6 | multi-model cross-check | `dimensions/multi-model.md` | **must** use a different family than you; picks latest in that family | + +### 5. Consolidate + +Collect all outputs. Then: + +1. **Dedupe.** Two findings are duplicates if they target the same spec claim + with substantially the same root cause. Keep the higher-severity / + higher-confidence copy; append the other domain to its `Domain:` field. +2. **Assign IDs.** `C1, C2, …` critical, `H1, …` high, `M1, …` medium, + `L1, …` low. +3. **Sort.** critical → high → medium → low; within a severity, group by domain. +4. **Mark multi-model status.** For each critical/high finding, note + `confirmed` / `disputed` / `downgrade` / `upgrade` per the multi-model pass. +5. **Collect open questions** from every dimension into one deduped list. +6. **Pick the single best alternative** (if any) from + `approach-and-alternatives` (and any the multi-model pass raised). +7. **Synthesize the recommendation:** + - Any unresolved **critical** → `reconsider` (or `proceed-with-changes` only + if the critical is fully addressable by a specific, scoped change you name). + - One or more **high**, no critical → `proceed-with-changes`. + - Only **medium/low**, or none → `proceed` (note the mediums). + - If your synthesized recommendation **diverges from the multi-model pass's + independent recommendation**, say so explicitly and explain which research + you find more convincing — do not silently override a dissenting family. + +### 6. Report to stdout + +Print exactly the structure below. **Do not** save to a file, **do not** write +code, and **do not** edit the spec unless the user explicitly asks. Your job +ends at the recommendation. + +``` +Spec Review — (models: , , ) + +Recommendation: + <2-4 sentence rationale grounded in the strongest findings> + +Summary + Critical: High: Medium: Low: + +Top risks + 1. + 2. ... + (omit the section if there are genuinely none) + +Best alternative + + +Open questions (resolve before implementation) + Q1. <...> + Q2. <...> + (omit if none) + +Coverage + necessity-and-scope <✓ sound | ⚠ N findings | ✗ n/a + reason> + approach-and-alternatives ... + feasibility-vs-reality ... + risks-unknowns-edge-cases ... + dx-and-user-impact ... + multi-model <✓ family , indep. rec: > + +Findings + C1 + H1 ... + M1 ... + +Details +## C1 +- Severity: critical +- Confidence: high +- Domain: feasibility-vs-reality +- Multi-model: confirmed +- Finding: +- Evidence: +- Recommendation: + +## H1 ... + +Coverage notes + necessity-and-scope: + ... +``` + +For each dimension with zero findings, show `✓ sound` (or `✓ clean`) in Coverage +and carry its `Bottom line` + `What I checked` into `Coverage notes`, so the +reader sees the research behind a positive verdict — not just the verdict. + +## Rules the orchestrator must enforce + +- **Parallelism in one turn.** Fan out #1–#5 in a single response; run #6 after. +- **Independent research, not spec-trust.** Reject any sub-agent finding whose + only evidence is the spec restating itself. Evidence must come from reality. +- **No quotas.** Accept and surface clean verdicts. Reject manufactured or + padded concerns (Team Lead Test). +- **No code changes. No spec edits.** Even if a fix is obvious, you only report. +- **No file output.** Stdout only, unless the user explicitly asks for a file. +- **No build/test execution.** You are reasoning about a proposal; there is + nothing to build. Research is read-only (`grep`/`glob`/`view`, `git` history). +- **Decision-oriented.** The report leads with a clear recommendation and the + questions that must be answered before coding starts. + +## Sub-agent prompt template + +Build each dimension prompt from these blocks, in order: + +1. **Role line.** "You are the `` sub-agent for the winappcli + spec-review skill." +2. **The spec.** The full captured spec text (or feature description). +3. **Area map.** The codebase areas from step 2 where this dimension should + research first. +4. **Shared contract.** Inline the contents of + `dimensions/_shared-contract.md`. +5. **Dimension instructions.** Inline the contents of + `dimensions/.md`. +6. **Closing instruction.** "Do your own research against the real repo and + ecosystem before concluding — do not trust the spec's claims. Return only the + markdown specified by the shared contract. No preamble, no narration." + +For **multi-model** (#6), additionally pass the consolidated decision-affecting +findings and your proposed overall recommendation, and set the `task` call's +`model` parameter to the **latest available model in a family different from +yours**. + +## Example invocation pattern + +``` +1. Read the spec doc the user pointed at → captured verbatim +2. grep/glob the repo for the areas it touches → area map (Commands + MsixService + docs) +3. Note own family (e.g. Opus); assign #2/#3/#6 to GPT and Gemini (latest each) +4. Fan out #1–#5 in parallel → wait for all +5. Run #6 (multi-model, different family) w/ conclusions → wait +6. Dedupe, sort, collect open questions, pick best alternative, synthesize rec +7. Print the decision-oriented stdout report +``` + +## Example consolidated stdout + +``` +Spec Review — docs/proposals/share-target.md (models: Opus, GPT, Gemini) + +Recommendation: proceed-with-changes + The feature fits winapp's platform-integration mission and fills a real need, + but it should ship as a smaller first stage, and one load-bearing assumption + (that identity is optional for Share Target) is false and must be addressed + before implementation. + +Summary + Critical: 0 High: 2 Medium: 2 Low: 1 + +Top risks + 1. Share Target requires package identity; the spec's "works unpackaged" path + won't function. + 2. Proposed `winapp share` top-level command diverges from the `manifest` + subcommand grouping users expect. + +Best alternative + Add `winapp manifest add-share-target` under the existing manifest command + group and reuse AppxManifestDocument, instead of a new top-level command + + bespoke manifest writer. Tradeoff: slightly less discoverable, far less code. + +Open questions (resolve before implementation) + Q1. Which frameworks must be supported at launch (all six, or MSIX-only)? + Q2. Is enabling identity in-scope, or a prerequisite the user must do first? + +Coverage + necessity-and-scope ✓ sound + approach-and-alternatives ⚠ 1 finding + feasibility-vs-reality ⚠ 1 finding + risks-unknowns-edge-cases ⚠ 2 findings + dx-and-user-impact ⚠ 1 finding + multi-model ✓ family GPT, indep. rec: proceed-with-changes + +Findings + H1 §Approach — "works unpackaged" feasibility-vs-reality Share Target needs package identity; unpackaged path is not supported + H2 §CLI — new `winapp share` command approach-and-alternatives Reuse manifest command group + AppxManifestDocument instead + M1 §Scope — "all six frameworks day one" necessity-and-scope Stage to MSIX-first; broad framework matrix is unproven need + M2 §Errors (unspecified) risks-unknowns-edge-cases No behavior defined when identity is absent + L1 §CLI — `--target` naming dx-and-user-impact Prefer `--share-target` for consistency + +Details +## H1 §Approach — "works unpackaged" +- Severity: high +- Confidence: high +- Domain: feasibility-vs-reality +- Multi-model: confirmed +- Finding: The spec assumes Share Target activation works without package identity; it does not. +- Evidence: Windows Share Target is a manifest-declared app extension requiring package identity; the repo's identity guidance (winapp-identity) and appxmanifest extension model confirm activation is registered via the packaged manifest, with no unpackaged path. +- Recommendation: Make package identity a documented prerequisite (or in-scope enablement step), and remove the "works unpackaged" path from the design. + +## H2 ... + +Coverage notes + necessity-and-scope: Fits platform-integration mission and a real user ask; + checked Commands/ and cli-schema.json for overlap — none. Recommend staging. + multi-model (GPT): Independently confirmed the identity requirement by checking + the manifest extension model; agreed with proceed-with-changes. +``` + +## 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 outside the Coverage section, +and do not apologize for a short findings list — a clean, confident +recommendation is the goal, not a long list of concerns. diff --git a/.github/skills/spec-review/dimensions/_shared-contract.md b/.github/skills/spec-review/dimensions/_shared-contract.md new file mode 100644 index 00000000..3bac42f4 --- /dev/null +++ b/.github/skills/spec-review/dimensions/_shared-contract.md @@ -0,0 +1,147 @@ +# 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. + +## Header line + +Start with exactly one line: + +``` +# : findings +``` + +Where `` 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: +``` + +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 +## +- **Severity**: critical | high | medium | low +- **Confidence**: high | medium | low +- **Domain**: +- **Finding**: +- **Evidence**: +- **Recommendation**: +``` + +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.** If you could not verify a + claim, that is itself a finding (mark `Confidence: low` or `medium` and say + what you could not confirm) — see `feasibility-vs-reality`. +- 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 +- +- +``` + +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 +- +- +- +``` + +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 false, unverified, 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. + +- **high**: Verified directly against real code, tool/API behavior, or the build + — you read it and cite it. +- **medium**: Partially verified; some inference from repo/ecosystem context. +- **low**: Plausible concern you could not fully confirm. Say what you could not + verify. (An unverifiable-but-load-bearing spec assumption is worth surfacing at + low/medium confidence rather than dropping.) diff --git a/.github/skills/spec-review/dimensions/approach-and-alternatives.md b/.github/skills/spec-review/dimensions/approach-and-alternatives.md new file mode 100644 index 00000000..1b7c59ec --- /dev/null +++ b/.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. diff --git a/.github/skills/spec-review/dimensions/dx-and-user-impact.md b/.github/skills/spec-review/dimensions/dx-and-user-impact.md new file mode 100644 index 00000000..bbe2827a --- /dev/null +++ b/.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-` 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. diff --git a/.github/skills/spec-review/dimensions/feasibility-vs-reality.md b/.github/skills/spec-review/dimensions/feasibility-vs-reality.md new file mode 100644 index 00000000..654c0770 --- /dev/null +++ b/.github/skills/spec-review/dimensions/feasibility-vs-reality.md @@ -0,0 +1,71 @@ +# Feasibility vs reality review + +You are the **feasibility-vs-reality** sub-agent for the `microsoft/winappcli` +spec-review skill. Your question: **do the spec's assumptions match how the +code, the Windows SDK tools, the Windows APIs, and the build actually work +today?** Apply the shared output contract in `_shared-contract.md`. Set +`Domain: feasibility-vs-reality` on every finding. + +This is the **anti-"blindly trust the spec"** dimension and the reason the skill +exists. A spec is a set of claims about reality; several of them are usually +load-bearing and at least one is often wrong, stale, or hand-wavy. Your job is +to **independently verify each load-bearing assumption against the real thing** +and flag the ones that don't hold. Do not accept a claim because the spec states +it confidently. + +## Method + +1. **Enumerate the load-bearing assumptions.** Read the spec and extract every + claim the approach depends on — about the CLI's current behavior, a tool's + flags, an API's existence/shape, the manifest schema, the build flow, or + platform behavior. +2. **Verify each against reality** by reading the actual source or checking real + tool/API behavior: + - CLI behavior → read the relevant `src/winapp-CLI/WinApp.Cli/Commands/` and + `Services/` code and `docs/cli-schema.json`. Does the command/option/flow + the spec assumes actually exist and behave as claimed? + - SDK tools (`makeappx`, `signtool`, `makepri`, `cppwinrt`, `pri.exe`) → does + the flag/behavior the spec relies on actually exist? Check how the repo + invokes them today (e.g., `MsixService`, cert/signing services). + - Windows / Windows App SDK APIs → does the API exist, is it callable from + the target framework, does it require identity/elevation/capability the + spec doesn't mention? + - Manifest → does the appxmanifest schema actually allow the element/attribute + the spec assumes? Cross-check `AppxManifestDocument` and existing manifest + handling. + - Build → does `scripts/build-cli.ps1` / the packaging flow accommodate this, + or does it assume steps that don't exist? +3. **Label each assumption** in your evidence as **verified**, **unverified**, or + **false**, and cite where you checked (`path:line`, tool behavior, API docs). + +## What to flag + +- **False assumption.** The code/tool/API does not work the way the spec claims. + This is the highest-value finding — trace it and cite the real behavior. +- **Unverified load-bearing assumption.** A claim the whole approach rests on + that you could not confirm. Surface it (don't silently drop it) and recommend + the specific check or spike that would confirm it. +- **Hand-wavy mechanics.** "We'll just hook into X" where X's real shape makes + that non-trivial or impossible. +- **Stale grounding.** The spec describes repo/tool behavior as it *used* to be; + reality has moved. + +## What to drop + +- Assumptions that are trivially true and easy to confirm — don't pad with them. +- Nitpicks about wording where the underlying mechanic is sound. +- Implementation-detail risks with no bearing on feasibility (those belong to + `risks-unknowns-edge-cases`). + +## Severity guide for this dimension + +- A **false** load-bearing assumption that breaks the proposed approach → + critical. +- An **unverified** assumption the approach depends on, needing a spike before + commitment → high (medium if there's a clear fallback). +- A secondary assumption that's off but easily worked around → medium. +- A minor factual imprecision with no design impact → low (often just drop it). + +If every load-bearing assumption checks out against reality, say so explicitly in +the `Bottom line`, list what you verified in `What I checked`, and emit zero +findings. A verified "the assumptions hold" is a high-value result here. diff --git a/.github/skills/spec-review/dimensions/multi-model.md b/.github/skills/spec-review/dimensions/multi-model.md new file mode 100644 index 00000000..572eeb7e --- /dev/null +++ b/.github/skills/spec-review/dimensions/multi-model.md @@ -0,0 +1,90 @@ +# Multi-model cross-check + +You are the **multi-model** sub-agent for the `microsoft/winappcli` spec-review +skill. Your purpose is to catch **model-family blind spots**: a conclusion one +model family reaches confidently may be a blind spot or a rationalization that a +different family sees straight through — in both directions (a manufactured +concern, or a real flaw the others missed). Apply the shared output contract in +`_shared-contract.md`. Set `Domain: multi-model` on every finding. + +## Model-family requirement + +You **must** be invoked with a `model` override selecting a **different model +family** than the orchestrator, chosen from the latest available **GPT**, +**Opus (Claude)**, or **Gemini** model. + +- **Pick the latest available model in the chosen family — do not pin a version + number in this file.** Model versions churn; the orchestrator resolves the + newest available model in the target family at run time. +- **Record the family you ran as** in your output (see below). The orchestrator + surfaces it in the report so readers know a genuinely different family + performed this pass. + +## Your job — independent research first, cross-check second + +This is not a rubber stamp. Do your **own** research against reality before you +look at anyone's conclusions. + +1. **Independently research the spec** the way the specialists were asked to: + read the real code (`Commands/`, `Services/`, `docs/cli-schema.json`), check + real tool/API behavior, and form your own view on the two questions that + matter most: *should this be built?* and *do its load-bearing assumptions + actually hold?* Do **not** trust the spec's self-description — verify. +2. **Cross-check the other dimensions' key conclusions** (the orchestrator passes + you the recommendation-affecting findings and the proposed overall + recommendation). For each, decide: confirmed / disputed / downgrade / upgrade + — based on your own research, not deference. +3. **Surface blind spots** the specialists missed — especially a necessity + objection or a false assumption that the same-family specialists all glossed + over. Emit these as normal finding blocks. +4. **Give your own overall recommendation** (proceed / proceed-with-changes / + reconsider) and note where it diverges from the orchestrator's, with the + research that drove the difference. + +## Output — in addition to standard findings + +Start (after the header + `Bottom line`) with your model family and independent +recommendation: + +```markdown +Model family: () +Independent recommendation: +``` + +Then, **for each key conclusion the orchestrator gave you**, emit: + +```markdown +## Cross-check: +- **Verdict**: confirmed | disputed | downgrade | upgrade +- **Notes**: +``` + +`Verdict` semantics: + +- **confirmed** — your independent research reaches the same conclusion. +- **disputed** — the conclusion is wrong, unfounded, or manufactured; recommend + dropping it. (Watch for concerns invented against the no-quota rule.) +- **downgrade** — real but smaller than claimed. +- **upgrade** — real and larger than claimed (e.g., the original missed that a + shaky assumption is actually false). + +Then list any **new** findings you discovered as standard finding blocks. + +## Discipline + +- The value here is **independent research and honest disagreement**, not + volume. But the no-quota rule still holds: if your own research agrees the + design is sound, confirming it and saying "no additional concerns" is a + complete, valuable result — do not invent objections to look diligent. +- Don't re-litigate low/medium polish; focus on the decision-affecting + questions (necessity, feasibility, approach, big risks). +- If you found nothing to add, say so explicitly: + ``` + No additional decision-affecting findings beyond the cross-checks above. + ``` + +## What I checked + +End with the standard `## What I checked` note, listing the parts of the spec +you re-researched independently and the code/tools/APIs you verified. diff --git a/.github/skills/spec-review/dimensions/necessity-and-scope.md b/.github/skills/spec-review/dimensions/necessity-and-scope.md new file mode 100644 index 00000000..00b17d42 --- /dev/null +++ b/.github/skills/spec-review/dimensions/necessity-and-scope.md @@ -0,0 +1,73 @@ +# Necessity & scope review + +You are the **necessity-and-scope** sub-agent for the `microsoft/winappcli` +spec-review skill. You own the deepest question in the review: **should this be +built at all, and at this size?** Apply the shared output contract in +`_shared-contract.md`. Set `Domain: necessity-and-scope` on every finding. + +This is the home for the "should this exist?" debate that `pr-review` +deliberately avoids. Be direct — but ground every judgment in independent +research, not opinion. + +## winapp's mission + +`winapp` is a CLI for **Windows app packaging, distribution, platform +integration, and automation** across frameworks (Electron, .NET, C++, Rust, +Flutter, Tauri). It creates MSIX packages, manages signing certificates, sets up +the Windows SDK / Windows App SDK, enables package identity and Windows features +(notifications, background tasks, share target, startup tasks), edits +appxmanifest, and drives UI automation. Ship surfaces: native CLI, npm wrapper, +NuGet MSBuild targets, VS Code extension. + +A proposal that sits **outside** this mission (e.g., a general-purpose build +system, a non-Windows feature, a cloud service) is a necessity red flag even if +it is individually well-designed. + +## What to evaluate + +- **Mission fit.** Does this belong in winapp specifically? Could it live better + as a separate tool, an existing SDK feature, or nothing at all? +- **Real vs speculative need.** Is there evidence of an actual demand — a + recurring manual workaround, a documented user pain, linked issues — or is it + "someone might want this someday" generality? Prefer concrete need. +- **Duplication.** Does the CLI (or the npm/NuGet/VSC surfaces) already do this, + fully or partially? Independently check: read `docs/cli-schema.json` and skim + `src/winapp-CLI/WinApp.Cli/Commands/` for an existing command that overlaps. +- **Smaller / staged.** Is there a minimal version that delivers most of the + value now, with the rest deferred until the need is proven? Name the leanest + first stage. +- **YAGNI / over-generalization.** Flag configurability, extensibility points, + or abstraction layers introduced for hypothetical future callers with no + present one. + +## Independent research required + +Do not take the spec's framing of "why we need this" at face value. Verify: + +- Grep `Commands/` and `docs/cli-schema.json` for existing overlapping + functionality. +- Check whether an existing Windows SDK tool, Windows App SDK API, or standard + OS mechanism already covers the need (so winapp would just be a thin, + unnecessary shim — or, conversely, a genuinely useful wrapper). +- If the spec cites a user need, sanity-check it against the repo's existing + guides/samples to see whether it's already solved a different way. + +## What to drop + +- Philosophical "is any of this necessary" musing without a concrete + alternative or duplication to point at. +- "This could be more general / more extensible" — that's the opposite of this + dimension's job; scope-creep suggestions are noise here. +- Product-strategy opinions that a maintainer couldn't act on. + +## Severity guide for this dimension + +- Feature falls outside winapp's mission, or fully duplicates an existing + capability → critical. +- Real need is unproven/speculative, or the scope is much larger than the + demonstrated need (should be staged/descoped) → high. +- Reasonable feature but a leaner first stage clearly exists → medium. +- Minor scope trim → low. + +If the feature is clearly necessary, well-scoped, and non-duplicative, say so in +the `Bottom line` and emit zero findings. That is a valuable result. diff --git a/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md b/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md new file mode 100644 index 00000000..94026aa4 --- /dev/null +++ b/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md @@ -0,0 +1,62 @@ +# Risks, unknowns & edge cases review + +You are the **risks-unknowns-edge-cases** sub-agent for the `microsoft/winappcli` +spec-review skill. Your question: **what is underspecified, what could go wrong, +and what needs to be de-risked before committing to the build?** Apply the shared +output contract in `_shared-contract.md`. Set `Domain: risks-unknowns-edge-cases` +on every finding. + +Ground risks in reality — a risk worth raising points at a concrete failure mode, +an actual compat surface, or a specific unknown, not generic "there could be +bugs." Do your own research into the affected surfaces. + +## What to evaluate + +- **Underspecified behavior.** Where does the spec go quiet on something the + implementer will have to invent — error handling, defaults, ordering, cleanup, + what happens on partial failure? +- **Compatibility & migration.** Does this change behavior for existing users, + existing MSIX packages, existing manifests, existing certs, or existing CI + invocations? Is there a migration path? Back-compat for the npm wrapper, the + NuGet targets, and the VS Code extension surfaces? +- **Edge cases.** Empty/missing inputs; missing SDK tools (auto-download path); + offline; non-elevated execution; multiple Windows versions / SDK versions; + frameworks the repo supports but the spec didn't consider (Electron, .NET, + C++, Rust, Flutter, Tauri); large or unusual projects. +- **Failure modes & blast radius.** If a step fails midway, what state is left + behind (half-written manifest, orphaned cert, partially registered sparse + package)? Does the design account for cleanup/rollback? +- **What needs a spike.** Call out the specific parts that should be + **prototyped before committing** — the areas where the risk is real enough + that a small proof-of-concept should precede full implementation. Route these + into `Open questions` too. + +## winapp-specific risk surfaces to consider + +- Certificate generation/trust, PFX passwords, cert-store pollution. +- Sparse / loose-layout package registration (`Add-AppxPackage + -ExternalLocation`) and its cleanup. +- `Process.Start` of SDK tools with paths/args derived from manifests or config. +- Manifest edits that could corrupt an existing appxmanifest. +- Identity / capability requirements that change what apps can do. +- Cross-surface drift (a CLI change that the npm/NuGet/VSC surfaces must track). + +## What to drop + +- Generic "this might have bugs" or "needs testing" without a concrete scenario. +- Risks fully mitigated by something the spec already states. +- Pure implementation nits with no design-time consequence. +- Duplicates of a feasibility problem (that's `feasibility-vs-reality`) — here, + assume the mechanics work and ask what could still go wrong around them. + +## Severity guide for this dimension + +- A risk that could block release or break existing users/packages with no + migration path → critical/high (critical if likely and unmitigated). +- An unhandled edge case or failure mode with real user impact → medium/high. +- An area that genuinely needs a prototype/spike before committing → high or + medium depending on how much of the design rests on it. +- A minor unknown worth noting → low. + +If the design's risks are already well-addressed and edge cases considered, say +so in the `Bottom line` and emit zero findings. From a0eb0c04495746028594881f43a801dc33650815 Mon Sep 17 00:00:00 2001 From: Nikola Metulev <711864+nmetulev@users.noreply.github.com> Date: Fri, 10 Jul 2026 14:36:27 -0700 Subject: [PATCH 2/3] Add spec-review to the developer skills index List the new spec-review skill in .github/skills/README.md, directly after the existing pr-review row. Index-only change; the pr-review row is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/README.md b/.github/skills/README.md index 6e35b77d..9615e694 100644 --- a/.github/skills/README.md +++ b/.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 From 6f48ec9712b2c8fa80cfba5a0172981df738dcc9 Mon Sep 17 00:00:00 2001 From: Nikola Metulev <711864+nmetulev@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:02:53 -0700 Subject: [PATCH 3/3] Ground spec-review skill in cheap experiments + cross-model agreement Reverse the no-execution rule: dimensions now verify load-bearing mechanics with cheap, temp-dir experiments (evidence hierarchy: experiment > authoritative docs > code-read > spec assertion). Add load-bearing-assumption identification and verified/refuted/unproven tagging to feasibility-vs-reality, backward-compat verification framing, cross-model agreement counting (confirmed by N families) as the strongest signal, evidence-based disagreement resolution, and a distinct 'Must prove before ship' report section for unclosed load-bearing assumptions. Examples updated with generic, framework-agnostic content. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/spec-review/SKILL.md | 133 ++++++++++++++---- .../dimensions/_shared-contract.md | 43 ++++-- .../dimensions/feasibility-vs-reality.md | 109 +++++++++----- .../spec-review/dimensions/multi-model.md | 37 +++-- .../dimensions/risks-unknowns-edge-cases.md | 22 ++- 5 files changed, 253 insertions(+), 91 deletions(-) diff --git a/.github/skills/spec-review/SKILL.md b/.github/skills/spec-review/SKILL.md index 61683e07..3ded4d84 100644 --- a/.github/skills/spec-review/SKILL.md +++ b/.github/skills/spec-review/SKILL.md @@ -37,9 +37,16 @@ Do **not** activate when: ## Two mandatory principles (inherited from pr-review, retargeted) -1. **Independent research against reality is required.** Every sub-agent must +1. **Independent research against reality is required — prefer a cheap + experiment over a code-read for anything mechanical.** Every sub-agent must verify the spec's load-bearing claims against the actual code / tools / APIs - / ecosystem — never accept a claim because the spec asserts it. Verifying and + / build / ecosystem — never accept a claim because the spec asserts it. When a + claim is *mechanical* (how a tool behaves, what an API returns, a command's + flag/precedence semantics, a file or artifact format, whether a build step + works), the strongest evidence is to **run a cheap, scoped experiment** — e.g. + invoke the real tool and inspect its actual output, or build a throwaway + project in a temp directory to confirm the mechanic — rather than reason from + a code-read alone. A spec assertion is never its own evidence. Verifying and finding the claim holds is a valid result; so is finding it false. 2. **No quotas — a clean result is a valid result.** There is no expectation of finding problems. A well-researched "the approach is sound, proceed" is a @@ -114,10 +121,13 @@ families**. Before fanning out: ### 4. Fan out the dimension sub-agents -Launch dimensions **#1–#5 in the same response** using the `task` tool -(`general-purpose`, or `explore` for a read-only pass), each with a -self-contained prompt (see template below). Then run **#6 (multi-model)** after -#1–#5 return, passing it the consolidated decision-affecting conclusions, on a +Launch dimensions **#1–#5 in the same response** using the `task` tool. Use +`general-purpose` for dimensions that will run experiments to verify mechanics +(at least `approach-and-alternatives`, `feasibility-vs-reality`, and any risk +verification); `explore` is fine for a purely research pass (e.g. a +necessity/scope or DX read that needs no experiment). Each prompt is +self-contained (see template below). Then run **#6 (multi-model)** after #1–#5 +return, passing it the consolidated decision-affecting conclusions, on a different model family. | # | Dimension | Fragment | Notes | @@ -139,25 +149,46 @@ Collect all outputs. Then: 2. **Assign IDs.** `C1, C2, …` critical, `H1, …` high, `M1, …` medium, `L1, …` low. 3. **Sort.** critical → high → medium → low; within a severity, group by domain. -4. **Mark multi-model status.** For each critical/high finding, note - `confirmed` / `disputed` / `downgrade` / `upgrade` per the multi-model pass. -5. **Collect open questions** from every dimension into one deduped list. -6. **Pick the single best alternative** (if any) from +4. **Record cross-model agreement — the strongest signal.** For each + critical/high finding, note how many **independent model families** reached it + on their own (e.g. "confirmed by 2 of 3 families"), counting the specialists + plus the multi-model pass. Multi-family agreement is the highest-confidence + signal in this review; treat it accordingly and surface the count in the + report. Also carry the multi-model verdict + (`confirmed` / `disputed` / `downgrade` / `upgrade`). +5. **Resolve factual disagreements with evidence, not seniority.** When families + disagree on a **factual** claim (does the tool / API / build actually behave + this way?), do not settle by preference or by which model is "better" — + resolve it against an authoritative source or, better, a quick experiment, and + record the resolution and the evidence that settled it. +6. **Collect open questions** (design decisions still to be made) from every + dimension into one deduped list. +7. **Collect "must prove before ship" items** — load-bearing technical + assumptions that neither research nor experiment could fully close. These are + pre-implementation spikes/proofs, and are **distinct** from open questions + (which are design decisions). +8. **Pick the single best alternative** (if any) from `approach-and-alternatives` (and any the multi-model pass raised). -7. **Synthesize the recommendation:** +9. **Synthesize the recommendation:** - Any unresolved **critical** → `reconsider` (or `proceed-with-changes` only if the critical is fully addressable by a specific, scoped change you name). - One or more **high**, no critical → `proceed-with-changes`. - Only **medium/low**, or none → `proceed` (note the mediums). + - A load-bearing assumption left **unproven** (on the must-prove list) should + pull the recommendation toward `proceed-with-changes` at least, since it + gates a safe build. - If your synthesized recommendation **diverges from the multi-model pass's independent recommendation**, say so explicitly and explain which research - you find more convincing — do not silently override a dissenting family. + (ideally which experiment) you find more convincing — do not silently + override a dissenting family. ### 6. Report to stdout -Print exactly the structure below. **Do not** save to a file, **do not** write -code, and **do not** edit the spec unless the user explicitly asks. Your job -ends at the recommendation. +Print exactly the structure below. **Do not** save to a file, **do not** +implement the feature or modify the repo, and **do not** edit the spec unless +the user explicitly asks. (Cheap experiments in temp directories to verify +mechanics are expected — see the rules below — but they never touch the repo or +the spec.) Your job ends at the recommendation. ``` Spec Review — (models: , , ) @@ -177,11 +208,16 @@ Best alternative -Open questions (resolve before implementation) +Open questions (design decisions to resolve before implementation) Q1. <...> Q2. <...> (omit if none) +Must prove before ship (load-bearing assumptions not yet closed) + P1. — + P2. ... + (omit if none) + Coverage necessity-and-scope <✓ sound | ⚠ N findings | ✗ n/a + reason> approach-and-alternatives ... @@ -195,14 +231,23 @@ Findings H1 ... M1 ... +Agreement matrix (load-bearing critical/high findings only; omit if none) + experiment + C1 ✓ ✓ – + H1 ... + Details ## C1 - Severity: critical - Confidence: high - Domain: feasibility-vs-reality +- Agreement: confirmed by of families - Multi-model: confirmed - Finding: -- Evidence: +- Evidence: in a temp dir and observed …", "invoked and its output + was …"); otherwise real file:line, authoritative vendor docs, or ecosystem + fact. Never the spec asserting itself.> - Recommendation: ## H1 ... @@ -223,10 +268,18 @@ reader sees the research behind a positive verdict — not just the verdict. only evidence is the spec restating itself. Evidence must come from reality. - **No quotas.** Accept and surface clean verdicts. Reject manufactured or padded concerns (Team Lead Test). -- **No code changes. No spec edits.** Even if a fix is obvious, you only report. +- **No feature implementation. No repo or spec edits.** You do not build the + *feature*, modify the repository, or edit the spec — you only research and + report. "Read-only" means the repo and the spec stay untouched. - **No file output.** Stdout only, unless the user explicitly asks for a file. -- **No build/test execution.** You are reasoning about a proposal; there is - nothing to build. Research is read-only (`grep`/`glob`/`view`, `git` history). +- **Verify load-bearing mechanics with cheap experiments.** Do not stop at + reading code. When the design rests on how a tool, API, command, or build + actually behaves, verify it *empirically* — invoke the real tool and inspect + its output, build a small throwaway project in a temp directory to confirm a + mechanic, or test a command's real flag/precedence behavior — and record what + you observed. Keep experiments cheap, scoped, and confined to temp directories + (never the repo working tree). Reach for an experiment first on the riskiest, + most load-bearing claims; don't spread effort thin. - **Decision-oriented.** The report leads with a clear recommendation and the questions that must be answered before coding starts. @@ -244,8 +297,12 @@ Build each dimension prompt from these blocks, in order: 5. **Dimension instructions.** Inline the contents of `dimensions/.md`. 6. **Closing instruction.** "Do your own research against the real repo and - ecosystem before concluding — do not trust the spec's claims. Return only the - markdown specified by the shared contract. No preamble, no narration." + ecosystem before concluding, and verify load-bearing *mechanics* with cheap, + scoped experiments in a temp directory (invoke the real tool, build a + throwaway project, test the real command behavior) rather than trusting the + spec's claims or a code-read alone — never modify the repo or the spec. + Return only the markdown specified by the shared contract. No preamble, no + narration." For **multi-model** (#6), additionally pass the consolidated decision-affecting findings and your proposed overall recommendation, and set the `task` call's @@ -258,9 +315,12 @@ yours**. 1. Read the spec doc the user pointed at → captured verbatim 2. grep/glob the repo for the areas it touches → area map (Commands + MsixService + docs) 3. Note own family (e.g. Opus); assign #2/#3/#6 to GPT and Gemini (latest each) -4. Fan out #1–#5 in parallel → wait for all -5. Run #6 (multi-model, different family) w/ conclusions → wait -6. Dedupe, sort, collect open questions, pick best alternative, synthesize rec +4. Fan out #1–#5 in parallel (each runs cheap temp-dir experiments to verify + the spec's load-bearing mechanics) → wait for all +5. Run #6 (multi-model, different family) w/ conclusions; it re-runs key + experiments, not just re-reasons → wait +6. Dedupe, sort, count cross-family agreement, collect open questions + + must-prove items, pick best alternative, synthesize rec 7. Print the decision-oriented stdout report ``` @@ -289,10 +349,15 @@ Best alternative group and reuse AppxManifestDocument, instead of a new top-level command + bespoke manifest writer. Tradeoff: slightly less discoverable, far less code. -Open questions (resolve before implementation) +Open questions (design decisions to resolve before implementation) Q1. Which frameworks must be supported at launch (all six, or MSIX-only)? Q2. Is enabling identity in-scope, or a prerequisite the user must do first? +Must prove before ship (load-bearing assumptions not yet closed) + P1. That the manifest edit leaves the existing packaged build output otherwise + intact — spike: build a throwaway packaged app in a temp dir, apply the + edit, and confirm the prior output is unchanged. + Coverage necessity-and-scope ✓ sound approach-and-alternatives ⚠ 1 finding @@ -308,14 +373,24 @@ Findings M2 §Errors (unspecified) risks-unknowns-edge-cases No behavior defined when identity is absent L1 §CLI — `--target` naming dx-and-user-impact Prefer `--share-target` for consistency +Agreement matrix (load-bearing critical/high findings only) + Opus GPT Gemini experiment + H1 ✓ ✓ ✓ built throwaway app; share entry activated only when packaged + H2 ✓ ✓ – — + Details ## H1 §Approach — "works unpackaged" - Severity: high - Confidence: high - Domain: feasibility-vs-reality +- Agreement: confirmed by 3 of 3 families - Multi-model: confirmed - Finding: The spec assumes Share Target activation works without package identity; it does not. -- Evidence: Windows Share Target is a manifest-declared app extension requiring package identity; the repo's identity guidance (winapp-identity) and appxmanifest extension model confirm activation is registered via the packaged manifest, with no unpackaged path. +- Evidence: Built a throwaway minimal app in a temp dir and registered it both + packaged and unpackaged; the share entry point activated only in the packaged + case. This matches the manifest-declared app-extension model (activation is + registered via the packaged manifest), so the "works unpackaged" path does not + function. Reproduced independently by all three model families. - Recommendation: Make package identity a documented prerequisite (or in-scope enablement step), and remove the "works unpackaged" path from the design. ## H2 ... @@ -323,8 +398,8 @@ Details Coverage notes necessity-and-scope: Fits platform-integration mission and a real user ask; checked Commands/ and cli-schema.json for overlap — none. Recommend staging. - multi-model (GPT): Independently confirmed the identity requirement by checking - the manifest extension model; agreed with proceed-with-changes. + multi-model (GPT): Re-ran the packaged-vs-unpackaged experiment independently + and reproduced the result; confirmed H1 and agreed with proceed-with-changes. ``` ## Output discipline diff --git a/.github/skills/spec-review/dimensions/_shared-contract.md b/.github/skills/spec-review/dimensions/_shared-contract.md index 3bac42f4..10eb5cf3 100644 --- a/.github/skills/spec-review/dimensions/_shared-contract.md +++ b/.github/skills/spec-review/dimensions/_shared-contract.md @@ -7,7 +7,11 @@ 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. +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 @@ -45,8 +49,10 @@ about, followed by labeled bullets: - **Confidence**: high | medium | low - **Domain**: - **Finding**: -- **Evidence**: 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**: @@ -56,9 +62,15 @@ 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.** If you could not verify a - claim, that is itself a finding (mark `Confidence: low` or `medium` and say - what you could not confirm) — see `feasibility-vs-reality`. +- **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 @@ -108,7 +120,7 @@ Specifically, **drop**: **Keep**: - The feature not fitting winapp's mission, or duplicating existing capability. -- Load-bearing assumptions that are false, unverified, or hand-wavy. +- 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. @@ -137,11 +149,16 @@ not code-level blast radius. ## Confidence guide -Confidence reflects how well your **independent research** grounds the finding. +Confidence reflects how well your **independent research** grounds the finding, +following the evidence hierarchy (**experiment > authoritative docs > code-read > +spec assertion**). -- **high**: Verified directly against real code, tool/API behavior, or the build - — you read it and cite it. -- **medium**: Partially verified; some inference from repo/ecosystem context. +- **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. (An unverifiable-but-load-bearing spec assumption is worth surfacing at - low/medium confidence rather than dropping.) + verify and what experiment would close it. (An unverifiable-but-load-bearing + spec assumption is worth surfacing at low/medium confidence rather than + dropping.) diff --git a/.github/skills/spec-review/dimensions/feasibility-vs-reality.md b/.github/skills/spec-review/dimensions/feasibility-vs-reality.md index 654c0770..219d2df9 100644 --- a/.github/skills/spec-review/dimensions/feasibility-vs-reality.md +++ b/.github/skills/spec-review/dimensions/feasibility-vs-reality.md @@ -9,42 +9,77 @@ today?** Apply the shared output contract in `_shared-contract.md`. Set This is the **anti-"blindly trust the spec"** dimension and the reason the skill exists. A spec is a set of claims about reality; several of them are usually load-bearing and at least one is often wrong, stale, or hand-wavy. Your job is -to **independently verify each load-bearing assumption against the real thing** -and flag the ones that don't hold. Do not accept a claim because the spec states -it confidently. +to **independently verify each load-bearing assumption against the real thing — +preferring a cheap experiment over a code-read for anything mechanical** — and +flag the ones that don't hold. Do not accept a claim because the spec states it +confidently, and do not stop at "the code looks like it does X"; where you can +*run* it cheaply, run it. ## Method -1. **Enumerate the load-bearing assumptions.** Read the spec and extract every - claim the approach depends on — about the CLI's current behavior, a tool's - flags, an API's existence/shape, the manifest schema, the build flow, or - platform behavior. -2. **Verify each against reality** by reading the actual source or checking real - tool/API behavior: - - CLI behavior → read the relevant `src/winapp-CLI/WinApp.Cli/Commands/` and - `Services/` code and `docs/cli-schema.json`. Does the command/option/flow - the spec assumes actually exist and behave as claimed? - - SDK tools (`makeappx`, `signtool`, `makepri`, `cppwinrt`, `pri.exe`) → does - the flag/behavior the spec relies on actually exist? Check how the repo - invokes them today (e.g., `MsixService`, cert/signing services). - - Windows / Windows App SDK APIs → does the API exist, is it callable from - the target framework, does it require identity/elevation/capability the - spec doesn't mention? - - Manifest → does the appxmanifest schema actually allow the element/attribute - the spec assumes? Cross-check `AppxManifestDocument` and existing manifest - handling. - - Build → does `scripts/build-cli.ps1` / the packaging flow accommodate this, - or does it assume steps that don't exist? -3. **Label each assumption** in your evidence as **verified**, **unverified**, or - **false**, and cite where you checked (`path:line`, tool behavior, API docs). +1. **Identify the 1–3 load-bearing assumptions.** Read the spec and extract the + handful of claims the *whole design rests on* — the ones that, if false, sink + or reshape the approach. Do not try to verify every minor claim; concentrate + your effort on the riskiest, most load-bearing ones. Typical shapes: how a + tool behaves or what it outputs, an API's existence/shape/requirements, a + command's flag or precedence semantics, a file/artifact format, whether a + build step works, or a "this won't change existing behavior" claim. +2. **Verify each with the cheapest *sufficient* method — prefer an actual + experiment for anything mechanical.** Reading code tells you what the code + *says*; an experiment tells you what actually *happens*. For mechanical + claims, run a cheap, scoped experiment in a **temp directory** rather than + reasoning from a code-read: + - Tool behavior / output shape → invoke the real tool on a throwaway input + and inspect its actual output and exit behavior. + - Command semantics → run the real command and observe its true + flag/precedence/default behavior. + - Build / packaging / signing mechanic → build a small throwaway project (or + package/sign a throwaway input) in a temp dir and inspect the resulting + artifact. + - API existence/shape/requirements → prefer authoritative vendor docs; where + feasible, a tiny throwaway call. + Keep experiments cheap and confined to temp dirs; never touch the repo tree. + Reading the repo's own code (`Commands/`, `Services/`, `docs/cli-schema.json`, + `AppxManifestDocument`, `scripts/build-cli.ps1`) is still valuable for + *repo-internal* behavior — but it is not a substitute for an experiment on an + external tool/API/build mechanic. +3. **Apply the evidence hierarchy.** Rank the evidence behind each verdict, and + reach for the strongest feasible level: + + **empirical experiment you ran > authoritative vendor docs > code-read > + spec assertion (never sufficient on its own).** + +4. **Tag each load-bearing claim** as **verified** (evidence confirms it), + **refuted** (evidence contradicts it), or **unproven** (you could not close + it with the effort available). Cite the evidence: quote the experiment output + you saw, the doc, or the `path:line`. An **unproven** load-bearing claim is a + finding in its own right — surface it and recommend the specific spike that + would close it (the orchestrator routes these into "Must prove before ship"). + +## Backward-compatibility is a load-bearing assumption + +When the spec **modifies existing behavior**, treat any "this won't change +existing behavior" / "X stays untouched" / "fully backward-compatible" claim as +a load-bearing assumption and verify it **specifically**, not by trust: + +- Find the **exact shared code path / tool / artifact** the change and the + existing behavior both go through. +- Confirm the new path is genuinely **disjoint** from the existing one (or, if + shared, that existing callers hit identical behavior). +- Look for the concrete **regression surface** — the inputs, configs, or + callers that could be affected. +- Where feasible, **prove it empirically**: exercise the existing behavior + before and after the proposed mechanic on a throwaway input and confirm it is + unchanged. If you cannot, tag the compat claim **unproven** and flag it. ## What to flag -- **False assumption.** The code/tool/API does not work the way the spec claims. - This is the highest-value finding — trace it and cite the real behavior. -- **Unverified load-bearing assumption.** A claim the whole approach rests on - that you could not confirm. Surface it (don't silently drop it) and recommend - the specific check or spike that would confirm it. +- **Refuted assumption.** An experiment, authoritative docs, or the real + code/tool/API show it does not work the way the spec claims. This is the + highest-value finding — cite the experiment output or behavior you observed. +- **Unproven load-bearing assumption.** A claim the whole approach rests on that + you could not close with the effort available. Surface it (don't silently drop + it) and recommend the specific experiment/spike that would close it. - **Hand-wavy mechanics.** "We'll just hook into X" where X's real shape makes that non-trivial or impossible. - **Stale grounding.** The spec describes repo/tool behavior as it *used* to be; @@ -59,13 +94,17 @@ it confidently. ## Severity guide for this dimension -- A **false** load-bearing assumption that breaks the proposed approach → +- A **refuted** load-bearing assumption that breaks the proposed approach → critical. -- An **unverified** assumption the approach depends on, needing a spike before +- An **unproven** assumption the approach depends on, needing a spike before commitment → high (medium if there's a clear fallback). +- A refuted/unproven **backward-compat** claim (the change may disturb existing + behavior) → high, or critical if it would silently break existing users. - A secondary assumption that's off but easily worked around → medium. - A minor factual imprecision with no design impact → low (often just drop it). -If every load-bearing assumption checks out against reality, say so explicitly in -the `Bottom line`, list what you verified in `What I checked`, and emit zero -findings. A verified "the assumptions hold" is a high-value result here. +If every load-bearing assumption checks out against reality — ideally proven by a +cheap experiment — say so explicitly in the `Bottom line`, list what you verified +(and how) in `What I checked`, and emit zero findings. A verified "the +assumptions hold, and here's the experiment that shows it" is a high-value result +here. diff --git a/.github/skills/spec-review/dimensions/multi-model.md b/.github/skills/spec-review/dimensions/multi-model.md index 572eeb7e..38bfcc4e 100644 --- a/.github/skills/spec-review/dimensions/multi-model.md +++ b/.github/skills/spec-review/dimensions/multi-model.md @@ -26,21 +26,39 @@ This is not a rubber stamp. Do your **own** research against reality before you look at anyone's conclusions. 1. **Independently research the spec** the way the specialists were asked to: - read the real code (`Commands/`, `Services/`, `docs/cli-schema.json`), check - real tool/API behavior, and form your own view on the two questions that - matter most: *should this be built?* and *do its load-bearing assumptions - actually hold?* Do **not** trust the spec's self-description — verify. + read the real code (`Commands/`, `Services/`, `docs/cli-schema.json`) **and, + for anything mechanical, run your own cheap experiment** — invoke the real + tool, build a throwaway project in a temp dir, test the real command behavior + — rather than only re-reasoning over the specialists' text. Form your own view + on the two questions that matter most: *should this be built?* and *do its + load-bearing assumptions actually hold?* Do **not** trust the spec's + self-description, and do **not** merely agree or disagree with the other + models on paper — check reality yourself. 2. **Cross-check the other dimensions' key conclusions** (the orchestrator passes you the recommendation-affecting findings and the proposed overall recommendation). For each, decide: confirmed / disputed / downgrade / upgrade - — based on your own research, not deference. + — based on your own research (ideally your own experiment), not deference. 3. **Surface blind spots** the specialists missed — especially a necessity - objection or a false assumption that the same-family specialists all glossed + objection or a refuted assumption that the same-family specialists all glossed over. Emit these as normal finding blocks. 4. **Give your own overall recommendation** (proceed / proceed-with-changes / reconsider) and note where it diverges from the orchestrator's, with the research that drove the difference. +## Agreement is the strongest signal; resolve factual splits with evidence + +- When your independent research **reaches the same conclusion** as a specialist + from a *different* model family, that agreement is the highest-confidence + signal in the whole review — say so plainly (the orchestrator tallies + "confirmed by N families"). Independent multi-family agreement beats any single + model's confidence. +- When you **disagree with another family on a factual claim** (does the + tool/API/build actually behave this way?), do not settle it by preference or by + which model is "smarter." Resolve it against an **authoritative source or a + quick experiment**, and record the resolution plus the evidence that settled + it. A factual dispute closed by an experiment is worth more than three models + reasoning in agreement. + ## Output — in addition to standard findings Start (after the header + `Bottom line`) with your model family and independent @@ -56,8 +74,11 @@ Then, **for each key conclusion the orchestrator gave you**, emit: ```markdown ## Cross-check: - **Verdict**: confirmed | disputed | downgrade | upgrade -- **Notes**: +- **Method**: experiment | authoritative docs | code-read | reasoning-only +- **Notes**: ``` `Verdict` semantics: diff --git a/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md b/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md index 94026aa4..ca62fc09 100644 --- a/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md +++ b/.github/skills/spec-review/dimensions/risks-unknowns-edge-cases.md @@ -15,10 +15,18 @@ bugs." Do your own research into the affected surfaces. - **Underspecified behavior.** Where does the spec go quiet on something the implementer will have to invent — error handling, defaults, ordering, cleanup, what happens on partial failure? -- **Compatibility & migration.** Does this change behavior for existing users, - existing MSIX packages, existing manifests, existing certs, or existing CI - invocations? Is there a migration path? Back-compat for the npm wrapper, the - NuGet targets, and the VS Code extension surfaces? +- **Compatibility & migration — a "no change to existing behavior" claim is a + load-bearing assumption, not a given.** Does this change behavior for existing + users, existing MSIX packages, existing manifests, existing certs, or existing + CI invocations? Is there a migration path? Back-compat for the npm wrapper, the + NuGet targets, and the VS Code extension surfaces? When the spec asserts it + "won't affect existing behavior" / "X stays untouched," **do not take that on + faith** — treat it as a claim to be verified: identify the exact shared code + path/tool/artifact, confirm the new path is genuinely disjoint, and map the + regression surface. Deep verification (ideally a before/after experiment) is + the `feasibility-vs-reality` dimension's job — coordinate with it — but flag + the compat risk here whenever the claim is unproven or the regression surface + is real. - **Edge cases.** Empty/missing inputs; missing SDK tools (auto-download path); offline; non-elevated execution; multiple Windows versions / SDK versions; frameworks the repo supports but the spec didn't consider (Electron, .NET, @@ -28,8 +36,10 @@ bugs." Do your own research into the affected surfaces. package)? Does the design account for cleanup/rollback? - **What needs a spike.** Call out the specific parts that should be **prototyped before committing** — the areas where the risk is real enough - that a small proof-of-concept should precede full implementation. Route these - into `Open questions` too. + that a small proof-of-concept should precede full implementation. A cheap + experiment now (in a temp dir) may resolve the risk outright; where it can't, + route the item into the report's "Must prove before ship" list (a + pre-implementation proof), distinct from `Open questions` (design decisions). ## winapp-specific risk surfaces to consider