From 906bcb808f4438bac5d9879729a79942cc9087ad Mon Sep 17 00:00:00 2001 From: David Xiao Date: Wed, 3 Jun 2026 21:04:22 -0400 Subject: [PATCH] update skills --- plan-eng-review/SKILL.md | 31 +++++++++++++++++++++++++++++-- plan-eng-review/SKILL.md.tmpl | 31 +++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/plan-eng-review/SKILL.md b/plan-eng-review/SKILL.md index c4ec10bb60..9001a99a23 100644 --- a/plan-eng-review/SKILL.md +++ b/plan-eng-review/SKILL.md @@ -905,9 +905,9 @@ Before reviewing anything, answer these questions: If the plan rolls a custom solution where a built-in exists, flag it as a scope reduction opportunity. Annotate recommendations with **[Layer 1]**, **[Layer 2]**, **[Layer 3]**, or **[EUREKA]** (see preamble's Search Before Building section). If you find a eureka moment — a reason the standard approach is wrong for this case — present it as an architectural insight. 5. **TODOS cross-reference:** Read `TODOS.md` if it exists. Are any deferred items blocking this plan? Can any deferred items be bundled into this PR without expanding scope? Does this plan create new work that should be captured as a TODO? -5. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the lake. +6. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the lake. -6. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: +7. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: - Is there a CI/CD workflow for building and publishing the artifact? - Are target platforms defined (linux/darwin/windows, amd64/arm64)? - How will users download or install it (GitHub Releases, package manager, container registry)? @@ -923,6 +923,32 @@ Always work through the full interactive review: one section at a time (Architec **Critical: Once the user accepts or rejects a scope reduction recommendation, commit fully.** Do not re-argue for smaller scope during later review sections. Do not silently reduce scope or skip planned components. +### Pre-Build Prerequisites Check + +Before the review sections, verify the plan covers these development prerequisites. If any are missing, flag them as gaps — a plan that doesn't specify how to build, run, and test the application is incomplete regardless of how good the architecture is. + +For each missing prerequisite, call AskUserQuestion individually, present options for what the plan should specify, and recommend the more complete option. Use the preamble's AskUserQuestion Format section. + +Handling modes for missing prerequisites: + +- **Block** — the prerequisite is critical and the review cannot proceed meaningfully without it. Use for prerequisites where the absence makes the subsequent review sections unreliable (e.g., no language/runtime specified means architecture review is speculative). AskUserQuestion should not offer a "skip" option; the user must specify or cancel the review. +- **Interactive generate** — the reviewer proposes a concrete specification by examining the codebase (existing config files, dependency manifests, Dockerfiles, scripts) and asks the user to confirm or edit. Use for prerequisites where the codebase already has partial answers and the reviewer can fill in the gaps. AskUserQuestion should offer: A) Accept proposed specification, B) Edit the proposal, C) Defer to "NOT in scope". +- **Flag and defer** — the prerequisite is important but not blocking. The gap is recorded and the review proceeds. Use for prerequisites that improve quality but don't invalidate earlier findings. + +1. **Language and runtime specification** — Does the plan specify what programming language(s) and runtime version(s) will be used? If the project uses multiple languages or runtimes, all must be listed. Example: "Python 3.12, Node 20.x, Ruby 3.3." **Handling: Block** if no language/runtime is specified at all (makes architecture review speculative). **Interactive generate** if partially specified — examine `package.json`, `pyproject.toml`, `Gemfile`, `go.mod`, `Cargo.toml`, `.tool-versions`, `.nvmrc`, `.python-version`, and similar manifests to propose the full specification. + +2. **Language, runtime, and dependency installation** — Does the plan specify how each language and runtime will be installed in the development environment when a new contributor first sets up the repo, and how project dependencies will be installed after that? Prefer isolated version managers (nvm, pyenv, rbenv, mise, asdf, etc.) over system-wide installs. The plan should specify both the runtime installation and the dependency install command (e.g., `npm install`, `bundle install`, `pip install -r requirements.txt`). For monorepos or projects with private registries, native dependencies (libvips, psycopg2), or workspace tooling (Turborepo, Nx, Lerna), the plan should call out these requirements explicitly. The full installation method should be documented in the plan or in a setup script/contributor guide that the plan references. If a `Dockerfile` or `devcontainer.json` provides this, the plan should reference it explicitly. **Handling: Interactive generate** — examine existing `Dockerfile`, `devcontainer.json`, `.tool-versions`, `Makefile`, `justfile`, `scripts/setup`, `bin/setup`, lockfiles (`package-lock.json`, `yarn.lock`, `Gemfile.lock`, `poetry.lock`), and monorepo configs (`turbo.json`, `nx.json`, `lerna.json`) to propose a combined runtime + dependency installation specification. If nothing exists, propose the appropriate version manager and install command based on the language. + +3. **Development environment build and run method** — Does the plan specify how the application will be built and run in the development environment? Many projects need a build or compilation step before running (TypeScript compilation, asset bundling with webpack/vite/esbuild, CSS preprocessing, protobuf codegen). The plan should specify: (a) the build command and whether it runs in watch mode during development, and (b) the run command to start the application. Prefer environments that are reproducible and similar to production (Docker containers, devcontainers, Nix flakes). If the plan specifies "run locally" without isolation, flag it and recommend containerization. If the plan assumes the app just starts without a build step but the codebase requires one, flag that as a gap. **Handling: Interactive generate** — examine `docker-compose.yml`, `Procfile`, `Makefile`, `justfile`, `package.json` scripts, `.env.example`, and build configs (`tsconfig.json`, `webpack.config.*`, `vite.config.*`, `esbuild.*`) to propose exact build and run commands. If containerization exists, reference it. If not, propose a `docker-compose.yml` or `devcontainer.json` scaffold. + +4. **Local data seeding** — Does the plan specify how to generate or seed data to populate the local environment so that features can be realistically tested? A development environment without realistic data makes testing impossible. The plan should include seed scripts, fixture generators, or data dump instructions — not leave it as a manual step. If the plan relies on production data copies, flag the security implications. **Handling: Interactive generate** — examine `db/seeds.rb`, `prisma/seed.ts`, `scripts/seed*`, `fixtures/`, `factories/`, and similar seed/fixture infrastructure. Propose a seeding approach based on what exists. If nothing exists, propose seed scripts appropriate to the stack (factory-based, fixture-based, or snapshot-based). + +5. **End-to-end testing with headless browser** — Does the plan include goals for end-to-end (E2E) testing of the application using a headless browser (Playwright, Cypress, Puppeteer, etc.)? E2E tests are the only way to verify that the full stack works together. The plan should specify: which E2E framework, which critical flows to cover, and how E2E tests will run in CI. If the plan omits E2E testing entirely, flag it as a gap — unit and integration tests alone cannot catch cross-layer integration failures. **Handling: Flag and defer** — E2E testing is important but rarely blocks an architecture review. Examine existing E2E config (`playwright.config.*`, `cypress.config.*`, `e2e/` directory) to assess what exists. If missing, recommend a framework based on the stack and offer to draft E2E test goals, but allow deferral to "NOT in scope" without blocking. + +6. **Environment variables and configuration** — Does the plan specify which environment variables are required vs optional, provide `.env.example` with sensible defaults, and document how to obtain secrets (e.g., "ask team lead for Stripe key")? The most common reason a new contributor cannot start an app is missing environment configuration. The plan should list required env vars, provide example values, and note which secrets need manual provisioning. **Handling: Interactive generate** — examine `.env.example`, `.env.local.example`, config modules, `docker-compose.yml` environment sections, and application config files to propose a complete env var specification. If nothing exists, scaffold a `.env.example` from the application's config loading code. + +For each gap found, document it in the plan's "NOT in scope" section if the user defers it, add it to the implementation tasks if the user accepts it, or block until specified if the handling mode requires it. + ## Review Sections (after scope is agreed) **Anti-skip rule:** Never condense, abbreviate, or skip any review section (1-4) regardless of plan type (strategy, spec, code, infra). Every section in this skill exists for a reason. "This is a strategy doc so implementation sections don't apply" is always wrong — implementation details are where strategy breaks down. If a section genuinely has zero findings, say "No issues found" and move on — but you must evaluate it. @@ -1543,6 +1569,7 @@ this run (an empty file means "ran, no findings" — distinct from "didn't run") ### Completion summary At the end of the review, fill in and display this summary so the user can see all findings at a glance: - Step 0: Scope Challenge — ___ (scope accepted as-is / scope reduced per recommendation) +- Pre-Build Prerequisites: ___ of 6 checks passed (language/runtime spec, installation, build+run, data seeding, E2E testing, env config) - Architecture Review: ___ issues found - Code Quality Review: ___ issues found - Test Review: diagram produced, ___ gaps identified diff --git a/plan-eng-review/SKILL.md.tmpl b/plan-eng-review/SKILL.md.tmpl index 09f5b163af..1fdaf971ad 100644 --- a/plan-eng-review/SKILL.md.tmpl +++ b/plan-eng-review/SKILL.md.tmpl @@ -107,9 +107,9 @@ Before reviewing anything, answer these questions: If the plan rolls a custom solution where a built-in exists, flag it as a scope reduction opportunity. Annotate recommendations with **[Layer 1]**, **[Layer 2]**, **[Layer 3]**, or **[EUREKA]** (see preamble's Search Before Building section). If you find a eureka moment — a reason the standard approach is wrong for this case — present it as an architectural insight. 5. **TODOS cross-reference:** Read `TODOS.md` if it exists. Are any deferred items blocking this plan? Can any deferred items be bundled into this PR without expanding scope? Does this plan create new work that should be captured as a TODO? -5. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the lake. +6. **Completeness check:** Is the plan doing the complete version or a shortcut? With AI-assisted coding, the cost of completeness (100% test coverage, full edge case handling, complete error paths) is 10-100x cheaper than with a human team. If the plan proposes a shortcut that saves human-hours but only saves minutes with CC+gstack, recommend the complete version. Boil the lake. -6. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: +7. **Distribution check:** If the plan introduces a new artifact type (CLI binary, library package, container image, mobile app), does it include the build/publish pipeline? Code without distribution is code nobody can use. Check: - Is there a CI/CD workflow for building and publishing the artifact? - Are target platforms defined (linux/darwin/windows, amd64/arm64)? - How will users download or install it (GitHub Releases, package manager, container registry)? @@ -125,6 +125,32 @@ Always work through the full interactive review: one section at a time (Architec **Critical: Once the user accepts or rejects a scope reduction recommendation, commit fully.** Do not re-argue for smaller scope during later review sections. Do not silently reduce scope or skip planned components. +### Pre-Build Prerequisites Check + +Before the review sections, verify the plan covers these development prerequisites. If any are missing, flag them as gaps — a plan that doesn't specify how to build, run, and test the application is incomplete regardless of how good the architecture is. + +For each missing prerequisite, call AskUserQuestion individually, present options for what the plan should specify, and recommend the more complete option. Use the preamble's AskUserQuestion Format section. + +Handling modes for missing prerequisites: + +- **Block** — the prerequisite is critical and the review cannot proceed meaningfully without it. Use for prerequisites where the absence makes the subsequent review sections unreliable (e.g., no language/runtime specified means architecture review is speculative). AskUserQuestion should not offer a "skip" option; the user must specify or cancel the review. +- **Interactive generate** — the reviewer proposes a concrete specification by examining the codebase (existing config files, dependency manifests, Dockerfiles, scripts) and asks the user to confirm or edit. Use for prerequisites where the codebase already has partial answers and the reviewer can fill in the gaps. AskUserQuestion should offer: A) Accept proposed specification, B) Edit the proposal, C) Defer to "NOT in scope". +- **Flag and defer** — the prerequisite is important but not blocking. The gap is recorded and the review proceeds. Use for prerequisites that improve quality but don't invalidate earlier findings. + +1. **Language and runtime specification** — Does the plan specify what programming language(s) and runtime version(s) will be used? If the project uses multiple languages or runtimes, all must be listed. Example: "Python 3.12, Node 20.x, Ruby 3.3." **Handling: Block** if no language/runtime is specified at all (makes architecture review speculative). **Interactive generate** if partially specified — examine `package.json`, `pyproject.toml`, `Gemfile`, `go.mod`, `Cargo.toml`, `.tool-versions`, `.nvmrc`, `.python-version`, and similar manifests to propose the full specification. + +2. **Language, runtime, and dependency installation** — Does the plan specify how each language and runtime will be installed in the development environment when a new contributor first sets up the repo, and how project dependencies will be installed after that? Prefer isolated version managers (nvm, pyenv, rbenv, mise, asdf, etc.) over system-wide installs. The plan should specify both the runtime installation and the dependency install command (e.g., `npm install`, `bundle install`, `pip install -r requirements.txt`). For monorepos or projects with private registries, native dependencies (libvips, psycopg2), or workspace tooling (Turborepo, Nx, Lerna), the plan should call out these requirements explicitly. The full installation method should be documented in the plan or in a setup script/contributor guide that the plan references. If a `Dockerfile` or `devcontainer.json` provides this, the plan should reference it explicitly. **Handling: Interactive generate** — examine existing `Dockerfile`, `devcontainer.json`, `.tool-versions`, `Makefile`, `justfile`, `scripts/setup`, `bin/setup`, lockfiles (`package-lock.json`, `yarn.lock`, `Gemfile.lock`, `poetry.lock`), and monorepo configs (`turbo.json`, `nx.json`, `lerna.json`) to propose a combined runtime + dependency installation specification. If nothing exists, propose the appropriate version manager and install command based on the language. + +3. **Development environment build and run method** — Does the plan specify how the application will be built and run in the development environment? Many projects need a build or compilation step before running (TypeScript compilation, asset bundling with webpack/vite/esbuild, CSS preprocessing, protobuf codegen). The plan should specify: (a) the build command and whether it runs in watch mode during development, and (b) the run command to start the application. Prefer environments that are reproducible and similar to production (Docker containers, devcontainers, Nix flakes). If the plan specifies "run locally" without isolation, flag it and recommend containerization. If the plan assumes the app just starts without a build step but the codebase requires one, flag that as a gap. **Handling: Interactive generate** — examine `docker-compose.yml`, `Procfile`, `Makefile`, `justfile`, `package.json` scripts, `.env.example`, and build configs (`tsconfig.json`, `webpack.config.*`, `vite.config.*`, `esbuild.*`) to propose exact build and run commands. If containerization exists, reference it. If not, propose a `docker-compose.yml` or `devcontainer.json` scaffold. + +4. **Local data seeding** — Does the plan specify how to generate or seed data to populate the local environment so that features can be realistically tested? A development environment without realistic data makes testing impossible. The plan should include seed scripts, fixture generators, or data dump instructions — not leave it as a manual step. If the plan relies on production data copies, flag the security implications. **Handling: Interactive generate** — examine `db/seeds.rb`, `prisma/seed.ts`, `scripts/seed*`, `fixtures/`, `factories/`, and similar seed/fixture infrastructure. Propose a seeding approach based on what exists. If nothing exists, propose seed scripts appropriate to the stack (factory-based, fixture-based, or snapshot-based). + +5. **End-to-end testing with headless browser** — Does the plan include goals for end-to-end (E2E) testing of the application using a headless browser (Playwright, Cypress, Puppeteer, etc.)? E2E tests are the only way to verify that the full stack works together. The plan should specify: which E2E framework, which critical flows to cover, and how E2E tests will run in CI. If the plan omits E2E testing entirely, flag it as a gap — unit and integration tests alone cannot catch cross-layer integration failures. **Handling: Flag and defer** — E2E testing is important but rarely blocks an architecture review. Examine existing E2E config (`playwright.config.*`, `cypress.config.*`, `e2e/` directory) to assess what exists. If missing, recommend a framework based on the stack and offer to draft E2E test goals, but allow deferral to "NOT in scope" without blocking. + +6. **Environment variables and configuration** — Does the plan specify which environment variables are required vs optional, provide `.env.example` with sensible defaults, and document how to obtain secrets (e.g., "ask team lead for Stripe key")? The most common reason a new contributor cannot start an app is missing environment configuration. The plan should list required env vars, provide example values, and note which secrets need manual provisioning. **Handling: Interactive generate** — examine `.env.example`, `.env.local.example`, config modules, `docker-compose.yml` environment sections, and application config files to propose a complete env var specification. If nothing exists, scaffold a `.env.example` from the application's config loading code. + +For each gap found, document it in the plan's "NOT in scope" section if the user defers it, add it to the implementation tasks if the user accepts it, or block until specified if the handling mode requires it. + ## Review Sections (after scope is agreed) **Anti-skip rule:** Never condense, abbreviate, or skip any review section (1-4) regardless of plan type (strategy, spec, code, infra). Every section in this skill exists for a reason. "This is a strategy doc so implementation sections don't apply" is always wrong — implementation details are where strategy breaks down. If a section genuinely has zero findings, say "No issues found" and move on — but you must evaluate it. @@ -271,6 +297,7 @@ Format: `Lane A: step1 → step2 (sequential, shared models/)` / `Lane B: step3 ### Completion summary At the end of the review, fill in and display this summary so the user can see all findings at a glance: - Step 0: Scope Challenge — ___ (scope accepted as-is / scope reduced per recommendation) +- Pre-Build Prerequisites: ___ of 6 checks passed (language/runtime spec, installation, build+run, data seeding, E2E testing, env config) - Architecture Review: ___ issues found - Code Quality Review: ___ issues found - Test Review: diagram produced, ___ gaps identified