diff --git a/AGENTS.md b/AGENTS.md index a3abc126..f6b934c1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -197,6 +197,12 @@ Three more rules, from the PR #849 review cycle (a fresh automated reviewer caug - **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. +Three more rules, from the PR #851 review cycle (the first run with all of the above loaded in context — the author's own seam-checklist pass did catch three real defects before the PR opened, so the habits fire, but a fresh-context reviewer still caught four more): + +- **A fix made during review is new code with zero review coverage.** #851's review pass fixed the fragile hand-enumerated field list in `ParserConfig.__getstate__` — and stopped there. The symmetric obligation that fix implied in `__setstate__` went unexamined (state pickled by an older version lacks later-added fields, and `__init__` never runs during unpickling, so those fields end up not defaulted but *unset*), and the external reviewer caught it. Touching one direction of a paired protocol (`__getstate__`↔`__setstate__`, save↔load, encode↔decode) obligates re-deriving the inverse direction — including version-skew inputs (old data into new code) that no current fixture produces. The seam checks apply to your own review fixes; the review isn't done when the fixes are written. +- **Verification means CI's literal commands from the repo root, not a plausible subset.** #851's local checks scoped to `parsedmarc/ tests/` declared the tree green while CI's `ruff format --check .` failed on a Python example inside `docs/source/usage.md` — ruff formats code blocks in Markdown, and `docs/` was outside the scoped run. Read the workflow file and run the same commands with the same scope before opening a PR. When repo-wide runs are noisy because of untracked local directories, fix the exclusion in config rather than narrowing the command — a narrowed command is a different check that happens to share a name. +- **Count enumerations against the set they enumerate.** The new library-usage docs listed "the main entry points" and the next paragraph asserted that "each of these functions" accepts `config=` — but the list named seven of the eight `config=`-accepting functions. Whenever prose enumerates a code-defined set (functions gaining a kwarg, supported outputs, config keys), derive the set from the code (grep the signatures) and count both sides; a reader can't tell an intentional subset from an omission. + ## Releases A release isn't done until built artifacts are attached to the GitHub release page. Full sequence: