From 0ffc804ea0b9a6bfcc24fc416e2a6ada5dd042c4 Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Tue, 21 Jul 2026 22:04:22 -0400 Subject: [PATCH] Add PR #839 review lessons to AGENTS.md Extend the "Review passes cover prose" section with two rules from the #839 Copilot findings: docstrings/comments get the same text-level review pass as docs and dashboard labels (with suspicion for dual-use terms like "nested" near Elasticsearch code), and inert config entries inside hunks a PR already rewrites should be cleaned rather than preserved to minimize the diff. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 1a17b3e1..07b8aa17 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -182,6 +182,11 @@ A review that only verifies functional/numeric correctness (queries return the r - **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. - **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. +Two more rules, drawn from the PR #839 review (Copilot caught both after a thorough Fable pass missed them): + +- **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. + ## Releases A release isn't done until built artifacts are attached to the GitHub release page. Full sequence: