| 1 | Reviewed `7fbc9479` against the clean worktree, after reading AGENTS.md. I found four P1 and four P2 issues. These concern implementation and rollout safety; they do not require reopening R1–R4. |
| 2 | |
| 3 | 1. **P1 — The pool exclusion is bypassed when a picker submits the guess.** |
| 4 | **Spec:** §6.1, §7, §9.3, §12. |
| 5 | |
| 6 | The door checks region capability without considering pools ([apiinstances.go:827](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:827)), then stamps `m.Pool` into the record ([apiinstances.go:883](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:883)). Reconciliation requires both the pool and region to match ([reconcile.go:335](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/provision/reconcile.go:335)). |
| 7 | |
| 8 | For an app whose pool exists only in Ireland, `/regions` can suggest an armed UK region. Titogo submits that guess explicitly, the door accepts it, and no box provisions the instance. Skipping the app-default fill does not protect this path. |
| 9 | |
| 10 | **Fix:** Return `guess: null` for pooled apps until candidates can prove pool eligibility. Add a test covering **guess → explicit create → reconciliation scope**, not just the default-fill exclusion. |
| 11 | |
| 12 | 2. **P1 — Browser adoption can silently change an existing app’s placement default.** |
| 13 | **Spec:** §5.5, §8.1, §10. |
| 14 | |
| 15 | Adoption is discovered inside `claimApp`, after checking existing bucket objects ([server.go:3502](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/server.go:3502), [server.go:3544](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/server.go:3544)). The proposed browser form always submits its preselected `region`, even when untouched. |
| 16 | |
| 17 | Consequently, an owner restoring an Ireland app while visiting London submits `region=uk` automatically. That satisfies §5.5’s “request names a region” exception, so subsequent instances move to UK by default despite the stated adoption safeguard. |
| 18 | |
| 19 | **Fix:** Preserve whether the person explicitly selected a region separately from the resolved guess. On adoption, ignore automatic guesses; permit an intentional override. Do not use the forgeable display label `guess_source` as that distinction. |
| 20 | |
| 21 | 3. **P1 — An unnamed home makes the promised override and recovery impossible.** |
| 22 | **Spec:** §7, §14, §17 Q14. |
| 23 | |
| 24 | `homeRegionIn` can translate a home choice only when `HomeRegion` is configured ([apiinstancemove.go:136](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstancemove.go:136)). The request currently decodes region into a string, without distinguishing omission from `""` ([apiinstances.go:636](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:636)). |
| 25 | |
| 26 | With an unnamed home and saved app region `uk`, both an omitted region and `region:""` trigger the proposed UK fill. If UK stops hosting and home is the only remaining choice, C16 says to pass another region—but no request can select home. The app’s default is immutable, so this blocks future creates. |
| 27 | |
| 28 | **Fix:** Require a named home before enabling non-home app defaults, or add an explicit-home selector that preserves legacy `""` semantics. A startup warning alone is insufficient. |
| 29 | |
| 30 | 4. **P1 — Rollout lacks a safe mixed-version and rollback rule.** |
| 31 | **Spec:** §14, §16. |
| 32 | |
| 33 | The current console derives placement solely from the request and fleet placement; it never reads an app region ([apiinstances.go:772](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:772), [apiinstances.go:798](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:798)). |
| 34 | |
| 35 | Once a new console saves `uk`, an omitted-region create served by an older console—or after rollback—writes a home-region record. With an unrestricted manifest, it succeeds outside the app’s promised region. Additive columns and tolerant JSON do not prevent this. |
| 36 | |
| 37 | **Fix:** Require every serving console to understand app defaults before enabling non-home writes. Specify that rollback must retain region-aware create behavior, or stop affected creates. Add a mixed-version/rollback test. Console-first versus CLI-first is otherwise compatible. |
| 38 | |
| 39 | 5. **P2 — `canary --region` has no defined behavior for an existing canary.** |
| 40 | **Spec:** §8.2, §11. |
| 41 | |
| 42 | `canaryRecordPlan` considers only host and channel ([canary.go:252](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:252)). Only the create branch calls `instanceCreate`; the other branches rechannel or reuse the existing instance ([canary.go:405](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:405)). Shipping follows afterward ([canary.go:457](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:457)). |
| 43 | |
| 44 | A builder following the listed change could accept `canary --region uk`, reuse an Ireland canary, and successfully deploy there. |
| 45 | |
| 46 | **Fix:** Define existing-canary behavior: equal region succeeds; a different explicit region refuses before mutation or upload. Moving existing instances remains outside this feature. |
| 47 | |
| 48 | 6. **P2 — Manifest absence and read failure are unspecified for the expanded regions endpoint.** |
| 49 | **Spec:** §6.1, §9.3, §11, §15. |
| 50 | |
| 51 | Today `/regions` needs no manifest ([apiregions.go:31](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiregions.go:31)). The natural existing helper, `instanceManifest`, returns 409 for absence and 502 for read failure ([apiinstances.go:117](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:117)). |
| 52 | |
| 53 | Reusing that helper would break region reads for apps that have not enabled instances, including the new deploy preflight. Treating every read failure as “no manifest” instead could return a guess excluded by the real allow-list. |
| 54 | |
| 55 | **Fix:** Specify both cases. Preserve the existing endpoint’s availability when the manifest is absent. When policy cannot be read, return no guess or an explicit error; never invent an unrestricted policy. Keep legacy topology fields separate from filtered guess candidates. |
| 56 | |
| 57 | 7. **P2 — The unavailable-region explanation disappears in the most relevant failure case.** |
| 58 | **Spec:** §6.1, §9.3, §10, §14. |
| 59 | |
| 60 | The edges reader drops missing, unreadable, and malformed files ([instancesplacement.go:98](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/instancesplacement.go:98)); region enumeration consequently contains only currently observed codes ([apiregions.go:40](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiregions.go:40)). |
| 61 | |
| 62 | If the saved UK region disappears entirely, the specified response has no UK entry on which to report `armed:false`. When only home remains armed, §10 also hides the explanatory Instances-tab line. Creates fail while the promised explanation vanishes. |
| 63 | |
| 64 | **Fix:** Expose saved-default availability separately, including absent regions. Show C8 whenever the applicable default cannot accept creates, regardless of the number of armed alternatives. |
| 65 | |
| 66 | 8. **P2 — Query-string hints violate the strict “never in a response body” privacy contract.** |
| 67 | **Spec:** §6.5, §9.3, §9.4. |
| 68 | |
| 69 | Canonical-host redirects preserve the complete query before routing or authentication ([server.go:1306](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/server.go:1306), [server.go:1326](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/server.go:1326)). A GET containing `ip` or `tz` can therefore be reflected in the redirect URL and its HTML response. |
| 70 | |
| 71 | **Fix:** Prefer a body-based inference endpoint while retaining the existing GET for topology, or explicitly redact these parameters from redirect/error paths. Add privacy assertions covering redirects, errors, logs, and audit writes—not only successful responses. I found no proposed need to persist coordinates. |
| 72 | |
| 73 | The central mechanisms otherwise check out. The §7 insertion preserves fleet precedence, replica refusal, and the pool/static/address-backed exclusions; sidecars use a separate door. Normalized guesses still meet the manifest predicate and armed checks, so they cannot bypass those controls. A stale guess may be refused at create time, which is appropriate. |
| 74 | |
| 75 | The migration correctly accounts for the ALTER-before-rebuild order ([store.go:402](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:402)) and the fixed rebuild column list ([store.go:487](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:487)). Rename and transfer update the same row. Existing migrated apps retain `""`, so the new unarmed-default refusal does **not** by itself strand them. Old CLI JSON compatibility is also supported by the permissive decoder and client unmarshalling; behavioral compatibility needs the safeguards above. |
| 76 | |
| 77 | My recommendations on §17: |
| 78 | |
| 79 | | Question | Recommendation | |
| 80 | |---|---| |
| 81 | | **Q1 — Ireland relief** | Make titogo adoption part of the operational completion criteria if relieving Ireland is the objective. Platform implementation alone does not change existing apps’ defaults or move their instances. | |
| 82 | | **Q2 — DNS and latency** | Verify flagship steering configuration and measure the complete request path before claiming a latency improvement. Do not silently enable non-static latency steering in this change. | |
| 83 | | **Q3 — Unarmed default** | Keep refusal. Provide a usable explicit override and visible availability status; silent relocation would undermine the placement promise. | |
| 84 | | **Q4 — Mutable app region** | Keep creation-only semantics for this release. A later owner-only change should affect future creates only and explicitly leave existing instances untouched. | |
| 85 | | **Q5 — Browser instance form** | Defer it. The CLI and API already exercise the shared create door; another form is not needed to implement these rulings. | |
| 86 | | **Q6 — GeoIP database** | Prefer operator-supplied DB-IP Lite with the required attribution on consuming pages. Correct “GeoLite2 forbids redistribution”: MaxMind’s terms permit some sharing subject to conditions and require attribution and updates. [DB-IP terms](https://db-ip.com/db/lite.php), [MaxMind EULA](https://www.maxmind.com/en/geolite/eula). | |
| 87 | | **Q7 — Dependency and fixtures** | Use the maintained reader, pinned to a reviewed version. Its license is ISC; MaxMind’s test repository offers MIT or Apache-2.0 licensing, so selected pinned fixtures avoid writing another database parser. [Reader license](https://github.com/oschwald/maxminddb-golang/blob/main/LICENSE), [fixture licensing](https://raw.githubusercontent.com/maxmind/MaxMind-DB/main/README.md). | |
| 88 | | **Q8 — Place names** | Keep the proposed picker style. Status labels may remain deliberately coarser; require geographic consistency, not identical wording. | |
| 89 | | **Q9 — Home coordinates** | Verify the flagship’s actual publication before rollout; this worktree cannot establish it. Use the existing `CARLOS_AWS_REGION` publisher override before adding another console override. | |
| 90 | | **Q10 — Sidecars** | Leave them unchanged. Following app defaults needs a separate design covering sidecar placement eligibility and its missing armed gate. | |
| 91 | | **Q11 — Pools** | Defer pool-aware candidates, but suppress pooled-app guesses now. Pool eligibility must eventually be proven by the same target box, not inferred from region-wide capability. | |
| 92 | | **Q12 — Home/replica overlap** | Recommend refusing newly created home records whose effective region appears in their replica set. Normalize home naming consistently and leave existing records untouched pending an audit. | |
| 93 | | **Q13 — Home allow-list spelling** | Preserve current normalization in this change; do not silently reinterpret existing manifests. Audit production manifests before rollout and document that named home currently maps to `""`. | |
| 94 | | **Q14 — Unnamed home** | A warning is insufficient. Require a named home before enabling non-home defaults, or implement an unambiguous explicit-home request. | |
| 95 | | **Q15 — Windows** | Accept the documented fallback for this release. Honor a valid `$TZ` when present; otherwise use IP, then home, with `--region` available. | |
| 96 | | **Q16 — AWS list** | The four questioned codes are present in AWS’s official list. Record the table’s source/version and separately verify the chosen metro coordinates. [AWS regions](https://docs.aws.amazon.com/global-infrastructure/latest/regions/aws-regions.html). | |
| 97 | |
| 98 | No files changed and no tests ran; this was a source-level design review. Production configuration was not inspected. |
| 99 | |
| 100 | **Verdict: not ready for planning.** Resolve the pooled-guess, adoption, explicit-home, and rollout gaps first, then make the remaining endpoint and CLI contracts precise enough that builders cannot implement conflicting behavior. |
amadan