amadan
#162: plan revision 2 — the review's corrections, per task, and custom domains
Summary
each task now opens with the corrections the plan review paid for, so a subagent reading only its own task cannot paste a brief that does not compile. Task 4 and 5 gain the custom-domain half; Task 7's six defects are spelled out; Task 9 loses the pathsFor call it could not make.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 file changed,
+56
−1
docs/superpowers/plans/2026-09-21-replica-placements.md+56 −1
CI passed — run details
diff --git a/docs/superpowers/plans/2026-09-21-replica-placements.md b/docs/superpowers/plans/2026-09-21-replica-placements.md| index 542ae5aff..ba50780f7 100644 |
| --- a/docs/superpowers/plans/2026-09-21-replica-placements.md |
| +++ b/docs/superpowers/plans/2026-09-21-replica-placements.md |
| @@ -407,6 +407,13 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 3: Reconcile mints a replica route — `replica_of`, the home's channel, the protect rule |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **`registry.Get` returns `(nil, nil)` on a miss**, so two assertions below are wrong as written. Replace `if _, err := ie.Get(replica.Host); err == nil { t.Error(...) }` with `rt, err := ie.Get(replica.Host); if err != nil { t.Fatal(err) } else if rt != nil { t.Error("the home's box minted a route for the replica") }`. |
| +> 2. **`TestReconcileProtectsReplicaWhoseHomeIsGone` as written proves nothing**: the record still exists, so the route survives regardless. What Step 5 actually prevents is minting or converging from a zero `home` record. Assert instead: after the home record is deleted, `mustGet(t, uk, replica.Host).Channel` is still `"stable"` (no converge to an empty channel), AND on a *fresh* `regWith(t)` registry with the home gone, `uk.Get(replica.Host)` returns `(nil, nil)` — nothing minted on a guess. |
| +> 3. **Where `all[rec.Host] = rec` goes**: after the empty-channel guard and before the scope check, so a malformed or channel-less home is absent from `all` and its replica is protected too. Inserting it earlier would mint a replica of a record `ValidateShape` rejected. |
| +> 4. **Where Step 5's protect goes**: the top of the mint/converge loop body (`for _, rec := range recs`), before `trashedFor`/`seedFor` run. |
| +> 5. Line drift: `recs` `:655`, `protect` `:666`, channel guard `:725-729`, scope check `:733`, loop close `:747`, mint loop `:837`, removal `protect` read `:1082`. Find anchors by name. |
| + |
| **Files:** |
| - Modify: `internal/provision/reconcile.go` (`ReconcileConfig` `:34`; `routeFor` `:348-364`; `convergeOwned` `:572-628`; record loop `:649-736`; removal phase `:1048-1090`) |
| - Test: `internal/provision/reconcile_test.go` |
| @@ -640,6 +647,12 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 4: The domains pass derives the replica alias |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **Custom domains are now in scope** (spec §5a, revision 3). Besides `rt.ReplicaOf -> rt.Host`, derive one alias per CLAIM whose `Route` is the replica's `ReplicaOf`: `claim.Host -> rt.Host`, same box, same `desired` set, same sweep. This is the half that carries titogo's real checkout traffic, because a live custom domain already CNAMEs to `edge.<apps-domain>` and needs no steering. The claims are already in scope in this function (`claims, err := cache.List(ctx, s)`); today's custom-domain derivation at `:95-100` only fires when the claim's route is a LOCAL route, which on the replica's box it is not. |
| +> 2. Use `release.RouteAccount(rt)` for the alias's `Account`, not `rt.Account` — that is what the rows beside it use (`adopt.go:862`; it falls back to `OpsAccount` for a non-sqid account). |
| +> 3. Add a third test: a claim `{Host: "tickets.example.com", Route: "acme.go.tito.io"}` plus the replica route derives `tickets.example.com -> r7x2k9m4qa.go.tito.io`, and survives a second pass. |
| +> 4. Line drift: `desired` is at `:85`, `byHost` is built at `:77-80` from `routes` at `:73`, sweep `:363-368`. |
| + |
| **Files:** |
| - Modify: `internal/domains/aliases.go` (`ReconcileAliases` `:68-84`, sweep `:361-368`) |
| - Test: `internal/domains/aliases_test.go` |
| @@ -770,6 +783,12 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 5: The edges file marks the replica alias, and publishes it unconditionally |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **The replica test must be `byHost[a.Host].ReplicaOf != ""`, not `== a.Alias`.** With custom domains in scope (Task 4), a replica alias's name is the custom domain while `ReplicaOf` is the account host, so the equality test would mark the account-host alias and silently withhold every custom-domain one. Any alias whose PRIMARY route is a replica is a replica alias — that is the whole rule. |
| +> 2. There is **no `byHost` map in `edgesFilePass`**; build one from the `routes` slice it already reads (`:174`) before the alias loop. |
| +> 3. Add a second test: a custom-domain alias (`tickets.example.com -> r7x2k9m4qa.go.tito.io`, unstamped) is published as `role: "replica"` too. |
| +> 4. Line drift: `edgeHost` `:40-59`, alias loop `:248-270`, the `pointing` gate `:255`, `withheldAliasSaid` `sync.Map` at `:166`. |
| + |
| **Files:** |
| - Modify: `internal/host/edgesfile.go` (`edgeHost` `:36-59`; alias section `:246-269`) |
| - Test: `internal/host/edgesfile_test.go` |
| @@ -876,6 +895,11 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 6: The home map ignores role `replica` on purpose |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **Step 4's fixtures do not exist.** There is no `newTestEdge` and no `serveUnix` in `internal/edge`. What the package has: `openReg(t) *registry.Registry` (`edge_test.go:28`), `hostEchoBackend(t, body) (*httptest.Server, *string)` (`:39`), `New(reg, nil)` + `httptest.NewServer(e)` + `getHost(t, srv.URL, host) (int, string)` (`:52`), and `upsertAlias(t, reg, alias, host)` (`alias_test.go:16`). `TestAliasServesPrimaryBackend` (`alias_test.go:29`) is the template to copy. Use a `Kind: "service"` route with `Addr` from `hostEchoBackend` rather than a unix socket — the edge reads the alias row, not `ReplicaOf`, so no socket is needed and the test then runs inside the sandbox. `New` already calls `Reload`, so upsert the route and the alias BEFORE `New`. |
| +> 2. **Do not claim a home map the test does not build.** `e.homes` is an `atomic.Pointer[homeMap]` built from a deployment store; the only fixture that seeds one is `backhaul_e2e_test.go:274-284`. Either build on that fixture (an edges file publishing `acme.go.tito.io` as `role: "home"` on another box, then assert the LOCAL upstream answered — this is the real "no backhaul" proof and is worth the extra work), or keep the simpler test and write its comment honestly as a shape pin. **Build the e2e version**: this assertion is why the design exists, and a shape pin that cannot fail is not worth the line. |
| +> 3. `edgeRoleHome` and `edgeRoleWildcard` are top-level consts at `:106` and `:111`; put `edgeRoleReplica` beside them. |
| + |
| **Files:** |
| - Modify: `internal/edge/homemap.go` (`foldHomes` `:529-584`) |
| - Test: `internal/edge/homemap_test.go` |
| @@ -998,6 +1022,18 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 7: The console doors — create, PATCH, delete, move, and the old-writer guard |
| +> **Revision 2 corrections — read before the steps. This task had six defects; none of the steps below is safe to paste unedited.** |
| +> 1. **Order in the create door.** The host check `ValidHost(host, m.Domain)` runs at `:631`, BEFORE the placement lookup at `:701`. A hostless replica create is refused there. So: parse and validate `role` right after `readJSON` (`:619`), refuse a non-empty `req.Host`/`req.Channel` for a replica there, and mint `host = mintReplicaHost(m.Domain)` before `:631` (`m` is available from `:574`). Keep every cross-record rail — the home `GetRecord`, steering, own-placement, opt-in, `placed` — after `:712`, where `placed` and `region` exist. |
| +> 2. **The PATCH door's request variable is `req`, not `body`.** Also: the "nothing to update" guard at `:1108` lists every pointer field and will 400 a body carrying only `replicas` unless `req.Replicas == nil` joins that condition; and `m` is NOT in scope in `handleAPIInstanceUpdate` — call `m, ok := s.instanceManifest(ctx, w, acct, app)` (it writes its own refusal) before using `m.Regions`. |
| +> 3. **`protectedFieldsEqual` takes `Role` and `Home` only, never `Replicas`** (spec §6, revision 3). `UpdateRecord` runs that function on every PATCH, so including `Replicas` would refuse the new door's own `replicas` PATCH — both console tests would fail at their first PATCH. And the guard is inert against an OLD console anyway (it runs its own copy). Keep only the `role`/`home` half of `TestProtectedFieldsRefuseAnOldWriter`, and put that test in `internal/provision/maintenance_test.go` — it uses `Record` and `protectedFieldsEqual` unqualified and will not compile in the console package. |
| +> 4. **The console tests cannot run on `titoManifest()`.** It has `Pool: "tito"` (so `steering: latency` is refused at `:1208` — "steering requires the shared pool") and `Regions: ["ie"]` (so `region: "uk"` is refused by the allow-list). Write a local `replicaManifest()` beside it with `Pool: ""` and `Regions: []string{"ie", "uk", "de"}`, leaving `de` out of the opt-in so the "placement not opted in" case tests the rail it is named for. Also: `enableInstances` takes FOUR arguments — `enableInstances(t, fx, "titogo", replicaManifest())` — the app is `titogo` and the account is `bab`, so URLs come from `instancesURL(fx, "bab", "titogo")`. |
| +> 5. **`homeRec = &rec` is a type error**: `provision.GetRecord` already returns `*Record`. And branch on `errors.Is(err, os.ErrNotExist)` for the 400 "no such instance"; any other error is a store failure and the door's convention is 502 (`:1130`). |
| +> 6. **Both the opt-in and own-placement comparisons go through `homeRegionOut` on both sides** (`region` is `""` for the deployment's home region). State the consequence in the refusal message: a replica cannot live in the deployment's home region at all (spec §2). |
| +> 7. **The CAS retry must re-mint more than the host**: by the `CreateRecord` call the name is baked into `rec.Host` and `rec.Slug` and has been through `staticHeadRefusal` and `ValidateShape`. Re-mint, reset both fields, re-run the shape check, and let the response carry the name that won. |
| +> 8. **The delete door must not fail open**: `if rec, err := GetRecord(...); err == nil` skips every check on a store failure. Refuse with 502 on any error except `os.ErrNotExist`. |
| +> 9. **The move rails have homes**: the `role == "replica"` refusal goes in `moveGate` (`apiinstancemove.go:30-60`, which already reads the record and returns it), so every move verb inherits it; the "into its own replica placement" rail goes in `handleAPIInstanceMoveRequest` after `to := s.homeRegionIn(region)` (`:190`). |
| +> 10. `apiinstances.go` does not import `crypto/rand`; add it. Hoist `placements(ctx, s.cfg.Releases)` out of the per-code loop. |
| + |
| **Files:** |
| - Modify: `internal/console/apiinstances.go` (create `:578-830`; update `:1041-1281`; delete `:1288-1350`; token `:1366-1406` — comment only) |
| - Modify: `internal/provision/maintenance.go` (`protectedFieldsEqual` `:79-94`) |
| @@ -1308,6 +1344,13 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 8: The list, get and CLI surface |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **The task omits the file the change actually lives in: `apimode.go`.** The root package does not use `internal/console`'s types; it decodes into `apiInstanceRecord` (`apimode.go:2812`), which has explicit fields and no `Role`/`Home`. Add `Role string \`json:"role"\`` and `Home string \`json:"home"\`` there, and `role`/`home` to `instanceCreate`'s signature and its anonymous body struct (`:2913-2924`). |
| +> 2. **`printInstances` does not exist** — the row printer is inline in `runInstancesList` (`:355-394`, header `:373`, row `:379`). Extract it as `printInstances(w io.Writer, recs []apiInstanceRecord)` so the test can call it, and say so in the commit message. |
| +> 3. **`runInstancesCreate(args []string)` returns nothing** and uses `flag.ExitOnError` with `log.Fatal` on a missing flag, so `err := runInstancesCreate(...)` does not compile and a `log.Fatal` would kill the test binary. Either extract the argument check into a function returning `error` and test that, or assert through `captureStdout` (`instances_e2e_test.go:94`) the way the existing tests do. |
| +> 4. **Add the `replicas` CLI path** (spec §12 lists it and Step 4's docs example uses it). Follow `runSteering` (`instances_cli.go:544`) or `runInstancesSetChannel` (`:591`), with the matching `apimode.go` client method. Without it the documented example names a flag nothing parses. |
| +> 5. `internal/docsite`'s gate checks that a fenced `carlos <verb>` is a real top-level command; it does NOT notice an undocumented flag. AGENTS.md's rule still stands — document `--role`, `--home` and the replicas verb — but do not expect the test to catch you. |
| + |
| **Files:** |
| - Modify: `internal/console/apiinstances.go` (`instanceListRecord` `:143-193`, `instanceGetRecord` `:521-529` — both embed `provision.Record`, so the three fields ride along; verify and add nothing if so) |
| - Modify: `instances_cli.go` (list columns `:372-393`; create flags `:262-303`) |
| @@ -1381,6 +1424,14 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| ### Task 9: The siblings file |
| +> **Revision 2 corrections — read before the steps.** |
| +> 1. **`pathsFor` is an unexported method on `*ExecBackend`** and `internal/host` cannot call it. Two options; **take the second**: let the activator own the question. `ExecBackend` already has `pathsFor`, so in `agentOwnedVars` it can `os.Stat(filepath.Join(p.dataDir, "siblings.json"))` and set or strip `CARLOS_SIBLINGS_FILE` itself — no new `SiblingsFile` field and no cross-package path duplication. The host-side writer computes its own directory the way `envPathFor` does (`internal/host/schedule_wiring.go:192-206`: `activator.Classify(*rt, activator.LayoutInputs{DataDir: dd}) == activator.LayoutNested` then `provision.InstanceDir(dd, host)`, else flat `dd`). |
| +> 2. **There is no bare `provision.Reconcile` call in `agent.go`.** Both legs go through `reconcileSerialized(ctx, store, reg, cfg)` (`:2330` on the slow instances leg, `:1787` on the fast converge leg) under `reconcileMu`. Set `cfg.OnRecords` on what `provisionTickConfig` returns so BOTH legs carry it, keep the callback to a map copy (it runs under the mutex), and call `siblingsPass` after `reconcileSerialized` returns. |
| +> 3. **`buildSiblings`'s `self` argument means two different things in its own tests.** Drop the argument: the pure core emits every file it can pair, and `siblingsPass` writes only those whose host has a local route in `reg.List()`. Filtering by placement is wrong anyway — a home in the deployment's home region has `Region == ""`. Fix both tests to call `buildSiblings(all, eipOf, keys)`. |
| +> 4. **`meshKeys()` returns `(current, next)`, not `(current, previous)`** — after a promotion the old key is simply gone. Spec §7 (revision 3) says where "previous" comes from: the siblings pass retains the derivative it last wrote per pair and republishes it for one further pass after it stops being derivable. Implement that memory in the pass (a `map[pairKey]string` on the loop's state), and write a test for it: pass 1 with key A, pass 2 with key B, the file carries both, pass 3 carries only B. |
| +> 5. **Imports the task will need**: `crypto/hmac`, `crypto/sha256`, `encoding/base64`, `encoding/json`, `os`, `path/filepath`, `sort`, plus `activator`, `provision`, `instancefs`. |
| +> 6. **Write Step 5's tests out rather than describing them**: mode 0600; the nested (`/data/instances/<id>`) and flat (`/data`) directories both resolved correctly; a second pass with an unchanged pair rewriting nothing; a stale `siblings.json` removed when a host stops being part of any pair — and that removal happens in tenant-writable ground, so it needs the same no-symlink care as the write. |
| + |
| **Files:** |
| - Create: `internal/host/siblings.go`, `internal/host/siblings_test.go` |
| - Modify: `internal/host/agent.go` (the instances loop around `:2336`; the edge-ready callback at `:510`) |
| @@ -1535,7 +1586,11 @@ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>" |
| - [ ] **Step 1: The additivity proof** |
| -Write one test (in `internal/host/edgesfile_test.go`) asserting that a registry with no replica rows produces an edges file byte-identical to the same registry before this branch — capture the expected body as a literal in the test, not as a computed value. |
| +Both halves already exist as literal pins; do not write a second copy. Confirm they still pass unchanged and cite them in the commit message: |
| +- the edges-file body: `TestEdgesFileRawWireBodyMatchesSchema` (`internal/host/edgesfile_test.go:292`); |
| +- the minted registry row: `TestReconcileMints`' `want` literal (`internal/provision/reconcile_test.go:152`). |
| + |
| +If either needed editing to pass, this branch is not additive and that is a stop-and-report, not a test to update. |
| - [ ] **Step 2: Run everything** |