diff --git a/AGENTS.md b/AGENTS.md index 24115501ad8..4395283b0fb 100644 --- a/AGENTS.md +++ b/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. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 892082d96c6..138d192fa12 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -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 diff --git a/TESTING.md b/TESTING.md index 0e64b740665..0e4aee87841 100644 --- a/TESTING.md +++ b/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