From fdddcedeed48e4bbc6321dfeb64e922c0805988c Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 8 Oct 2026 15:10:11 +0200 Subject: [PATCH 1/4] docs(agents): add a code review guide for agents Adds .agents/guidelines/code-review.md with evidence rules, per-area checks, structural maintainability findings, verification expectations, and comment format, and points to it from AGENTS.md. --- .agents/guidelines/code-review.md | 144 ++++++++++++++++++++++++++++++ AGENTS.md | 2 + 2 files changed, 146 insertions(+) create mode 100644 .agents/guidelines/code-review.md diff --git a/.agents/guidelines/code-review.md b/.agents/guidelines/code-review.md new file mode 100644 index 000000000000..e1290c40a1db --- /dev/null +++ b/.agents/guidelines/code-review.md @@ -0,0 +1,144 @@ +# Code review + +This document is canonical for reviewing pull requests and diffs. +`AGENTS.md` owns the pointer to this file. + +Read each changed function in full, and read enough of the surrounding code to know the contract it implements. +Use the linked issue or specification to establish the intended outcome. +Treat PR descriptions, test names, and earlier bot reviews as claims to check, not as evidence. + +## Scope and evidence + +A finding describes either a reachable supported case or a specific violation of a repository rule. +Reading the source is enough when the causal path is complete. +Label inference and unexecuted examples as such. +When a claim depends on compiler, bundler, browser, or dependency behavior, run a focused probe if execution is available and permitted. +Post the probe as a command a maintainer can rerun. + +## Checks by changed code + +### Exported types, package entries, or dependencies + +- Follow `package.json` `exports` and declaration generation; an internal filename does not establish a public export. +- Check accepted consumer expressions, inference, supported versions, and dependency ranges in affected sibling packages. +- A caller search inside this repository does not establish compatibility for external consumers. +- Check migration guidance when supported usage or peer dependency floors change. +- Internal APIs follow "migrate every caller, then delete the old path" in the same PR. Public APIs follow the deprecation process instead, so a compatibility layer there is expected, not debt. + +### Shared core, docs blocks, framework or renderer adapters + +- Trace the producer, the shared representation, and every consumer. +- Keep framework-specific policy at its owner unless a deliberate shared contract requires otherwise. A `framework === '...'` or renderer check inside shared code is a finding. +- Check source precedence and consumers that should be unaffected. +- Before requesting reuse of an existing helper, name it and show that its semantics fit. + +### Metadata, inheritance, or composed APIs + +- Follow aliases, inherited members, input-only and output-only exposure, and serialization through downstream consumers. + +### Boundaries: config, CLI arguments, channel events, user files + +- Validate and parse external input where it enters: `main.ts` and preset options, CLI arguments, environment variables, channel messages between manager and preview, and user-authored story files. +- Inside the system, trust the parsed types. Re-validation, defensive `?.` chains, and null checks deep in a call chain mean the boundary is in the wrong place. +- Do not leak a transport or file-format representation through a public API. + +### Workers, child processes, temporary files, or concurrent work + +- Cancel and await started work, or drain it, before removing resources it can still use. A fail-fast `Promise.all` does not cancel its other operations. +- When two actors write the same file, cache entry, or state object, first ask whether they need to share it. Separate write targets beat a lock. +- Flag related updates that can be left half-applied when a more atomic structure is available. + +### CLI commands, automigrations, init, and setup scripts + +- Ask what happens when the operation runs twice, and when the previous run stopped at any point. +- Follow the ordering rules in `code/lib/cli-storybook/src/automigrate/README.md` for anything that can fail after dependencies were already changed. + +### React UI and accessibility + +- Follow the Testing Expectations in `AGENTS.md`. +- Check user-visible state, keyboard and pointer interaction, accessible names and descriptions, and the consumers of the surrounding component. +- Review fresh-page and first-attempt behavior when tests depend on focus, hover, timers, or retained state. + +### Comments, docs, or agent guidance + +- Check factual statements against the reviewed code and configuration. +- State the operational or user consequence of incorrect guidance. A stale sentence is not as severe as a runtime defect. + +## Structure and maintainability + +Correct code can still make the codebase worse. +Do not approve only because the behavior is correct. +Treat each of the following as a finding the author must justify: + +- **File growth.** The diff pushes a file from under 1,000 lines to over 1,000. Compare `wc -l` on base and head. +- **One more branch.** A feature extends an existing `if`/`else` chain or `switch` by one more case, or adds a second boolean that must stay in sync with the first. Ask for the structure that encodes the domain instead: a discriminated union, a lookup table, a state machine. +- **Special cases in busy flows.** New conditionals or edge-case handling dropped into an unrelated or already busy function. +- **Threaded signals.** A new flag passed through several layers of types, options, or pipelines to reach one consumer. Look for a more direct path. +- **Repeated decisions.** The same choice made in several places instead of once, with the result passed along. +- **Layers that do not compress.** A wrapper with one caller, an adapter with one implementation, or a layer that repeats the methods and arguments of the one below it. +- **Hidden state.** New module state, mutable fields, or synced copies where a local, a return value, or a derived value would do. A reader should answer "where does X come from?" and "what can change X?" quickly. +- **Loose types.** Casts, `any`, non-null assertions, or optional fields that permit contradictory combinations. If a comment is needed to explain which field combinations are valid, the type should be a union. A `switch` over a union needs an exhaustive `never` check so the next variant fails to compile. +- **Parallel types.** A hand-written type that duplicates a shape another module or schema already owns. +- **Rearranged complexity.** A refactor that moves code without reducing the number of branches, modes, flags, or layers a reader has to follow. + +Ask what the code would look like if the new requirement had existed from the start. +When that version deletes whole branches or concepts, propose it with a short sketch of the simpler shape. +If it reaches beyond the PR's scope, suggest it as a follow-up instead of blocking on it. + +Do not ask for new helpers, file splits, abstractions, or rewrites that do not remove complexity. +Three similar statements are better than a premature abstraction. +When the same finding would apply to future PRs, suggest the lint rule, type, or check that would catch it, not just the local fix. + +## Verification + +### Bug fixes + +- Prefer a focused test that fails on the unchanged base for the reported reason and passes on head. A setup failure on base is not a reproduction. +- Check that the fix addresses the root cause. A guard, fallback, or `try`/`catch` that silences the symptom is a finding. +- Search for the same pattern elsewhere and say whether other instances share the defect. +- Reuse suitable existing coverage rather than requiring a new test file. Name the uncovered behavior when asking for a regression test. + +### Tests in the diff + +Ask whether each test would still pass if every function it imports returned `undefined`. +If it would, it cannot detect a defect. +The common shapes are: + +- no assertion, or only `toBeDefined`, `toBeTruthy`, or `not.toThrow` +- only `toHaveBeenCalled` or an empty-result assertion, without the payload or resulting state +- an expected value computed by the code under test +- an assertion that restates a constant, default, or table row +- an assertion on data the test built, where the subject never runs + +Ask for one concrete input and the literal expected output or observable effect instead. + +### Refactors and performance changes + +- Tests may pass on both revisions. Check output equivalence plus the claimed improvement. +- Where practical, use a small negative control to show that the assertion detects the relevant defect. +- A performance number needs a run count, a spread, and the input size it was measured on. Ask what else it could be measuring: failed or skipped work, a warm cache, an untouched code path, or noise. + +### Type changes + +- Use consumer examples and type tests (`*.test-d.ts`). Runtime tests with `any` do not prove type compatibility. +- Inspect emitted declarations when packaging is part of the claim. + +### Flaky or retry-dependent tests + +- Inspect the first attempt and rerun the focused case without retries if execution is available. +- Do not prescribe arbitrary repeat counts for unrelated tests. + +## Feedback and severity + +Base severity on the impact you have shown, not on a reporting threshold. +Rank broken supported behavior, public compatibility, resource leaks, data loss, and security defects first, by reach and consequence. +Rank structural regressions from the maintainability list next. +Prefer a few high-confidence comments over many cosmetic ones. + +For a maintainability finding, name the concrete cost, for example two conflicting owners of the same launch policy. + +Write one comment per root cause. +Each comment has a short title, the smallest useful changed line range, the trigger and consequence, the supporting evidence, and a direction for the fix. +Link other affected locations when needed. +Keep open product questions separate from demonstrated defects, and optional suggestions separate from required changes. +Do not mention your own review process or tooling in contributor-facing comments. diff --git a/AGENTS.md b/AGENTS.md index 6b556d268f68..ef42279b5033 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -211,6 +211,8 @@ canonical for its topic; this file owns the pointers. - [Sandboxes](.agents/guidelines/sandboxes.md) — read before generating or debugging a sandbox. - [Testing](.agents/guidelines/testing.md) — read before writing or running tests beyond the commands above; covers memfs filesystem tests and global stubbing rules. +- [Code review](.agents/guidelines/code-review.md) - read before reviewing a PR or a diff; + covers evidence, per-area checks, maintainability findings, and comment format. ## Learned User Preferences From c3d4f88e6ce94c614d53c7becf3fbf6fdc120398 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 8 Oct 2026 15:13:25 +0200 Subject: [PATCH 2/4] docs(agents): give code review its own AGENTS.md section --- AGENTS.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ef42279b5033..eda21059d664 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -185,6 +185,11 @@ Code should be self-explanatory. A comment is only justified when the code canno Before writing or editing any code file, read [`.agents/guidelines/comments-and-jsdoc.md`](.agents/guidelines/comments-and-jsdoc.md) and follow it. Read it once per session, not once per file. +## Code Review + +Before reviewing a pull request or a diff, read [`.agents/guidelines/code-review.md`](.agents/guidelines/code-review.md) and follow it. +It covers what counts as evidence, checks per kind of changed code, structural findings the author must justify, verification expectations, and how to write review comments. + ## Maintenance Rules For Agents - `AGENTS.md` owns the kickstart: identity, base rules, common commands, @@ -211,8 +216,6 @@ canonical for its topic; this file owns the pointers. - [Sandboxes](.agents/guidelines/sandboxes.md) — read before generating or debugging a sandbox. - [Testing](.agents/guidelines/testing.md) — read before writing or running tests beyond the commands above; covers memfs filesystem tests and global stubbing rules. -- [Code review](.agents/guidelines/code-review.md) - read before reviewing a PR or a diff; - covers evidence, per-area checks, maintainability findings, and comment format. ## Learned User Preferences From 1f45a52169cd514afae06650134479eea6f59caa Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 8 Oct 2026 15:15:45 +0200 Subject: [PATCH 3/4] docs(agents): move AGENTS.md detail into linked guidelines AGENTS.md keeps only always-on rules, the never-run list, and a read-before table. Toolchain and commands move to development.md, PR requirements to pull-requests.md, authoring principles and learned preferences to code-authoring.md, and testing expectations into testing.md. --- .agents/guidelines/architecture.md | 2 +- .agents/guidelines/code-authoring.md | 19 ++ .agents/guidelines/code-review.md | 2 +- .agents/guidelines/development.md | 73 ++++++++ .agents/guidelines/nx-and-yarn-task.md | 2 +- .agents/guidelines/pull-requests.md | 17 ++ .agents/guidelines/sandboxes.md | 2 +- .agents/guidelines/testing.md | 24 ++- AGENTS.md | 240 ++++--------------------- 9 files changed, 159 insertions(+), 222 deletions(-) create mode 100644 .agents/guidelines/code-authoring.md create mode 100644 .agents/guidelines/development.md create mode 100644 .agents/guidelines/pull-requests.md diff --git a/.agents/guidelines/architecture.md b/.agents/guidelines/architecture.md index 898235c1214c..99dcb62b2736 100644 --- a/.agents/guidelines/architecture.md +++ b/.agents/guidelines/architecture.md @@ -1,6 +1,6 @@ # Architecture -This document is canonical for repo structure and architecture: renderer vs builder vs framework, the core package layout, key flows, open services and toolsets, and agent-facing skills. `AGENTS.md` owns the pointer to this file and the kickstart commands. +This document is canonical for repo structure and architecture: renderer vs builder vs framework, the core package layout, key flows, open services and toolsets, and agent-facing skills. `AGENTS.md` owns the pointer to this file. ## Repository Structure diff --git a/.agents/guidelines/code-authoring.md b/.agents/guidelines/code-authoring.md new file mode 100644 index 000000000000..e6f903bf1401 --- /dev/null +++ b/.agents/guidelines/code-authoring.md @@ -0,0 +1,19 @@ +# Code Authoring Principles + +This document is canonical for how to write code in this repo. +`AGENTS.md` owns the pointer to this file. + +These are recurring failure modes in agent-authored changes to this repo. +Apply them when writing or reviewing code, not just when asked. + +- **Write comments only for the two reasons in [Comments and JSDoc](./comments-and-jsdoc.md).** That document is the rule; this list does not restate it. +- **Verify environment assumptions empirically before encoding them.** If a design rests on "the bundler strips X" or "this metadata is empty here", prove it with a throwaway probe before building on it (and before writing it into a comment as fact). A 10-line experiment is cheaper than a wrong architecture. +- **Encode assumptions with static checks first.** If an assumption is expected to always hold, prefer making it impossible via TypeScript types and existing lint rules. When static checks are not practical, add a cheap runtime assertion close to the boundary so violations fail loudly at the source. +- **Avoid redundant tests already covered elsewhere.** Do not add tests for code patterns already guaranteed by TypeScript or linting, and do not duplicate coverage that already exists in Storybook `play` functions or Playwright tests. +- **Test contracts (including side effects), not private implementation details.** It is valid to assert side effects when they are part of the public contract. Avoid assertions about internals that are not part of an exported contract, user-visible DOM output, or externally observable behavior. +- **Bias toward broader coverage for security and migrations.** For security-sensitive code paths and legacy data migration logic, prefer handling more edge cases and documenting evidence for the chosen safeguards. Migration compatibility code should be explicitly version-scoped so it can be removed once the support window ends. +- **Prefer deletion and simplicity over speculative generality.** No abstraction, fallback, or "flexibility" for a consumer or scenario that does not exist in this codebase today. If a change adds many lines, check whether the right change removes them. Implement small logic inline when a shared helper is not clearly reused. +- **Keep e2e and long interaction tests as a readable continuous flow.** Do not force DRY with loops or heavy helpers when repetition is clearer. +- **Put a story's human-facing description in JSDoc above the `meta` const**, not in a `description` parameter. +- **Keep historical telemetry string names stable** when renaming tools or APIs that emit telemetry, unless the field is brand-new and has no existing data. +- **Use git worktrees for parallel or experimental work.** Keep the primary workspace clean and base feature branches on `origin/next`, not a dirty local `next`. diff --git a/.agents/guidelines/code-review.md b/.agents/guidelines/code-review.md index e1290c40a1db..8dcd5d8db86c 100644 --- a/.agents/guidelines/code-review.md +++ b/.agents/guidelines/code-review.md @@ -55,7 +55,7 @@ Post the probe as a command a maintainer can rerun. ### React UI and accessibility -- Follow the Testing Expectations in `AGENTS.md`. +- Follow [Testing](./testing.md). - Check user-visible state, keyboard and pointer interaction, accessible names and descriptions, and the consumers of the surrounding component. - Review fresh-page and first-attempt behavior when tests depend on focus, hover, timers, or retained state. diff --git a/.agents/guidelines/development.md b/.agents/guidelines/development.md new file mode 100644 index 000000000000..ec201a26218d --- /dev/null +++ b/.agents/guidelines/development.md @@ -0,0 +1,73 @@ +# Development Workflow + +This document is canonical for the toolchain, everyday commands, the edit-compile-check loop, and build troubleshooting. +`AGENTS.md` owns the pointer to this file. + +## Toolchain + +- **Node.js**: the version in `.nvmrc`. It runs `.ts` files natively through type stripping, so no loader is needed. +- **Package manager**: Yarn Berry. +- **Task orchestration**: NX plus the custom `yarn task` runner. See [NX and `yarn task`](./nx-and-yarn-task.md). +- **Linting**: oxlint (root `.oxlintrc.json`, extended by `code/.oxlintrc.json` and `scripts/.oxlintrc.json`; custom rules load via `jsPlugins`). ESLint is no longer used for repo linting; `code/lib/eslint-plugin` remains as the published `eslint-plugin-storybook` package. +- **Formatting**: oxfmt (root `.oxfmtrc.json`). +- **CI environment**: Linux and Windows. +- **TS execution**: the repo is migrating from `jiti` to native `node` for running `.ts` files. New scripts use `node ./path/file.ts` with explicit `.ts` import extensions (enabled by `allowImportingTsExtensions`). Legacy scripts still use `jiti` and should be migrated over time. +- **Type checking**: per-package checks (`yarn task check`, `scripts/check/check-package.ts`) and Vitest type tests (`*.test-d.ts`) run on the TypeScript 7 native compiler (the `typescript-native` npm alias), and check diagnostics are filtered to the checked package. A package that is not strict (Angular, Angular-Vite) can list files such as type tests in a `tsconfig.strict.json`; those are also checked with `strict: true`, counting only diagnostics in the listed files. `@storybook/vue3`, `@storybook/docgen-harness` (for its `.vue` fixtures), and `@storybook/svelte` use `vue-tsc` / `svelte-check` (TS 6 based). The workspace `typescript` dependency stays on TS 6 for IDEs and API consumers, so tsconfigs must remain valid for both (for example, no `baseUrl`). + +## Commands + +Run commands from the repository root unless stated otherwise. +Prefer the faster non-production commands first. +Add `-c production` only when you need sandbox-related NX tasks or are explicitly matching CI behavior. + +| Scenario | Command | +| ------------------------------- | ---------------------------------------------------------------------------- | +| Install | `yarn` | +| Compile everything quickly | `yarn nx run-many -t compile` (or `yarn task compile`) | +| Compile one project | `yarn nx compile ` | +| Check TypeScript errors quickly | `yarn nx run-many -t check` (or `yarn task check`) | +| Lint everything | `yarn lint` | +| Lint one file | `yarn --cwd code lint:js:cmd --fix` | +| Format | `cd code && yarn fmt:write` | +| Start the internal Storybook UI | `cd code && yarn storybook:ui` | +| Build the internal Storybook UI | `cd code && yarn storybook:ui:build` | +| Run unit tests | `yarn test` (`yarn test:watch` for watch mode) | +| Run Storybook Vitest tests | `yarn storybook:vitest` | +| Generate a sandbox | `yarn task sandbox --template react-vite/default-ts --start-from auto` | +| Run sandbox E2E tests | `yarn task e2e-tests-dev --template react-vite/default-ts --start-from auto` | +| Run the docgen perf bench | `yarn workspace @storybook/docgen-harness bench:docgen-perf` | +| Run the docgen memory gate | `yarn workspace @storybook/docgen-harness bench:docgen-memory` | +| Verify sandbox docgen baselines | `yarn workspace @storybook/docgen-harness baselines:sandbox` | +| List docs via tools CLI | `cd code && node core/dist/bin/dispatcher.js tools docs list` | +| Require attach / force local | add `--attach` or `--no-attach` before the toolset name | + +## Normal code changes + +1. Install if needed: `yarn` +2. Compile with NX: `yarn nx run-many -t compile` +3. Make changes +4. Recompile affected packages +5. Validate there are no TypeScript errors with `yarn nx run-many -t check` +6. Format, then run relevant lint and tests +7. Validate behavior in the internal Storybook UI first, then switch to sandbox or `-c production` flows only if you need template or CI parity + +## Addon, framework, or renderer work + +1. Edit the relevant package under `code/addons/`, `code/frameworks/`, or `code/renderers/` +2. Recompile with NX, starting without `-c production` +3. Generate a matching sandbox (see [Sandboxes](./sandboxes.md)) +4. Run the relevant Vitest, E2E, or Storybook UI validation flow + +## Formatting and linting + +Always run `cd code && yarn fmt:write` once you are done editing. +Hand-written formatting is frequently wrong for oxfmt, so do not skip this step. +The pre-commit hook detects AI agents (via `std-env`) and switches from check-only to write mode, so formatting is also auto-fixed when agents commit. + +## Troubleshooting + +- Build failures are often fixed by rerunning `yarn` and `yarn nx run-many -t compile`. +- Large compiles may require more Node.js memory. +- The internal Storybook UI uses port `6006` by default. +- Use `--debug` for verbose CLI output. +- Check generated sandbox directories and `.cache/` for build artifacts. diff --git a/.agents/guidelines/nx-and-yarn-task.md b/.agents/guidelines/nx-and-yarn-task.md index 6249d5e47e23..dddbe1cd59ef 100644 --- a/.agents/guidelines/nx-and-yarn-task.md +++ b/.agents/guidelines/nx-and-yarn-task.md @@ -1,6 +1,6 @@ # NX and `yarn task` -This document is canonical for NX and the `yarn task` runner: caching, task dependencies, sandbox and CI-parity flags, and the environment variable reference. `AGENTS.md` owns the pointer to this file and the common commands. +This document is canonical for NX and the `yarn task` runner: caching, task dependencies, sandbox and CI-parity flags, and the environment variable reference. `AGENTS.md` owns the pointer to this file; everyday commands live in [Development workflow](./development.md). Use NX when you want better caching and dependency tracking. Prefer these faster defaults first, and only add `-c production` or `--no-link` when you specifically need sandbox parity or CI-like behavior. diff --git a/.agents/guidelines/pull-requests.md b/.agents/guidelines/pull-requests.md new file mode 100644 index 000000000000..fe1dbafaf1a7 --- /dev/null +++ b/.agents/guidelines/pull-requests.md @@ -0,0 +1,17 @@ +# Pull Request Requirements + +This document is canonical for what a Storybook pull request needs before it can merge. +`AGENTS.md` owns the pointer to this file. + +All PRs target `next`, not `main`. + +The `Danger` status check validates PR metadata, not the diff. +It reads only the title, body, labels, and reviews, and it executes `scripts/dangerfile.ts` from the base branch, so nothing committed inside a PR can change that PR's own Danger result. +Reproduce it locally against a real PR with `DANGER_GITHUB_API_TOKEN="$(gh auth token)" node_modules/.bin/danger pr --dangerfile scripts/dangerfile.ts`. + +- **Title** must match `Area: Summary`, with both parts starting with a capital letter, for example `CSF: Add conservative story mutation API`. Conventional-commit titles such as `fix(csf-tools): add ...` fail the check, even though commit messages in this repo do use that form. +- **Body** must contain a `#### Manual testing` heading followed by steps a maintainer can follow. The check bypasses `OWNER` and `MEMBER` authors, but private org membership resolves to `CONTRIBUTOR` for the CI token, so write the section even when you are a member. +- **For non-release PRs targeting `next`, labels** must satisfy the exactly-one rules in [`scripts/dangerfile.ts`](../../scripts/dangerfile.ts). The valid change types come from the `pr-log` configuration in [`code/package.json`](../../code/package.json); the `ci:` and `qa:` label sets are defined in the dangerfile. Check those sources instead of copying their current values into agent guidance. The dangerfile also defines the separate release-PR rule. `other` is listed in the PR template but is not a valid change type for Danger, and `BREAKING CHANGE` is rejected while the dangerfile pins the branch version to minor. +- **Review** must include an approving review from the `core` or `developer-experience` team. The author's own approval is ignored, so this one clears only once a human on those teams approves. + +For a pkg.pr.new canary, follow the [canary skill](../skills/canary/SKILL.md) and [`CONTRIBUTING/RELEASING.md`](../../CONTRIBUTING/RELEASING.md). diff --git a/.agents/guidelines/sandboxes.md b/.agents/guidelines/sandboxes.md index d40add7ecc33..853110ac72b1 100644 --- a/.agents/guidelines/sandboxes.md +++ b/.agents/guidelines/sandboxes.md @@ -1,6 +1,6 @@ # Sandbox Notes -This document is canonical for sandbox generation and use: where sandboxes live, the sandbox command shape, and the common templates. `AGENTS.md` owns the pointer to this file and the sandbox-failure fallback. +This document is canonical for sandbox generation and use: where sandboxes live, the sandbox command shape, and the common templates. `AGENTS.md` owns the pointer to this file. Sandboxes are generated outside the repository at `../storybook-sandboxes/` by default. diff --git a/.agents/guidelines/testing.md b/.agents/guidelines/testing.md index 2b6a687eeb5a..503c66868d03 100644 --- a/.agents/guidelines/testing.md +++ b/.agents/guidelines/testing.md @@ -1,19 +1,27 @@ # Testing Expectations -This document is canonical for detailed testing practice: watch-mode commands, component and unit test guidance, `memfs` filesystem tests, and global stubbing rules. `AGENTS.md` owns the pointer to this file and the core testing expectations. +This document is canonical for testing practice: which kind of test to write, how to run each suite, component and unit test guidance, `memfs` filesystem tests, global stubbing rules, and type tests. +`AGENTS.md` owns the pointer to this file. -Watch-mode commands: - -```bash -yarn test:watch -yarn storybook:vitest -``` +> [!IMPORTANT] +> **For React components, write Storybook stories with `play` functions. Do NOT write `*.test.tsx` unit tests.** +> Behavior, accessibility, and interaction assertions belong in `*.stories.tsx` co-located with the component, executed via the Storybook Vitest project. +> Unit tests (`*.test.ts(x)`) are reserved for pure utilities, hooks, and non-React modules where rendering is not involved. + +| Suite | Command | +| ------------------------- | ---------------------------------------------------------------------------------------------------------------- | +| Story tests (components) | `yarn storybook:vitest`, or `vitest run --config code/vitest.config.storybook.ts ` | +| Unit tests | `yarn test ` (e.g. `yarn test csf-tools`); the full suite is large, so run focused patterns while iterating | +| Unit tests in watch mode | `yarn test:watch` | +| Visual validation | Storybook UI or Chromatic | +| E2E | `yarn task e2e-tests --start-from auto` or `yarn task e2e-tests-dev --start-from auto` | +| Smoke checks | `yarn task smoke-test --start-from auto` | +| Tools attach coverage | `cd code && yarn playwright test -c e2e-internal/playwright.config.ts e2e-internal/tools-attach.spec.ts` (same checkout as the running internal UI) | When writing tests for components: - Add or update `.stories.tsx` with stories covering each behavior; use `play` functions with `expect`, `userEvent`, `within` from `storybook/test` - Mock external context (e.g. `ManagerContext.Provider`) inside story decorators or `beforeEach` -- Run `vitest --config code/vitest.config.storybook.ts ` to verify play assertions When writing unit tests (utilities, hooks, non-React modules): diff --git a/AGENTS.md b/AGENTS.md index eda21059d664..d19ce58a922c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,226 +1,46 @@ # Storybook Agent Instructions -Keep this file, `AGENTS.md`, up to date when Storybook's architecture, tooling, workflows, or contributor guidance changes. +This file is the canonical instruction source for coding agents; `CLAUDE.md` and other entrypoints point here. +Storybook is a TypeScript monorepo: code lives in `code/`, build tooling in `scripts/`, and every PR targets `next`. -This file is the canonical instruction source for coding agents. Files like `CLAUDE.md` should point here instead of duplicating instructions. +## Always -## Repository Overview +- Run commands from the repository root unless stated otherwise. +- After editing, format with `cd code && yarn fmt:write`. +- Test React components with stories and `play` functions, never `*.test.tsx`. +- Log through `storybook/internal/node-logger` or `storybook/internal/client-logger`, not `console.*`. +- Use explicit extensions on relative TS imports (`./foo.ts`), except framework component files such as `.vue` and `.svelte`. +- Do not commit incidental changes to generated files such as `code/core/src/manager/globals/exports.ts`. -Storybook is a large TypeScript monorepo. The git root is the repo root, the main code lives in `code/`, and build tooling lives in `scripts/`. The default branch is `next`. +## Never Run -- **Base branch**: `next` (all PRs should target `next`, not `main`) -- **Node.js**: `22.22.3` (see `.nvmrc`) — supports `.ts` natively via type stripping (no loader needed) -- **Package Manager**: Yarn Berry -- **Task orchestration**: NX plus the custom `yarn task` runner -- **Linting**: oxlint (root `.oxlintrc.json`, extended by `code/.oxlintrc.json` and `scripts/.oxlintrc.json`; custom rules load via `jsPlugins`). ESLint is no longer used for repo linting — `code/lib/eslint-plugin` remains as the published `eslint-plugin-storybook` package. -- **Formatting**: oxfmt (root `.oxfmtrc.json`) -- **CI environment**: Linux and Windows -- **TS execution**: Migrating from `jiti` to native `node` for running `.ts` files. New scripts should use `node ./path/file.ts` with explicit `.ts` import extensions (enabled by `allowImportingTsExtensions` in tsconfig). Legacy scripts still use `jiti` but should be migrated over time. -- **Type checking**: Per-package checks (`yarn task check`, `scripts/check/check-package.ts`) and Vitest type tests (`*.test-d.ts`) run on the TypeScript 7 native compiler (the `typescript-native` npm alias); check diagnostics are filtered to the checked package. A package that is not strict (Angular, Angular-Vite) can list files such as type tests in a `tsconfig.strict.json`; those are also checked with `strict: true`, counting only diagnostics in the listed files. `@storybook/vue3`, `@storybook/docgen-harness` (for its `.vue` fixtures), and `@storybook/svelte` use `vue-tsc` / `svelte-check` (TS 6 based). The workspace `typescript` dependency stays on TS 6 for IDEs and API consumers, so tsconfigs must remain valid for both (e.g. no `baseUrl`). +- `yarn task dev` without an explicit sandbox template +- `yarn start` -## Common Commands +Both start long-running dev servers. +`.github/workflows/claude.yml` `--disallowed-tools` mirrors this list; update both together. -Run commands from the repository root unless stated otherwise. +## Read Before You Start -For routine agent work, prefer the faster non-production commands first. Add `-c production` only when you need sandbox-related NX tasks or you are explicitly matching CI behavior. +| Before you... | Read | +| --------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------- | +| install, compile, lint, run commands, or debug a build | [Development workflow](.agents/guidelines/development.md) | +| write or edit code | [Code authoring](.agents/guidelines/code-authoring.md) and [Comments and JSDoc](.agents/guidelines/comments-and-jsdoc.md) | +| write or run tests | [Testing](.agents/guidelines/testing.md) | +| open or update a PR | [Pull requests](.agents/guidelines/pull-requests.md) | +| touch `code/core` internals, presets, open services, or the tools CLI | [Architecture](.agents/guidelines/architecture.md) | +| do sandbox, E2E, or CI-parity work | [NX and `yarn task`](.agents/guidelines/nx-and-yarn-task.md) and [Sandboxes](.agents/guidelines/sandboxes.md) | +| add or change an automigration | [`code/lib/cli-storybook/src/automigrate/README.md`](code/lib/cli-storybook/src/automigrate/README.md) | -### Install and compile - -```bash -yarn -yarn task compile -yarn nx run-many -t compile -yarn nx compile -``` - -### Lint and typecheck - -```bash -yarn lint -yarn --cwd code lint:js:cmd --fix -yarn task check -yarn nx run-many -t check -``` - -### Development and tests - -```bash -cd code && yarn storybook:ui -cd code && yarn storybook:ui:build -yarn test -yarn test:watch -yarn storybook:vitest -``` - -### Common task scenarios - -| Scenario | Command | -| ------------------------------- | ------------------------------------------------------------------------------ | -| Compile everything quickly | `yarn nx run-many -t compile` | -| Compile one project | `yarn nx compile ` | -| Check TypeScript errors quickly | `yarn nx run-many -t check` | -| Start the internal Storybook UI | `cd code && yarn storybook:ui` | -| Build the internal Storybook UI | `cd code && yarn storybook:ui:build` | -| Run unit tests | `yarn test` | -| Run Storybook Vitest tests | `yarn storybook:vitest` | -| Generate a sandbox | `yarn task sandbox --template react-vite/default-ts --start-from auto` | -| Run sandbox E2E tests | `yarn task e2e-tests-dev --template react-vite/default-ts --start-from auto` | -| Run the docgen perf bench | `yarn workspace @storybook/docgen-harness bench:docgen-perf` | -| Run the docgen memory gate | `yarn workspace @storybook/docgen-harness bench:docgen-memory` | -| Verify sandbox docgen baselines | `yarn workspace @storybook/docgen-harness baselines:sandbox` | -| List docs via tools CLI | `cd code && node core/dist/bin/dispatcher.js tools docs list` | -| Require attach / force local | add `--attach` or `--no-attach` before the toolset name | - -## Sandbox Notes - -- If sandbox generation fails, fall back to `cd code && yarn storybook:ui` - -## How To Work In This Repo - -### For normal code changes - -1. Install if needed: `yarn` -2. Compile with NX: `yarn nx run-many -t compile` -3. Make changes -4. Recompile affected packages -5. Validate there are no TypeScript errors with `yarn nx run-many -t check` -6. Run relevant lint and tests -7. Validate behavior in the internal Storybook UI first, then switch to sandbox or `-c production` flows only if you need template or CI parity - -### For addon, framework, or renderer work - -1. Edit the relevant package under `code/addons/`, `code/frameworks/`, or `code/renderers/` -2. Recompile with NX, starting without `-c production` -3. Generate a matching sandbox -4. Run the relevant Vitest, E2E, or Storybook UI validation flow - -## Pull Request Requirements - -The `Danger` status check validates PR metadata, not the diff. -It reads only the title, body, labels, and reviews, and it executes `scripts/dangerfile.ts` from the base branch, so nothing committed inside a PR can change that PR's own Danger result. -Reproduce it locally against a real PR with `DANGER_GITHUB_API_TOKEN="$(gh auth token)" node_modules/.bin/danger pr --dangerfile scripts/dangerfile.ts`. - -- **Title** must match `Area: Summary`, with both parts starting with a capital letter, for example `CSF: Add conservative story mutation API`. Conventional-commit titles such as `fix(csf-tools): add ...` fail the check, even though commit messages in this repo do use that form. -- **Body** must contain a `#### Manual testing` heading followed by steps a maintainer can follow. The check bypasses `OWNER` and `MEMBER` authors, but private org membership resolves to `CONTRIBUTOR` for the CI token, so write the section even when you are a member. -- **For non-release PRs targeting `next`, labels** must satisfy the exactly-one rules in [`scripts/dangerfile.ts`](scripts/dangerfile.ts). The valid change types come from the `pr-log` configuration in [`code/package.json`](code/package.json); the `ci:` and `qa:` label sets are defined in the dangerfile. Check those sources instead of copying their current values into agent guidance. The dangerfile also defines the separate release-PR rule. `other` is listed in the PR template but is not a valid change type for Danger, and `BREAKING CHANGE` is rejected while the dangerfile pins the branch version to minor. -- **Review** must include an approving review from the `core` or `developer-experience` team. The author's own approval is ignored, so this one clears only once a human on those teams approves. - -## Testing Expectations - -> [!IMPORTANT] -> **For React components, write Storybook stories with `play` functions — do NOT write `*.test.tsx` unit tests.** Behavior, accessibility, and interaction assertions belong in `*.stories.tsx` co-located with the component, executed via the Storybook Vitest project (`yarn storybook:vitest` or `vitest run --config code/vitest.config.storybook.ts`). Unit tests (`*.test.ts(x)`) are reserved for pure utilities, hooks, and non-React modules where rendering is not involved. - -- Use `yarn storybook:vitest` to run Storybook story tests (the primary test path for components) -- Use `yarn test` for unit tests of utilities, hooks, and non-React modules -- Prefer focused unit-test runs during iteration — the full suite is large: `yarn test ` (e.g. `yarn test csf-tools`) -- Use Storybook UI or Chromatic for visual validation -- Use `yarn task e2e-tests --start-from auto` or `yarn task e2e-tests-dev --start-from auto` for E2E coverage -- Use `yarn task smoke-test --start-from auto` for smoke checks -- Use `cd code && yarn playwright test -c e2e-internal/playwright.config.ts e2e-internal/tools-attach.spec.ts` for tools attach coverage (same checkout as the running internal UI) - -## Quality and Logging - -After changing files: - -1. **Always** format with `yarn fmt:write`, run from the `code/` directory (`cd code && yarn fmt:write`), once you are done editing. The repo uses `oxfmt`, so hand-written formatting will frequently be wrong — do not skip this step. -2. Lint with `yarn --cwd code lint:js:cmd --fix` or `cd code && yarn lint:js:cmd ` -3. Run relevant tests before submitting a PR - -Use Storybook loggers instead of raw `console.*` in normal code paths: - -- Server-side: `storybook/internal/node-logger` -- Client-side: `storybook/internal/client-logger` - -For TypeScript source in the repo, prefer explicit file extensions for relative code imports and exports such as `./foo.ts` or `./bar.tsx` when the target is another TS/JS module in this repository. Keep framework-specific component imports like `.vue` and `.svelte` in the form already expected by their package tooling. - -The pre-commit hook automatically detects AI agents (via `std-env`) and switches from check-only to write mode, so formatting is auto-fixed when agents commit. - -Avoid `console.log`, `console.warn`, and `console.error` unless the file is isolated enough that importing the logger is not reasonable. - -## Troubleshooting - -- Build failures are often fixed by rerunning `yarn` and `yarn nx run-many -t compile` -- Storybook UI uses port `6006` by default -- Large compiles may require more Node.js memory -- Sandbox paths are `../storybook-sandboxes/`, not `./sandbox` or `code/sandbox/` -- Use `--debug` for verbose CLI output -- Check generated sandbox directories and `.cache/` for build artifacts - -## Canary Releases - -When you need a pkg.pr.new canary, follow [`.agents/skills/canary/SKILL.md`](.agents/skills/canary/SKILL.md) and [`CONTRIBUTING/RELEASING.md`](CONTRIBUTING/RELEASING.md). - -## Commands To Avoid - -- **DO NOT RUN** `yarn task dev` without an explicit sandbox template -- **DO NOT RUN** `yarn start` - -These usually start long-running development servers and are the wrong default for agents. - -## Automigrations - -- Read `code/lib/cli-storybook/src/automigrate/README.md` before adding or changing an automigration. - -## Repository skills - -- Canonical contributor skills live in `.agents/skills/`; `.claude/skills/` contains references to them. -- Use [principle-encode-lessons-in-structure](.agents/skills/principle-encode-lessons-in-structure/SKILL.md) to turn recurring corrections into enforceable checks. - -## Code Authoring Principles - -These are recurring failure modes in agent-authored changes to this repo. Apply them when writing or reviewing code, not just when asked. - -- **Write comments only for the two reasons in [Comments and JSDoc](#comments-and-jsdoc).** That section is the rule; this list does not restate it. -- **Verify environment assumptions empirically before encoding them.** If a design rests on "the bundler strips X" or "this metadata is empty here", prove it with a throwaway probe before building on it (and before writing it into a comment as fact). A 10-line experiment is cheaper than a wrong architecture. -- **Encode assumptions with static checks first.** If an assumption is expected to always hold, prefer making it impossible via TypeScript types and existing lint rules. When static checks are not practical, add a cheap runtime assertion close to the boundary so violations fail loudly at the source. -- **Avoid redundant tests already covered elsewhere.** Do not add tests for code patterns already guaranteed by TypeScript or linting, and do not duplicate coverage that already exists in Storybook `play` functions or Playwright tests. -- **Test contracts (including side effects), not private implementation details.** It is valid to assert side effects when they are part of the public contract. Avoid assertions about internals that are not part of an exported contract, user-visible DOM output, or externally observable behavior. -- **Bias toward broader coverage for security and migrations.** For security-sensitive code paths and legacy data migration logic, prefer handling more edge cases and documenting evidence for the chosen safeguards. Migration compatibility code should be explicitly version-scoped so it can be removed once the support window ends. -- **Prefer deletion and simplicity over speculative generality.** No abstraction, fallback, or "flexibility" for a consumer or scenario that does not exist in this codebase today. If a change adds many lines, check whether the right change removes them. -- **Don't commit accidental overrides to generated code.** Files like `code/core/src/manager/globals/exports.ts` are auto-generated, as stated in their JSDoc header. Only commit changes if they match changes you made on your PR, otherwise leave them untouched and flag flaky generated files in the PR description. - -## Comments and JSDoc - -Code should be self-explanatory. A comment is only justified when the code cannot explain itself (a non-obvious _why_) or when a public API needs explanation. Never comment to record that you did x, y, z. - -Before writing or editing any code file, read [`.agents/guidelines/comments-and-jsdoc.md`](.agents/guidelines/comments-and-jsdoc.md) and follow it. Read it once per session, not once per file. +Contributor skills live in `.agents/skills/`; `.claude/skills/` references them. ## Code Review Before reviewing a pull request or a diff, read [`.agents/guidelines/code-review.md`](.agents/guidelines/code-review.md) and follow it. -It covers what counts as evidence, checks per kind of changed code, structural findings the author must justify, verification expectations, and how to write review comments. - -## Maintenance Rules For Agents - -- `AGENTS.md` owns the kickstart: identity, base rules, common commands, - guardrails, and the pointers below. Keep it lean; push detail into the - linked documents instead of inlining it. -- Each linked document is canonical for its topic. Update that document, not - this file, when its topic changes. If a topic disappears, remove the - document and its pointer together. -- Keep `CLAUDE.md` and other agent entrypoints as thin references to - `AGENTS.md`. -- `.github/workflows/claude.yml` `--disallowed-tools` mirrors the Commands To - Avoid section; update both together. -- Do not reintroduce duplicated instruction files when a reference will do. - -## Deep dives - -Read these when the task calls for them, not by default. Each document is -canonical for its topic; this file owns the pointers. - -- [Architecture and repository structure](.agents/guidelines/architecture.md) — read before - touching `code/core` internals, presets, open services, or the tools CLI. -- [NX and `yarn task`](.agents/guidelines/nx-and-yarn-task.md) — read before sandbox, E2E, or - CI-parity work. Includes the environment variable reference. -- [Sandboxes](.agents/guidelines/sandboxes.md) — read before generating or debugging a sandbox. -- [Testing](.agents/guidelines/testing.md) — read before writing or running tests beyond the - commands above; covers memfs filesystem tests and global stubbing rules. -## Learned User Preferences +## Maintaining These Instructions -- Prefer git worktrees for parallel or experimental work; keep the primary workspace clean and base feature branches on `origin/next`, not a dirty local `next`. -- Prefer simplicity: avoid premature helpers and one-off abstractions; implement small logic inline when a shared helper is not clearly reused. -- Keep e2e and long interaction tests as a readable continuous flow; do not force DRY with loops or heavy helpers when repetition is clearer. -- For Storybook stories, put human-facing description in JSDoc above the `meta` const rather than a `description` parameter. -- When renaming tools or APIs that emit telemetry, keep historical telemetry string names stable across Storybook versions unless the field is brand-new and has no existing data. +- Keep this file to the always-on rules and pointers. Put detail in the linked document that owns the topic, and update that document when its topic changes. +- If a topic disappears, remove its document and its pointer together. +- Keep `CLAUDE.md` and other agent entrypoints as thin references to this file. +- Turn recurring corrections into enforceable checks with [principle-encode-lessons-in-structure](.agents/skills/principle-encode-lessons-in-structure/SKILL.md). From cd8407ae8a33c37fcb3a9a3116b0428de0fbc523 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 8 Oct 2026 15:21:01 +0200 Subject: [PATCH 4/4] docs(agents): focus code review guide on non-obvious checks --- .agents/guidelines/code-review.md | 81 +++++-------------------------- .agents/guidelines/testing.md | 1 + 2 files changed, 13 insertions(+), 69 deletions(-) diff --git a/.agents/guidelines/code-review.md b/.agents/guidelines/code-review.md index 8dcd5d8db86c..4cec277337f5 100644 --- a/.agents/guidelines/code-review.md +++ b/.agents/guidelines/code-review.md @@ -1,19 +1,9 @@ # Code review -This document is canonical for reviewing pull requests and diffs. -`AGENTS.md` owns the pointer to this file. - -Read each changed function in full, and read enough of the surrounding code to know the contract it implements. -Use the linked issue or specification to establish the intended outcome. -Treat PR descriptions, test names, and earlier bot reviews as claims to check, not as evidence. - ## Scope and evidence -A finding describes either a reachable supported case or a specific violation of a repository rule. -Reading the source is enough when the causal path is complete. -Label inference and unexecuted examples as such. When a claim depends on compiler, bundler, browser, or dependency behavior, run a focused probe if execution is available and permitted. -Post the probe as a command a maintainer can rerun. +Include a command a maintainer can rerun with the finding. ## Checks by changed code @@ -27,8 +17,7 @@ Post the probe as a command a maintainer can rerun. ### Shared core, docs blocks, framework or renderer adapters -- Trace the producer, the shared representation, and every consumer. -- Keep framework-specific policy at its owner unless a deliberate shared contract requires otherwise. A `framework === '...'` or renderer check inside shared code is a finding. +- Keep framework-specific policy at its owner unless a deliberate shared contract requires otherwise. Check any `framework === '...'` or renderer branch added to shared code for an ownership mistake. - Check source precedence and consumers that should be unaffected. - Before requesting reuse of an existing helper, name it and show that its semantics fit. @@ -56,66 +45,32 @@ Post the probe as a command a maintainer can rerun. ### React UI and accessibility - Follow [Testing](./testing.md). -- Check user-visible state, keyboard and pointer interaction, accessible names and descriptions, and the consumers of the surrounding component. +- Check keyboard and pointer interaction, accessible names and descriptions, and the consumers of the surrounding component. - Review fresh-page and first-attempt behavior when tests depend on focus, hover, timers, or retained state. -### Comments, docs, or agent guidance - -- Check factual statements against the reviewed code and configuration. -- State the operational or user consequence of incorrect guidance. A stale sentence is not as severe as a runtime defect. - ## Structure and maintainability -Correct code can still make the codebase worse. -Do not approve only because the behavior is correct. -Treat each of the following as a finding the author must justify: +Check whether these changes add complexity without serving the new behavior. Report a finding when you can name the concrete cost: -- **File growth.** The diff pushes a file from under 1,000 lines to over 1,000. Compare `wc -l` on base and head. -- **One more branch.** A feature extends an existing `if`/`else` chain or `switch` by one more case, or adds a second boolean that must stay in sync with the first. Ask for the structure that encodes the domain instead: a discriminated union, a lookup table, a state machine. +- **One more branch.** A feature extends a chain of cases that would be simpler as a discriminated union, lookup table, or state machine, or adds a second boolean that must stay in sync with the first. - **Special cases in busy flows.** New conditionals or edge-case handling dropped into an unrelated or already busy function. - **Threaded signals.** A new flag passed through several layers of types, options, or pipelines to reach one consumer. Look for a more direct path. - **Repeated decisions.** The same choice made in several places instead of once, with the result passed along. -- **Layers that do not compress.** A wrapper with one caller, an adapter with one implementation, or a layer that repeats the methods and arguments of the one below it. +- **Layers that do not compress.** A wrapper or adapter that only repeats the methods and arguments of the layer below it. - **Hidden state.** New module state, mutable fields, or synced copies where a local, a return value, or a derived value would do. A reader should answer "where does X come from?" and "what can change X?" quickly. -- **Loose types.** Casts, `any`, non-null assertions, or optional fields that permit contradictory combinations. If a comment is needed to explain which field combinations are valid, the type should be a union. A `switch` over a union needs an exhaustive `never` check so the next variant fails to compile. +- **Loose types.** Casts or `any` that hide incompatible shapes, or optional fields that allow contradictory combinations. Use a union when only certain combinations are valid; check that a new variant cannot silently fall through a `switch`. - **Parallel types.** A hand-written type that duplicates a shape another module or schema already owns. -- **Rearranged complexity.** A refactor that moves code without reducing the number of branches, modes, flags, or layers a reader has to follow. - -Ask what the code would look like if the new requirement had existed from the start. -When that version deletes whole branches or concepts, propose it with a short sketch of the simpler shape. -If it reaches beyond the PR's scope, suggest it as a follow-up instead of blocking on it. - -Do not ask for new helpers, file splits, abstractions, or rewrites that do not remove complexity. -Three similar statements are better than a premature abstraction. -When the same finding would apply to future PRs, suggest the lint rule, type, or check that would catch it, not just the local fix. ## Verification +Read [Testing](./testing.md) when reviewing tests in the diff. + ### Bug fixes - Prefer a focused test that fails on the unchanged base for the reported reason and passes on head. A setup failure on base is not a reproduction. -- Check that the fix addresses the root cause. A guard, fallback, or `try`/`catch` that silences the symptom is a finding. -- Search for the same pattern elsewhere and say whether other instances share the defect. -- Reuse suitable existing coverage rather than requiring a new test file. Name the uncovered behavior when asking for a regression test. -### Tests in the diff +### Performance changes -Ask whether each test would still pass if every function it imports returned `undefined`. -If it would, it cannot detect a defect. -The common shapes are: - -- no assertion, or only `toBeDefined`, `toBeTruthy`, or `not.toThrow` -- only `toHaveBeenCalled` or an empty-result assertion, without the payload or resulting state -- an expected value computed by the code under test -- an assertion that restates a constant, default, or table row -- an assertion on data the test built, where the subject never runs - -Ask for one concrete input and the literal expected output or observable effect instead. - -### Refactors and performance changes - -- Tests may pass on both revisions. Check output equivalence plus the claimed improvement. -- Where practical, use a small negative control to show that the assertion detects the relevant defect. - A performance number needs a run count, a spread, and the input size it was measured on. Ask what else it could be measuring: failed or skipped work, a warm cache, an untouched code path, or noise. ### Type changes @@ -126,19 +81,7 @@ Ask for one concrete input and the literal expected output or observable effect ### Flaky or retry-dependent tests - Inspect the first attempt and rerun the focused case without retries if execution is available. -- Do not prescribe arbitrary repeat counts for unrelated tests. - -## Feedback and severity - -Base severity on the impact you have shown, not on a reporting threshold. -Rank broken supported behavior, public compatibility, resource leaks, data loss, and security defects first, by reach and consequence. -Rank structural regressions from the maintainability list next. -Prefer a few high-confidence comments over many cosmetic ones. -For a maintainability finding, name the concrete cost, for example two conflicting owners of the same launch policy. +## Feedback -Write one comment per root cause. -Each comment has a short title, the smallest useful changed line range, the trigger and consequence, the supporting evidence, and a direction for the fix. -Link other affected locations when needed. -Keep open product questions separate from demonstrated defects, and optional suggestions separate from required changes. -Do not mention your own review process or tooling in contributor-facing comments. +Each finding has a short title, the smallest useful changed line range, the trigger and consequence, supporting evidence, and a direction for the fix. diff --git a/.agents/guidelines/testing.md b/.agents/guidelines/testing.md index 503c66868d03..e0ce29a6f179 100644 --- a/.agents/guidelines/testing.md +++ b/.agents/guidelines/testing.md @@ -27,6 +27,7 @@ When writing unit tests (utilities, hooks, non-React modules): - Export functions that need direct tests - Test real behavior, not just syntax patterns +- Make the input and expected result easy to read together. For a mapper with stable output, prefer `toMatchInlineSnapshot` when it shows the full result more clearly, as in the automigration fix tests. - Use coverage when useful: `yarn vitest run --coverage ` - Mock external dependencies like file system access and loggers - Use Node's path.resolve to wrap expected FS paths when writing path-related tests, so they work on Windows