From ebf64b8a7978d0ebc69cb0a863bf1bc83053deca Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 17 Jul 2026 20:40:55 -0700 Subject: [PATCH] docs: fold in Rod's PR-review feedback into GUIDELINES + CONTRIBUTING (#6211) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs: fold in Rod's PR-review feedback into GUIDELINES + CONTRIBUTING Based on rodboev's feedback distilled from 50+ recent PRs. Four accepted points plus one clause, folded into existing rules rather than adding new ones: - Rule 6: load the reporter's shipped reproduction, don't rebuild a fixture from your reading of it (the one genuine hole — a fix and test from the same wrong model agree with each other and certify a no-op). - Rule 2: confirm a value is authoritative (declared at the point of intent), not inferred from id prefix / content shape / emptiness / DOM state. - Rule 4: read prior PRs and review threads to find a subsystem's real variants instead of inventing axes from the single case handed to you. - Rule 1: the chokepoint is the smallest boundary that contains the fault, not the widest you can reach (don't disable a whole pipeline to suppress one output). - "Show your work": name who owns the truth for any claim the repo doesn't own. CONTRIBUTING.md carries the two contributor-facing points (repro-loading, proof-ownership) in the PR-description section, deferring detail to GUIDELINES.md. Co-authored-by: rodboev * docs: tighten Rule 6 escape hatch per Rod — gate on shape under-specified, not file absent Rod's review: the #5749 no-op came from a fully prose-specified repro (fenced JSON + field-level conditions + steps), not a missing file. The old hatch ('why the issue gave you nothing to load') reads as 'no downloadable attachment', letting someone walk past a binding JSON block. Gate the hatch on the SHAPE being under-specified instead: a fenced JSON structure / field conditions / step list pin the shape as bindingly as a file; if pinned, satisfy every condition and don't add a property the shape never had to make a guard fire; only say 'constructed, assumed X' when the shape is truly unpinned. Co-authored-by: rodboev * docs: distill Rule 6 repro-shape guidance into a principle (Rod style note) Rod: 'distill into durable principles, don't enumerate lists; strip negative conditions that read like narration.' Reworked the addition to lead with the principle ('a reproduction is whatever pins the bug's shape'), collapse the capture/JSON/conditions/steps enumeration into flowing prose, and convert the 'don't add a property...' negative into a positive imperative ('bind your fixture to that shape: satisfy every condition... instead of granting...'). Same for the CONTRIBUTING bullet. --------- Co-authored-by: nesquena-hermes Co-authored-by: rodboev --- CONTRIBUTING.md | 13 +++++++++++++ docs/GUIDELINES.md | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 32cd1802..f7efe7b6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -97,6 +97,19 @@ There is currently no PR template in this repo, so include the important section - Risks / Follow-ups - Model Used +Two things reviewers look for specifically in Verification, both detailed in +[`docs/GUIDELINES.md`](docs/GUIDELINES.md): + +- When the issue pins the bug's shape — a session capture, a script, exact steps, or + a fenced JSON block / field-level trigger conditions — bind your test to *that* shape + rather than a fixture rebuilt from your reading of it, satisfying every condition it + names; otherwise the test can pass while the reported bug goes untouched. Reconstruct + the shape yourself only when the issue leaves it unpinned, and then say what you assumed. +- For any claim the repo doesn't own (browser behavior, a provider's API, a registry, + an OS convention), name who owns the truth and confirm your proof is one they'd accept. + In-page automation proves page behavior, not browser behavior; a mock proves your + intent, not the provider's contract. + If the change is user-visible, include screenshots or a short video. If the change is release-note-worthy, include concise release-note wording in diff --git a/docs/GUIDELINES.md b/docs/GUIDELINES.md index 74b52143..b351bf5a 100644 --- a/docs/GUIDELINES.md +++ b/docs/GUIDELINES.md @@ -48,11 +48,21 @@ Before writing the fix, search for every sibling — other call sites, producers alternate backends, companion endpoints, other layouts, every lifecycle exit — and fix at the **shared chokepoint**. If you deliberately leave one out of scope, name it in the PR and say why. One guard in the shared function is a smaller and more correct change than one guard per caller. +The chokepoint is the *smallest* boundary that contains the fault, not the widest one you can +reach: fixing the class means guarding every sibling of the bug, not disabling a whole stage or +pipeline to suppress one bad output. If your fix blocks more than the fault occupies, it has grown +past the task (rule 9). ### 2. Trace one authoritative value end-to-end. Follow the value from `input → normalize → decision → action → persist → cleanup`, and use the **same** resolved value at every stage. Don't let the code that *decides* see one value while the code that *acts* uses the stale or un-normalized one — that split is a silent correctness bug. +Before you key a decision on a value, confirm it's *authoritative*: written at the point of intent +(a canonical kind field, a provenance marker stamped by the action itself, a recorded origin) — not +inferred from an id prefix, content shape, emptiness, or DOM state. Search for an existing canonical +field first; inference that happens to work on the case in front of you is the most common way a +guard misfires on a sibling. If no authoritative field exists, add a test with an adjacent case that +matches your heuristic and must *not* trigger. ### 3. When you can't confirm something, fail closed and say so. For anything touching authority, capability, identity, or containment: if you cannot *confirm* it's @@ -64,7 +74,12 @@ not verify X" in the PR is good engineering; hiding it is how a security or reli Write down which dimensions the change touches — entry point, backend, item count (0 / 1 / many / duplicate), lifecycle exit (success / error / cancel / replace / shrink / teardown), auth on vs off, concurrency (two profiles or workers at once), input shape (empty / hostile / aliased) — and cover -each, or mark it out of scope on purpose. Most redo rounds are one un-considered dimension. +each, or mark it out of scope on purpose. Most redo rounds are one un-considered dimension. Those +axes are generic; the ones that actually bite are subsystem-specific and are not obvious to a +first-time contributor. Before editing an unfamiliar subsystem, find its real variants: read the +closed PRs and review threads that touched the same code, where the discriminating dimensions +(every session-lineage kind, every replay/recovery source, each stream response shape) were already +enumerated. Reuse that list rather than inventing your own from the one case you were handed. ### 5. Assume inputs and check-then-use gaps are adversarial. Input crossing a trust boundary can be crafted to break a naive check (odd delimiters, casing, YAML @@ -79,7 +94,16 @@ Run your new test against the current code *first* and confirm it fails for the then apply the fix. Assert the **observable behavior** (the row updates, the secret is absent from the log, the request carries the right value) — not a source string, and not through a mock that stands in for the very thing under test. If the bug is about picking the right one of several items, -the test must include several items, or it can't catch picking the wrong one. +the test must include several items, or it can't catch picking the wrong one. When the issue ships a +reproduction — a session capture, a script, exact steps, an attachment — your test loads **that**, +not a fixture you rebuilt from your reading of it. A fix and a test written from the same wrong model +of the bug will base-fail and head-pass on the fixture forever while the real symptom sits untouched; +loading the reporter's actual scene is what breaks that agreement. A reproduction is whatever *pins +the bug's shape* — a downloadable capture, but a fenced JSON structure, the field-level trigger +conditions, or exact steps bind it just as tightly. Bind your fixture to that shape: satisfy every +condition it names and let the assertion fail when any one is violated, instead of granting the +fixture a property the report never had so a guard will fire. Reconstruct the shape yourself only +when the issue genuinely leaves it unpinned — and then say so, and say what you assumed. ### 7. Name the owner of every piece of state and prove it's released. For each resource or mutation you introduce (a cache entry, a lock, a temporary env change, a @@ -119,6 +143,11 @@ to discover the gaps. Make your PR description carry the evidence: - **Before/after images** for any visible change (desktop + narrow). - **What you could not verify** — an explicit list. An admitted gap is fine; a hidden one is a bug waiting to be found in review. +- **Who owns the truth** for any claim the repo doesn't — browser behavior, a provider's API, a + registry, an OS convention. Name the owner and check your proof is one they'd accept: in-page + automation proves page behavior, not that the browser didn't swallow the event first; a mock + proves your intent, not the provider's contract; a synthetic fixture proves nothing about the live + system. The failure mode here isn't knowing you couldn't verify — it's thinking you did. Following this imperfectly still helps enormously, because the reviewer can see exactly where your coverage stops instead of finding it the hard way.