| 1 | # Review brief: GeoIP plan 1, the built branch |
| 2 | |
| 3 | You are GPT-6 reviewing code independently of its author. Your output is a |
| 4 | findings report, not an edit. Do not modify any file. |
| 5 | |
| 6 | ## What to read |
| 7 | |
| 8 | - The diff: `git diff 9bed97b5..HEAD -- . ':!docs'` on branch |
| 9 | `geoip-for-apps` (9bed97b5 is the base, origin/main when the branch was |
| 10 | cut). Read every changed Go file in full where it matters, not just the |
| 11 | hunks. |
| 12 | - The spec it implements: `docs/superpowers/specs/2026-10-06-platform-geoip-design.md` |
| 13 | (rev 9, nine review rounds). §0's R rulings and D decisions are fixed. |
| 14 | - The plan and its review dispositions: |
| 15 | `docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md` (the |
| 16 | dispositions table at the end says where the code is authoritative over |
| 17 | the plan text). |
| 18 | |
| 19 | ## What matters most |
| 20 | |
| 21 | - **Security of the header contract** (spec §7, §8): any path where a |
| 22 | client-chosen value reaches an app in any `X-Carlos-Geo-*` header or |
| 23 | `X-Carlos-Hop-Geo`; any path where an edge locates something that is not |
| 24 | the visitor (the box, the sender, the hub); trust granted to a transport |
| 25 | that should not have it. Read `internal/edge/edge.go` ServeHTTP end to |
| 26 | end for every branch that can reach a proxy, a static handler, a |
| 27 | redirect, a cached replay or an activator wake, and check each sees the |
| 28 | stripped and decided headers. |
| 29 | - **Concurrency and crash safety:** `internal/edge/geoipmirror.go`, |
| 30 | `internal/geo/swappable.go`, `internal/console/geoippublish.go` |
| 31 | (slot ownership across the handler-to-job handoff, panics, every error |
| 32 | path releasing the mutex exactly once), `internal/console/apisystemgeoip.go`. |
| 33 | - **Resource bounds:** every read of bucket or upload data capped while |
| 34 | copying; temp files removed on every path. |
| 35 | - **Behaviour changes to existing code:** `geo.Locate`, `release.Store` |
| 36 | implementations, analytics merge and read paths, the systemJob type, the |
| 37 | tunnelclient Loop, status heartbeats. Anything that changes what an |
| 38 | existing caller sees. |
| 39 | - **Tests that would pass for the wrong reason.** |
| 40 | - **Cross-compilation** (darwin, windows) and anything Linux-only. |
| 41 | |
| 42 | ## Output |
| 43 | |
| 44 | Numbered findings, most severe first: severity (P1 = a bug that would ship |
| 45 | or a security hole, P2 = likely wrong in some real case, P3 = clarity or |
| 46 | maintainability), file:line, a concrete failure scenario, and a fix. End |
| 47 | with a one-line verdict: merge-ready, or revise. |
| 48 | |
amadan