| 1 | Round 2 review of `50009443`. |
| 2 | |
| 3 | | Round 1 finding | Disposition | Evidence from revision 2 | |
| 4 | |---|---|---| |
| 5 | | 1. Connection-nominated headers | Resolved | §8.3 restores headers in every `Rewrite` from the result used for cache matching. | |
| 6 | | 2. Blob deletion races | Resolved | §3 serializes publication, off and cleanup; repeated publication is idempotent and the sweep preserves the selected blob. | |
| 7 | | 3. Backhauled door location | Resolved | §8.2 makes verified downstream edges adopt the ingress result, including its absence. | |
| 8 | | 4. GeoLite stale-data obligations | Partly resolved | D1 adds expiry, but §11.3 only closes the override, and §3 does not address retained S3 versions. Ordinary deletion does not erase those versions. [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/DeletingObjectVersions.html) | |
| 9 | | 5. Database/digest atomicity | Resolved | §§5.2–5.3 replace the separate digest with immutable generations and an atomically selected `current`. | |
| 10 | | 6. Console off transition | Resolved | §11.3 explicitly specifies `Swap(nil)` and startup without a file. | |
| 11 | | 7. Resource bounds | Partly resolved | §§4.1 and 10.3 bound publication and feed operations; §5.3 still gives mirror downloads no deadline, including when expiry depends on that loop advancing. | |
| 12 | | 8. Streaming interfaces | Resolved | §4.1 reuses `OpenObject` and specifies `PutStreamer` and wrapper forwarding. | |
| 13 | | 9. `Locate` compatibility | Resolved | §6 preserves its separate success and fallback rules, with regression cases. | |
| 14 | | 10. Tunnel country analytics | Resolved | §8.2 reconstructs `geoResult` from adopted headers. | |
| 15 | | 11. Country aggregation consumers | Resolved | §9 specifies pageviews, merging, read aggregation, API output and rendering. | |
| 16 | | 12. Historical attribution | Resolved | §9 persists `GeoSources`; §§11.1 and 13 specify answering-reader attribution and downstream duties. | |
| 17 | | 13. Operator authority | Resolved | §4.2 explicitly selects the person-only `apiSystemRoller` gate. | |
| 18 | | 14. Validation comparisons | Partly resolved | §3 adds authoritative `node_count` and §10.3 checks configured types, but §4.1 applies shrinking checks unconditionally while §10.3 limits them to pointers from a feed with the same type. | |
| 19 | | 15. Alert delivery pipeline | Resolved | §10.4 specifies a box rule using host delivery and the console watcher. | |
| 20 | | 16. Rollout verification | Partly resolved | §5.5 distinguishes tunnel consumers, but its verdict covers only fresh heartbeats; missing, stale or unreadable serving boxes remain unaccounted for. | |
| 21 | | 17. Tunnel public listeners | Resolved | §8.4 acknowledges ordinary listeners and requires their requests to remain unmarked. | |
| 22 | | 18. DB-IP checksums | Resolved | §10.3 acknowledges the published checksums and explicitly declines HTML scraping. | |
| 23 | | 19. Cache variant budget | Resolved | §7 documents eviction and multiplied variation. | |
| 24 | | 20. Coordinate accuracy | Resolved | §7 distinguishes formatting precision from location accuracy. | |
| 25 | | 21. Schema probe | Resolved | §4.1 specifies concrete lookup probes instead of metadata inspection. | |
| 26 | | 22. Original request | Resolved | Appendix A supplies the requested quotation. | |
| 27 | |
| 28 | 1. **P1 — §8.2: failed hop verification still produces the sending box’s location.** During mesh-key propagation, a legitimate backhaul can fail verification. The specified unverified-hop fallback then looks up the sender’s `RemoteAddr`, producing authoritative-looking headers for the edge rather than the visitor. This also reintroduces box coordinates for a backhauled door request in that failure case. Evidence: [mesh.go:52](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/mesh.go:52) explicitly documents this rotation window; [homemap.go:930](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/homemap.go:930) removes failed-hop identity; [analytics.go:86](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/analytics.go:86) falls back to the peer address. **Fix:** preserve a rejected-hop classification and suppress GeoIP on it, while retaining ordinary request-serving behavior. Test mismatched rotation keys and rejected door backhaul. |
| 29 | |
| 30 | 2. **P1 — §8.4: the tunnel marker does not necessarily establish an authenticated hub.** The proposed wrapper marks every request served by `tunnelclient`, but its runtime dialer accepts plain HTTP. With that configuration, an on-path peer can impersonate the hub and supply arbitrary, syntactically valid geo headers. The registration token authenticates the box to its peer; it does not authenticate that peer to the box. Evidence: [tunnelclient.go:211](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/tunnelclient/tunnelclient.go:211) accepts HTTP, and [tunnel_client.go:49](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/host/tunnel_client.go:49) passes environment configuration through without enforcing HTTPS. **Fix:** require authenticated HTTPS in production tunnel wiring before granting geo trust. Keep plaintext test connections behind an explicit test seam, and test production refusal. |
| 31 | |
| 32 | 3. **P2 — §§8.1, 8.4: stripping precedes the operation that establishes hop trust.** §8.1 requires parsing trusted headers and stripping all geo headers at the top of `ServeHTTP`, before any branch. At that location, a backhaul request does not yet carry `hopVerifiedKey`. A literal implementation discards its geo result before authentication, then cannot adopt it later. Evidence: [edge.go:1343](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/edge.go:1343) begins the handler, while [edge.go:1402](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/edge.go:1402) calls `acceptHop` after reserved-host branches. **Fix:** specify the ordering explicitly: capture and strip inbound geo values immediately, authenticate the hop, then adopt or compute before either backhaul branch, analytics or caching. Captured values must remain untrusted until authentication succeeds. |
| 33 | |
| 34 | 4. **P2 — §§5.4, 6, 9, 11.1: lookup and source are separate snapshots.** `Swappable.Lookup` returns a `Result` without source metadata; callers must separately call `Source`. A swap between those calls can label DB-IP coordinates as GeoLite2, or attach an empty source after `off`. Individually locking both methods does not prevent this. Evidence: [mmdb.go:22](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/geo/mmdb.go:22) associates lookup with one reader; the proposed split interface is at [design.md:323](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:323). **Fix:** return the result, source and database-availability state together under one read lock. Use that snapshot for headers, analytics and endpoint attribution. Test a swap between lookup completion and response construction. |
| 35 | |
| 36 | 5. **P2 — §§7–9: downstream analytics cannot distinguish a lookup miss from GeoIP being off.** Both states produce no geo headers. After a hop or tunnel, reconstructing `geoResult` therefore cannot implement §9’s rule that a miss counts as `_unknown` while an unavailable database contributes nothing. Consulting the receiving edge’s database is also wrong: tunnel boxes have none, and mirror generations can differ. Evidence: [tunnelclient.go:265](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/tunnelclient/tunnelclient.go:265) creates a new HTTP request context, and [analytics.go:119](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/analytics.go:119) constructs the observation at the receiving edge. **Fix:** define an authenticated internal representation carrying ingress availability and source even on misses, stripped before app delivery; alternatively revise the counting rule explicitly. Test miss, disabled and door states across both transports. |
| 37 | |
| 38 | 6. **P2 — §3: conditional pointer deletion has no store operation.** The revised protocol requires deleting `current.json` with `ifMatch`, but `Store.Delete` accepts no condition. Implementers must either invent another interface or silently weaken the protocol; a read followed by ordinary deletion is not equivalent. Evidence: [store.go:54](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/release/store.go:54) defines unconditional deletion, and [store.go:797](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/release/store.go:797) implements it without a precondition. **Fix:** specify conditional deletion and forwarding through supported stores, including conflict behavior and tests, or explicitly remove this guarantee under the single-writer assumption. |
| 39 | |
| 40 | 7. **P2 — §10.4: “no pointer means no rule” suppresses feed-failure alerts.** A fresh deployment whose configured feed fails seven times never obtains a pointer, so the clearing rule disables the very alert intended to report that failure. D1 deleting an expired pointer can similarly turn a continuing outage into a cleared alert. Evidence: [alertwatch.go:143](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/alertwatch.go:143) only delivers published rule state; it does not independently evaluate feed failures. **Fix:** evaluate feed health independently of pointer existence. Distinguish explicit off, first-publication failure and expiry, and test each transition. |
| 41 | |
| 42 | 8. **P3 — §10.4: existing hysteresis does not provide a 30-minute clearing delay.** The spec attributes delayed clearing to the existing rule machinery, but that machinery clears immediately when a reading crosses the clearing threshold. Evidence: [alert.go:172](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/metrics/alert.go:172). **Fix:** explicitly add sustained-clear bookkeeping for `geoip`, with restart behavior and tests, or specify immediate clearing. |
| 43 | |
| 44 | 9. **P3 — §§4.2, 11.3: the console cannot observe off within one minute of the command.** The console watches the edge’s local `current`, and the edge polls the bucket every ten minutes with jitter. Its one-minute watcher adds to that delay. Evidence: [main.go:295](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/cmd/console/main.go:295) currently installs a separate console reader; the proposed dependency and polling intervals are at [design.md:292](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:292) and [design.md:678](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:678). **Fix:** describe the bound as one edge poll plus one console poll, including jitter, or specify a direct off notification. |
| 45 | |
| 46 | Verdict: revise. |
amadan