Merge maint into master (advert filter, PR-REVIEW)

CHANGELOG.md

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- [`PR-REVIEW.md`](PR-REVIEW.md) — the 13-criteria PR review checklist
+  the maintainer runs against every incoming PR, published at the
+  repo root so contributors can run the same pass on their own change
+  (directly or by handing the document to a coding agent) before
+  opening. Linked from `CONTRIBUTING.md` under "Submitting pull
+  requests" and "Further reading". Running the checklist before
+  opening surfaces problems that would otherwise come back as review
+  comments, saving a round trip.
+
 ### Changed
 
 - Nostr discovery startup is now non-blocking. `Node::start` no
@@ -92,6 +103,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Nostr discovery: filter unroutable direct UDP/TCP advert endpoints.
+  Publisher and validator now retain only endpoints that parse as
+  concrete socket addresses with routable IPs and nonzero ports.
+  `udp:nat` rendezvous endpoints and Tor endpoints pass through
+  unchanged. Adverts that collapse to zero usable endpoints after
+  filtering are rejected with a clear "missing publicly routable
+  endpoints" error. Before this change, misconfigured nodes could
+  publish RFC1918, loopback, link-local, CGNAT 100.64/10, IPv6 ULA,
+  or IPv6 link-local endpoints into Nostr discovery, and consumers
+  would cache and dial them; in mixed LAN/VPN/NAT environments, that
+  could prefer a misleading one-way private path over the intended
+  `udp:nat` bootstrap.
 - Coord cache invalidation made surgical at parent-position-change
   and root-change sites. Replaces the previous unconditional
   `CoordCache::clear()` calls with two targeted methods:

CONTRIBUTING.md

@@ -148,6 +148,20 @@ that touches your code path.
 This is the same matrix that runs on GitHub Actions. Catching a
 regression locally is much cheaper than catching it in CI.
 
+### Self-review against the project review checklist
+
+The 13-criteria checklist the maintainer runs on every incoming PR is
+published at [PR-REVIEW.md](PR-REVIEW.md). Run your own change through
+it before opening — or hand the document to your coding agent with
+"review my branch against this checklist" and let it do the pass. The
+checklist covers PR hygiene (body, commit shape, base freshness), diff
+content (does the change do what the description says, does it fit the
+codebase as a natural extension), and cross-cutting concerns (tests,
+docs, dependencies, security, contributor-conventional Rust patterns).
+
+This is the first thing the maintainer does on any submission, so
+running it yourself saves a review round trip.
+
 ### Additional requirements for feature PRs
 
 - **New CI coverage.** Features added without a test that exercises
@@ -233,6 +247,9 @@ home yet, file a GitHub issue with the `design` label.
 
 ## Further reading
 
+- [PR-REVIEW.md](PR-REVIEW.md) — the 13-criteria PR review checklist
+  the maintainer runs on every incoming PR; run it yourself before
+  opening to save a round trip.
 - [docs/design/README.md](docs/design/README.md) — protocol design tree.
 - [docs/branching.md](docs/branching.md) — full release workflow and
   merge-direction rationale.

PR-REVIEW.md

@@ -0,0 +1,199 @@
+# PR Review Checklist
+
+<!-- markdownlint-disable MD013 -->
+
+This is the 13-criteria checklist the maintainer runs against every
+incoming PR. The first pass on any submission is exactly this list,
+so executing it yourself before opening — or after pushing a fresh
+revision — saves a review round trip and surfaces problems faster.
+
+The document is also written so you can hand it to a coding agent
+(Claude Code, Copilot, Cursor, Aider, etc.) with "review my branch
+against this checklist" and get a structured pass. The agent gets
+better results than a free-form "review my PR" because every concern
+the maintainer cares about is enumerated below.
+
+## Step 1 — Should this even be reviewed?
+
+Skip the review (and say so) if the PR is:
+
+- closed, merged, or marked draft
+- automated (bot author, dependabot, etc.) and trivially OK
+- so small and obviously correct (typo fix, single-line doc tweak)
+  that a thirteen-point pass is overkill — a one-paragraph informal
+  review is better in that case
+
+## Step 2 — Gather context
+
+Read these *before* analyzing the diff so the review is grounded:
+
+1. PR metadata. Title, body, author, head ref, base ref, head SHA,
+   base SHA, mergeable status, CI rollup, commit list.
+
+   ```bash
+   gh pr view <num> --json title,body,author,headRefName,baseRefName,headRefOid,baseRefOid,mergeable,statusCheckRollup,commits
+   ```
+
+2. The diff.
+
+   ```bash
+   gh pr diff <num>
+   ```
+
+3. Base-branch freshness. How many commits have landed on the PR's
+   base since the PR forked from it.
+4. Project guidance. Read [CLAUDE.md](CLAUDE.md) at the repo root and
+   any nested `CLAUDE.md` in directories the diff touches. These
+   describe project-specific conventions and constraints not visible
+   from the diff alone.
+5. Related work on GitHub. Skim the [open issues](https://github.com/jmcorgan/fips/issues)
+   and other [open PRs](https://github.com/jmcorgan/fips/pulls) for
+   work that overlaps, duplicates, partially addresses, or is unblocked
+   by this PR.
+6. For "this looks wrong" observations later: `git blame` the modified
+   lines and read recent commit history on the same files for context
+   before flagging something as a problem. What looks like a bug at
+   first glance is often a deliberate workaround documented in a prior
+   commit message.
+
+## Step 3 — The 13 criteria
+
+The review must address all 13 criteria below at some point. They
+group naturally into PR hygiene, diff content, and cross-cutting
+concerns — but the report itself is *not* organized this way; see
+Step 4.
+
+### Group A — PR hygiene (structural review)
+
+1. **PR body and issue cross-reference**. Does the body accurately
+   describe the change (feature added or bug fixed) and match what
+   the diff actually does? Is there an associated issue that
+   should be referenced via `Closes #N` / `Fixes #N`?
+2. **Commit hygiene and base freshness**. Is the PR a clean set of
+   commits (or a single commit) representing appropriately chunked
+   development items, or are there intermediate "WIP" / "fix typo" /
+   "address review" commits that should have been squashed? Is the
+   branch based off a recent `maint` / `master` / `next`, or has the
+   base diverged far enough that rebase work is needed?
+3. **Commit message quality**. Are the commit messages well-structured
+   (subject + body where the change warrants), accurately referencing
+   everything actually in each commit, and free of extraneous footers
+   — particularly coding-assistant attribution (`Generated with
+   Claude Code`, `Co-Authored-By: Claude`, similar from other AI
+   tools)?
+
+### Group B — Diff content
+
+4. **Does it do what it says it does**. Walk each claimed behavior
+   from the PR body against the actual diff lines.
+5. **Coherent whole**. Are all parts of the diff in service of the
+   stated goal, or are there drive-by formatting changes, unrelated
+   touch-ups, or scope creep?
+6. **Fits the codebase as a natural extension**. Does the new code
+   use existing idioms, helpers, error types, and patterns, or does
+   it introduce new ones where existing ones would have served?
+
+### Group C — Cross-cutting concerns
+
+7. **New dependency surface**. Any new crates, system deps,
+   build-time requirements, or external-service dependencies?
+8. **New test coverage**. Are the new code paths covered, are the
+   tests scoped correctly (unit / integration / end-to-end), and
+   are there obvious test gaps? Don't reflag anything CI already
+   enforces (formatting, lint, type errors, unit-test pass/fail).
+9. **Documentation impact**. Does this need a CHANGELOG entry,
+   rustdoc updates, design-doc changes
+   ([docs/design/](docs/design/)), README adjustments, or operator
+   doc updates in [docs/](docs/)?
+10. **Security vulnerabilities**. Any new attack surface,
+    untrusted-input parsing, `unsafe` blocks, panic-on-untrusted
+    paths, secret-handling concerns, or side-channel exposure?
+11. **Rust and OSS best practices**. Idiomatic error handling, no
+    silently-swallowed errors, no `unwrap` / `expect` on untrusted
+    input, no `#[allow]` without justification, appropriate
+    visibility (`pub` vs `pub(crate)` vs private), naming, and
+    module shape.
+12. **Overlap with existing work**. Cross-check open issues and
+    other open PRs (and recently closed/merged ones) for related
+    work that overlaps, duplicates, partially addresses, or is
+    unblocked by this PR.
+13. **Other concerns**. Anything not captured above — wire-format
+    implications, branch-flow questions (`maint` vs `master` vs
+    `next`; see [docs/branching.md](docs/branching.md)),
+    deployment / packaging impact, contributor coordination needs,
+    fragility notes for future maintainers.
+
+## Step 4 — Compose the review
+
+The review report is **not** a Q&A walk through the 13 criteria.
+Write it as natural prose in a coherent, integrated narrative that
+reads start-to-finish. All 13 criteria must be addressed at some
+point in the body, but ordering, grouping, and emphasis follow the
+actual shape of THIS PR — lead with what matters most for this PR,
+not a fixed template.
+
+A typical shape that often falls out naturally:
+
+- **Opening paragraph**: what the PR does and the headline
+  observations (subsumes criteria 1 and 4).
+- **Substantive body**: diff analysis, design fit, cross-cutting
+  concerns, surprises, fragilities, missing coverage,
+  cross-PR/issue overlap, anything unusual. Don't reference
+  criterion numbers in the prose.
+- **Closing**: short summary and a proposed disposition — *land*,
+  *land-with-followups* (list them), *request-changes* (with the
+  blocking items called out), or *hold-for-thematic-batch*.
+
+Short subheadings are fine where they aid scanning. Bullets are fine
+for enumerable items (test names, file paths, follow-up actions).
+Avoid bullets that just enumerate criterion responses.
+
+## Step 5 — Filter aggressively
+
+Quality over quantity. Do not flag:
+
+- Pre-existing issues on lines the PR did not modify
+- Issues that linter, type-checker, formatter, or CI would catch
+- Pedantic style nitpicks a senior engineer would not call out
+- Likely intentional changes related to the broader goal
+- Things explicitly silenced by an `#[allow]` with justification
+- Stylistic preferences not anchored in `CLAUDE.md` or the
+  surrounding codebase's idioms
+
+When in doubt about whether something is worth surfacing: would a
+senior maintainer skim past it, or would they want it raised?
+Skim-past items don't belong in the report.
+
+For every issue you *do* surface, include a concrete fix suggestion
+inline ("rename X to Y", "extract this into the existing helper at
+`foo.rs:42`", "add a test exercising the `Err` branch") so the
+author can act without a round-trip.
+
+## Step 6 — Citation discipline
+
+When the review references a specific code location, use full-SHA
+GitHub permalinks so the link survives future history rewrites:
+
+```text
+https://github.com/jmcorgan/fips/blob/<full-40-char-sha>/<path>#L<start>-L<end>
+```
+
+For multi-line ranges include at least one line of context before
+and after the line(s) being discussed. After `gh pr checkout <num>`,
+use `git rev-parse HEAD` to grab the full SHA — never partial SHAs
+in permalinks.
+
+## Notes
+
+- The review is one human's read of the PR. Confidence calibration
+  matters: distinguish "this is a blocker" from "this is worth asking
+  about" from "this is a fragility note for future maintainers." The
+  closing disposition makes the action explicit.
+- If a re-review is triggered after the author pushes new commits,
+  lead with the delta from the prior review rather than re-walking
+  the whole PR.
+- This checklist exists to surface problems, not to assign blame.
+  If you're running it as the author or via an agent, treat each
+  finding as "would the maintainer ask about this?" — and either fix
+  it before opening, or pre-empt it in the PR body so the maintainer
+  doesn't have to ask.

src/discovery/nostr/runtime.rs

@@ -57,6 +57,67 @@ fn endpoint_summary(endpoints: &[OverlayEndpointAdvert]) -> String {
         .join(",")
 }
 
+fn is_unroutable_direct_advert_ip(ip: std::net::IpAddr) -> bool {
+    match ip {
+        std::net::IpAddr::V4(v4) => {
+            v4.is_private()
+                || v4.is_loopback()
+                || v4.is_link_local()
+                || v4.is_unspecified()
+                || v4.is_multicast()
+                || v4.is_broadcast()
+                || v4.is_documentation()
+                || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xc0) == 64)
+        }
+        std::net::IpAddr::V6(v6) => {
+            v6.is_loopback()
+                || v6.is_unspecified()
+                || v6.is_unique_local()
+                || v6.is_multicast()
+                || (v6.segments()[0] & 0xffc0) == 0xfe80
+        }
+    }
+}
+
+fn endpoint_advert_is_publicly_usable(endpoint: &OverlayEndpointAdvert) -> bool {
+    let addr = endpoint.addr.trim();
+    if addr.is_empty() {
+        return false;
+    }
+
+    if endpoint.transport == super::types::OverlayTransportKind::Udp
+        && addr.eq_ignore_ascii_case("nat")
+    {
+        return true;
+    }
+    if addr.eq_ignore_ascii_case("nat") {
+        return false;
+    }
+
+    match endpoint.transport {
+        super::types::OverlayTransportKind::Udp | super::types::OverlayTransportKind::Tcp => {
+            let Ok(socket_addr) = addr.parse::<SocketAddr>() else {
+                let Some((host, port)) = addr.rsplit_once(':') else {
+                    return false;
+                };
+                let host = host.trim().trim_start_matches('[').trim_end_matches(']');
+                if host.is_empty() || port.trim().parse::<u16>().ok().is_none_or(|p| p == 0) {
+                    return false;
+                }
+                if host.eq_ignore_ascii_case("localhost") {
+                    return false;
+                }
+                return host
+                    .parse::<std::net::IpAddr>()
+                    .ok()
+                    .is_none_or(|ip| !is_unroutable_direct_advert_ip(ip));
+            };
+            socket_addr.port() != 0 && !is_unroutable_direct_advert_ip(socket_addr.ip())
+        }
+        super::types::OverlayTransportKind::Tor => true,
+    }
+}
+
 /// Cached STUN-derived public address for an advert-eligible UDP transport
 /// bound to a wildcard. Lives on `NostrDiscovery` so the freshness window
 /// survives advert refresh cycles.
@@ -851,6 +912,7 @@ impl NostrDiscovery {
 
         advert.identifier = ADVERT_IDENTIFIER.to_string();
         advert.version = ADVERT_VERSION;
+        advert.endpoints.retain(endpoint_advert_is_publicly_usable);
         // Defensive: build_overlay_advert returns None on empty endpoints,
         // so this is only reachable from non-lifecycle callers.
         if advert.endpoints.is_empty() {
@@ -1393,12 +1455,11 @@ impl NostrDiscovery {
                 "missing required endpoints".to_string(),
             ));
         }
-        for endpoint in &advert.endpoints {
-            if endpoint.addr.trim().is_empty() {
-                return Err(BootstrapError::InvalidAdvert(
-                    "endpoint addr cannot be empty".to_string(),
-                ));
-            }
+        advert.endpoints.retain(endpoint_advert_is_publicly_usable);
+        if advert.endpoints.is_empty() {
+            return Err(BootstrapError::InvalidAdvert(
+                "missing publicly routable endpoints".to_string(),
+            ));
         }
 
         let has_nat = advert.has_udp_nat_endpoint();

src/discovery/nostr/tests.rs

@@ -39,7 +39,7 @@ fn can_reach(local_nat: NatType, remote_nat: NatType) -> bool {
 
 fn signed_overlay_advert_event(created_at_secs: u64, expiration_secs: Option<u64>) -> nostr::Event {
     let keys = nostr::Keys::generate();
-    let content = r#"{"identifier":"fips-overlay-v1","version":1,"endpoints":[{"transport":"tcp","addr":"203.0.113.10:443"}]}"#;
+    let content = r#"{"identifier":"fips-overlay-v1","version":1,"endpoints":[{"transport":"tcp","addr":"8.8.8.8:443"}]}"#;
     let mut builder = EventBuilder::new(Kind::Custom(ADVERT_KIND), content)
         .custom_created_at(Timestamp::from(created_at_secs));
     if let Some(expiration_secs) = expiration_secs {
@@ -118,6 +118,57 @@ fn rejects_invalid_overlay_adverts() {
     assert!(NostrDiscovery::validate_overlay_advert(wrong_identifier).is_err());
 }
 
+#[test]
+fn validate_overlay_advert_filters_unroutable_direct_endpoints() {
+    let advert = OverlayAdvert {
+        identifier: ADVERT_IDENTIFIER.to_string(),
+        version: ADVERT_VERSION,
+        endpoints: vec![
+            OverlayEndpointAdvert {
+                transport: OverlayTransportKind::Tcp,
+                addr: "192.168.1.10:443".to_string(),
+            },
+            OverlayEndpointAdvert {
+                transport: OverlayTransportKind::Udp,
+                addr: "100.64.1.2:2121".to_string(),
+            },
+            OverlayEndpointAdvert {
+                transport: OverlayTransportKind::Tcp,
+                addr: "8.8.8.8:443".to_string(),
+            },
+        ],
+        signal_relays: None,
+        stun_servers: None,
+    };
+
+    let validated = NostrDiscovery::validate_overlay_advert(advert).unwrap();
+    assert_eq!(validated.endpoints.len(), 1);
+    assert_eq!(validated.endpoints[0].addr, "8.8.8.8:443");
+}
+
+#[test]
+fn validate_overlay_advert_rejects_only_unroutable_direct_endpoints() {
+    let advert = OverlayAdvert {
+        identifier: ADVERT_IDENTIFIER.to_string(),
+        version: ADVERT_VERSION,
+        endpoints: vec![
+            OverlayEndpointAdvert {
+                transport: OverlayTransportKind::Tcp,
+                addr: "127.0.0.1:443".to_string(),
+            },
+            OverlayEndpointAdvert {
+                transport: OverlayTransportKind::Udp,
+                addr: "10.0.0.2:2121".to_string(),
+            },
+        ],
+        signal_relays: None,
+        stun_servers: None,
+    };
+
+    let err = NostrDiscovery::validate_overlay_advert(advert).unwrap_err();
+    assert!(err.to_string().contains("missing publicly routable"));
+}
+
 #[test]
 fn advert_freshness_rejects_expired_events() {
     let now_secs = Timestamp::now().as_secs();