amadan
geoip: code review brief
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+47
−0
docs/superpowers/plans/CODEX-REVIEW-BRIEF-GEOIP-CODE1.md+47 −0
CI cancelled — superseded by a newer push — run details
diff --git a/docs/superpowers/plans/CODEX-REVIEW-BRIEF-GEOIP-CODE1.md b/docs/superpowers/plans/CODEX-REVIEW-BRIEF-GEOIP-CODE1.md| new file mode 100644 |
| index 000000000..8f11927c1 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-REVIEW-BRIEF-GEOIP-CODE1.md |
| @@ -0,0 +1,47 @@ |
| +# Review brief: GeoIP plan 1, the built branch |
| + |
| +You are GPT-6 reviewing code independently of its author. Your output is a |
| +findings report, not an edit. Do not modify any file. |
| + |
| +## What to read |
| + |
| +- The diff: `git diff 9bed97b5..HEAD -- . ':!docs'` on branch |
| + `geoip-for-apps` (9bed97b5 is the base, origin/main when the branch was |
| + cut). Read every changed Go file in full where it matters, not just the |
| + hunks. |
| +- The spec it implements: `docs/superpowers/specs/2026-10-06-platform-geoip-design.md` |
| + (rev 9, nine review rounds). §0's R rulings and D decisions are fixed. |
| +- The plan and its review dispositions: |
| + `docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md` (the |
| + dispositions table at the end says where the code is authoritative over |
| + the plan text). |
| + |
| +## What matters most |
| + |
| +- **Security of the header contract** (spec §7, §8): any path where a |
| + client-chosen value reaches an app in any `X-Carlos-Geo-*` header or |
| + `X-Carlos-Hop-Geo`; any path where an edge locates something that is not |
| + the visitor (the box, the sender, the hub); trust granted to a transport |
| + that should not have it. Read `internal/edge/edge.go` ServeHTTP end to |
| + end for every branch that can reach a proxy, a static handler, a |
| + redirect, a cached replay or an activator wake, and check each sees the |
| + stripped and decided headers. |
| +- **Concurrency and crash safety:** `internal/edge/geoipmirror.go`, |
| + `internal/geo/swappable.go`, `internal/console/geoippublish.go` |
| + (slot ownership across the handler-to-job handoff, panics, every error |
| + path releasing the mutex exactly once), `internal/console/apisystemgeoip.go`. |
| +- **Resource bounds:** every read of bucket or upload data capped while |
| + copying; temp files removed on every path. |
| +- **Behaviour changes to existing code:** `geo.Locate`, `release.Store` |
| + implementations, analytics merge and read paths, the systemJob type, the |
| + tunnelclient Loop, status heartbeats. Anything that changes what an |
| + existing caller sees. |
| +- **Tests that would pass for the wrong reason.** |
| +- **Cross-compilation** (darwin, windows) and anything Linux-only. |
| + |
| +## Output |
| + |
| +Numbered findings, most severe first: severity (P1 = a bug that would ship |
| +or a security hole, P2 = likely wrong in some real case, P3 = clarity or |
| +maintainability), file:line, a concrete failure scenario, and a fix. End |
| +with a one-line verdict: merge-ready, or revise. |