Repository navigation
Agents: Add a code review guide and slim AGENTS.md down to pointers #36677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
fdddced
docs(agents): add a code review guide for agents
valentinpalkovic c3d4f88
docs(agents): give code review its own AGENTS.md section
valentinpalkovic 1f45a52
docs(agents): move AGENTS.md detail into linked guidelines
valentinpalkovic cd8407a
docs(agents): focus code review guide on non-obvious checks
valentinpalkovic 2d24882
Merge remote-tracking branch 'origin/next' into valentin/agents-code-…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| # Code review | ||
|
|
||
| ## Scope and evidence | ||
|
|
||
| When a claim depends on compiler, bundler, browser, or dependency behavior, run a focused probe if execution is available and permitted. | ||
| Include a command a maintainer can rerun with the finding. | ||
|
|
||
| ## 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 | ||
|
|
||
| - 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. | ||
|
|
||
| ### 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 [Testing](./testing.md). | ||
| - 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. | ||
|
|
||
| ## Structure and maintainability | ||
|
|
||
| Check whether these changes add complexity without serving the new behavior. Report a finding when you can name the concrete cost: | ||
|
|
||
| - **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 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 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. | ||
|
|
||
| ## 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. | ||
|
|
||
| ### Performance changes | ||
|
|
||
| - 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. | ||
|
|
||
| ## Feedback | ||
|
|
||
| Each finding has a short title, the smallest useful changed line range, the trigger and consequence, supporting evidence, and a direction for the fix. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
|
valentinpalkovic marked this conversation as resolved.
|
||
| - **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 <nx-project-name>` | | ||
| | 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 <file-relative-to-code-folder> --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. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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`. | ||
|
valentinpalkovic marked this conversation as resolved.
|
||
|
|
||
| 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. | ||
|
valentinpalkovic marked this conversation as resolved.
|
||
| Reproduce it locally against a real PR with `DANGER_GITHUB_API_TOKEN="$(gh auth token)" node_modules/.bin/danger pr <pr-url> --dangerfile scripts/dangerfile.ts`. | ||
|
valentinpalkovic marked this conversation as resolved.
|
||
|
|
||
| - **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. | ||
|
valentinpalkovic marked this conversation as resolved.
|
||
| - **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). | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.