diff --git a/.claude/skills/verify-changes/SKILL.md b/.claude/skills/verify-changes/SKILL.md new file mode 100644 index 000000000..333caa186 --- /dev/null +++ b/.claude/skills/verify-changes/SKILL.md @@ -0,0 +1,112 @@ +--- +name: verify-changes +description: How to test and verify work in the dev-3.0 repo — which vitest config covers what, how to write a test that fits the house style, mocking Electrobun RPC and i18n providers, what coverage is actually expected, and the browser QA hand-off. Use when writing or fixing tests, deciding what a change needs covered, hitting a failing or flaky suite, or preparing a change for review. Triggers — "write tests for this", "which config runs this", "how do I mock the RPC", "is this covered enough", "the suite is failing". +--- + +# verify-changes — testing and verification in dev-3.0 + +The two hard gates (lint + touched tests before push, full suite before a PR) live in +`AGENTS.md` and apply whether or not you read this file. Everything here is the detail +behind them: which runner covers what, how to write a test that fits, and what "enough" +means. + +## Which config runs what + +**Vitest** with `happy-dom` and React Testing Library. Three configs, three independent +processes: + +| Config | Covers | Script | +|---|---|---| +| `vitest.config.ts` | renderer (`src/mainview/`) | part of `bun run test` | +| `vitest.config.bun.ts` | backend (`src/bun/`) | `bun run test:bun` | +| `vitest.config.cli.ts` | CLI (`src/cli/`) | `bun run test:cli` | + +```bash +bun run test # mainview + bun + cli in parallel, minus 3 slow e2e files (~6s) +bun run test:full # everything incl. slow e2e (~42s) — CI/PR only +bun run test:watch # watch mode +``` + +Running vitest directly (outside `bun run`): `bunx vitest run`, never `npx`. + +Narrowing while iterating: `bunx vitest run `, `-t ""`, `--repeat ` for +suspected flakes. Because the three configs are separate processes, a failure in one says +nothing about the others — read which prefix (`[mainview]` / `[bun]` / `[cli]`) failed. + +**Local E2E policy:** do not run the full E2E suite locally — `bun run test:full` and any +equivalent unfiltered command are reserved for CI/PR validation. Investigating a specific +behavior means running that one E2E file or test case. + +## What to test + +- **Unit (mandatory)** — state reducer actions and their edge cases, all pure + functions/utils/parsers, every RPC handler (happy path plus 2-3 error cases), CLI commands + (parsing, validation, output), data-layer CRUD plus corrupt-data handling, git operations + with mocked spawn, i18n interpolation and pluralization for every locale. +- **Component (mandatory)** — every major interactive component: board views, task cards, + modals, settings panels. +- **E2E (CLI-based)** — full lifecycle through the CLI plus Unix socket against a real app + process in a tmpdir: task lifecycle (create → move statuses → complete), project CRUD, + worktree creation and cleanup, notes CRUD, CLI context auto-detection, concurrent writes + with no data corruption. + +## House style + +- One logical assertion per test; no dependencies between tests. +- Always `userEvent`, never `fireEvent`. Test behavior, not implementation. +- Mock only external boundaries — git, tmux, fs, Electrobun. Never mock internal modules. +- No `sleep` and no timer juggling; use proper `async`/`await`. +- Tests live in `__tests__/` next to their module, e.g. + `src/mainview/components/__tests__/Dashboard.test.tsx`. +- Avoid the patterns that make a suite flake under CI load: index-based queries + (`getAllBy…[0]`), structural traversal (`.parentElement`, `.closest(…)`), and assertions + wedged between two `userEvent` interactions without an intervening `waitFor`. + +### Mocking Electrobun RPC and providers + +Components importing `api` from `rpc.ts` need the Electrobun native module mocked: + +```ts +vi.mock("../../rpc", () => ({ + api: { request: { listDirectory: vi.fn(), addProject: vi.fn() /* … */ } }, +})); +``` + +Components calling `useT()` must render inside `` (import from `../../i18n`). + +Handler tests mock the `tmux` singleton the same way they mock `rpc.ts`; the tmux client's +own tests inject a fake spawn instead. + +## Bug fixing — reproduce first + +Write the failing test that reproduces the bug (red), then fix until it passes (green), and +commit test and fix together. The rare exception is a bug that genuinely cannot be +reproduced in a test (OS-specific timing, hardware, unmockable third-party behavior) — +default to writing the test. + +For a suspected flake, reproduce **under load** before theorising: `--repeat`, several +concurrent vitest processes, and the file run together with its neighbours rather than +alone (cross-test leakage vanishes in isolation). Never "fix" a flake with `retry`, `skip`, +or a bumped timeout on its own. + +## Coverage — expectations, not a gate + +Two numbers, no per-metric split: **~70% for normal code, ~85% for critical modules.** + +- **Critical modules** (a silent regression here is expensive): `state.ts`, + `src/shared/types.ts` helpers, `src/mainview/i18n/`, `src/cli/`, `src/bun/data.ts`, + `src/bun/git.ts`, `src/bun/tmux/`, `src/mainview/utils/`. +- **Not expected to be covered** (bootstrap/wrappers that only make sense in e2e): + `src/bun/index.ts`, `updater.ts`, `shell-env.ts`, `spawn.ts`, `src/mainview/rpc.ts`, + `main.tsx`. + +**No coverage provider is wired up** — no `coverage` block in any vitest config, nothing +installed, no CI gate. These are review-time expectations, so never cite a percentage as if +a tool measured it. What gets rejected in review is a change that leaves its area, or a +critical module, visibly less tested than before. + +## Visual surfaces + +A change to anything the user sees is not verified by a green suite. Drive the running UI in +a browser, screenshot it, read the console — the recipe (isolated browser session per task, +streamer mode, serving the app) is the **`/debug-ui`** skill. diff --git a/AGENTS.md b/AGENTS.md index d1efa1a27..dda74cd44 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,7 +36,7 @@ Before designing or implementing **anything** UI/UX-related — a new screen, su If the manifest is stale or missing, regenerate it with `/ux-create-manifest`. Keep `docs/ux/` updated whenever surfaces or the action taxonomy change. -**Bookend `/ux-principal` with `/debug-ui`:** *before* planning, drive the current UI and screenshot the zones you're about to touch — grounding the plan in what's actually on screen beats reasoning from memory; *after* implementing, verify in a browser before review. See [Manual UI QA in a browser](#manual-ui-qa-in-a-browser). +**Screenshot the zone before you plan it.** Driving the current UI beats reasoning from memory about what is on screen — same tooling as the mandatory QA pass afterwards, see [Manual UI QA in a browser](#manual-ui-qa-in-a-browser-mandatory). ## No native dialogs — ever (remote/browser mode) (MANDATORY) @@ -167,17 +167,9 @@ Rules: **Count:** 0 by default; 1 if the gate passes; 2 only for a genuine flagship (score 5). Ship tips in the same commit as the feature. **Never write a tip for:** self-describing UI, visible visual states (spinners, glows, badges), settings toggles that restate their label, behavior users already expect, anything met naturally on the happy path. -**Files:** registry `src/mainview/tips.ts` (`ALL_TIPS` array); i18n keys `tip..title` / `tip..body` in `{en,ru,es}.ts`. **Content:** title 3–6 words; body one sentence max ~120 chars — tell the user *what to do*, no fluff; icon = Nerd Font glyph (`\u{XXXXX}`). +**Files:** registry `src/mainview/tips.ts` (`ALL_TIPS` array); i18n keys `tip..title` / `tip..body` in `{en,ru,es}.ts`. **Content:** title 3–6 words; body one sentence max ~120 chars — tell the user *what to do*, no fluff. -**Coolness score (mandatory `score` field, 1–5 where 5 is coolest).** Tips surface highest tier first, random within a tier (see `selectTip` in `tips.ts`). Self-assign the score with this rubric — do NOT ask the user: - -- **5** — flagship demo-reel "wow" that sells the product: multi-agent variants, bug-hunter swarm, CoW worktree deps, AI Review, live terminal preview. -- **4** — strong distinctive capability most users will love: agent-driven PRs, command palette, OSC52 clipboard, port auto-allocation, image/large-text paste. -- **3** — useful everyday convenience that is still non-obvious: search operators, right-click open, hover previews. -- **2** — minor convenience or settings toggle. Below the gate — normally no tip. -- **1** — niche/power-user trivia. Never add. - -When unsure between two tiers, pick the lower — and below 3 the answer is usually "no tip". Append new tips at the end of `ALL_TIPS`. **Registry hygiene:** if a new tip supersedes/overlaps an older one, delete or merge the old one in the same commit (its `ALL_TIPS` entry + keys in all three locales). +**The 1–5 coolness rubric lives on the `TipScore` type** in `src/mainview/tips.ts` — read it there when scoring, and keep it there when it changes. Append new tips at the end of `ALL_TIPS`. **Registry hygiene:** if a new tip supersedes/overlaps an older one, delete or merge the old one in the same commit (its `ALL_TIPS` entry + keys in all three locales). ## Keyboard shortcuts @@ -233,7 +225,7 @@ bun run dev # Main local development flow (build, package, launch local bun run start # Alternative launch path (reuses existing Vite output) bun run build # Build (staging channel) bun run build:prod # Build (production channel) -bun run lint # TypeScript type-check — must pass before committing +bun run lint # TypeScript type-check — must pass before pushing ``` **HMR / Vite watch is NOT used in this project.** Never run `bun run watch`, `bun run hmr`, or any `vite --watch` flow — the only supported dev loop is `bun run dev`. **Never run `bun run bump`** — versioning is owned by the user, not AI agents. @@ -344,19 +336,17 @@ All user-facing renderer strings are localized via `src/mainview/i18n/`; locales ## Testing -**Framework: Vitest** with `happy-dom` and React Testing Library. Three configs: `vitest.config.ts` (mainview), `vitest.config.bun.ts` (backend), `vitest.config.cli.ts` (CLI). +**Framework: Vitest** with `happy-dom` and React Testing Library. Three configs run as three independent processes: `vitest.config.ts` (renderer), `vitest.config.bun.ts` (backend), `vitest.config.cli.ts` (CLI). ```bash -bun run test # Fast — mainview + bun + cli in parallel, excludes 3 slow e2e files (~6s) -bun run test:full # Everything incl. slow e2e files (~42s, for CI/PR) -bun run test:bun # Backend tests only -bun run test:cli # CLI tests -bun run test:watch # Watch mode +bun run test # renderer + backend + cli in parallel, minus 3 slow e2e files (~6s) +bun run test:full # everything incl. slow e2e (~42s) — CI/PR only, not local +bun run test:bun # backend only +bun run test:cli # CLI only +bun run test:watch # watch mode ``` -Running vitest directly (outside `bun run`): use `bunx vitest run`, not `npx`. - -**Local E2E policy:** Do not run the complete E2E suite locally (`bun run test:full` or an equivalent unfiltered command); it is reserved for CI/PR validation. When investigating or verifying a specific behavior, run only the targeted E2E file or test case. +**Everything else about testing here — which config covers what, what a change needs covered, house style, mocking Electrobun RPC and `useT()`, reproducing flakes, coverage expectations — is the [`/verify-changes`](.claude/skills/verify-changes/SKILL.md) skill.** Load it when writing or fixing tests. The gates below apply whether or not you read it. ### Verification gates (MANDATORY) @@ -365,60 +355,29 @@ Two gates, escalating. Committing itself has no gate — commit freely, verify b 1. **Before `git push`** — `bun run lint` plus the tests covering what you touched. A push that breaks type-checking is unacceptable even if the tests pass. 2. **Before `gh pr create`, before enabling auto-merge, and again after any rebase** — the **full** `bun run test`, green end-to-end. Only the file you edited is NOT sufficient: sibling test files assert against the same components (e.g. `TaskCard.tsx` is covered by both `TaskCard.test.tsx` AND `TaskCardSeq.test.tsx`), and a rebase pulls in code your run never saw. Fix and re-run until green BEFORE opening the PR — don't open it and watch CI go red. -### Manual UI QA in a browser - -**Self-QA UI changes in a browser before review — it's the default.** Any change touching what the user sees can break subtly (layout shift, overflow on one viewport, console error, wrong state render). Drive the running UI, look at a screenshot, check console errors before handing off. **"It's small" is not a reason to skip** — small UI changes slip through the most. Only real exceptions: no visual surface at all, or the UI genuinely can't be brought up. When in doubt, QA it. - -With the dev-server running and the project's Port Allocation ≥ 1, the dev app already serves the web UI at `http://localhost:/?token=` (`dev3 dev-server status` → `DEV3_PORT0`; see [decision 093](decisions/093-dev-remote-port-from-pool.md)). Otherwise serve it yourself: `dev3 remote --no-tunnel --static-code --port `. Point `agent-browser` at the URL. **Each task must drive its own isolated browser session** — `export AGENT_BROWSER_SESSION="dev3-${DEV3_TASK_ID%%-*}"` — otherwise parallel agents share one global browser and stomp each other's QA. Full recipe: the **`/debug-ui`** skill (`.claude/skills/debug-ui/SKILL.md`; dev-internal tooling, not dev3-shipped). - -**Screenshots are taken in streamer mode — always (MANDATORY).** The developer's real accounts/emails/paths are live in the app, and any screenshot can end up in a PR, an issue, or a recording. Append `&streamer=on` to the app URL when opening it with `agent-browser` — it enables **streamer mode** (privacy masking: blurs account emails/labels, orgs, home-dir paths, tunnel URLs, the remote-access QR; see [decision 161](decisions/161-streamer-mode-css-blur-masking.md)). The only exception: the task itself is about verifying those unmasked values — then capture the minimum needed and say so. Users toggle the same mode via Settings → Appearance or the ⇧⌘P palette ("Toggle streamer mode"). - -### Coverage expectations - -Two numbers, no per-metric split: **~70% for normal code, ~85% for critical modules.** - -**Critical modules** (the ones where a silent regression is expensive): `state.ts`, `src/shared/types.ts` (helpers), `src/mainview/i18n/`, `src/cli/`, `src/bun/data.ts`, `src/bun/git.ts`, `src/bun/tmux/`, `src/mainview/utils/`. - -**Not expected to be covered** (bootstrap/wrappers that only make sense in e2e): `src/bun/index.ts`, `updater.ts`, `shell-env.ts`, `spawn.ts`, `src/mainview/rpc.ts`, `main.tsx`. +### Manual UI QA in a browser (MANDATORY) -**No coverage provider is wired up** — the vitest configs have no `coverage` block, nothing is installed, and CI does not gate on it. So these are review-time expectations, not a machine check: a change that leaves a critical module visibly less tested than it was gets rejected in review, and nobody should cite a percentage as if a tool measured it. +**A green suite does not verify a visual surface.** Any change touching what the user sees can break subtly — layout shift, overflow on one viewport, console error, wrong state render. Drive the running UI, look at a screenshot, read the console before handing off. **Being a small change is not a reason to skip it** — small UI changes slip through the most. Only real exceptions: no visual surface at all, or the UI genuinely cannot be brought up. The full recipe (serving the app, the per-task isolated browser session, `agent-browser` usage) is the [`/debug-ui`](.claude/skills/debug-ui/SKILL.md) skill. -### What to test - -- **Unit (mandatory):** state reducer actions + edge cases, all pure functions/utils/parsers, every RPC handler (happy path + 2-3 error cases), CLI commands (parsing + validation + output), data layer CRUD + corrupt data handling, git operations with mocked spawn, i18n interpolation + pluralization for all locales. -- **Component (mandatory):** all major interactive components — board views, task cards, modals, settings panels. Always `userEvent`, not `fireEvent`. Test behavior, not implementation. -- **E2E (CLI-based):** full lifecycle through CLI + Unix socket against a real app process with tmpdir — task lifecycle (create → move statuses → complete), project CRUD, worktree creation + cleanup, notes CRUD, CLI context auto-detection, concurrent writes (no data corruption). - -### Bug fixing workflow — reproduce first - -**Always start by writing a failing test that reproduces the bug** (red), then fix the code until it passes (green); commit test + fix together. Exception (rare): the bug genuinely can't be reproduced in a test (OS-specific timing, hardware, unmockable third-party behavior) — default to writing the test first. - -### Test writing rules - -- One logical assertion per test; no dependencies between tests. -- Mock only external boundaries (git, tmux, fs, Electrobun), not internal modules. -- No `sleep`/timers — use proper async/await. -- Every new feature or bug fix must include tests; a PR that leaves its area visibly less tested than before gets rejected. -- Tests live in `__tests__/` directories next to their modules (e.g., `src/mainview/components/__tests__/Dashboard.test.tsx`). - -### Mocking Electrobun RPC / providers - -Components that import `api` from `rpc.ts` need the Electrobun native module mocked: - -```ts -vi.mock("../../rpc", () => ({ - api: { request: { listDirectory: vi.fn(), addProject: vi.fn() /* … */ } }, -})); -``` +**Screenshots are taken in streamer mode — always.** The developer's real accounts, emails, and paths are live in the app, and any screenshot can end up in a PR, an issue, or a recording. Append `&streamer=on` to the app URL — it blurs account emails/labels, orgs, home-dir paths, tunnel URLs, and the remote-access QR (see [decision 161](decisions/161-streamer-mode-css-blur-masking.md)). The only exception is a task about verifying those unmasked values: capture the minimum needed and say so. -Components using `useT()` must be rendered inside `` (import from `../../i18n`). +## Key files -## Key config files +Open the file itself rather than trusting a paraphrase — this list exists so you know it is there. - `electrobun.config.ts` — Electrobun app config (name, identifier, build copy rules) - `vite.config.ts` — Vite config (root: `src/mainview`, output: `dist/`) - `tailwind.config.js` — Tailwind scans `src/mainview/**/*.{html,js,ts,jsx,tsx}` - `tsconfig.json` — strict mode, ES2020 target, bundler module resolution +- `src/shared/types.ts` — `AppRPCSchema`, `Task`/`Project`, `STATUS_COLORS` +- `src/bun/rpc-handlers.ts` — barrel indexing every `rpc-handlers/*.ts` domain +- `src/mainview/state.ts` — the reducer: every action and state field the UI has +- `src/mainview/index.css` — design tokens, both themes +- `src/mainview/keymap.ts` — every app-level keyboard shortcut +- `src/bun/agent-skills.ts` — `SKILL_CONTENT`, the skill text shipped to agent config dirs +- `src/shared/cli-exit-codes.ts` — public CLI exit-code contract +- `.github/workflows/` — what CI actually runs (incl. the sharded test job) +- `decisions/` — 280+ records of why non-obvious things are as they are; grep before assuming ## Documentation diff --git a/change-logs/2026/07/25/docs-verify-changes-skill.md b/change-logs/2026/07/25/docs-verify-changes-skill.md new file mode 100644 index 000000000..f41c2695c --- /dev/null +++ b/change-logs/2026/07/25/docs-verify-changes-skill.md @@ -0,0 +1 @@ +Moved the testing detail out of `AGENTS.md` into a new repo-local `/verify-changes` skill (which vitest config covers what, what a change needs covered, house style, mocking Electrobun RPC and `useT()`, reproducing flakes under load, coverage expectations), leaving the two verification gates and the browser-QA mandate inline where they cannot be missed. The 1-5 tip coolness rubric now lives on a `TipScore` union type in `src/mainview/tips.ts` instead of being restated in prose, and the key-files list gained the entries an agent is most likely to overlook: shared types, the RPC handler barrel, the reducer, design tokens, the keymap, shipped skill text, CLI exit codes, CI workflows, and the decision records. diff --git a/src/mainview/tips.ts b/src/mainview/tips.ts index eec7aa83e..3c9f91bea 100644 --- a/src/mainview/tips.ts +++ b/src/mainview/tips.ts @@ -9,19 +9,30 @@ import type { TipState } from "../shared/types"; */ export type TipContext = "board" | "terminal" | "diff" | "settings" | "preparing"; +/** + * Coolness tier, surfaced highest-tier-first (all 5s before any 4), random + * within a tier — see selectTip(). Self-assess honestly; never ask the user: + * + * - 5 — flagship demo-reel "wow" that sells the product: multi-agent variants, + * bug-hunter swarm, CoW worktree deps, AI Review, live terminal preview. + * - 4 — strong distinctive capability most users will love: agent-driven PRs, + * command palette, OSC52 clipboard, port auto-allocation, image paste. + * - 3 — useful everyday convenience that is still non-obvious: search + * operators, right-click open, hover previews. Lowest tier worth adding. + * - 2 — minor convenience or a settings toggle. Below the bar: normally no tip. + * - 1 — niche/power-user trivia. Never add. + * + * Torn between two tiers? Pick the lower. Below 3 the answer is usually "no + * tip at all" — see AGENTS.md "Feature discovery tips" for when one is earned. + */ +export type TipScore = 1 | 2 | 3 | 4 | 5; + export interface Tip { id: string; titleKey: TranslationKey; bodyKey: TranslationKey; icon: string; // Nerd Font codepoint - /** - * Coolness / priority tier, 1–5 where 5 is the coolest. - * Tips are surfaced highest-tier-first (all 5s before any 4, etc.), - * picked at random within a tier. See selectTip(). - * When adding a new tip, score it with the rubric in AGENTS.md - * ("Feature discovery tips"). - */ - score: number; + score: TipScore; /** * Surfaces where this tip is most relevant (required, non-empty). The tip * carrier for a given surface passes its context to selectTip(), which