electron/REVIEW.md

# Review instructions

Instructions for automated review of pull requests to electron/electron. The
single most important rule: **only make claims you can support from files you
can actually read** — and what you can read depends on your checkout.

## First, determine what you can see

This repository is only the Electron layer. For a real build it is checked
out as `src/electron/` inside a much larger tree that `gclient` (`e sync`)
assembles from the pins in `DEPS`: Chromium at `src/`, Node.js at
`src/third_party/electron_node/`, V8, BoringSSL, …. Before reviewing, check:

- **Full checkout:** the repository's parent directory is a Chromium `src`
  tree: `../content/`, `../chrome/`, `../v8/` and
  `../third_party/electron_node/` all exist relative to the repo root.
- **Standalone clone:** those paths do not exist; you have electron/electron
  and nothing else. If in doubt, or only some exist, assume this.

Follow only whichever of the next two sections matches; everything from
"`patches/`" onward applies to both. Either way, generated headers (`*.mojom.h`
and friends, `electron/buildflags/buildflags.h`, other `electron/…` paths made
from templates here) exist only in build output (`../out/…`), never in the
source tree; if no build output is present they won't be either, so never
report them as missing.

## If you have a standalone clone

None of the upstream code is present. `#include "shell/..."` paths are files
in this repository; everything else (`base/`, `content/`, `chrome/`,
`third_party/blink/`, `v8/`, `gin/`, `mojo/`, Node's headers, …) is upstream
code you cannot see — including the `//chrome` sources `chromium_src/BUILD.gn`
compiles. Only `//electron/…` GN targets (and relative `:name` labels in this
repo's BUILD files) are defined here. So:

- Do not state what a Chromium, Node.js or V8 function does, what its
  signature or threading/ownership contract is, whether an override matches a
  virtual method, or that a call site here misuses an upstream API, unless the
  evidence is in this repo's files or in the PR diff itself — likewise for
  Node.js internals (`internal/…` modules, the `fs`/`module` code ASAR wraps).
- When a concern genuinely depends on upstream behaviour, ask the author ("I
  can't see `content::WebContentsObserver` from this checkout — can you confirm
  this runs on the UI thread?") at Nit severity at most, never as a defect.
- In `patches/`, do **not** claim a hunk will fail to apply, conflicts with
  upstream, targets the wrong line or is no longer needed — you cannot see the
  upstream file; CI applies every patch to the pinned revisions. Ask instead.

## If you have a full checkout

The previous section's restrictions do not apply to you, as long as the tree
is synced to this PR's `DEPS` pins (check when the PR changes `DEPS`; if
unsure, treat the rolled dependency as code you cannot read). Use the tree:

- Verify directly in the synced tree (`../`, `../v8/`,
  `../third_party/electron_node/`, …) the upstream signatures, the virtual
  methods `shell/` overrides, threading/ownership contracts and the Node.js
  internals `lib/` wraps; report real mismatches as findings.
- For `patches/` changes, open the upstream file each hunk targets and check
  the hunk against it. A synced tree normally already has the existing
  `patches/` applied: a new patch should apply to what you see, but for a patch
  the PR modifies you are looking at upstream with its old version applied —
  judge the new hunks against that. Then a hunk that cannot apply, or a patch
  upstream no longer needs, is a real finding here.
- Cite upstream evidence by path relative to the Chromium `src` root (e.g.
  `content/browser/…:123`) so it is clear which tree a claim rests on. You
  still never propose editing upstream files — such changes go via `patches/`.

## `patches/`

Each subdirectory of `patches/` (`chromium/`, `node/`, `v8/`, …) holds
git-format patches applied, in the order of its `.patches` file, on top of the
matching upstream checkout (mapping in `patches/config.json`). They are
exported by tooling (`e patches` / `script/git-export-patches`), not written
by hand. Review a patch change as a diff of a diff:

- A new `.patch` needs a subject and a body that meaningfully explains why
  Electron needs it and, if it is meant to be temporary, an upstream link or
  how to tell when it can be dropped (see `docs/development/patches.md`; Nit
  if empty or boilerplate). It must be added to that directory's `.patches`
  list; a deleted patch must be removed from it.
- Check whether the `+`/`-` lines inside the patch plausibly do what the
  description says, and whether Electron-side code in `shell/` or `lib/` that
  relies on the patch changes consistently in the same PR.
- Churn in context lines, `index` hashes and line offsets of otherwise
  untouched patches is normal re-export noise; ignore it. Upstream behaviour
  changes belong in `patches/`; never suggest editing files under `../`.

## What a change here usually needs (checkable in this repo)

- `shell/` (C++) and `lib/` (TypeScript bundled into the binary) are split by
  process type (`browser/` = main process, `renderer/`, `sandboxed_renderer/`,
  `utility/`, `common/`, …). Check that new code sits in the directory for the
  process it runs in, and be careful about what becomes reachable from a
  sandboxed or context-isolated renderer. New C++ sources in `shell/` are
  normally listed in `filenames.gni`; if a PR adds one and no `.gn`/`.gni`
  file in the diff references it, mention it once.
- In `lib/`, `@electron/internal/…` imports resolve to `lib/…`, and
  `process._linkedBinding('electron_…')` objects are C++ under `shell/`, typed
  by hand in `typings/` (`internal-ambient.d.ts`, `internal-electron.d.ts`) —
  check the two against each other.
- Public API lives in `docs/api/*.md`; `electron.d.ts` is generated from them
  at build time and not checked in. A PR that adds or changes a public method,
  event, property or option should update `docs/api/` (types, platform tags,
  and the YAML "API history" block when an API is added, changed or
  deprecated) and `docs/breaking-changes.md` for deprecations and breakage.
- Behaviour changes and bug fixes usually come with a `spec/` test; if there is
  none, ask whether one is feasible rather than asserting it is required (some
  platform- or GPU-specific behaviour cannot be tested in CI).
- Treat IPC handlers in `lib/browser/` and `shell/browser/api/`, anything
  touching `contextBridge`, `webPreferences` defaults (`contextIsolation`,
  `sandbox`, `nodeIntegration`), navigation/permission/protocol handlers,
  fuses and ASAR integrity as security-sensitive: validate arguments that come
  from a renderer, check which process and world the code runs in, and cite
  the exact lines. No speculative security findings about code you can't read.
- PRs based on a release line (`NN-x-y`) are normally backports from `main`,
  often opened by the backport bot: review them for faithful application to
  the older branch (missing hunks, code that differs on that branch), not to
  re-argue the original design.

## Post no findings on

- Formatting, include order and lint (CI runs clang-format, ESLint, docs lint,
  GN format and patch-file lint); `filenames.auto.gni` (generated and
  CI-checked even if the PR touches it), lockfiles, generated/vendored files.
- PR-description conventions (release-notes `Notes:` line, "Backport of #…"):
  bots already enforce them.
- In `DEPS` roll PRs ("chore: bump chromium to …" / "… node to …"), the version
  bump itself and what changed upstream between the two versions; their
  `shell/`, `lib/`, `patches/` and build changes are reviewed normally.

## Evidence bar and tone

- Every Important finding needs a `path:line` citation to a file you actually
  read — in this repository, or in the synced tree if you have one — and a
  concrete failure mode. "This might be wrong depending on how Chromium
  handles X" is not a finding; it is at most a question. For anything you
  cannot read, prefer "I can't see X from this checkout — can you confirm Y?";
  if you cannot name what you would need to see to be sure, drop the comment.
- Keep reviews short: at most five Nits, the rest as a count in the summary.
  On re-review after a push, report Important findings and whether earlier
  ones were addressed; no new Nits. Lead the summary with which kind of
  checkout you reviewed from and whether you found anything you could verify.