Document the seam-review lesson from PR #839's review cycle (#845)

Every defect the author-side reviews missed across the PR's 19 external
review rounds was a relation between two individually-verified places —
contract halves, comment vs. declaration, UI string vs. docs. Record
the pattern and the three habits that close it: grep for the other half
of any contract you touch, verify with fixtures that include what the
sample corpus lacks, and finish with a cold re-read of the final diff
checking that hunks agree with each other.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Sean Whalen
2026-07-25 12:41:33 -04:00
committed by GitHub
co-authored by Claude Fable 5
parent 23c5ea9ad6
commit 03dc92519a
+4
View File
@@ -187,6 +187,10 @@ Two more rules, drawn from the PR #839 review (Copilot caught both after a thoro
- **Docstrings and comments are prose surface too — and beware dual-use terms.** A regression-test docstring described DKIM/SPF results as "stored as nested object arrays"; in Elasticsearch/OpenSearch "nested" is a specific mapping type, and the fix under review hinged on the fields being dynamic-mapped as plain `object`, *not* `nested`. The reviewer had held both facts all session, so the blended sentence pattern-matched as true — author's-context blindness that a fresh reader doesn't share. Give docstrings/comments the same text-level pass as docs and dashboard labels, with extra suspicion for words that are both colloquial English and load-bearing technical terms near the code in question ("nested", "index", "keyword" in anything Elasticsearch-adjacent).
- **Clean inert config inside hunks the diff already rewrites.** Stale entries (e.g. orphaned `renameByName` keys in a Grafana panel) sitting inside a block the PR is editing anyway cost nothing to remove and confuse every later reader if kept; "minimize the diff" is the wrong tiebreaker there. It remains the right tiebreaker for untouched panels/files — don't expand a PR's blast radius to chase pre-existing cruft elsewhere.
One meta-rule, distilled from the full PR #839 review cycle (19 external review rounds; every defect the author's own reviews missed had the same shape):
- **Check the seams, not just the artifacts.** The missed defects were all relations between two individually-verified places: a docstring's never-raises guarantee vs. a statement outside the try block it described; a fixed write-side kwarg vs. an unchecked read-side key (`types.py` says `additional_info_uri`; the saver read the long-form name — the parser and the saver were each "correct"); a freshly corrected comment vs. the `Nested()` declaration one screen away; a panel's displayed title vs. the docs naming it. Three habits close them: when fixing one half of a contract, grep for the other half (write↔read against `types.py`, comment↔declaration, guarantee↔every statement in its scope, UI string↔docs); build verification fixtures that include what the sample corpus lacks — optional fields, injected errors, over-the-cap sizes — because an absent field makes the wrong key and the right key behave identically; and end a review with a cold re-read of the final diff asking "do these hunks agree with *each other*", not "is each hunk correct".
## Releases
A release isn't done until built artifacts are attached to the GitHub release page. Full sequence: