| 1 | Round 3 review of `5311a1ad`. |
| 2 | |
| 3 | | Previous finding | Disposition | Evidence from revision 3 | |
| 4 | |---|---|---| |
| 5 | | R2-1. Failed-hop location | Resolved | §8.2 explicitly makes rejected hops `off`, including rotation-window and door-backhaul tests. | |
| 6 | | R2-2. Unauthenticated tunnel | Resolved | §8.4 grants the tunnel mark only after certificate-verified TLS; plaintext tunnels receive no geo trust. | |
| 7 | | R2-3. Strip/authentication ordering | Resolved | §8.1 specifies capture → strip → authenticate → decide → expose before forwarding or caching. | |
| 8 | | R2-4. Separate lookup/source snapshots | Resolved | §5.4 returns result, source and availability together under one read lock. | |
| 9 | | R2-5. Miss indistinguishable from off | Resolved | §§8.1a and 9 carry authenticated `hit/miss/off` state and source across both transports. | |
| 10 | | R2-6. Unsupported conditional deletion | Resolved | §3 explicitly chooses unconditional deletion under the single-writer mutex. | |
| 11 | | R2-7. Missing-pointer suppression of feed alerts | Resolved | §10.4 evaluates `feed.json` independently of pointer existence; §3 preserves an expired-pointer state. | |
| 12 | | R2-8. Unsupported clearing delay | Resolved | §10.4 now specifies immediate clearing. | |
| 13 | | R2-9. Console off propagation | Resolved | §§4.2 and 11.3 describe one edge poll followed by one console poll. | |
| 14 | | R1-4, carried forward: destruction obligations | Partly resolved | §15 adds noncurrent-version expiration, but §3 leaves completed orphan uploads for a future sweep. After a crash before pointer publication, those blobs remain current versions and are outside that lifecycle action. [AWS lifecycle documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/intro-lifecycle-rules.html) | |
| 15 | | R1-7, carried forward: resource bounds | Resolved | §5.3 adds a mirror-pass deadline and moves expiry onto an independent ticker. | |
| 16 | | R1-14, carried forward: validation comparisons | Resolved | §4.1 explicitly applies the shrink check whenever database types match, irrespective of origin. | |
| 17 | | R1-16, carried forward: rollout verification | Partly resolved | §5.5 covers stale and unreadable listed records, but still substitutes a seven-day root-prefix listing for expected inventory. Fleet heartbeats use `fleets/<fleet>/status/boxes/`, so they remain omitted: [agent.go:3416](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/host/agent.go:3416). | |
| 18 | |
| 19 | 1. **P1 — §5.3: installation can reactivate an expired database, and expiry can evict its fresh replacement.** The new-generation path checks size and digest, then installs without checking expiry at commit time. A download that crosses its expiry can therefore become active. If the console cannot mark the bucket pointer expired, subsequent polls can reload the same expired generation after the ticker removes it. Separately, no coordination is specified between checking the loaded generation’s age and calling `Swap(nil)`: the ticker can inspect expired A, the mirror install fresh B, and the ticker then remove B. |
| 20 | |
| 21 | **Evidence:** the independent ticker and unconditional installation/removal sequences are at [design.md:326](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:326). The underlying reader performs no age check during lookup: [mmdb.go:71](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/geo/mmdb.go:71). |
| 22 | |
| 23 | **Fix:** check candidate expiry immediately before installation. Serialize generation selection, expiry and local cleanup, with expiry conditional on the inspected generation still being current. Keep network downloads outside that lock. Test a download crossing expiry, repeated polls against an unmarked expired pointer, and replacement between expiry inspection and removal. |
| 24 | |
| 25 | 2. **P2 — §§5.4, 6, 11.3: the shared console reader lacks a protected `Locate` path.** `Swappable` exposes only `Lookup` and `Swap`, while the console’s existing region-guess interface requires `Locate`. Consequently, the proposed wrapper cannot directly replace `Config.GeoIP`. Retaining or exposing its underlying `*MMDB` would let region guesses bypass the lock that prevents lookup against a closed mapping. |
| 26 | |
| 27 | **Evidence:** [server.go:493](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/server.go:493) declares `GeoIP geo.Locator`; [regionguess.go:44](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/regionguess.go:44) calls `Locate`. The proposed wrapper’s method set is at [design.md:369](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:369). |
| 28 | |
| 29 | **Fix:** specify `Swappable.Locate`, holding the same read lock through the existing `MMDB.Locate` operation and returning a miss when empty. Wire that wrapper into `Config.GeoIP`; include concurrent region guesses in the swap/close test. |
| 30 | |
| 31 | 3. **P2 — §§6, 8.1a, 9: valid coordinate-only results disappear across hops and tunnels.** §6 permits `Lookup` to succeed with coordinates and no country. §8.1a nevertheless converts an adopted `hit` without a country into `off`. The same visitor therefore receives coordinates on a local route but loses them when forwarded. At ingress, §9’s instruction to count a hit’s country also leaves this valid result without a defined country bucket. |
| 32 | |
| 33 | **Evidence:** the conflicting rules are at [design.md:439](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:439) and [design.md:515](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:515). Coordinates already succeed independently of country in [mmdb.go:88](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/geo/mmdb.go:88). |
| 34 | |
| 35 | **Fix:** accept a trusted hit containing either a valid country or a valid coordinate pair. Define coordinate-only pageviews as `_unknown` for country aggregation while retaining their source. Test identical app headers and analytics through local, backhaul and tunnel routes. |
| 36 | |
| 37 | 4. **P2 — §5.5: healthy boxes periodically fail the readiness check.** The new five-minute freshness threshold is shorter than the normal heartbeat interval. Every healthy box can therefore become “stale” between scheduled publications, making readiness fluctuate during an otherwise completed rollout. |
| 38 | |
| 39 | **Evidence:** [agent.go:2922](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/host/agent.go:2922) schedules heartbeats at `SnapshotPeriod + rand.N(time.Minute)`; [fleet.go:119](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/status/fleet.go:119) defines that period as five minutes. Existing readers use the shared `BoxStale` rule at [fleet.go:137](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/status/fleet.go:137). |
| 40 | |
| 41 | **Fix:** use `status.BoxStale`, or explicitly choose a threshold exceeding the maximum scheduled interval with allowance for publication time. Test readiness just before the next normally jittered heartbeat. |
| 42 | |
| 43 | Verdict: revise. |
amadan