docs(pr-review): coverage-delta semantics, migrated-area redirects, and the campaign-hotspots channel#5812
Conversation
…nd the campaign-hotspots channel The 24/7 review loop and contribution handler now know: honest coverage decreases are acceptable (bucket-based gate, only engine_regress is fatal; marker-name collision trap); migrated-area conformance is a REDIRECT lens backed by gates, not an extra rejection layer; generated-file dirt, export nondeterminism, and maintainer-caused staleness are never contributor burden. Adds the sweep-start read of .agents/pr-review/campaign-hotspots.toml (lead-maintained, untracked).
There was a problem hiding this comment.
Code Review
This pull request updates the PR contribution and review loop skills to document guidelines on migrated-area conformance, coverage-delta semantics, and contributor friction, as well as refining the criteria for 'enhancement' and 'feature' labels. The review feedback suggests replacing non-ASCII characters (such as ellipses and arrows) and curly quotes with standard ASCII equivalents to ensure compatibility across different editors and parsers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| ## Coverage-Delta Semantics and Contributor Friction | ||
|
|
||
| **An honest coverage decrease is acceptable — never ask a contributor to "restore" it.** A parser change that converts a silently-swallowed clause into an explicit `Effect::Unimplemented` moves cards from lying-green to honest-red; that is an improvement, and the campaign's coverage-honesty policy explicitly permits the count to drop. The CI coverage-regression gate is **bucket-based, not count-based**: the only fatal bucket is `engine_regress` — a card LOSING a handler the baseline marked supported. Any number of honest supported→unsupported flips with new marker names passes. One trap to check when a PR introduces new `Effect::unimplemented(name, …)` markers: the name must not case-insensitively collide with a baseline-supported handler name (the current baseline set is seven generic names, e.g. `Effect:choose`, `Effect:have`) — a collision trips a false-fatal `engine_regress` in the merge queue. |
There was a problem hiding this comment.
Use standard ASCII ellipsis ... instead of the non-ASCII ellipsis character … to ensure maximum compatibility across different editors, terminals, and parsers.
| **An honest coverage decrease is acceptable — never ask a contributor to "restore" it.** A parser change that converts a silently-swallowed clause into an explicit `Effect::Unimplemented` moves cards from lying-green to honest-red; that is an improvement, and the campaign's coverage-honesty policy explicitly permits the count to drop. The CI coverage-regression gate is **bucket-based, not count-based**: the only fatal bucket is `engine_regress` — a card LOSING a handler the baseline marked supported. Any number of honest supported→unsupported flips with new marker names passes. One trap to check when a PR introduces new `Effect::unimplemented(name, …)` markers: the name must not case-insensitively collide with a baseline-supported handler name (the current baseline set is seven generic names, e.g. `Effect:choose`, `Effect:have`) — a collision trips a false-fatal `engine_regress` in the merge queue. | |
| **An honest coverage decrease is acceptable — never ask a contributor to "restore" it.** A parser change that converts a silently-swallowed clause into an explicit `Effect::Unimplemented` moves cards from lying-green to honest-red; that is an improvement, and the campaign's coverage-honesty policy explicitly permits the count to drop. The CI coverage-regression gate is **bucket-based, not count-based**: the only fatal bucket is `engine_regress` — a card LOSING a handler the baseline marked supported. Any number of honest supported→unsupported flips with new marker names passes. One trap to check when a PR introduces new `Effect::unimplemented(name, ...)` markers: the name must not case-insensitively collide with a baseline-supported handler name (the current baseline set is seven generic names, e.g. `Effect:choose`, `Effect:have`) — a collision trips a false-fatal `engine_regress` in the merge queue. |
|
|
||
| ## Sweep Protocol | ||
|
|
||
| At sweep start, read `.agents/pr-review/campaign-hotspots.toml` if present (lead-maintained, untracked, updated at campaign routing/ship events). It lists: migrated areas with the rule now in force and the REDIRECT pointer for authors; coverage-delta semantics (an honest coverage DECREASE — silent swallows converted to explicit `Effect::Unimplemented` — is acceptable and must never be "restored"; in CI only the `engine_regress` bucket, a card LOSING a previously-supported handler, is fatal); diff content outside the contributor's control (generated-file dirt, export nondeterminism, staleness caused by maintainer-side churn — strip/normalize/rebase these yourself, never request changes for them); and files owned by in-flight campaign units (a contributor PR racing those files → hold + surface to the maintainer). Hotspots inform routing and comment content; they are NOT an additional review gate — if a hotspot prohibition reaches review without CI redding it, that is a gate gap: file it, don't make the contributor absorb the miss. |
There was a problem hiding this comment.
Use standard ASCII arrow -> instead of the non-ASCII arrow character → to ensure maximum compatibility across different editors, terminals, and parsers.
| At sweep start, read `.agents/pr-review/campaign-hotspots.toml` if present (lead-maintained, untracked, updated at campaign routing/ship events). It lists: migrated areas with the rule now in force and the REDIRECT pointer for authors; coverage-delta semantics (an honest coverage DECREASE — silent swallows converted to explicit `Effect::Unimplemented` — is acceptable and must never be "restored"; in CI only the `engine_regress` bucket, a card LOSING a previously-supported handler, is fatal); diff content outside the contributor's control (generated-file dirt, export nondeterminism, staleness caused by maintainer-side churn — strip/normalize/rebase these yourself, never request changes for them); and files owned by in-flight campaign units (a contributor PR racing those files → hold + surface to the maintainer). Hotspots inform routing and comment content; they are NOT an additional review gate — if a hotspot prohibition reaches review without CI redding it, that is a gate gap: file it, don't make the contributor absorb the miss. | |
| At sweep start, read `.agents/pr-review/campaign-hotspots.toml` if present (lead-maintained, untracked, updated at campaign routing/ship events). It lists: migrated areas with the rule now in force and the REDIRECT pointer for authors; coverage-delta semantics (an honest coverage DECREASE — silent swallows converted to explicit `Effect::Unimplemented` — is acceptable and must never be "restore"; in CI only the `engine_regress` bucket, a card LOSING a previously-supported handler, is fatal); diff content outside the contributor's control (generated-file dirt, export nondeterminism, staleness caused by maintainer-side churn — strip/normalize/rebase these yourself, never request changes for them); and files owned by in-flight campaign units (a contributor PR racing those files -> hold + surface to the maintainer). Hotspots inform routing and comment content; they are NOT an additional review gate — if a hotspot prohibition reaches review without CI redding it, that is a gate gap: file it, don't make the contributor absorb the miss. |
| - **Is the coverage unique?** Grep for existing tests over the same card, mechanic, or issue before accepting. Duplicated coverage of an already-tested path is a reason to decline, not merge. | ||
| - **Is the blast radius justified?** Large test files that re-link the engine (see the `no_top_level_test_binaries` guard) must earn their compile cost. | ||
|
|
||
| **Evaluate every test-only PR independently.** A series of narrowly scoped test PRs earns no collective presumption of value: each PR must cite the specific existing coverage it does *not* duplicate, name the distinct regression it guards, and demonstrate a failure against the plausible pre-fix or wrong implementation. “This is another test for the same area” is not merge value. |
There was a problem hiding this comment.
Use standard straight quotes " instead of curly quotes “ and ” to maintain consistent formatting and avoid potential encoding issues.
| **Evaluate every test-only PR independently.** A series of narrowly scoped test PRs earns no collective presumption of value: each PR must cite the specific existing coverage it does *not* duplicate, name the distinct regression it guards, and demonstrate a failure against the plausible pre-fix or wrong implementation. “This is another test for the same area” is not merge value. | |
| **Evaluate every test-only PR independently.** A series of narrowly scoped test PRs earns no collective presumption of value: each PR must cite the specific existing coverage it does *not* duplicate, name the distinct regression it guards, and demonstrate a failure against the plausible pre-fix or wrong implementation. "This is another test for the same area" is not merge value. |
The 24/7 review loop and contribution handler now know: honest coverage
decreases are acceptable (bucket-based gate, only engine_regress is fatal;
marker-name collision trap); migrated-area conformance is a REDIRECT lens
backed by gates, not an extra rejection layer; generated-file dirt, export
nondeterminism, and maintainer-caused staleness are never contributor burden.
Adds the sweep-start read of .agents/pr-review/campaign-hotspots.toml
(lead-maintained, untracked).