mirror of
https://github.com/domainaware/parsedmarc.git
synced 2026-07-30 04:14:56 +00:00
A fresh automated reviewer caught five defects across two rounds of PR #849 that the authoring model's own review passes missed. Three new review rules capture the shape of those misses: moved code is new code (a verbatim extraction carried a latent FileHandler-dedup asymmetry past review because "pure move" felt pre-verified), an extracted helper is new API surface (the promoted pool helper was missing input validation its old call site had made unnecessary, and its docstring drifted from its stop behavior), and end with a fresh-context review rather than a self re-read (the author's cold re-read is never cold). Also documents the CHANGELOG release convention in the Releases section: feature/fix PRs accumulate entries under "## Unreleased" and never pick a version number; the release PR renames the heading and bumps parsedmarc/constants.py together. PR #849 initially guessed a "10.4.0" heading and had to walk it back. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
48445c639e
commit
4f4733003c
@@ -191,11 +191,17 @@ One meta-rule, distilled from the full PR #839 review cycle (19 external review
|
||||
|
||||
- **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".
|
||||
|
||||
Three more rules, from the PR #849 review cycle (a fresh automated reviewer caught all of these across two rounds after the authoring model's own review passes missed them):
|
||||
|
||||
- **Moved code is new code.** Framing an extraction as a "pure move" is a claim about behavior preservation, not an exemption from review — the moved lines land in the diff as new code, usually with new callers. On #849, a verbatim logging-setup extraction carried a latent asymmetry (StreamHandlers were deduplicated, FileHandlers were not, three lines apart) straight past review because "verbatim" felt pre-verified; the move had also just added a new caller that widened the bug's exposure. Review extracted code cold, as if seeing it for the first time, and be *more* suspicious when a hunk gains callers than when it changes logic.
|
||||
- **An extracted helper is new API surface.** Code promoted out of a call site for reuse inherits none of that call site's implicit guarantees. The original inline code could assume its inputs were already clamped; the reusable helper cannot — it needs its own input validation (raised eagerly, not deferred until a generator is first iterated) and its own docstring↔behavior check, even when every *current* caller happens to be safe. On #849 the promoted pool helper was missing an `n_procs` bound check its old call site had made unnecessary, and its docstring described a stop path that didn't match what the implementation actually did.
|
||||
- **End with a fresh-context review, not a self re-read.** The author's "cold re-read" is never cold — it confirms the model of the code the author already holds, which is exactly the blindness a fresh reader doesn't share (see the #839 lesson above; #849 reproduced it two rounds running). Before opening a PR, run a review pass whose reviewer has seen *only* the final diff — no plan, no conversation history, no memory of writing it (e.g. a subagent given just the diff, or an external reviewer). Findings from that pass are triaged like any external review: fix what's real, push back with cited reasoning on what isn't.
|
||||
|
||||
## Releases
|
||||
|
||||
A release isn't done until built artifacts are attached to the GitHub release page. Full sequence:
|
||||
|
||||
1. Bump version in `parsedmarc/constants.py`; update `CHANGELOG.md` with a new section under the new version number.
|
||||
1. Bump version in `parsedmarc/constants.py`; rename `CHANGELOG.md`'s `## Unreleased` heading to the new version number (these two edits always land together, in the release PR only). Feature/fix PRs accumulate their entries under `## Unreleased` and never touch `constants.py` or pick a version number — choosing the number is a release-time decision.
|
||||
2. Commit on a feature branch, open a PR, merge to master.
|
||||
3. `git fetch && git checkout master && git pull`.
|
||||
4. `git tag -a <version> -m "<version>" <sha>` and `git push origin <version>`.
|
||||
|
||||
Reference in New Issue
Block a user