diff --git a/AGENTS.md b/AGENTS.md index 0e3f8d05..bc61d58d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,7 +132,7 @@ These rules govern *every* test added to `tests/`. They exist because the projec `[tool.coverage.run]` in `pyproject.toml` sets `source = ["parsedmarc"]` and omits `*/parsedmarc/resources/maps/*.py` (maintainer scripts that ship out of the wheel). Counting the test files in the denominator inflates the headline by ~8 percentage points without telling anyone anything useful — pytest discovers test files and runs them, so they're trivially "covered". The number that matters is "what fraction of the installed library does the test suite actually exercise". Don't reintroduce `tests/*` to the coverage scope, don't expand the `omit` list to hide gaps, don't add `# pragma: no cover` to dodge ugly branches. If a branch is genuinely unreachable, delete it; if it's reachable but hard to test, write the test. -### Honest tests assert on observable behaviour +### Honest tests assert on observable behavior A test that mocks every dependency and asserts that the mocks were invoked is testing the mocks, not the code. The benchmark for a good test is: *would this test fail if the code under test were silently wrong?* If the answer is no — if the test would pass regardless of whether the function does what its docstring claims — it isn't a test, it's coverage-padding. @@ -154,13 +154,13 @@ If a test surfaces something that looks like a bug, cite the spec before changin 1. **The relevant RFC** for protocol or report-format questions (RFC 9989 for DMARC policy, RFC 9990 for aggregate reports, RFC 9991 for failure reports, RFC 8460 for SMTP TLS reports, RFC 6591 for legacy ARF). 2. **The internal type contract** (`parsedmarc/types.py` TypedDicts) for project-internal data shapes. 3. **The installed SDK source in the venv** for third-party API questions where the docs are inaccessible — `find venv -name '*.py' -path '**'` and grep, rather than asking a subagent to synthesize an answer. -4. **The official upstream documentation** (Python docs, vendor docs) for language- or platform-level behaviour. The `append_json` bug fix in #775 cited the explicit "writes in `a`/`a+` mode always go to EOF regardless of seek" line from . +4. **The official upstream documentation** (Python docs, vendor docs) for language- or platform-level behavior. The `append_json` bug fix in #775 cited the explicit "writes in `a`/`a+` mode always go to EOF regardless of seek" line from . -Cite the source in the commit message and the test docstring. A reviewer should be able to look at the test and confirm both *what* changed and *why the prior behaviour was wrong*. Two examples worth pattern-matching are #775's SMTP-TLS-to-S3 fix (RFC 8460 §4.3 cited) and the `append_json` fix (Python docs quoted). +Cite the source in the commit message and the test docstring. A reviewer should be able to look at the test and confirm both *what* changed and *why the prior behavior was wrong*. Two examples worth pattern-matching are #775's SMTP-TLS-to-S3 fix (RFC 8460 §4.3 cited) and the `append_json` fix (Python docs quoted). ### Bugs found while writing tests are fixed in the same PR -When a test for the documented behaviour fails because the code is wrong, the right move is to fix the code, not to lock in the broken behaviour. Don't write `self.assertRaises(KeyError)` to make a passing test out of a known bug, and don't skip the test with a "TODO: file separately". If the fix is small and clearly correct against the cited authority above, it belongs in the same PR as the test that found it — the test then doubles as the regression guard. List each fix in `CHANGELOG.md` under the in-progress version's **Bug fixes** section (introducing the heading if it's not there yet). +When a test for the documented behavior fails because the code is wrong, the right move is to fix the code, not to lock in the broken behavior. Don't write `self.assertRaises(KeyError)` to make a passing test out of a known bug, and don't skip the test with a "TODO: file separately". If the fix is small and clearly correct against the cited authority above, it belongs in the same PR as the test that found it — the test then doubles as the regression guard. List each fix in `CHANGELOG.md` under the in-progress version's **Bug fixes** section (introducing the heading if it's not there yet). ### File layout is non-negotiable @@ -174,34 +174,48 @@ If a config file is listed in `.gitignore`, treat its contents as secret. Do not Before rewriting a tracked list/data file from freshly-generated content (anything under `parsedmarc/resources/maps/`, CSVs, `.txt` lists), check the existing file first — `git show HEAD: | wc -l`, `git log -1 -- `, `git diff --stat`. Files like `known_unknown_base_reverse_dns.txt` and `base_reverse_dns_map.csv` accumulate manually-curated entries across many sessions, and a "fresh" regeneration that drops the row count is almost certainly destroying prior work. If the new content is meant to *add* rather than *replace*, use a merge/append pattern. Treat any unexpected row-count drop in the pending diff as a red flag. -## Review passes cover prose, not just function +## Review discipline -A review that only verifies functional/numeric correctness (queries return the right values, files import cleanly, types check) will sail past exactly the defects a text-first reviewer catches. On PR #834, four such misses survived a thorough functional review: two long-standing typos inside the OSD ndjson ("SMPT TLS", "filed DMARC"), a typo on an *unchanged* line adjacent to a docs edit, and hand-written bootstrap glue that duplicated the script's existing `wait_for()` helper. Rules drawn from that: +The rules here were distilled from real review cycles (#834, #839, #849, #851, #858) in which defects survived thorough author-side reviews — in the later cycles, fresh-context diff reviews as well; each parenthetical incident is what its rule would have caught. Grouped by theme, not by PR. -- **Whole-file canonical exports put every line in the diff — review them as text, too.** Re-exporting `dashboards/opensearch/opensearch_dashboards.ndjson` or a Grafana JSON from a running instance rewrites the entire file, so pre-existing user-facing strings (saved-object titles, markdown panels, column labels) are formally part of the change. A semantic before/after comparison ("attributes identical") proves no unintended changes but deliberately looks through pre-existing content problems; add one text-level pass over titles and markdown before committing. -- **Proofread the whole hunk around prose edits, not just the `+`/`-` lines.** Typos one line away from an edit are in the reviewer's context window and fair game; they should be in yours. Proofread the *rendered* text, not just the wording: on PR #839 a comment wrapped so `#169` landed right after the `#` comment marker, making the raw source read `# #169;` — no line was ever wrong, but the wrap point was. Watch how wraps interact with markers and punctuation (`#` before an issue number, a trailing `-`, a code span split across lines) and reflow rather than argue the text is technically correct. -- **Code written mid-incident gets the same review bar as planned code.** Before writing new shell/infra glue while firefighting, check the file for an existing helper that already does it (e.g. `wait_for()` in `dashboard-dev-bootstrap.sh`), and give your own inline code the same scrutiny you'd give a subagent's. +### Review prose as prose -Two more rules, drawn from the PR #839 review (Copilot caught both after a thorough Fable pass missed them): +A review that only verifies functional correctness (queries return the right values, files import cleanly, types check) sails past exactly the defects a text-first reviewer catches. -- **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. +- **Whole-file canonical exports put every line in the diff — review them as text, too.** Re-exporting a dashboard ndjson or Grafana JSON from a running instance rewrites the entire file, so pre-existing user-facing strings are formally part of the change; a semantic before/after comparison ("attributes identical") deliberately looks through them. Add a text-level pass over titles, labels, and markdown (#834: `SMPT TLS` and `filed DMARC` survived such an export review). +- **Proofread the whole hunk and the *rendered* text, not just the `+`/`-` lines.** Typos one line away from an edit are in your context window and fair game, and wrap points interact with markers and punctuation (`#` before an issue number, a trailing `-`, a code span split across lines) — reflow rather than argue the text is technically correct (#834: a typo on an *unchanged* line adjacent to a docs edit; #839: a comment wrapped so the raw source read `# #169;` though no line was ever wrong). +- **Clean inert config inside hunks the diff already rewrites** — stale entries cost nothing to remove and confuse every later reader; "minimize the diff" is the wrong tiebreaker there, and remains the right one for untouched panels/files (#839). +- **Docstrings and comments are prose surface too — beware dual-use terms.** Words that are both colloquial English and load-bearing technical terms near the code in question ("nested", "index", "keyword" in anything Elasticsearch-adjacent) pattern-match as true for an author who holds both facts (#839: a docstring said results were "stored as nested object arrays" when the fix under review hinged on the fields *not* being `nested`-mapped). +- **A plain-type docstring is wrong when `None` is a semantic state.** Documenting optionals with bare types (`ip_db_path (str)`) is fine while `None` merely means "not provided"; when `None` is a meaningful third state, document the union type and the sentinel's meaning, reading the entry as a naive caller who doesn't share your context (#858: `delete_aggregate (bool)` hid that `None`, not `False`, selects inherit-from-`delete`). -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): +### Nothing is pre-verified -- **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". +Code that *feels* already-reviewed — or exempt from review — has zero review coverage. Five disguises, each waved through by an author pass and caught externally: -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** (#849: a "verbatim extraction" carried a latent handler-dedup asymmetry past review, and had just gained a new caller that widened its exposure). A "pure move" is a claim about behavior preservation, not an exemption from review — read extractions cold, and be *more* suspicious when a hunk gains callers than when it changes logic. +- **Extracted helpers** (#849: a promoted helper lacked the bound check its old call site had made unnecessary, and its docstring misdescribed its stop path). A helper inherits none of its call site's implicit guarantees: it needs its own eager input validation and docstring↔behavior check even when every current caller is safe. +- **Fixes made during review** (#851: fixing `__getstate__` and stopping there left `__setstate__`'s version-skew hole — old pickles into new code — unexamined; `__init__` never runs during unpickling, so missing fields end up not defaulted but *unset*). Touching one direction of a paired protocol (`__getstate__`↔`__setstate__`, save↔load, encode↔decode) obligates re-deriving the inverse, including inputs no current fixture produces. The review isn't done when the fixes are written. +- **Rewritten code, for coverage** (#858: a log-equivalent rewrite of the disposal loop shipped its error handler uncovered). Rewritten lines are new patch lines even when behavior is intentionally identical. +- **Mid-incident glue** (#834: hand-written bootstrap code duplicated the script's existing `wait_for()` helper). Firefighting is not an exemption: before writing new shell/infra code mid-incident, check the file for an existing helper that already does it, and give your own inline code the same scrutiny you'd give a subagent's. -- **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. +### Check claims against what they range over -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): +The defects that author-side reviews miss are rarely inside one artifact — they are relations between two individually-correct places (#839's review cycle: every missed defect had this shape). -- **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 the `parsedmarc/` and `tests/` directories 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. +- **When fixing one half of a contract, grep for the other half**: write↔read against `types.py`, comment↔declaration, a docstring guarantee↔every statement in its scope, a UI string↔the docs naming it (#839: `types.py` said `additional_info_uri`, the saver read the long-form key — parser and saver each "correct"). +- **Count enumerations against the code-defined set they enumerate** — derive the set from the code and count both sides; a reader can't tell an intentional subset from an omission (#851: docs listed seven of the eight `config=`-accepting functions). +- **A quantified claim is an enumeration in disguise, and "pre-existing" triage stops applying when the diff extends its set** (#858: the `config=` paragraph's "arguments listed above are ignored" was filed as pre-existing looseness — but the PR added four arguments to the list that claim quantifies over, making it false for them). +- **Build verification fixtures containing 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 (#839). + +### Verify what CI enforces, not a plausible subset + +- **Run CI's literal commands from the repo root** — read the workflow file (#851: checks scoped to `parsedmarc/` and `tests/` declared the tree green while repo-wide `ruff format --check .` failed on a Python block inside `docs/source/usage.md`). 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. +- **Cover CI's gates, not just its commands.** Patch coverage corresponds to no replayable workflow command, so command-replay never asks "does a test execute every new line?" — compare coverage's missing-lines set against the diff (`pytest --cov --cov-report=term-missing`, or diff `coverage.xml` against the patch) before opening a PR (#858: the disposal loop's delete-error handler shipped uncovered). +- **An ad hoc check that matches nothing is broken, not green** (#858: a post hoc script filtered `coverage.xml` on `parsedmarc/__init__.py`, the report stores source-relative `__init__.py`, and the empty result read as "all covered"). Build one-off checks to fail loudly on zero matches; silence is not success. + +### End with a fresh-context review, not a self re-read + +The author's "cold re-read" is never cold — it confirms the model the author already holds, which is exactly the blindness a fresh reader doesn't share; every cycle above reproduced this. 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 (a subagent given just the diff, or an external reviewer) — and end it asking "do these hunks agree with *each other*?", not "is each hunk correct?". Triage its findings like any external review: fix what's real, push back with cited reasoning on what isn't. ## Releases