amadan
docs: Astra's round-4 review of the nearest-region plan, unedited
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+86
−0
docs/superpowers/plans/CODEX-PLAN-NEAREST-REGION-R4.md+86 −0
diff --git a/docs/superpowers/plans/CODEX-PLAN-NEAREST-REGION-R4.md b/docs/superpowers/plans/CODEX-PLAN-NEAREST-REGION-R4.md| new file mode 100644 |
| index 000000000..f34d134e5 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-PLAN-NEAREST-REGION-R4.md |
| @@ -0,0 +1,86 @@ |
| +Reviewed plan `5600586a` against spec `9ccfaca6`, amendment `8baddbaa`, the three earlier reviews, and this worktree. R1–R9 remain settled. |
| + |
| +| Earlier finding still open after round 3 | Status | Why | |
| +|---|---|---| |
| +| R1-13 — copy-review completeness | Resolved | O19 is inventoried; new C30c/O20 are included, with review required before product-file writes. | |
| +| R1-17 — release/build/scratch gates | **Partly; decline unjustified** | Release and cross-build gates are complete. The supplied instructions contain no `/tmp/g` exception; socket-path limits do not authorize one. The tzdata and probe scripts also retain `/tmp` fallbacks. | |
| +| R2-4 — canary mismatch checked after writing | Resolved | The exact record is now checked before `instancesEnable`. | |
| +| R3-1 — capped/partial listing bypasses refusal | Resolved | Direct record GET replaces listing-based inference; capped-list and failed-read cases assert zero writes. | |
| +| R3-2 — pooled availability ignores allow-list | Resolved | Availability now applies `pol.allows("")` to the effective home. | |
| +| R3-3 — fixture provenance breaks approved-copy equality | Resolved | The immutable SHA is resolved before review; approved F1 is copied without substitution. | |
| +| R3-4 — missing same-box eligibility proof | Resolved | Split-capability fixtures and real reconciler pairing cover the required distinction. | |
| +| R3-5 — missing persisted-record compatibility proof | Resolved | Home/non-home stored-byte goldens and the steering-stamp mutation cover it. | |
| +| R3-6 — replicas lose their place name | Resolved | Every non-fleet response receives `region_name`, with endpoint and CLI coverage. | |
| + |
| +For R1-17, replace the exception at [plan:107](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:107) with an allowed on-disk directory and shorten test socket paths where necessary. Remove the `/tmp` fallbacks too. A passing test under `/tmp/g` establishes functionality, not authorization. |
| + |
| +**New findings** |
| + |
| +1. **P1 — Tasks 15 and 22: pooled/fleet branches violate R9a when the stored-region read fails.** |
| + |
| + **Plan:** [5324](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:5324), [6944](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:6944). |
| + **Code:** proposed `internal/console/regiondefault.go:56` and `:62` within Task 15’s file block; integration at [apiregions.go:31](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiregions.go:31) and [canary.go:381](/home/paulca/amadan.net/carlos/platform-nearest-region/canary.go:381). |
| + |
| + With `rerr != nil`, a known fleet returns `region_known:false` but `effective:"fleet/acme"`. A known pooled manifest returns a home-valued `effective` and nonnil `available`. [R9a explicitly requires both fields to remain unknown](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/specs/2026-10-01-nearest-region-default-design.md:791): empty `effective`, omitted `available`. |
| + |
| + `TestAppDefaultKnownFlags` exercises the failed read only without either overriding placement, so it misses both cases. |
| + |
| + **Fix:** condition `Effective` and `Available` on a successful region read. Preserve canary’s refusal independently: simply clearing `Effective` would defeat Task 22’s fleet-prefix check. Use independently confirmed placement information, or conservatively refuse that combined state. Add failed-region-read × pooled/fleet endpoint cases and a fleet-canary zero-write test. |
| + |
| +2. **P2 — Tasks 12 and 14: refusal messages recommend choices the corresponding door rejects.** |
| + |
| + **Plan:** [4041](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4041), [4044](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4044), [4859](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4859). |
| + **Code:** proposed `internal/console/regionclaim.go:35` and `:38`; existing rejection predicates at [apiinstances.go:799](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:799) and [apiinstances.go:827](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:827). |
| + |
| + There are three concrete cases: |
| + |
| + - C18 lists every published code, including non-hosting `nz`. The Task 12 test explicitly expects that suggestion, then verifies that choosing `nz` produces C19. |
| + - C19 filters hosting capability but still suggests regions that `NoKeyProven` makes fail with C31. |
| + - C16 lists eligible regions without the manifest allow-list. An unavailable UK default with `Regions:["uk"]` can recommend home `ie`, which the next request rejects. |
| + |
| + This misses [§8.1’s requirement that refusals list what would pass](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/specs/2026-10-01-nearest-region-default-design.md:709), and makes C16’s recovery advice unreliable. |
| + |
| + **Fix:** construct alternatives using the relevant door’s predicates, including normalized allow-lists for instance requests and the explicit R9 refusal for app requests. Keep explicit-choice rules distinct from automatic eligibility. Add tests that submit every advertised alternative under the same fixture and verify acceptance. |
| + |
| +3. **P2 — Task 13: the unchanged-form contract is neither preserved nor tested.** |
| + |
| + **Plan:** [4265](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4265), [4505](/home/paulca/amadan.net/carlos/platform-nearest-region/docs/superpowers/plans/2026-10-02-nearest-region-default.md:4505). |
| + **Code:** [app_new.html:14](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/templates/app_new.html:14), [server.go:3586](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/server.go:3586). |
| + |
| + `TestNewAppPageWithoutPickerIsUnchanged` checks only that `name="region"` is absent, and only with the switch off. It would pass if existing form content changed or disappeared. |
| + |
| + The proposed untrimmed `{{with .RegionPicker}}` block also leaves additional whitespace when the picker is nil. That fails the explicit byte-identical requirement in §§10 and 16, even though the rendered appearance remains unchanged. |
| + |
| + **Fix:** preserve the existing no-picker output exactly and compare the rendered form against a baseline golden with deterministic CSRF data. Cover both switch-off and switch-on/one-eligible-region cases. The proposed insertion should fail that comparison before correction. |
| + |
| +**Other requested checks** |
| + |
| +The additive `instance_domain` field is sound. It supplies the authoritative hostname suffix needed for a direct preflight read, without changing legacy `regions` or `default`. Checking the subsequent enable response against the preflight hostname is appropriate. |
| + |
| +I found no further mismatch in the interface blocks, migration carry through `migrateAppsKey`, create-door precedence, or old-CLI/old-console handling. Task 1 correctly reuses reconciliation parsing: `CARLOS_POOLS=",tito"` does **not** advertise shared-pool eligibility. All specified C identifiers, five Review Focus cases, and rollout stages have task coverage; the behavioral gaps above remain despite that coverage. |
| + |
| +The dependency choices have no identified blocker: [MaxMind v1.13.1 declares Go 1.21](https://github.com/oschwald/maxminddb-golang/blob/v1.13.1/go.mod), compatible with this repository’s Go 1.26 declaration, and [IANA publishes tzdata 2026b and its signature](https://data.iana.org/time-zones/releases/). The plan includes signature verification and all five required cross-build targets with failure propagation. |
| + |
| +This was static, read-only review. I did not run the proposed implementation’s tests or cross-builds; its implementation files do not yet exist. The worktree remains clean. |
| + |
| +--- |
| + |
| +**FIRST PROMPT:** |
| +Independent round-4 review of the nearest-region implementation plan. |
| + |
| +**WHAT:** |
| +Check prior dispositions, spec coverage, interfaces, compatibility and execution gates. |
| + |
| +**STATUS:** |
| +Review complete: three new findings; R1-17 remains partly unresolved. |
| + |
| +**NEEDED:** |
| +No additional user input. |
| + |
| +**RECOMMENDED:** |
| +Correct the findings and scratch instructions before implementation. |
| + |
| +**SESSION:** |
| +Safe to close; no files changed. |
| + |
| +**Verdict: not ready to execute. R9a is violated, refusal alternatives and form-preservation tests need correction, and the declined scratch-rule finding remains unresolved.** |
| \ No newline at end of file |