mirror of
https://github.com/block/buzz.git
synced 2026-10-27 20:41:57 +00:00
docs: add review-proven failure-path & async-state rules to AGENTS.md (#7061)
Mining the last 25 PRs' review threads (45 substantive findings, 11 reviewed PRs, avg **4.8 review rounds** each) shows **53% of findings are repeats** of five clusters: swallowed failures, stale-async-state races, tests that don't bind the production seam, unbounded resources/retry loops, and non-atomic multi-step persistence. PR #6956 alone burned 4 rounds converging on one of these classes. A second, independent mining pass over **71 agent-review rooms (303 findings, Aug 18–29)** confirmed the same clusters and added outcome data — how often authors actually fix each finding class once flagged: test-seam binding and unbounded-resource findings **100%**, swallowed errors **90%**, stale-state races **70%**. It also surfaced two clusters the GitHub-thread pass under-sampled: **assistive-semantics defects** (44 findings, second-largest cluster) and **input-modality divergence** (27 findings), now rules 7–8. This PR distills those clusters into eight imperative rules in AGENTS.md so agents apply them **before writing code**, adds one client-consumption invariant to ARCHITECTURE.md §5, and places the test-quality rule in TESTING.md (per the team decision that testing docs are the canonical guide for review standards), cross-referenced from AGENTS.md. Each rule cites the PRs where it was litigated. Raw mining data: `reviews.jsonl` / `comments.jsonl` + `backfill/buzz-review-findings.jsonl` (review-mining artifacts, not committed). No code changes. CLAUDE.md is a symlink to AGENTS.md and picks this up automatically. 🤖 Drafted by Jude's agent from automated mining of this repo's last 25 PRs' review threads and 71 agent-review rooms; every rule cites the PRs where it was litigated. Jude reviews and owns the result. Mining method + raw cluster data available on request. --------- Signed-off-by: Jude Edwards <judeedwards@squareup.com>
This commit is contained in:
85
AGENTS.md
85
AGENTS.md
@@ -151,6 +151,91 @@ Additional rules:
|
||||
|
||||
---
|
||||
|
||||
## Review-Proven Rules
|
||||
|
||||
These rules distill the recurring findings from the last 25 PRs' review
|
||||
threads — 53% of substantive review findings were repeats of the clusters
|
||||
below, and reviewed PRs averaged ~5 review rounds. A second, independent
|
||||
mining pass over 71 agent-review rooms (303 findings, Aug 18–29) confirmed
|
||||
the same clusters and measured how often authors actually fix each class
|
||||
once flagged: test-seam binding and unbounded-resource findings were fixed
|
||||
**100%** of the time, swallowed-error findings **90%**, stale-state races
|
||||
**70%** — these are not style opinions, they are defects authors agree
|
||||
with on sight. Apply the rules **before writing code**; each cites the
|
||||
PRs where reviewers litigated it.
|
||||
|
||||
1. **Every caught failure must leave a durable retry record or propagate.**
|
||||
Never catch-log-and-return-success (opt-out revocation permanently
|
||||
abandoned, PR #6269), never convert a terminal failure into an
|
||||
authoritative success/empty result (cold-history `error` → `success`
|
||||
with `[]`, PR #7013), and never delete the durable journal an operation
|
||||
depends on before its retry has actually succeeded (PR #6269). If a
|
||||
partial failure can orphan committed state (installations, endpoints),
|
||||
schedule its cleanup/renewal durably (PRs #6269, #6996, #7013).
|
||||
|
||||
2. **Fence async results by generation; clear derived metadata on every
|
||||
removal path.** A completing in-flight probe or fetch must verify it is
|
||||
still the newest before writing its result (stale login-shell probe
|
||||
recached a false-negative PATH, PR #6904). Provenance/ownership metadata
|
||||
attached to synthetic state must be updated or cleared on *all* paths
|
||||
that remove or refresh that state — typed deletion, toolbar removal,
|
||||
profile/name refresh; enumerate the paths and test each (PR #6956 burned
|
||||
4 rounds on this one class). Backfill and live subscriptions must
|
||||
overlap — a gap between a finite history REQ and the live subscription
|
||||
silently drops events (PR #3995); a retired chunk must not keep a stale
|
||||
scope fence (PR #6996). (PRs #3995, #6904, #6956, #6996)
|
||||
|
||||
3. **Regression tests must bind the production seam and be falsifiable.**
|
||||
See "Review-Proven Test Standards" in [TESTING.md](TESTING.md) for the
|
||||
full rule — in short: a guard whose removal doesn't fail any test
|
||||
protects nothing; bind regression tests to the production code path,
|
||||
not test-only helpers. (PRs #6807, #6980, #6996, #7013)
|
||||
|
||||
4. **Bound every resource, loop, and process tree.** Cap captured
|
||||
output (unbounded discovery temp files exhausted disk and overran the
|
||||
deadline, PR #6904). Containment failures are errors, not warnings — a
|
||||
tolerated Job Object creation failure or a `setsid` escape leaks whole
|
||||
process trees (PR #6904). Retry/re-subscribe loops need backoff and a
|
||||
terminal state: a persistent failure must not self-amplify into an
|
||||
unbounded refresh loop (PR #6996), and check zero-delay edge cases
|
||||
(`remainingMs()==0` selected the wrong fallback window, PR #6996).
|
||||
(PRs #6904, #6996)
|
||||
|
||||
5. **One user action = one atomic persist.** Implementing a single user
|
||||
commit as N independent durable writes leaves torn state on partial
|
||||
failure (theme "Set" as three independent notifier persists, PR #6944;
|
||||
relay-commit vs. local-save recovery gap, PR #6269). Persist one
|
||||
snapshot, or order the writes so every prefix is consistent and the
|
||||
remainder is durably retried per rule 1. (PRs #6269, #6944)
|
||||
|
||||
6. **A guard that hides the only recovery affordance is a functional
|
||||
failure.** Before adding a visibility predicate or state fence, ask:
|
||||
if the state it assumes goes wrong, does the user still have a way
|
||||
back? A fence that permanently suppresses "jump to latest" after a
|
||||
bounded correction fails strands the user silently — two reviewers
|
||||
flagged this independently (PR #6807).
|
||||
|
||||
7. **Audit assistive semantics on every new visual component.** The
|
||||
agent-review lanes flagged accessibility defects on 44 findings across
|
||||
the Aug 18–29 window — the second-largest cluster — and authors fixed
|
||||
the concrete ones (duplicate VoiceOver stops on native controls,
|
||||
actionable labels owned by two widgets at once, PR #6680; missing or
|
||||
decorative-leaking semantics on new UI, PRs #6611, #6702, #6885, #6905,
|
||||
#6908). New UI ships with: one owner per actionable label, no duplicate
|
||||
screen-reader stops, and explicit semantics for every interactive
|
||||
element. (PRs #6611, #6680, #6702, #6885, #6905, #6908, #6980)
|
||||
|
||||
8. **Every input modality is a first-class seam.** Keyboard, pointer, and
|
||||
hotkey paths must not silently diverge: `Shift+Space` treated as plain
|
||||
`Space` because the guard omitted `shiftKey` (PR #6862), keyboard
|
||||
ownership not released on blur, modifier keys dropped on the non-mouse
|
||||
path (PRs #5958, #6793, #6860, #6908, #7006). When adding an input
|
||||
handler, enumerate the modalities that can reach it and test the
|
||||
non-primary ones — that's where the defects were. (PRs #5958, #5972,
|
||||
#6793, #6860, #6862, #6908, #7006)
|
||||
|
||||
---
|
||||
|
||||
## Key Patterns
|
||||
|
||||
**Nostr-first HTTP surface**: Buzz's primary API is NIP-29 over WebSocket. The relay also exposes a narrow HTTP surface: NIP-11/NIP-05 metadata, `POST /events`, `POST /query`, `POST /count`, workflow webhooks at `/hooks/{id}`, Blossom media, git smart HTTP, git policy hooks, and health probes. These HTTP paths all preserve the same host-derived community boundary.
|
||||
|
||||
@@ -325,6 +325,15 @@ This prevents a race where a non-member receives live fan-out events from a priv
|
||||
|
||||
After registering, the REQ handler queries Postgres for stored events matching the filters (up to 500 per filter, hard cap). These are sent as `["EVENT", sub_id, event]` frames before `["EOSE", sub_id]`. New events arriving after EOSE are delivered via the fan-out path.
|
||||
|
||||
**Client consumption invariant.** A client rebuilding channel state must
|
||||
open its live subscription before (or overlapping) the finite history
|
||||
REQ — a gap between the last backfill page and live delivery silently
|
||||
drops events and rebuilds stale state (PR #3995). When the relay sends a
|
||||
terminal CLOSED, the subscription is removed server-side; any client-side
|
||||
ownership tied to it (chunk/scope fences) must be released in the same
|
||||
step, or live delivery stops permanently while the client believes it is
|
||||
subscribed (PR #6996).
|
||||
|
||||
---
|
||||
|
||||
## 6. Crate Reference
|
||||
|
||||
15
TESTING.md
15
TESTING.md
@@ -16,6 +16,21 @@ just test # unit + integration (starts Docker if needed)
|
||||
cargo test -p buzz-test-client -- --ignored
|
||||
```
|
||||
|
||||
### Review-Proven Test Standards
|
||||
|
||||
Mined from the last 25 PRs' review threads (see Review-Proven Rules in
|
||||
[AGENTS.md](AGENTS.md)); this is the test-quality rule reviewers litigated
|
||||
most:
|
||||
|
||||
**Regression tests must bind the production seam and be falsifiable.**
|
||||
A guard whose removal doesn't fail any test protects nothing — mutations
|
||||
survived the full mobile suite twice (PRs #6996, #7013). Don't bind a
|
||||
regression test to a test-only helper instead of the production code
|
||||
path (PR #7013). Give pure predicates a table test over the full input
|
||||
combination space (PR #6807). Scope Playwright locators — unscoped
|
||||
`getByText` in a required smoke test is a strict-mode flake (PR #6980).
|
||||
(PRs #6807, #6980, #6996, #7013)
|
||||
|
||||
---
|
||||
|
||||
## Live Local Relay
|
||||
|
||||
Reference in New Issue
Block a user