docs: add REVIEW.md and treat it as a docs-only change in CI (#53798)

.github/workflows/build.yml

@@ -88,13 +88,14 @@ jobs:
             - '.github/dependabot.yml'
             - '.github/faraday.yml'
             - README.md
+            - REVIEW.md
             - SECURITY.md
             - CONTRIBUTING.md
             - CODE_OF_CONDUCT.md
           pgo:
             - 'build/pgo_profiles/*.pgo.txt'
           src:
-            - '!{docs/**,.claude/**,.github/CODEOWNERS,.github/ISSUE_TEMPLATE/**,.github/PULL_REQUEST_TEMPLATE.md,.github/config.yml,.github/copilot-instructions.md,.github/dependabot.yml,.github/faraday.yml,README.md,SECURITY.md,CONTRIBUTING.md,CODE_OF_CONDUCT.md,build/pgo_profiles/*.pgo.txt}'
+            - '!{docs/**,.claude/**,.github/CODEOWNERS,.github/ISSUE_TEMPLATE/**,.github/PULL_REQUEST_TEMPLATE.md,.github/config.yml,.github/copilot-instructions.md,.github/dependabot.yml,.github/faraday.yml,README.md,REVIEW.md,SECURITY.md,CONTRIBUTING.md,CODE_OF_CONDUCT.md,build/pgo_profiles/*.pgo.txt}'
           # Changes that can affect compiled native code (C/C++/Objective-C,
           # GN build config, patches, and the Chromium checkout inputs).
           # Currently gates clang-tidy; designed to be reused by future

REVIEW.md

@@ -0,0 +1,142 @@
+# 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.