amadan
#162: plan review findings, and Task 1's two defects fixed
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+632
−2
FABLE-FINDINGS-162-plan.md+627 −0docs/superpowers/plans/2026-09-21-replica-placements.md+5 −2
CI cancelled — superseded by a newer push — run details
diff --git a/FABLE-FINDINGS-162-plan.md b/FABLE-FINDINGS-162-plan.md| new file mode 100644 |
| index 000000000..33a974b1b |
| --- /dev/null |
| +++ b/FABLE-FINDINGS-162-plan.md |
| @@ -0,0 +1,627 @@ |
| +# Findings: replica placements plan (#162), round 3 |
| + |
| +Second reader's findings against commit 314c4919 on `replica-placements-162`, |
| +read against `docs/superpowers/plans/2026-09-21-replica-placements.md`, the |
| +spec at revision 2, and `FABLE-FINDINGS-162-spec.md`. Every citation below |
| +was read in this worktree. Round 2's decisions are taken as paid for; where |
| +a finding touches one, it is because the plan's mechanism for it does not do |
| +what the decision says, not because the decision is being reopened. |
| + |
| +Severity: P1 would produce broken code or a wrong tree if a fresh subagent |
| +followed the task as written; P2 costs an implementer a round trip; P3 is |
| +worth knowing. |
| + |
| +Summary: the plan's architecture is faithful to the spec and its file:line |
| +citations are mostly within a few lines of true. What would break is in the |
| +briefs' code: two of Task 3's tests assert against a `Get` that returns |
| +`(nil, nil)` on a miss, so one always fails and one passes vacuously; Task |
| +6's routing test names two fixtures that do not exist; Task 7 places the |
| +role block after a host check that has already refused the request, names |
| +the PATCH body by the wrong variable, uses a manifest the update door never |
| +loads, and adds the new fields to a guard that then refuses the new door's |
| +own PATCH while stopping no old writer; Task 7's tests use a manifest whose |
| +pool makes steering unreachable; Task 8 tests functions and types that do |
| +not exist in the root package and omits the file (`apimode.go`) the CLI |
| +change actually lives in. Below that: missing imports in Tasks 1 and 9, an |
| +unexported `pathsFor` the host package cannot call, a reconcile call the |
| +plan says to hook "right after" that does not exist in `agent.go`, and a |
| +`meshKeys` pair whose second element is the spec's "next", not its |
| +"previous". |
| + |
| +Counts: P1 8 (A2, A5, A8, A10, A11, A12, A13, A21), P2 15 (A1, A9, A14-A19, |
| +A23-A26, C1, C2, E2), P3 14 (A3, A4, A6, A7, A20, A22, A27, A28, C3, C4, |
| +E4, E5, the Task 5 `byHost` note, and the line-drift list). E1 and E3 |
| +restate P1 items as risk and are not counted twice. |
| + |
| +--- |
| + |
| +## A. Citations |
| + |
| +### Task 1 (`internal/provision/provision.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| struct close `:142` | `:143` (P3) | |
| +| `ValidSteering` `:354-367` | `:362-368`; "directly after" means after `:368` (P3) | |
| +| `ValidateShape` `:392-451` | `:384-450`; the `ValidSteering` check is `:388-390`, not `:396-398` (P3) | |
| +| `ValidRegion` `:551` | correct | |
| +| `KindOrDefault` | exists, `:269` | |
| +| `CreateRecord` "`:31-33`" (spec) | the create-only comment is `:33-35`; the function is `:651` (P3) | |
| + |
| +**A1. P2. Missing imports.** `provision.go` imports `errors`, `fmt`, |
| +`sort`, `strings` but not `slices`; Step 5's `slices.Contains` needs it. |
| +`provision_test.go` imports `encoding/json` but not `slices` or `bytes`; |
| +Step 1's `slices.Equal` and `bytes.Contains` need both. Go 1.26 (`go.mod`), |
| +so `slices` is available. |
| + |
| +**A2. P1. `TestValidReplicas` contradicts `ValidReplicas` and the spec.** |
| +The test hands `[]string{" uk ", "ie", "uk", ""}` and expects `["ie","uk"]` |
| +with `err == nil`. Step 4's `ValidReplicas` returns |
| +`errors.New("replicas: empty placement code")` on the trimmed `""`, and spec |
| +§2 says entries are "refused when empty". `TestValidateShapeReplicaRails` |
| +("empty placement") and the spec agree with the implementation; the first |
| +test is wrong. Drop `""` from that input. Also fix the doc comment on |
| +`ValidReplicas` ("trim, drop empties, ...") which describes the behaviour |
| +the test expected, not the one the code has. |
| + |
| +### Task 2 (`internal/registry/registry.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| `Route` close `:260`; `CreatedAt int64` at `:259` | close `:265`; `CreatedAt` `:264`; `Steering` `:262` (P3) | |
| +| migrations `:442-473`, `max_run_seconds` at `:472` | `:442-476`; `max_run_seconds` at `:473` (P3) | |
| +| `routeCols` `:722-727`, "40 names" | correct; note the comment says adding a column means "FOUR places" (comment count is the plan's fifth) | |
| +| `scanRoute` `:729-744` | `:729-746` | |
| +| `Upsert` `:916-971` | `:916-975`; `VALUES` has 40 `?` at `:944` | |
| +| `SetSteering` `:1300-1312` | `:1301-1313`; the SetChannel/SetUpstreams "errors on unmatched host" contract is stated in its doc comment `:1295-1300` | |
| +| `reg.Get` | `func (r *Registry) Get(host string) (*Route, error)` `:780`; **returns `(nil, nil)` when the host is absent** (`sql.ErrNoRows` -> `nil, nil`, `:783-785`). This matters for Task 3, see A5. | |
| +| `reg.Delete` | `func (r *Registry) Delete(host string) error` `:794`, no error on a missing host | |
| +| `TestUpsertRoundTripsAllColumns` | `registry_test.go:63`, compares `*got != want` on a literal `Route`; a new `""`-default field keeps it green, and a column shift breaks it, as the plan says | |
| + |
| +**A3. P3.** `Upsert`'s `ON CONFLICT` list at `:945-970`: `steering=excluded.steering` is on the `addrs=..., health=..., steering=...` line (`:966`); "beside steering" is fine. The plain overwrite means any *other* `Upsert` caller re-upserting a replica row (`carlos route add`, a hand edit) clears `replica_of` until the next converge restores it. Acceptable; say it in the doc comment so nobody reads a transient `""` as a bug. |
| + |
| +### Task 3 (`internal/provision/reconcile.go`, `reconcile_test.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| `ReconcileConfig` `:34` | correct | |
| +| `routeFor` `:348-364` | `:348-364` correct, signature `(rec Record, dataDir, seed string)` | |
| +| `convergeOwned` `:572-628` | `:572-631` | |
| +| record loop `:649-736` | `:650-747`; `recs` at `:655` (plan `:654`); `protect` at `:666` (plan `:663`) | |
| +| channel guard "`:718`" | `:725-729` | |
| +| scope check "`:725`" | `:733` | |
| +| collection loop closes "`:736`" | `:747` | |
| +| removal phase `:1048-1090`, `protect` read at `:1076` | `:1049-1094`; `protect[rt.Host]` at `:1082` | |
| +| `routeFor` call sites `:871`, `:947`, `:965` | **correct, all three**; no other caller in the package (`grep` over non-test files) | |
| +| `convergeOwned` call sites `:888`, `:982`, `:997` | **correct, all three** | |
| +| test call sites `reconcile_test.go:925, 2508, 2527, 2540, 2556, 2704, 2708` | **correct, all seven** (`:2511` is inside a message string, not a call) | |
| +| `TestReconcileConvergesSteering` `:523` | correct; `putReplace(t, s, RecordKey(sqid, "tito", rec.Host), mustJSON(t, rec))` is exactly its shape | |
| +| helpers `setup`, `acmeRecord`, `regWith`, `mustGet`, `putReplace`, `mustJSON`, `ctx`, `dirStore` | all exist in `reconcile_test.go` (`:127`, `:139`, `:37`, `:51`, `:83`, `:115`, `:23`, `:32`). `mustGet` returns `*registry.Route`. `acmeRecord` sets `Pool: "tito", Region: "ie", Channel: "stable"` and no `Steering`, so `replicaSetup`'s overrides are right. `RecordKey(acct, app, host)` `:201` | |
| +| `DeleteRecord(ctx, s, acct, app, host)` | `:885`, signature matches the test | |
| +| the mint/converge loop | `for _, rec := range recs` at `:837`, `existing, ok := byHost[rec.Host]` `:845`, `switch` `:846` | |
| + |
| +**A4. P3. Where Step 5's protect goes.** "In the mint/converge arm, before |
| +either call" is the top of the loop body at `:837-845`, before |
| +`trashedFor` or `seedFor` run; say so, and note that `continue` there is |
| +correct because the removal phase's `recs[rt.Host]` lookup (`:1076`) still |
| +finds the record and never treats it as "record gone", so `protect` there |
| +matters only for the trash case (`:1077-1081`). |
| + |
| +**A5. P1. Two Task 3 tests are wrong about `Get`.** Because `Get` returns |
| +`(nil, nil)` on a miss: |
| + |
| +- `TestReconcileMintsReplicaRouteOnItsOwnBoxOnly`: |
| + `if _, err := ie.Get(replica.Host); err == nil { t.Error(...) }` fires |
| + whether or not the route exists. The test can never pass. Want |
| + `if rt, err := ie.Get(replica.Host); err != nil { t.Fatal(err) } else if rt != nil { t.Error(...) }`. |
| +- `TestReconcileProtectsReplicaWhoseHomeIsGone`: |
| + `if _, err := uk.Get(replica.Host); err != nil { t.Fatal(...) }` passes |
| + vacuously even if the route was removed. And the route would survive |
| + anyway (the record still exists, A4), so the test does not exercise the |
| + protect at all. What Step 5 actually prevents is a mint or converge with |
| + an empty channel from a zero `home`. Assert that: after the home is |
| + deleted, `mustGet(t, uk, replica.Host).Channel == "stable"` still; and on |
| + a *fresh* registry (`regWith(t)`) with the home gone, `Get` returns `nil` |
| + (nothing minted on a guess). |
| + |
| +**A6. P3. `OnRecords` and the `all` map.** `all[rec.Host] = rec` "before |
| +the scope check" means before `:733`, after the channel guard `:725-729`, |
| +so a protected home is absent from `all` and its replica is protected too, |
| +which is what spec §2 wants. Say that explicitly; a subagent that puts the |
| +insert before the `ValidateShape` call would silently mint a replica of a |
| +malformed home. |
| + |
| +### Task 4 (`internal/domains/aliases.go`, `aliases_test.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| `ReconcileAliases` `:68-84` | `:68` signature `(ctx, s release.Store, reg, now func() time.Time, deploymentRoutes map[string]bool, cache *ClaimCache)`; six args, test calls match | |
| +| `desired := map[string]bool{}` at `:84` | `:85` | |
| +| `byHost` from `reg.List()` `:78-81` | `routes, err := reg.List()` `:73`; `byHost` `:77-80` | |
| +| sweep `:361-368` | `:363-368` | |
| +| `aliasRegWith`, `putClaim`, `aliasNow` | `aliases_test.go:19`, `:36`, `:68` | |
| +| `release.NewDirStore` | `release/store.go:263`, returns `*DirStore` | |
| +| imports | `aliases.go` has `log`, `registry`; `aliases_test.go` has `context`, `registry`, `release`. Nothing missing. | |
| + |
| +**A7. P3.** The claims loop uses `release.RouteAccount(rt)` (`adopt.go:862`, |
| +falls back to `OpsAccount` when `rt.Account` is not a valid sqid) for the |
| +alias's `Account`. Step 3 writes `rt.Account` raw. Use `RouteAccount` for |
| +consistency with the rows beside it. |
| + |
| +### Task 5 (`internal/host/edgesfile.go`, `edgesfile_test.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| `edgeHost` `:36-59` | `:40-59` | |
| +| alias section `:246-269`; the `pointing` gate `:255` | `:244-270`; gate `:255` correct; `withheldAliasSaid.Delete` `:265`; `hosts[a.Alias] = edgeHost{Role: "home", Alias: true}` `:269` | |
| +| `routes` at `:174` | correct | |
| +| `byHost` | **there is no `byHost` in `edgesFilePass`**; the plan's hedge applies. Build it from `routes` (`:174`) before the `hosts` loop. (P3) | |
| +| `withheldAliasSaid` | `sync.Map` at `:166` | |
| +| `edgesFile{Box, Placement}` | fields exist (`:77-90`); `Hosts map[string]edgeHost` `:130` | |
| +| `edgesFilePass(ctx, s, reg, ident edgesFile, eip string, last *[]byte)` | `:172`, matches the test | |
| +| `s.Get` -> `(body, etag, err)` | yes; `got, _, err` is right; key `control/edges/uk-1.json` matches `edgesFileKey` (`:34`) | |
| +| `edgesTestRegistry` | `edgesfile_test.go:20`; `seedRoute` `:33` | |
| +| `TestEdgesFilePublishesPointingAliasesOfServedPrimaries` `:669` | correct | |
| +| test imports | `context`, `encoding/json`, `registry`, `release` present | |
| + |
| +The served-row filter at `:187-219` admits the test's route (`Status: |
| +"ready"`, not a sidecar). Fine. |
| + |
| +### Task 6 (`internal/edge/homemap.go`, `homemap_test.go`, `alias_test.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| `foldHomes` `:529-584` | correct; `default: continue` at `:563-564` correct | |
| +| `edgeRoleHome` / `edgeRoleWildcard` | top-level consts at `:106` and `:111`; `const edgeRoleReplica = "replica"` beside them is right | |
| +| `edgesFileView{V, Box, EIP, Hosts}`, `edgesHostView{Role, Platform, Alias, ...}` | `:130-135`, `:139-150`; `homeEntry{eip, box, ...}` `:155` | |
| +| `foldHomes(files, "us-1")` returning `map[string]homeEntry` | correct | |
| +| `homeMap.Home` `:611-625` returns `(eip, box string, ok bool)` | correct | |
| +| `meshKeys` `:689` returns `(current, next []byte)` | correct (plan's Task 9 says `:689`; the spec's `:693-696` is the body) | |
| + |
| +**A8. P1. Step 4's fixtures do not exist.** There is no `newTestEdge` and |
| +no `serveUnix` anywhere in `internal/edge/*_test.go`. What the package has: |
| + |
| +- `openReg(t) *registry.Registry` (`edge_test.go:28`) |
| +- `hostEchoBackend(t, body) (*httptest.Server, *string)` (`:39`), a TCP |
| + upstream that echoes the Host it saw |
| +- `New(reg, nil)` then `httptest.NewServer(e)` and |
| + `getHost(t, srv.URL, host) (int, string)` (`:52`) |
| +- `upsertAlias(t, reg, alias, host)` (`alias_test.go:16`) |
| +- `TestAliasServesPrimaryBackend` (`alias_test.go:29`) is the exact |
| + template; `TestUnixSocketUpstream` (`edge_test.go:164`) shows the |
| + `t.Skipf` on EPERM for a unix-socket upstream in the sandbox. |
| + |
| +`New` already calls `Reload` (`edge.go:298`), so upsert the route and alias |
| +first, then `New`. `e.ServeHTTP` exists (`edge.go:1229`) and `e.Reload()` |
| +(`:452`), so the plan's direct-call shape also works once the edge exists. |
| +Prefer a `Kind: "service", Addr:` route from `hostEchoBackend` over a |
| +socket: the edge does not read `ReplicaOf`, only the alias row, so nothing |
| +in this test needs a socket, and it then runs inside the sandbox. |
| + |
| +**A9. P2. The test's comment promises a home map it never seeds.** "The |
| +home map is deliberately seeded with the account host homed on ANOTHER |
| +box" is not in the code, and seeding one is not a one-liner: `e.homes` is |
| +`atomic.Pointer[homeMap]` (`edge.go:219`), built by `e.newHomeMap()` |
| +(`homemap.go:239`) from a deployment store, and the only test that does it |
| +is the store-backed fixture at `backhaul_e2e_test.go:274-284` |
| +(`be.homes = e.newHomeMap(); e.homes.Store(be.homes)`). Without it the |
| +test is `TestAliasServesPrimaryBackend` with a `ReplicaOf` column the edge |
| +ignores, and proves nothing about "no backhaul". Either drop the claim from |
| +the comment and keep it as a shape pin, or build it on the e2e fixture with |
| +an edges file publishing `acme.go.tito.io` as `role: "home"` on `ie-1` and |
| +assert the local upstream answered. Say which. |
| + |
| +### Task 7 (`internal/console/apiinstances.go`, `apiinstancemove.go`, `maintenance.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| create `:578-830` | request struct `:578-618`; `host := strings.TrimSpace(req.Host)` `:626`; `ValidHost(host, m.Domain)` `:631`; manifest `m, ok := s.instanceManifest(ctx, w, acct, app)` `:574` (returns `*provision.Manifest`); registry+`appPlacement` `:695-706`; `region := s.homeRegionIn(...)` `:710`; `if placed != "" { region = placed }` `:711-712`; armed gate `:741-755`; channel `:757-766`; `staticHeadRefusal` `:770`; `rec := provision.Record{` `:774`; `ValidateShape` `:836`; `CreateRecord` + `ErrCASConflict` `:840-849`; response `writeJSON(w, http.StatusCreated, recordView(rec))` `:880` | |
| +| update `:1041-1281` | `:1041`; request struct **is named `req`, not `body`** `:1094-1104` (`Steering *string`); "nothing to update" guard `:1108-1112`; `ctx` `:1122`; `GetRecordETag` `:1124`; `stored := *rec` `:1136`; `rec.Steering = *req.Steering` `:1151-1152`; `ValidateShape(*rec)` `:1188`; shared-pool steering rail `:1208-1212`; `UpdateRecord` `:1236` | |
| +| delete `:1288-1350` | `:1288`; `refuseControlPlane` `:1308` correct; `host := r.PathValue("host")` `:1311`; `DeleteRecord` `:1330` | |
| +| token `:1366-1406`, `NewAppToken` `:1392` | correct | |
| +| `instanceRegionAllowed` `:1573` | correct | |
| +| `instanceListMax` | `const instanceListMax = 500` `:57` | |
| +| `provision.GetRecord` | `(ctx, s, acct, app, host) (*Record, error)` `provision.go:941`; `os.ErrNotExist` on a miss | |
| +| `provision.ListRecords` | `(ctx, s, acct, app, max) (recs []Record, truncated bool, err error)` `:980`, matches `replicaPlacementsInUse` | |
| +| `release.ErrCASConflict` | `release/store.go:35` | |
| +| `homeRegionIn`/`homeRegionOut` `apiinstancemove.go:136-148` | correct | |
| +| `placements(ctx, s.cfg.Releases)` | `instancesplacement.go:173`, returns `(caps map[string]placementCapability, published bool)`; `caps[code].Hosting`, `.Reachable` exist | |
| +| `s.appPlacement(reg, acct, app)` `:701` | correct | |
| +| `recordView` | `:541`, returns `apiRecordView{Record: rec, ...}` which embeds the record, so `role`/`home`/`host` ride along in the create response | |
| +| `instanceListRecord` `:143`, `instanceGetRecord` `:521` | both embed `provision.Record`; Task 8's "verify and add nothing" holds | |
| +| `protectedFieldsEqual` `:79-94` | `maintenance.go:81-95`; callers `provision.go:715` (the guarded mutate path) and `provision.go:926` (`UpdateRecord`) | |
| +| test helpers | `instanceFixture(t, code string) (*consoleFixture, string)` `:101`; **`enableInstances(t, fx, app string, m provision.Manifest)`** `:116`; `titoManifest()` `:123` (App `"titogo"`, Domain `go.tito.io`, **Pool `"tito"`, Regions `["ie"]`**); `instancesURL(fx, acct, app)` `:128`; `rawReq(t, method, u, bearer, body) (int, http.Header, string)` `:42`; `cliToken` `authcli_test.go:474`. No `createInstance`/`patchInstance`/`deleteInstanceRaw` exist; the plan's "write them" applies. | |
| +| move door "`apiinstancemove.go`" | `moveGate` (reads the record at `:43`, refuses non-instance kinds at `:53-56`, returns the record as its fourth value) and `handleAPIInstanceMoveRequest` `:179` (`to := s.homeRegionIn(region)` `:190`) | |
| +| imports | `apiinstances.go` has `slices`, `errors`, `strings`, `context`; **no `crypto/rand`** (needed by `mintReplicaHost`) | |
| + |
| +**A10. P1. The role block is placed after a check that has already refused |
| +the request.** Step 3 says "after the manifest and placement lookups |
| +(`:701-712`) and before the host validation". The host validation is |
| +`:626-634`, *before* the placement lookup. A `role=replica` body with no |
| +`host` hits `ValidHost("", m.Domain)` at `:631` and is refused "invalid |
| +host" before any role code runs; `TestAPIInstanceCreateReplica` cannot |
| +pass. The mint must precede `:631`, and `m.Domain` is available there |
| +(`:574`), but `placed` is not until `:701`. Order that works: parse and |
| +validate `role` right after `readJSON` (`:619`); for a replica, refuse a |
| +non-empty `req.Host`/`req.Channel` there and set `host = |
| +mintReplicaHost(m.Domain)` before `:631`; keep every cross-record rail |
| +(`GetRecord` of the home, steering, own placement, opt-in, `placed`) after |
| +`:712`. The sibling-apex check at `:636-690` then runs on the minted name, |
| +which is harmless. |
| + |
| +**A11. P1. The PATCH door as written does not compile and would refuse the |
| +request anyway.** Three separate facts: |
| + |
| +- The request variable is `req` (`:1094`), not `body`. `body.Replicas`, |
| + `body.Steering` are undefined. |
| +- The "nothing to update" guard at `:1108` lists every pointer field; a |
| + body carrying only `replicas` is refused with 400 unless `req.Replicas == |
| + nil` joins that condition. |
| +- `m` is not in scope in `handleAPIInstanceUpdate`; the door never loads |
| + the manifest. `instanceRegionAllowed(m.Regions, code)` needs `m, ok := |
| + s.instanceManifest(ctx, w, acct, app)` first (it writes its own refusal |
| + on failure, `:117`). |
| + |
| +**A12. P1. `protectedFieldsEqual` is the wrong seam for the old-writer |
| +guard, and adding the fields to it breaks the new door.** `UpdateRecord` |
| +(`provision.go:926`) calls `protectedFieldsEqual(*before, rec)` and returns |
| +`ErrMaintenanceProjection` on any inequality. If `Replicas` joins it with |
| +plain equality, the new console's own `PATCH {"replicas":["uk"]}` differs |
| +from `before` and is refused; `TestAPIInstanceReplicasPatchRails` and |
| +`TestAPIInstanceCreateReplica` both fail at their first `replicas` PATCH. |
| +And the guard cannot reach the writer it is for: an old console runs its |
| +*own* `UpdateRecord` with its *own* `protectedFieldsEqual`, which has never |
| +heard of the fields, so its stripped write goes through unchanged. The |
| +bucket has etag CAS and nothing else. Spec §6 (fable B-7) chose this |
| +mechanism; the plan should say plainly that it is inert against old |
| +binaries and pick what the tree will actually do: |
| + |
| +- protect `Role` and `Home` (they are create-only by spec §2 and never |
| + change, so plain equality is right for those two) and leave `Replicas` |
| + out, letting the PATCH door's own rails police it; and |
| +- carry B-7's other option as the rollout rule in the handoff: no replica |
| + record for an app until every console that can PATCH that app's records |
| + is on a build carrying the fields (Tito's console is pinned older). |
| + |
| +`TestProtectedFieldsRefuseAnOldWriter` then keeps only its `role`/`home` |
| +half. |
| + |
| +**A13. P1. The console tests cannot reach the replica create on the |
| +fixture they use.** As written: |
| + |
| +- `instanceFixture(t, "tito")` passes `"tito"` as the OAuth *code*, which |
| + works, but the app is `"titogo"` and the account is `"bab"` |
| + (`instancesURL(fx, "bab", "titogo")`, `:280`), not `"tito"`. |
| +- `enableInstances(t, fx)` has two arguments; it takes four: |
| + `enableInstances(t, fx, "titogo", titoManifest())`. |
| +- `titoManifest()` has `Regions: []string{"ie"}`, so `region: "uk"` is |
| + refused by `instanceRegionAllowed` at `:713` on create and by the plan's |
| + own PATCH rail; and `"de"` ("placement not opted in") is refused by the |
| + allow-list, not by the opt-in rule the case is named for. |
| +- `titoManifest()` has `Pool: "tito"`, and the update door refuses |
| + `steering: latency` on any non-shared pool (`:1208-1212`, 409 "steering |
| + requires the shared pool"). `patchInstance(..., {"steering":"latency"})` |
| + fails, so nothing after it runs. |
| + |
| +The tests need a manifest with `Pool: ""` and `Regions: []string{"ie", |
| +"uk"}` (a local `replicaManifest()` beside `titoManifest()`), and the |
| +"placement not opted in" case needs a region that is *allowed* but not in |
| +`Replicas` (add `"de"` to `Regions` and leave it out of the opt-in). |
| + |
| +**A14. P2. `homeRec = &rec` is a type error.** `provision.GetRecord` |
| +returns `*Record`; `rec` is already a pointer, so `homeRec = rec`. Also |
| +`GetRecord`'s non-`os.ErrNotExist` errors (a store failure) are answered |
| +with 400 "no such instance" in Step 3; the door's convention is 502 for a |
| +store failure (`:1130-1133`). Branch on `errors.Is(err, os.ErrNotExist)`. |
| + |
| +**A15. P2. `TestProtectedFieldsRefuseAnOldWriter` is in the wrong code |
| +block.** Step 1 puts it in one block with the two console tests, but it |
| +uses `Record` and `protectedFieldsEqual` unqualified: it belongs in |
| +`internal/provision/maintenance_test.go` (which exists and imports |
| +`bytes`, `context`, `encoding/json`, `errors`, `strings`, `testing`, |
| +`time`, `maintenanceguard`, `release`; add `slices` if the set comparison |
| +is tested). A fresh subagent pasting the block into `apiinstances_test.go` |
| +gets undefined symbols. Split the block and label each. |
| + |
| +**A16. P2. The move door is named by file only.** Step 5 says "in the move |
| +door (`apiinstancemove.go`)". The place is `moveGate` (`:30-60`), which |
| +already reads the record (`:43`) and refuses by kind (`:53-56`); add the |
| +`rec.Role == "replica"` refusal there so every move verb inherits it. The |
| +"into its own replica placement" rail belongs in `handleAPIInstanceMoveRequest` |
| +after `to := s.homeRegionIn(region)` (`:190`), comparing |
| +`s.homeRegionOut(to)` against the record's `Replicas` (`moveGate`'s fourth |
| +return is the record; the handler discards it with `_` at `:180`). |
| + |
| +**A17. P2. The CAS retry needs to rebuild more than the host.** "Around |
| +the `CreateRecord` call, retry twice on `ErrCASConflict` for a replica, |
| +minting a fresh host each time": by `:840` the name is baked into |
| +`rec.Host`, `rec.Slug` (`:776-777`), and has been through |
| +`staticHeadRefusal` (`:770`) and `ValidateShape` (`:836`). The retry loop |
| +must re-mint `host`, reset `rec.Host`/`rec.Slug`, and re-run the shape |
| +check; the response at `:880` then carries the name that won. |
| + |
| +**A18. P2. The opt-in comparison must go through `homeRegionOut` on both |
| +sides, and the plan should say what it implies.** Step 3's |
| +`slices.Contains(rec.Replicas, region)` compares a normalised `region` |
| +(which is `""` for the deployment's home region, `:710`) against |
| +`Replicas` entries, which are always codes. Compare |
| +`s.homeRegionOut(region)`. Then state the consequence, which the spec |
| +leaves implicit: a replica *in the deployment's home region* is impossible |
| +under spec §2's "empty `Region` refused" rail, because the console |
| +normalises that region to `""` before the record is written. That is a |
| +consequence of two paid-for decisions, not a new one; the door should |
| +refuse it with a message that says so rather than with "no region". |
| + |
| +**A19. P2. The delete door fails open on a record read error.** Step 5 |
| +guards with `if rec, err := provision.GetRecord(...); err == nil && rec.Role == ""`. |
| +A store failure on `GetRecord` skips every check and deletes. Spec §6 |
| +says a failed list refuses the mutation; a failed read of the record itself |
| +is the same class. Refuse with 502 on any error other than |
| +`os.ErrNotExist` (which the existing door tolerates by design, `:1310-1325`). |
| + |
| +**A20. P3.** `placements(ctx, s.cfg.Releases)` inside the per-code loop in |
| +Step 4 reads every edges file once per placement; hoist it. |
| + |
| +### Task 8 (`instances_cli.go`, `apimode.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| list columns `:372-393` | `runInstancesList` `:355-394`; header `:373`; row `:379`; **the printer is inline, there is no `printInstances`** | |
| +| create flags `:262-303` | `runInstancesCreate(args []string)` `:262-309`; **returns nothing**; `flag.ExitOnError` and `log.Fatal(instancesRecordsUsage)` on a missing `--host` (`:279-281`) | |
| +| `instanceListRecord` | a `internal/console` type; the root package decodes into **`apiInstanceRecord`** (`apimode.go:2812`), which has explicit fields (`Host`, `Slug`, `Pool`, `Region`, `Channel`, `CreatedAt`, `CreatedBy`, `Kind`, `StagedVersion`, ...) and **no `Role` or `Home`** | |
| +| the request | `func (s *apiSession) instanceCreate(ctx, a apiAcct, app, host, region, channel, kind, environment string, addrs []string, probe *instanceProbeReq, messages ...string) (apiInstanceRecord, error)` `apimode.go:2913`, with an anonymous body struct at `:2915-2924` that has no `role`/`home` | |
| +| `instances_cli_test.go` | exists; `captureStdout(t)` (`instances_e2e_test.go:94`) is how list output is asserted | |
| +| `docs/site/cli.md` | `### carlos instances` at `:540` | |
| +| `internal/docsite` gate | `TestFencedCarlosCommandsAreReal` (`fences_test.go:57`) checks that every fenced `carlos <verb>` is a real **top-level command**; it does not check flags | |
| + |
| +**A21. P1. The Task 8 tests do not compile and the task omits the file the |
| +change lives in.** `err := runInstancesCreate(...)` assigns from a function |
| +with no return; `printInstances` and `instanceListRecord` do not exist in |
| +the root package. The change needs: `Role string \`json:"role"\`` and |
| +`Home string \`json:"home"\`` on `apiInstanceRecord` (`apimode.go:2812`); |
| +`role`/`home` on `instanceCreate`'s body and signature (`:2913-2924`) or a |
| +sibling `instanceCreateReplica`; and `apimode.go` in the Files list. For |
| +the tests: extract the row printer into a `printInstances(w io.Writer, recs |
| +[]apiInstanceRecord)` as the plan allows, and for the local `--home` |
| +refusal either extract the arg check into a function returning `error` or |
| +assert through `captureStdout` plus a `log` swap the way |
| +`instances_e2e_test.go` does; a `log.Fatal` in a test exits the binary. |
| + |
| +**A22. P3.** The claim that `internal/docsite` "fails the gate over an |
| +undocumented verb or flag" is backwards: the gate fails when the docs name |
| +a `carlos` verb that does not exist. It will not notice an undocumented |
| +`--role`. AGENTS.md's rule still applies; just do not rely on the test. |
| + |
| +### Task 9 (`internal/host/siblings.go`, `agent.go`, `homemap.go`, `backend_exec.go`) |
| + |
| +| Plan says | Code | |
| +|---|---| |
| +| instances loop "around `:2336`" | `:2325-2344`; the pass is **`reconcileSerialized(ctx, store, reg, cfg)`** at `:2330`, which runs `provision.Reconcile` *and* `ReconcileSidecars` under `reconcileMu` (`:977-984`). There is no bare `provision.Reconcile` call in `agent.go`. The fast-converge leg also runs it at `:1787` with `provisionTickConfig(os.Getenv)`; `cfg` for the slow loop comes from `provisionTickConfig` (`:2024`) | |
| +| edge-ready callback `:510` | `edge.RunWithActivatorReady(args, act, cache, func(e *edge.Edge) {` `:510`; `setUpstreamSource(e.UpstreamStates)` `:517` | |
| +| `upstreamSource` pattern `:2490-2510` | correct (`atomic.Value`, `setUpstreamSource` `:2504`) | |
| +| `homeMap.Home` `:611`, `meshKeys` `:689` | correct; `e.homes` is `atomic.Pointer[homeMap]` (`edge.go:219`); both methods are nil-receiver safe, so `e.homes.Load().Home(host)` is the wrapper | |
| +| `agentOwnedVars` `:977-995` | correct; `withVar` `:1200`, `withoutVars(env, keys...)` `:1219`; `childEnv` strip `:1174-1176`; calls from `startChild` `:895` and `startUnit` `:1055` (the functions start at `:796` and `:1037`) | |
| +| `ExecBackend` | struct at `:56`; `Region` at `:82`; `DataDir` present | |
| +| `TestExecBackendStartChildRegion` | `backend_exec_test.go:72`, the shape the plan points at | |
| +| `pathsFor(rt).dataDir` `activator/paths.go:38-42` | `:36-44`, but **`pathsFor` is an unexported method on `*ExecBackend`**; `internal/host` cannot call it | |
| +| `schedule_wiring.go:193-203` | `envPathFor` `:192-206`: `activator.Classify(*rt, activator.LayoutInputs{DataDir: dd}) == activator.LayoutNested` then `provision.InstanceDir(dd, host)`, else flat. This is the callable precedent | |
| +| `instancefs.WriteAtomic(dir, name string, data []byte, perm os.FileMode) error` | `instancefs_linux.go:104`, `instancefs_other.go:17`; `prepare.go:154` is the call the plan cites | |
| +| `provision.InstanceDir(dataDir, host)` | `provision.go:1084` | |
| + |
| +**A23. P2. `pathsFor` is not reachable from `internal/host`.** Say the |
| +writer computes the directory as `envPathFor` does: `activator.Classify` |
| +plus `provision.InstanceDir`, flat `dd` otherwise. Or have the activator |
| +own the path: `ExecBackend` already has `pathsFor`, so instead of a |
| +`SiblingsFile func(host) string` callback from host, the activator can |
| +`os.Stat(filepath.Join(p.dataDir, "siblings.json"))` in `agentOwnedVars` |
| +and set or strip the variable itself, with no new field. Either is fine; |
| +the plan should pick one, because the two put the "does the file exist" |
| +question in different packages. |
| + |
| +**A24. P2. The wiring sentence names a call that is not there.** "Call |
| +`siblingsPass` in the instances loop right after `provision.Reconcile`, |
| +using the map `OnRecords` captured" needs to become: set `cfg.OnRecords` |
| +on the config `provisionTickConfig` returns (both legs, `:1787` and |
| +`:2330`, or the file is refreshed only every five minutes and not on a |
| +serial bump), and call `siblingsPass` after `reconcileSerialized` returns. |
| +`OnRecords` runs inside `reconcileMu`; keep the callback to a map copy and |
| +do the writes outside. |
| + |
| +**A25. P2. `buildSiblings`'s `self` argument means two different things in |
| +its own tests.** `TestBuildSiblingsPairsHomeAndReplica` passes `self = |
| +"uk"` and expects *both* the home's file (`acme`, placement `ie`) and the |
| +replica's; `TestBuildSiblingsOmitsASiblingWithNoEIP...` passes `"ie"` and |
| +expects the home's. So `self` cannot be a placement filter. Either drop |
| +the argument (the pure core emits every file; `siblingsPass` writes only |
| +the ones whose host has a local route in `reg.List()`) or make the tests |
| +agree. The pass must filter by local *routes*, not by placement, because |
| +the home's own box is `Region ""` for a home in the deployment's home |
| +region. |
| + |
| +**A26. P2. `meshKeys` gives `(current, next)`; the spec's list is |
| +`(current, previous)`.** `homeMap.meshKeys()` returns the mirrored pair |
| +during a rotation and `(new current, nil)` after promotion; nothing keeps |
| +the old key. The plan's "publish both, current first, dropping nils" maps |
| +`next` into the slot spec §7 reserves for `previous`, so after promotion a |
| +sibling on a slower box still signing with the old derivative finds no |
| +match in the file, which is the window fable B-11 accepted the second key |
| +to close. State how "previous" is obtained: the siblings pass retains the |
| +last derivative it wrote per pair and keeps publishing it for one pass |
| +after it stops being derivable, or the edge exposes the previous key. And |
| +say the order during the window: the derivative of whichever key |
| +`signingKey` prefers first (`mesh.go:202-207` prefers `next`), or the two |
| +sides sign with different firsts. |
| + |
| +**A27. P3. Imports Task 9 will need and does not list:** `crypto/hmac`, |
| +`crypto/sha256`, `encoding/base64`, `encoding/json`, `path/filepath`, `os`, |
| +`sort` (deterministic `replicas` order), plus `activator`, `provision`, |
| +`instancefs` in `internal/host/siblings.go`. Step 5's writer tests ("mode |
| +0600", "second pass rewrites nothing", nested and flat directories) are |
| +described but not written; a fresh subagent will write weaker ones. Write |
| +them out. Stale-file removal ("removes a stale `siblings.json` when a host |
| +is no longer part of any pair") happens in tenant-writable ground; use the |
| +same care as the write (no symlink-following remove). |
| + |
| +### Task 10 |
| + |
| +**A28. P3.** `TestEdgesFileRawWireBodyMatchesSchema` (`edgesfile_test.go:292`) |
| +already pins the raw edges-file body as a literal. Cite it as the |
| +additivity proof for the edges-file layer rather than writing a second copy, |
| +and add the registry-row half (spec §11 test 10: `TestReconcileMints`' |
| +`want` literal at `reconcile_test.go:152` is that proof; say so). |
| + |
| +--- |
| + |
| +## B. Compile shape, per task |
| + |
| +- **Task 1:** A1 (imports), A2 (test/impl contradiction). Otherwise the |
| + code compiles: `KindOrDefault`, `ValidRegion`, `errors`, `fmt`, `sort`, |
| + `strings` all present. |
| +- **Task 2:** compiles as written; `fmt`, `filepath` present in both files. |
| +- **Task 3:** A5. The `all` map, `OnRecords`, both signature changes and |
| + their six call sites plus seven test call sites are complete. `log`, |
| + `slices` present. |
| +- **Task 4:** compiles; `log`, `registry` present. |
| +- **Task 5:** needs the `byHost` build (P3, hedged in the plan). |
| +- **Task 6:** A8. `homemap_test.go` half compiles. |
| +- **Task 7:** A10, A11, A12, A13, A14, A15; plus `crypto/rand` import. |
| + `replicaPlacementsInUse` compiles (`errors`, `slices`, `context`, |
| + `release` present). |
| +- **Task 8:** A21. |
| +- **Task 9:** A23, A25, A27. `siblingsFile`/`siblingEntry` types are fine; |
| + `Keys []string` with base64 strings matches spec §7's JSON. |
| +- **Task 10:** nothing to compile. |
| + |
| +--- |
| + |
| +## C. Spec coverage |
| + |
| +**Covered with a task:** §2 fields and every rail (T1); channel via `all` |
| +and the protect rule (T3); §3 column, `routeFor`/`convergeOwned`, alias |
| +derivation, sweep survival (T2, T3, T4); §4 mark, unconditional publish, |
| +explicit `foldHomes` case (T5, T6); §6 create/PATCH/delete/move rails and |
| +the token comment (T7); §7 siblings file, keys, env var (T9); §12 list/get |
| +and CLI (T8); §11 tests 1-8 and 10 (edges-file half). |
| + |
| +**C1. P2. No task gives the CLI a way to set `replicas`.** Spec §12 lists |
| +`replicas` on the CLI surface; Task 8 Step 4's worked example says "set |
| +`--replicas`", but Task 8 Step 3 adds only `--role` and `--home` to |
| +create, and no task adds a verb or flag that PATCHes `replicas` (the |
| +existing pattern is `runSteering`, `instances_cli.go:544`, and |
| +`runInstancesSetChannel`, `:591`). Without it the docs example names a flag |
| +nothing parses. Add it to Task 8 (a `--replicas` on `carlos steering`'s |
| +sibling, or an `instances set-replicas` verb, and the `docs/site/cli.md` |
| +entry), with the `apimode.go` client method. |
| + |
| +**C2. P2. §11 test 9 is only half written.** The pure-core tests exist; |
| +"written for both sides ... 0600, in the right directory for a nested and |
| +a flat row, refreshed when the replica set changes" is one sentence in |
| +Task 9 Step 5. See A27. |
| + |
| +**C3. P3. §11 test 10's "registry-row and route-table output" halves have |
| +no task.** Task 10 Step 1 covers the edges file only. A28 names the |
| +existing pins. |
| + |
| +**C4. P3. Things the plan adds that the spec does not name, all |
| +reasonable:** `ReconcileConfig.OnRecords` (the mechanism for §2's `all` |
| +map reaching §7's writer), `SetReplicaOf`, `ExecBackend.SiblingsFile` |
| +(A23 offers a field-free alternative), `mintReplicaHost`'s alphabet |
| +(`0123456789abcdefghjkmnpqrstvwxyz` is Crockford lower-case, and |
| +`256 % 32 == 0` so the modulo is unbiased). Nothing here contradicts a §13 |
| +ruling. |
| + |
| +--- |
| + |
| +## D. Ordering and independence |
| + |
| +Each task compiles and its tests pass given only its predecessors, with |
| +these exceptions: |
| + |
| +- **Task 3** cannot pass as written regardless of order (A5). |
| +- **Task 6 Step 4** cannot compile as written (A8); Steps 1-3 are |
| + independent of everything and pass today. |
| +- **Task 7** cannot pass as written (A10-A13); once fixed it depends only |
| + on Task 1 (`ValidRole`, `ValidReplicas`) and Task 3 is not needed for |
| + its tests. `Consumes: Tasks 1-3` overstates. |
| +- **Task 8** cannot compile as written (A21); once fixed it depends on |
| + Task 7's door. |
| +- **Task 9** depends on Task 3's `OnRecords` and Task 1's fields, as |
| + stated; its edge accessors depend on nothing. |
| +- **Task 5** seeds its alias directly, so it does not need Task 4 at test |
| + time; the plan says so. Correct. |
| +- No hidden cross-task signature change: `routeFor` and `convergeOwned` |
| + have no callers outside `reconcile.go` and its test (grep confirmed), and |
| + `ReconcileSidecars` shares `ReconcileConfig` without reading the new |
| + field. |
| + |
| +--- |
| + |
| +## E. Dangerous |
| + |
| +**E1. P1 (A12 again, stated as risk).** Adding `Replicas` to |
| +`protectedFieldsEqual` gates *every* PATCH through `UpdateRecord` and the |
| +mutate path at `provision.go:715`. A mistake there refuses unrelated |
| +PATCHes for every account with the maintenance-projection error. |
| + |
| +**E2. P2. Steering requires the shared pool, so the feature is unusable |
| +for a pooled app.** `handleAPIInstanceUpdate` refuses `steering: latency` |
| +on any record with a non-empty `Pool` (`:1208-1212`), and spec §5 makes |
| +`latency` a precondition of `Replicas`. titogo's instances live in pool |
| +`tito` (the test manifest and every `acmeRecord` fixture say so; the |
| +flagship should be checked). If that is the flagship's shape, no titogo |
| +home can be steered, so no titogo home can opt into a replica, and the |
| +first real use of this branch hits a 409 at step 1 of spec §9. This is |
| +not a plan defect and not a spec decision to reopen; it is a deployment |
| +fact the spec's "expected answer is none" (§5) did not connect to the pool |
| +rail. Surface it in the handoff beside amadan #169. |
| + |
| +**E3. P2. Vacuous or mis-attributed tests.** |
| +- Task 3: both `Get`-based assertions (A5). |
| +- Task 6 Step 4: identical in effect to `TestAliasServesPrimaryBackend` |
| + without a home map (A9). |
| +- Task 7: "placement not opted in" (`de`) is refused by the manifest |
| + allow-list, and "home's own placement" (`ie`) is refused by the opt-in |
| + rule as much as by the own-placement rule; neither case isolates the |
| + rail it is named for (A13). |
| +- Task 7's PATCH test would stop at the first `steering` PATCH (A13). |
| + |
| +**E4. P3. Hot paths touched.** Task 4 edits `ReconcileAliases`, which |
| +runs on the seconds-cadence fast leg (`agent.go:1800`); the addition is |
| +one loop over `routes` with no I/O unless a `ReplicaOf` row exists, so an |
| +account with no replicas pays nothing. Task 3 adds a second map of every |
| +decoded record to a pass that already decodes them; memory only. Task 5's |
| +exemption is keyed on a column that is `""` on every existing row, so the |
| +alias loop's behaviour for ordinary aliases is unchanged. Task 2's |
| +`Upsert` change is a plain overwrite of a `""` column. All consistent with |
| +"byte-identical for an account with no replicas". |
| + |
| +**E5. P3. Sweep and retirement.** The chain in spec §3 holds against the |
| +code: instance pass retires the replica route on "record gone" |
| +(`reconcile.go:1075-1092`, the record must be absent from `recs` *and* not |
| +in `protect`), the next `ReconcileAliases` finds no `ReplicaOf` row and |
| +sweeps the alias (`aliases.go:363-368`), the next `edgesFilePass` drops the |
| +entry. Trash follows the same path (`:1077-1081`). One interaction to name |
| +in Task 3: a home whose steering is turned off out of band (hand edit of |
| +the bucket) fails `ValidateShape`'s new rail on the *home's* box, is |
| +protected there, and is absent from `all` on the *replica's* box, so the |
| +replica is protected too and its alias keeps serving. That is the spec's |
| +"protect, never guess" and is correct, but it means a bad home record |
| +freezes its replicas rather than retiring them; say so in the log line. |
| + |
| +--- |
| + |
| +## Line-number drift, collected (all P3) |
| + |
| +`provision.go`: struct close `:143`; `ValidSteering` `:362-368`; |
| +`ValidateShape` `:384-450`, its steering check `:388-390`. |
| +`registry.go`: `Route` close `:265`, `CreatedAt` `:264`; last migration |
| +`:473`; `SetSteering` `:1301`. |
| +`reconcile.go`: `recs` `:655`; `protect` `:666`; channel guard `:725-729`; |
| +scope check `:733`; loop close `:747`; mint loop `:837`; removal `protect` |
| +read `:1082`. |
| +`aliases.go`: `desired` `:85`; sweep `:363-368`. |
| +`edgesfile.go`: `edgeHost` `:40-59`; alias loop `:248-270`. |
| +`backend_exec.go`: `startChild` `:796`, `startUnit` `:1037` (the `:895`, |
| +`:1055` cited are the `agentOwnedVars` calls inside them, which is what |
| +the spec meant). |
| +`instancesplacement.go:68-70` (served_by join) is `:66-71`. |
diff --git a/docs/superpowers/plans/2026-09-21-replica-placements.md b/docs/superpowers/plans/2026-09-21-replica-placements.md| index 83c21951c..542ae5aff 100644 |
| --- a/docs/superpowers/plans/2026-09-21-replica-placements.md |
| +++ b/docs/superpowers/plans/2026-09-21-replica-placements.md |
| @@ -33,7 +33,7 @@ |
| - [ ] **Step 1: Write the failing tests** |
| -Add to `internal/provision/provision_test.go`: |
| +Add to `internal/provision/provision_test.go`. Its import block needs `bytes`, `encoding/json` and `slices` for these tests; add whichever are missing rather than assuming they are there. |
| ```go |
| func TestValidRole(t *testing.T) { |
| @@ -50,13 +50,16 @@ func TestValidRole(t *testing.T) { |
| } |
| func TestValidReplicas(t *testing.T) { |
| - got, err := ValidReplicas([]string{" uk ", "ie", "uk", ""}) |
| + got, err := ValidReplicas([]string{" uk ", "ie", "uk"}) |
| if err != nil { |
| t.Fatal(err) |
| } |
| if want := []string{"ie", "uk"}; !slices.Equal(got, want) { |
| t.Errorf("ValidReplicas = %v, want %v", got, want) |
| } |
| + if _, err := ValidReplicas([]string{"ie", ""}); err == nil { |
| + t.Error("ValidReplicas accepted an empty placement: an empty entry is a caller bug, not a blank to skip") |
| + } |
| if _, err := ValidReplicas([]string{"UK"}); err == nil { |
| t.Error("ValidReplicas accepted an upper-case placement code") |
| } |