carlos / platform Public

geoip plan 1: review dispositions; GeoLite2 measurement outstanding

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago ad23baa04444a03bfeb89251dc2bd09bfd44f7ef parent c1b3173

3 files changed, +85 −0

docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md +9 −0
docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md +31 −0
docs/superpowers/plans/CODEX-FINDINGS-GEOIP-PLAN1-R1.md +45 −0

diff --git a/docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md b/docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md

index f5a9e34c6..fdf13eb4f 100644

--- a/docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md

+++ b/docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md

@@ -49,3 +49,12 @@ resolve:

Types: `GeoIP2-City` (1,547 nodes), `GeoIP2-Country` (1,704 nodes), `Test`

(163 nodes, IPv4 only, no country data: the "refuse a database with no

country data" case). All three build_epoch 2026-02-04.

+## Outstanding

+- **GeoLite2 City has not been measured** (spec §17, Astra plan R1-22).

+ - Downloading it needs a MaxMind account and licence key, which this

+ session does not have.

+ - Before the first GeoLite2 publish (or plan 2's feed), measure its size,

+ `Verify` memory, subdivision ISO codes, and the five probe addresses.

+ Record them here.

diff --git a/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md b/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md

index f591289e5..4bd753e88 100644

--- a/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md

+++ b/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md

@@ -4177,3 +4177,34 @@ git commit -m "console+docs: countries on the analytics page with attribution; h

- **Plan 2:** the feed and the `geoip` box alert rule.

- **Plan 3:** the console's mirrored reader, `/geo`, and `carlos geo`.

- **Rolling, and applying Task 1's tofu:** Paul's go-ahead.

+---

+## Review dispositions (Astra plan round 1, `CODEX-FINDINGS-GEOIP-PLAN1-R1.md`)

+The plan was executed natively, and each finding was fixed in the code as it was built. Where the code and the plan text above differ, the code and its tests on the branch are authoritative.

+| # | Disposition |

+|---|---|

+| 1 | Fixed: `geoip.StripAll` removes the whole `X-Carlos-Geo-` namespace, case-insensitively (b00c58bd); the edge forgery matrix sends `X-Carlos-Geo-City` |

+| 2 | Fixed: the verdict uses `LoadBoxesComplete` on every prefix, reconciled against edges records and active fleet boxes, keyed by (namespace, label), with `GeneratedAt` |

+| 3 | Fixed: `.incoming-*` is cleaned only at boot; test `TestMirrorExpiryDuringADownloadKeepsTheDownload`, which was checked by mutation |

+| 4 | Fixed: a sync failure after the rename is logged, not returned; test `TestMirrorSyncFailureAfterRenameKeepsTheGeneration` |

+| 5 | Fixed: GeoIP is booted and its loop started before `ready(e)` and before the dev-mode return |

+| 6 | Fixed: a blob CAS conflict streams the existing object and checks sha+size; test `TestPublishRefusesAWrongObjectAtTheBlobKey` |

+| 7 | Fixed: `geoip.ErrUnknownVersion` means off; test `TestMirrorUnknownPointerVersionTurnsOff` |

+| 8 | Fixed: rejections reset whenever the observed pointer changes, including on removal; test `TestMirrorRetriesARejectedShaAfterThePointerMoves` |

+| 9 | Fixed: `DirStore.PutFrom` uses Stat for create-only and a streamed hash for replace-only |

+| 10 | Fixed: the helper is named `geoipCtxReader` |

+| 11 | Fixed: `n > limit` is refused |

+| 12 | Fixed: `res.Countries` |

+| 13 | Moot under native execution; every commit was built and tested from the working tree, and CI checks the pushed tip |

+| 14 | Fixed: the PUT answers 202, a refusal is the job's error, and a member gets the bare 404. Service credentials get the closed-door 403 (closedDoors) |

+| 15 | Fixed: `systemJob.Result`, returned by the jobs door, read by the CLI |

+| 16 | Moot: D2 (rev 7) made the tunnel mark transport-independent, so no TLS seam was needed |

+| 17 | Fixed: one consistent rule. The remote makes zero observations, the hub one per served response, with the hub's answer (`TestTunnelledPageviewsAreCountedOnceAtTheHub`, including a hub cache hit) |

+| 18 | Fixed: `TestForgeryMatrixAcrossLocalProxyTypes` (unix, tcp, multi, with Connection nomination and unknown suffixes); backhaul and tunnel delivery and miss tests; rejected hop; door |

+| 19 | Fixed: a valid missing-generation sha; injected sync failure; concurrent expiry during download; off and newer publish during download |

+| 20 | Fixed: the node count is re-inflated before the different-type case; foreign-pointer CAS and publish/off race tests added; the pointer is now read at Stage (a real bug the new test found) |

+| 21 | Fixed: an upgrade test with its own header-reporting backend |

+| 22 | **Outstanding:** GeoLite2 City was not measured (it needs a MaxMind account). See FACTS.md |

diff --git a/docs/superpowers/plans/CODEX-FINDINGS-GEOIP-PLAN1-R1.md b/docs/superpowers/plans/CODEX-FINDINGS-GEOIP-PLAN1-R1.md

new file mode 100644

index 000000000..94aa9140e

--- /dev/null

+++ b/docs/superpowers/plans/CODEX-FINDINGS-GEOIP-PLAN1-R1.md

@@ -0,0 +1,45 @@

+1. **P1 — Task 5, Step 3: `StripAll` leaves forged headers in the reserved namespace.** [Plan:1458](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:1458) deletes only the five known app headers and the hop header. A client-supplied `X-Carlos-Geo-City`, for example, survives both ingress and `setGeo`. Spec §8.1 requires stripping **every** `X-Carlos-Geo-*` header. Iterate over header keys using a case-insensitive prefix match, remove the hop header, and test unknown suffixes through the proxies.

+2. **P1 — Task 12, Step 3: the prescribed status loader can falsely report “ready.”** [Plan:3982](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3982) explicitly calls `systemBoxes`, contradicting the task’s prohibition at line 3879. `internal/console/apisystemupdate.go:233` and `:258` use `LoadBoxes`, which omits unreadable records; the proposed verdict function also receives no expected-box inventory. Replace this recipe with `LoadBoxesComplete` for every prefix, reconciled against platform edges records and active fleet boxes. Test missing and unreadable heartbeats. Use `Box.GeneratedAt`, not the nonexistent `Updated` named at line 3880.

+3. **P1 — Task 7, Step 3: expiry cleanup deletes an in-progress replacement download.** [Plan:2510](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2510) makes `cleanLocked` remove every `.incoming-*`, while `download` writes those files outside `genMu` at line 2609. If A expires while B downloads, `ExpireTick → clearLocked → cleanLocked` unlinks B. Its later rename fails, leaving GeoIP unavailable until another poll. Restrict stale-download cleanup to boot, or track active downloads and exclude them. Add a deterministic test that pauses B’s download while A expires.

+4. **P1 — Task 7, Step 3: a directory-sync failure leaves `current` naming a deleted generation.** [Plan:2495](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2495) renames `current` before returning the directory-sync result. On any returned error, `install` deletes the newly named generation at line 2667. Restart then encounters a dangling `current` and cleans away the previous generation too. Distinguish failures before and after the rename; never delete a generation that `current` may already name. Inject a post-rename sync failure and verify restart recovery.

+5. **P1 — Task 7, Step 3: initialization occurs after the edge has been published and after the dev-mode return.** [Plan:2792](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2792) inserts initialization around `edge.go:3088`, but `ready(e)` runs at `internal/edge/edge.go:3015`, and dev mode returns at `:3038`. Dev edges therefore never initialize or run the mirror. In production, Task 8’s heartbeat callback can read `geoMode` and `geoMirror` concurrently with their later assignments. Initialize and boot GeoIP before `ready`, and start its loop for both serving modes.

+6. **P1 — Task 6, Step 3: blob CAS conflicts are accepted without the required size check.** [Plan:1962](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:1962) treats every `ErrCASConflict` as proof that the exact blob exists. Spec §3 requires checking that the existing object’s size matches. A truncated object under that key would be published as current, followed by deletion of the previous good blob. Verify existence and length after a conflict; refuse mismatches without moving the pointer or sweeping. Cover both an existing wrong-size object and a conflict without a readable object.

+7. **P1 — Task 7, Step 3: unknown pointer versions retain the active database.** [Plan:2555](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2555) handles every parse error by retaining the current reader. Spec §3, lines 190–191, explicitly treats an unknown `v` as no pointer: GeoIP must turn off. Distinguish unsupported versions from ordinary read/parse failures, clear the generation, and log once. Test a loaded database followed by a version-2 pointer.

+8. **P1 — Task 7, Step 3: rejected-digest suppression survives pointer changes.** [Plan:2570](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2570) returns for the loaded SHA before clearing `skipSHA`; the missing-pointer branch also leaves it intact. After rejecting B, observing A again—or observing off—and then republishing repaired B, the mirror still refuses to download B. Spec §5.3 permits retry once the pointer changes. Track the observed pointer separately and reset suppression whenever that identity changes, including removal.

+9. **P1 — Task 4, Step 3: the streaming DirStore implementation reads the existing blob into memory.** [Plan:1005](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:1005) calls `os.ReadFile(p)` before checking create-only existence. Republishing an existing database therefore allocates its entire contents—potentially 1 GiB—despite introducing this interface to bound memory. Use `Stat` for create-only checks and streamed hashing when an existing ETag must be compared.

+10. **P1 — Task 6, Step 3: `ctxReader` collides with an existing test type.** [Plan:2009](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2009) declares `ctxReader` and its `Read` method in package `console`. `internal/console/apidb_test.go:642` already declares both, with a different shape and behavior. The console test package will not compile. Give the publisher helper a distinct name and update `copyCapped`.

+11. **P1 — Task 7, Steps 1 and 3: the supplied oversize test fails against the supplied implementation.** [Plan:2317](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2317) sets `maxBytes = p.Size - 1`. The implementation reads `limit + 1` bytes, obtaining exactly `p.Size`, then checks only `n != p.Size` at line 2630. The blob passes both size and digest checks. Reject `n > limit` as well, or reject a declared size above the configured cap before downloading.

+12. **P1 — Task 11, Step 3: the CLI insertion references an undefined variable.** [Plan:3827](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3827) prescribes `printAnalyticsTable(w, "Countries", resp.Countries)`. The renderer uses `res`, including the adjacent call at `analytics_cmd.go:149`. Change the insertion to `res.Countries`; the literal instruction otherwise breaks the root package build.

+13. **P1 — Tasks 9 and 12, Step 5: commit commands omit required implementation changes.** [Plan:3401](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3401) omits `backhaul_e2e_test.go`, although Task 9’s new test depends on fields added there. The Task 12 command at line 4068 omits `geoippublish.go`, although its handler depends on the new `Stage`/`Commit` split. Following these commands produces commits missing dependencies that local working-tree tests can conceal. Include both files and verify the committed result from a clean checkout.

+14. **P2 — Task 12, Step 1: the HTTP expectations contradict the asynchronous contract and existing authorization behavior.** [Plan:3899](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3899) requires successful PUTs to return 200 and invalid databases to return 422, while the same task requires staging followed by a 202 job. Its service-token expectation is also 404, whereas `internal/console/authcli.go:438` returns 403 for an empty role set. Update the tests to expect 202 and poll for validation results, and preserve the existing 403 service-credential gate.

+15. **P2 — Task 12, Step 3: the reused job API cannot return the promised pointer.** [Plan:3935](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3935) requires the existing job type, and lines 3939 and 4036 require polling to return the publication pointer. `systemJob` at `internal/console/apisystemupdate.go:48` stores only steps, completion, and error; its handler at `:587` exposes only those fields. Explicitly add an optional result to the job, locked snapshot, response schema, and CLI polling path. Preserve the result belonging to that job rather than rereading the potentially changed current pointer.

+16. **P2 — Task 10, Step 3: `Loop` drops the TLS configuration required by its new integration test.** [Plan:3536](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3536) introduces `DialConfig` and updates the handler used by `Loop`, but does not replace `Loop`’s existing `Dial(ctx, cfg.URL, cfg.Name, cfg.Token)` call at `internal/tunnelclient/tunnelclient.go:105`. That call cannot carry `cfg.TLSConfig`, so the test server’s certificate pool is lost. Explicitly change `Loop` to call `DialConfig(ctx, cfg)` and verify the TLS integration test with certificate verification enabled.

+17. **P2 — Tasks 10 and 11, Step 1: tunnel analytics assertions contradict each other and the ingress counting rule.** [Plan:3475](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3475) asks the remote meter to record GB; line 3682 requires zero remote observations, and line 3684 asks to complete the earlier assertion. Line 3682 also expects zero hub countries over plaintext, although the hub still computes its own ingress result. Replace these with one consistent table: zero remote observations for either transport, and hub country counts from the hub’s result. With the stated GB setup, both transports produce two GB pageviews at the hub.

+18. **P2 — Tasks 9 and 10, Step 1: required trust-boundary tests are unassigned.** [Plan:3094](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3094) tests forgery through the TCP helper, while Task 10’s scene covers a successful tunnel and plaintext fallback. Neither task assigns the full spec §8.4 matrix: every proxy type with connection nomination and cache replay, the tunnel box’s public listener, a backhauled door, rotation-window backhaul, and transported miss/off states. Assign these cases explicitly to the relevant task, with assertions on backend headers and lookup/counting behavior.

+19. **P2 — Task 7, Step 1: the recovery tests do not exercise the promised failure paths.** [Plan:2227](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:2227) uses `deadbeef` as a SHA, so the “missing file” test succeeds through invalid-SHA handling before opening a generation. The expiry test at line 2291 is sequential, and line 2272 tests pointer metadata rather than expired downloaded metadata. Add a valid missing-generation SHA, injected disk/crash failures, pointer-read and deleted-blob failures, and controlled concurrent expiry/install tests, as required by spec §§5.3 and 14.

+20. **P2 — Task 6, Step 1: the different-type shrink assertion passes without enforcing its intended property.** [Plan:1715](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:1715) force-publishes the Country fixture, resetting the inflated node count from 99,999 to 1,704. The subsequent City fixture has 1,547 nodes, so it passes even if the implementation incorrectly applies the shrink check across types. Reinstate the inflated count before the different-type assertion, or isolate that case. Also assign the spec-required publish/off race and foreign-pointer CAS tests; the sequential sweep/off test at line 1765 covers neither.

+21. **P2 — Task 9, Step 1: the WebSocket helper does not expose request headers.** [Plan:3284](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:3284) claims `upgradeEchoBackend` echoes received headers. At `internal/edge/edge_test.go:2012`, it actually reads a client-supplied line after upgrading and echoes that line with a tag. The named bypass-recording test also does not supply the claimed real-client handshake. Add a backend that records the actual upgrade request headers, and use the real connection pattern from `TestMultiUpstreamServesWebSocketUpgrade` at `:2030`.

+22. **P2 — Task 1 and the final gate: the required real GeoLite2 measurement has no assigned step.** [Plan:116](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md:116) makes Task 1 solely the bucket lifecycle change; the final gate does not restore the measurement requirement. Spec §17 explicitly assigns real DB-IP and GeoLite2 City probe and `Verify` memory measurements to Plan 1. `2026-10-06-platform-geoip-FACTS.md:30` confirms GeoLite2 was not measured. Add an explicit measurement step and record its results, or mark that requirement outstanding pending access to a real GeoLite2 City file.

+Verdict: revise.

\ No newline at end of file