| 1 | Reviewed `fcf15ae9` against spec `9ccfaca6` and the worktree, including the full `4604a146 → 9ccfaca6` spec diff first. R1–R9 remain settled. **Fourteen round-1 findings are resolved; four are partly resolved.** |
| 2 | |
| 3 | **Round-1 dispositions** |
| 4 | |
| 5 | | # | Finding | Status | Why | |
| 6 | |---|---|---|---| |
| 7 | | 1 | GeoIP errors disclose IP | Resolved | Fixed-category logging and an actual reader-error regression test. | |
| 8 | | 2 | Deploy continues after failed region read | Resolved | Transport/decode errors now stop; successful legacy responses retain compatibility. | |
| 9 | | 3 | Old-console fleet canary inconsistency | Resolved | Missing `app_default` now refuses the flag consistently before mutation. | |
| 10 | | 4 | Proxy defeats service gate | Resolved | Proxy/config suppression and connected-address verification are included. | |
| 11 | | 5 | `postJSON` map treated as bytes | Resolved | Tests now inspect the decoded map. | |
| 12 | | 6 | Replacement fixtures use create-only writes | Resolved | Helpers replace using the current ETag and check errors. | |
| 13 | | 7 | Missing account in `TestNonHomeAppRegions` | Resolved | The fixture now runs the account backfill. | |
| 14 | | 8 | Incorrect HTML assertions | Resolved | Positive assertions escape text; suppression uses an element ID. | |
| 15 | | 9 | Migration preservation unproved | Partly | Nonempty values and rename/transfer checks were added, but the new fixture fails before those checks complete; see new finding 6. | |
| 16 | | 10 | Vacuous guess property | Resolved | Expected existence is calculated independently and the real allow-list predicate is used. | |
| 17 | | 11 | Tests miss their claimed boundaries | Resolved | Reader invocation, service-principal HTTP wiring, failed claims and successful excluded creates are now exercised. | |
| 18 | | 12 | Missing integration coverage | Partly | Most cases were added; existing-canary refusal still lacks the required zero-write proof. | |
| 19 | | 13 | Copy-review ordering and completeness | Partly | Ordering is fixed, but new diagnostics such as [plan:3165](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:3165) remain outside the inventory; checking approved-string presence cannot detect unreviewed additions. | |
| 20 | | 14 | Unreadable default rendered as available | Resolved | C7 now requires confirmed availability; unreadable cases suppress the line. | |
| 21 | | 15 | Missing-coordinate diagnostic absent | Resolved | Implementation and once-per-code test added. | |
| 22 | | 16 | Moving fixture downloads | Resolved | One immutable revision is resolved before fetching all fixture material. | |
| 23 | | 17 | Incomplete release/cross-build gates | Partly | Targets and rollout prerequisites are supplied, but [plan:6849](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6849) still mandates `/tmp/g` and incorrectly calls it disk-backed, contrary to the supplied instructions. | |
| 24 | | 18 | C25 displays code instead of place | Resolved | Existing regions are named through `nameOf`, with a code fallback. | |
| 25 | |
| 26 | **New findings** |
| 27 | |
| 28 | New implementation files do not yet exist; their code locations below refer to the numbered plan blocks, alongside relevant worktree code. |
| 29 | |
| 30 | 1. **P1 — Task 10: incomplete edge reads can falsely prove a region keyless.** |
| 31 | |
| 32 | **Plan/code:** [plan:2954](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:2954), [plan:3077](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:3077); existing best-effort reader [instancesplacement.go:98](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/instancesplacement.go:98). |
| 33 | |
| 34 | `readEdgesFilesStrict` silently skips failed GETs and malformed files. `NoKeyProven` then quantifies over only the surviving files. If a readable UK box is keyless while a matching-key or old-writer UK box is unreadable, an explicit create receives C31 despite the mismatch being unknown. R9 requires proof about **every** reachable hosting box. |
| 35 | |
| 36 | **Fix:** preserve inventory completeness and suppress negative proof when relevant files cannot be assessed. Add failed-GET and malformed-file tests alongside a readable keyless box. Positive automatic eligibility and proof of universal key absence need different treatment. |
| 37 | |
| 38 | 2. **P1 — Task 10: ineligible boxes determine a candidate’s location.** |
| 39 | |
| 40 | **Plan/code:** [plan:3055](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:3055); topology fields [instancesplacement.go:35](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/instancesplacement.go:35). |
| 41 | |
| 42 | The AWS-region tally includes every box with an EIP, before eligibility is calculated. One eligible London box and two keyless or no-inbound Sydney boxes sharing placement `uk` therefore locate the eligible candidate in Sydney. This changes both its displayed place and nearest-region selection. Spec §6.1 explicitly takes coordinates from eligible boxes. |
| 43 | |
| 44 | **Fix:** tally non-home coordinates only from individually eligible boxes; retain the home’s exemption separately. Test mixed eligibility and eligible-box ties. |
| 45 | |
| 46 | 3. **P1 — Tasks 15, 21 and 22: HTTP 200 with `policy-unreadable` bypasses CLI protection.** |
| 47 | |
| 48 | **Plan/code:** [plan:4840](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4840), [plan:5893](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:5893), [plan:6260](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6260); [fleets.go:479](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/fleets.go:479), [apiinstances.go:798](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:798). |
| 49 | |
| 50 | `appDefault` returns a nonnil block marked `policy-unreadable`, but both preflights treat its fields as authoritative. A failed app-region read manufactures home-valued `region`, allowing `deploy --region ie` without knowing the stored region. A failed policy/placement read leaves a non-fleet `effective`, allowing canary creation even though fleet placement was never established. |
| 51 | |
| 52 | **Fix:** distinguish confirmed stored-region and fleet-placement facts from unavailable ones. Refuse the corresponding flag check when its required fact is unknown. Add command tests using **successful HTTP responses** with unreadable state, asserting zero writes/uploads. |
| 53 | |
| 54 | 4. **P1 — Task 22: existing-canary mismatch is checked after a write.** |
| 55 | |
| 56 | **Plan/code:** [plan:6323](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6323); [canary.go:381](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:381), [apimode.go:2827](/home/paulca/amadan.net/carlos/platform-nearest-region/apimode.go:2827). |
| 57 | |
| 58 | The first check has no records. The second runs after `instancesEnable`, which sends a PUT. An existing UK canary requested with `--region ie` therefore mutates before C25. This directly violates §16’s “before any write or upload.” The new command test covers only refusals from the first check. |
| 59 | |
| 60 | **Fix:** obtain the domain and existing records through reads, and complete the mismatch check before enabling instances. Add an existing-canary mismatch case asserting zero mutation requests and unchanged stored state. |
| 61 | |
| 62 | 5. **P1 — Tasks 12, 20 and 22: the prescribed edits leave three compile failures.** |
| 63 | |
| 64 | - **Task 12:** [plan:3522](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:3522), proposed `regionclaim_test.go`, imports `net` without using it. Remove the import. |
| 65 | - **Task 20:** [plan:5715](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:5715) changes `offerCreateApp`’s signature, but [deploy_wizard_test.go:104](/home/paulca/amadan.net/carlos/platform-nearest-region/deploy_wizard_test.go:104) retains the old arguments. The task’s `createApp(` search does not find `offerCreateApp`. Update that caller and include it in the file/commit list. |
| 66 | - **Task 22:** [plan:6308](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6308) inserts an `appRegions(ctx, …)` call directly after `resolvedApp`; `ctx` is declared on the following line at [canary.go:375](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:375). Move its declaration before the inserted block. |
| 67 | |
| 68 | 6. **P1 — Task 8: the new migration test cannot pass its rename step.** |
| 69 | |
| 70 | **Plan/code:** [plan:2174](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:2174), [plan:2205](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:2205); [store.go:1804](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:1804), [store.go:1720](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:1720). |
| 71 | |
| 72 | The legacy database contains an app but no source account. `Open` migrates without running the account backfill. `RenameAppIn` consequently returns `sql.ErrNoRows` when looking up the source sqid; transfer also requires that account. The region-preservation implementation cannot make this test green. |
| 73 | |
| 74 | **Fix:** seed the source account or explicitly run `BackfillOpsMemberships` before rename/transfer, while retaining the nonempty pre-rebuild region values. |
| 75 | |
| 76 | 7. **P2 — Task 14: R9’s addr-backed exemption has no regression test.** |
| 77 | |
| 78 | **Plan/code:** [plan:4442](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4442); shape predicate integration [apiinstances.go:828](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:828). |
| 79 | |
| 80 | The keyless-region test checks static exemption, socket/replica refusal and unknown-writer compatibility, but never creates an addr-backed record in the keyless region. The earlier addr-backed test uses an eligible region without an explicit region, so it cannot detect an incorrectly widened C31 gate. |
| 81 | |
| 82 | **Fix:** add an explicit `region:"uk"` addr-backed create against `ukNoKey`, require 201, and confirm a mutation that includes addr-backed records in the C31 gate fails this test. |
| 83 | |
| 84 | 8. **P2 — Task 9: the missing-key startup diagnostic is incorrectly switch-gated.** |
| 85 | |
| 86 | **Plan/code:** [plan:2549](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:2549); existing optional-key configuration [main.go:467](/home/paulca/amadan.net/carlos/platform-nearest-region/cmd/console/main.go:467). |
| 87 | |
| 88 | `logRegionConfig` returns immediately when app-region assignment is off. However, the guess endpoint remains operational with that switch off, and R9 requires the missing console key to be logged once. Non-home guesses silently disappear during the staged console rollout. |
| 89 | |
| 90 | **Fix:** gate only the unnamed-home diagnostic on `AppRegions`; independently report the missing secrets public key. Test the switch-off configuration. |
| 91 | |
| 92 | 9. **P2 — Task 11: the second mutation check does not prove its invariant.** |
| 93 | |
| 94 | **Plan/code:** [plan:3455](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:3455), proposed `regionguess.go`; policy predicate [apiinstances.go:1797](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:1797). |
| 95 | |
| 96 | The instructions first make the function always return null, then change the home condition without restoring the first mutation. The second run is already red and never reaches the mutated home branch. It proves nothing about disallowed-home fallback. |
| 97 | |
| 98 | **Fix:** restore and verify the passing baseline between mutations. Require each independent mutation to fail its intended assertion. |
| 99 | |
| 100 | 10. **P2 — Tasks 22 and 25: cross-build failure returns success and permits continuation.** |
| 101 | |
| 102 | **Plan/code:** [plan:6413](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6413), [plan:6846](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6846); authoritative gate [20-cross@5:25](/home/paulca/amadan.net/carlos/platform-nearest-region/.amadan/ci.d/20-cross@5:25). |
| 103 | |
| 104 | `go build … || { echo "FAILED $t"; break; }` exits the loop successfully. Task 22 then proceeds directly to staging and committing; Task 25 continues into native tests. I confirmed the loop returns status **0** when its build command fails. |
| 105 | |
| 106 | **Fix:** exit nonzero on failure, or accumulate failures and return a failing status before any subsequent commit. Apply this to every copied cross-build loop. |
| 107 | |
| 108 | The parser reuse in Task 1 correctly handles `CARLOS_POOLS=",tito"`. The migration SQL carries both new fields through `migrateAppsKey`; the fixture is the problem. The main create-door precedence and additive old-client wire compatibility remain consistent with the spec. All specified C identifiers are represented, subject to the outstanding copy-review completeness issue. |
| 109 | |
| 110 | No new dependency-selection finding: the pinned [MaxMind v1.13.1 module](https://github.com/oschwald/maxminddb-golang/blob/v1.13.1/go.mod) exists, and IANA lists the [2026b archive and signature](https://data.iana.org/time-zones/releases/). I did not execute the proposed Go tests, cross-builds or signature verification. |
| 111 | |
| 112 | --- |
| 113 | |
| 114 | **FIRST PROMPT:** |
| 115 | Independent read-only round-2 review of the nearest-region implementation plan against its amended spec and code. |
| 116 | |
| 117 | **WHAT:** |
| 118 | Check prior dispositions, implementation correctness, compatibility and required proof. |
| 119 | |
| 120 | **STATUS:** |
| 121 | Review complete; worktree unchanged. Four prior findings remain partly resolved, with ten new findings above. |
| 122 | |
| 123 | **NEEDED:** |
| 124 | Nothing for this review. |
| 125 | |
| 126 | **RECOMMENDED:** |
| 127 | Revise the identified tasks and re-review the affected implementation blocks and tests. |
| 128 | |
| 129 | **SESSION:** |
| 130 | Safe to close; no changes or running work. |
| 131 | |
| 132 | **Verdict: not ready to execute. The plan still contains incorrect placement/refusal behavior, a write-before-refusal path, compile blockers, a failing migration fixture, and incomplete validation gates.** |
amadan