diff --git a/AGENTS.md b/AGENTS.md index 0ad30991..a3abc126 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 -m "" ` and `git push origin `.