amadan
docs: Astra's round-3 findings on the nearest-region spec
Codex (GPT-6) review of rev 3 (8e59f598), copied unedited: all round-1 findings resolved; six new (3 P1, 2 P2, 1 P3). Verdict: not ready.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+82
−0
docs/superpowers/specs/CODEX-FINDINGS-NEAREST-REGION-R3.md+82 −0
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-NEAREST-REGION-R3.md b/docs/superpowers/specs/CODEX-FINDINGS-NEAREST-REGION-R3.md| new file mode 100644 |
| index 000000000..3793945fe |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-NEAREST-REGION-R3.md |
| @@ -0,0 +1,82 @@ |
| +Reviewed `8e59f598` against the clean worktree after reading AGENTS.md. R1–R7 remain settled. |
| + |
| +| Round-1 finding | Status | Why | |
| +|---|---|---| |
| +| 1. Picker bypasses pool exclusion | Resolved | Pooled manifests now produce no candidates or guess. | |
| +| 2. Browser adoption changes the default | Resolved | `@auto` remains distinct from an explicit choice; inference happens after adoption detection. | |
| +| 3. Unnamed home prevents override | Resolved | Writing non-home app defaults requires a named home, which provides an explicit override. | |
| +| 4. Mixed versions and rollback unspecified | Resolved | Read-first rollout, the switch, and the binary floor are specified; R7 accepts operational enforcement. | |
| +| 5. Existing-canary `--region` undefined | Resolved | A different region refuses before writes or upload; an equal region proceeds. | |
| +| 6. Manifest absence versus failure unspecified | Resolved | Absent, known, and unreadable policies have distinct behavior; legacy topology remains available. | |
| +| 7. Absent-region explanation disappears | Resolved | `app_default` represents absent regions independently; C8 no longer depends on alternative-region count. | |
| +| 8. Query hints reflected by redirects | Resolved | Hints travel in POST bodies, with redirect, error, logging, and audit assertions. | |
| + |
| +The following findings are new. |
| + |
| +1. **P1 — Arming per-host steering can break DNS for existing, unsteered siblings.** |
| + **Spec:** §13.2–§13.4, §13.6, §17 Phase L. |
| + |
| + The documented deployment uses one `*.<apps-domain>` record ([README.md:442](/home/paulca/amadan.net/carlos/platform-nearest-region/README.md:442)). The converger writes individual steered names and `edge.<apps-domain>`, without account-level fallback records ([steering.go:369](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/steering/steering.go:369)). |
| + |
| + Suppose `old.bap.apps.example` currently resolves through the root wildcard. Publishing a latency record for `new.bap.apps.example` creates the intermediate DNS node `bap.apps.example`. Queries for `old.bap.apps.example` then seek `*.bap.apps.example`, which does not exist; they cannot fall back to `*.apps.example`. Existing instances can therefore become unreachable when another instance is steered. This follows the closest-encloser rule in [RFC 4592 §3.3.1](https://www.rfc-editor.org/rfc/rfc4592.html#section-3.3.1). |
| + |
| + **Fix:** Specify how fallback DNS remains available beneath every ancestor introduced by steering. Include unstamped siblings, nested canaries, opt-out, and deletion in authoritative-DNS tests. Install and verify that coverage before Phase L writes per-host records. Checking only exact-name conflicts does not protect these names. |
| + |
| +2. **P1 — “One latency record per edge” is invalid when two edges share an AWS region.** |
| + **Spec:** §13.1, §13.3, §16, §17. |
| + |
| + `desiredState` emits one record per box ([steering.go:386](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/steering/steering.go:386)); `desiredRRSet` uses the box as `SetIdentifier` and its AWS region as `Region` ([route53.go:597](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/steering/route53.go:597)). Route53 allows only one latency record per AWS region within a name/type group. Distinct identifiers do not remove that restriction. [AWS latency-record requirements](https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/resource-record-sets-values-latency.html#resource-record-sets-values-latency-region) |
| + |
| + With two London boxes, the desired set contains two `eu-west-2` records for each hostname, including the synthetic steering hostname. Route53 rejects the affected change batch; retries reproduce it. The spec explicitly accommodates multiple placement codes in one AWS region but does not constrain steering topology. |
| + |
| + **Fix:** Define a valid regional aggregation strategy, such as one latency alias per region targeting a separately health-checked regional routing group. Alternatively, explicitly constrain and validate the supported topology before arming. Add this AWS restriction to the Route53 fake and test duplicate-region edges. |
| + |
| +3. **P1 — The prescribed pool-publication test would advertise a box that cannot provision shared instances.** |
| + **Spec:** §6.1, §16 “Edges file and eligibility.” |
| + |
| + §16 says `CARLOS_POOLS=,tito` includes the shared pool. The actual parser discards empty elements, producing only `["tito"]` ([agent.go:2033](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/host/agent.go:2033)). `poolServed(["tito"], "")` is false ([reconcile.go:1339](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/provision/reconcile.go:1339)). |
| + |
| + A publisher implemented to satisfy that test would report shared eligibility while reconciliation rejects the resulting ordinary instances. This is a new inconsistency in the proposed fix for R2-1. |
| + |
| + **Fix:** Derive publication from the same parsed configuration and predicate reconciliation uses. Under current semantics, `,tito` must publish false. Supporting shared-plus-named pools through that spelling requires an explicit parser change and corresponding tenancy tests, beyond adding publication fields. |
| + |
| +4. **P2 — Home fallback is not consistently constrained by the manifest allow-list.** |
| + **Spec:** §6.1, §6.3–§6.4, §16. |
| + |
| + Candidate filtering is correct, but §6.4 separately says that missing home coordinates produce home “whatever the inputs”; §6.1 also substitutes home after a listing failure. The null rule covers an excluded home only when “no nearest exists.” |
| + |
| + For a manifest allowing only `uk`, a mapped, eligible UK candidate, and an unmapped home, these instructions conflict: UK has a nearest point, yet the unconditional fallback says home. A literal implementation can return a suggestion the create door rejects. Home normalization produces `""` ([apiinstancemove.go:136](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstancemove.go:136)), and the allow-list compares that exact value ([apiinstances.go:1797](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:1797)). |
| + |
| + **Fix:** State one final invariant: every non-null guess, including every home fallback, must pass the normalized policy predicate. If a fallback selects an excluded home, return null. Test missing home coordinates, unmappable hints, and edges-read failure against a non-home-only allow-list. The door still prevents a residency bypass; the defect is an unusable suggestion. |
| + |
| +5. **P2 — Large DNS withdrawals are refused indefinitely, not spread over passes.** |
| + **Spec:** §13.4, §16. |
| + |
| + §13.4 says the shrink limit may spread a mass deletion over passes. In fact, exceeding the limit returns before **any** changes ([route53.go:207](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/steering/route53.go:207)). The existing loop documentation explicitly says this repeats forever without intervention ([steering_loop.go:154](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/host/steering_loop.go:154)). |
| + |
| + For example, removing three of four steered hosts with three edges withdraws nine of fifteen records, counting the synthetic hostname. The limit is seven. Every pass refuses; old DNS remains, and unrelated additions in that pass also stop. |
| + |
| + **Fix:** Replace the automatic-progress claim with a defined recovery procedure, or design an explicit authorized withdrawal mechanism. Preserve the protection against incomplete reads. Add repeated-pass tests and require verification of actual DNS removal after bulk deletion or opt-out. |
| + |
| +6. **P3 — §4 reverses which requests require positive eligibility.** |
| + **Spec:** §4 versus §7. |
| + |
| + §4 says a **“non-app-derived”** value additionally needs positive eligibility. §7 specifies the opposite: app-derived values require it; explicit requests retain legacy dormancy, implemented at [apiinstances.go:848](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/apiinstances.go:848). |
| + |
| + **Fix:** Change “non-app-derived” to “app-derived.” Otherwise a builder following the summary could tighten explicit requests while leaving automatic defaults insufficiently checked. |
| + |
| +The other requested checks support the design. The fill preserves fleet precedence and the pool, replica, static, and address-backed exclusions; sidecars retain their separate door. The migration correctly covers both the ALTER-before-rebuild order and the fixed rebuild columns ([store.go:402](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:402), [store.go:487](/home/paulca/amadan.net/carlos/platform-nearest-region/internal/console/store.go:487)). Old JSON clients remain compatible through permissive decoding and unmarshalling. |
| + |
| +Migrated apps retain `""`, so the new positive-eligibility refusal does not newly strand their creates. Non-home apps can require capacity or policy repair if every allowed region becomes unavailable; an explicit override cannot override the manifest. I found no additional IP or coordinate retention requirement in the revised interfaces. |
| + |
| +Your section references match an earlier revision: rev 3 has rollout in §17, no open questions in §18, and dispositions in §21. For the former rev-2 questions: |
| + |
| +| Former question | Recommendation under the settled rulings | |
| +|---|---| |
| +| Q1 — titogo completion | Apply R5: existing-app relief comes from moves. Keep titogo adoption separate from acceptance of the new-app default. | |
| +| Q2 — latency | Implement R6, including the DNS corrections above. Retain the separate, approved Phase L rollout and before/after measurements. | |
| +| Q3 — rollback floor | Apply R7’s runbook floor. Name the exact version before activation and preserve region-aware reads when disabling new assignments. | |
| + |
| +All code citations supporting these findings were checked in this worktree. No files changed, no tests ran, and production configuration was not inspected. |
| + |
| +**Verdict: not ready for planning.** The earlier findings are addressed, but wildcard DNS continuity, duplicate-region latency records, and the pool-publication mismatch need concrete corrections. None requires reopening R1–R7. |
| \ No newline at end of file |