diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 89580a62b7..fb6964bd53 100644 --- a/.github/workflows/build.yml +++ b/.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 diff --git a/REVIEW.md b/REVIEW.md new file mode 100644 index 0000000000..209333815a --- /dev/null +++ b/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.