amadan
#162: branch review findings, and the three that are spec text not code
Summary
the whole-branch review's findings committed unedited (1 P1, 3 P2, 11 P3). The P1 and the two P2s are being fixed in code. Recorded here: the rotation memory is per process and does not survive a restart inside the promotion window; a pass that learned nothing writes nothing, so a deployment with no records keeps its siblings files; the delete door is no longer read-free for any account, which the additivity claim never covered because that claim is about byte-identical output; and a replica record's Channel is a create-time snapshot the list view keeps showing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+350
−1
FABLE-FINDINGS-162-branch.md+323 −0docs/superpowers/specs/2026-09-21-replica-placements-design.md+27 −1
CI passed — run details
diff --git a/FABLE-FINDINGS-162-branch.md b/FABLE-FINDINGS-162-branch.md| new file mode 100644 |
| index 000000000..2e7cbc1d1 |
| --- /dev/null |
| +++ b/FABLE-FINDINGS-162-branch.md |
| @@ -0,0 +1,323 @@ |
| +# Findings: replica placements branch (#162), whole-branch final review |
| + |
| +Second reader's findings against `replica-placements-162` at 73dfb461 |
| +(base 91cf6888, `git diff 91cf6888..73dfb461`), read against spec revision 3 |
| +(`docs/superpowers/specs/2026-09-21-replica-placements-design.md`), the plan |
| +with its revision 2 corrections, and the three prior findings files. Every |
| +citation was read in this worktree at the tip. Decisions recorded in spec |
| +§13 or in a plan corrections block are taken as settled and not reopened. |
| + |
| +Verified before writing: `go test` over the eight touched packages is green |
| +under the prescribed env; `GOOS=darwin` and `GOOS=windows` both build; |
| +`TestReplicaAliasServesLocallyWithoutBackhaul` runs (it is not skipped) and |
| +seeds a real home map, so the one assertion the design rests on is real. |
| + |
| +Severity: P1 must fix before merge, P2 should fix, P3 worth knowing. |
| + |
| +Counts: P1 1, P2 3, P3 11. |
| + |
| +Summary. The branch is faithful to spec revision 3 section by section, the |
| +additivity claim holds at every layer I could check (the edge request path |
| +is untouched, every new field is `omitempty`, every new column defaults |
| +empty, the edges-file and registry-row pins are unchanged), and the siblings |
| +file is written with the care §7 asks for. What is missing is the second |
| +half of the second gate: the reconcile pass and the siblings builder verify |
| +that a replica record's `home` *exists* but never that it is the same |
| +account, the same app, a home rather than a replica, or that its `Replicas` |
| +names this placement. The console door refuses all four; a record written to |
| +the bucket by anything else (a region box's IAM role can, an operator can) |
| +is minted, aliased, and paired without them. That is the one P1. Below it: |
| +the set-channel door accepts a replica and lies about the result, a home's |
| +password gate does not reach its replica's route, and wildcard claims are |
| +not derived on the replica box though §3 says "every claim". The P3s are |
| +mostly tests whose comment promises more than their assertion checks. |
| + |
| +--- |
| + |
| +## P1 |
| + |
| +### B1. The pass mints, aliases and pairs a replica of ANY host: no tenancy, role or opt-in check where the record map is in hand. |
| + |
| +Spec §2 promises that "a hand-written bucket record cannot mint a replica |
| +the console would have refused" and that the pass will "never mint on a |
| +guess". §6 lists the cross-record rails the door enforces: home in the same |
| +account and app, home's `Role == ""`, region listed in the home's |
| +`Replicas`, home steered. The door does all of that |
| +(`internal/console/apiinstances.go:807-861`). |
| + |
| +The pass checks one thing: that `all[rec.Home]` exists |
| +(`internal/provision/reconcile.go:900`). `all` is keyed by host across every |
| +account and app (`:781`). So a record |
| + |
| + apps/<attacker-sqid>/<attacker-app>/instances/r0000000000.go.tito.io.json |
| + {"role":"replica","home":"victim.go.tito.io","region":"uk",...} |
| + |
| +is minted on the uk box with `ReplicaOf = victim.go.tito.io` and the |
| +victim's channel (`:373`), the domains pass derives |
| +`victim.go.tito.io -> r0000000000.go.tito.io` with no tenancy check at all |
| +on that half (`internal/domains/aliases.go:97-115`; the custom-domain half |
| +at `:135` does inherit the claim/route tenancy test, so the victim's custom |
| +domains are safe), and the uk edge serves the victim's public host from the |
| +attacker's process. Then, on the VICTIM's home box, `buildSiblings` pairs |
| +the two (`internal/host/siblings.go:128-131` checks only `homeRec.Role != |
| +"replica"`; the account comparison is absent, and the label is derived from |
| +`homeRec.Account`, so both sides agree), and the victim's `siblings.json` |
| +now lists the attacker's instance as a replica with a dial address and a |
| +pair key. The victim's app is thereby told to grant leases to, and accept |
| +drains from, an instance its owner never opted into. |
| + |
| +Who can write that record. The region-host IAM role has `s3:PutObject` on |
| +`${deployment_bucket_arn}/*` (`infra/modules/region-host/main.tf:166-170`), |
| +so any region box can. A compromised box could already serve whatever it |
| +likes locally, so the local-alias half is not a new capability for it; the |
| +NEW capability is cross-box: making another account's home box write a |
| +siblings file that vouches for the attacker. The operator-typo case (a hand |
| +edit naming the wrong home) is the same code path with the same silent |
| +outcome, where the door would have said "no such instance in this app". |
| + |
| +Also unchecked in the pass: a home that is itself a replica (chain), and a |
| +placement the home never opted into. `buildSiblings` goes further and pairs |
| +"anyway" when the home's `Replicas` does not name the replica's placement |
| +(`siblings.go:139-145`); its comment calls that "mid-edit", but no door can |
| +produce it (the PATCH door refuses withdrawing a placement under a live |
| +replica), so the only ways to reach it are an old console stripping |
| +`replicas` or a hand-written record. |
| + |
| +What to do, both places, cheap because the map is already in hand: |
| + |
| +- `reconcile.go:900`: `home, ok := all[rec.Home]`; protect (same log line, |
| + same `continue`) unless `ok && home.Account == rec.Account && home.App == |
| + rec.App && home.Role == "" && slices.Contains(home.Replicas, rec.Region)`. |
| + Protect, not retire: a home whose `replicas` an old console stripped keeps |
| + its replica serving from the existing row, which is the fail-safe |
| + direction the spec chose for a missing home. `slices` is already imported. |
| +- `siblings.go:128`: the same four conditions before pairing, and drop the |
| + "pair it anyway" branch, or keep it but only for the opt-in half and say |
| + in the comment which writer can produce it. |
| +- A test for each: a replica record whose `home` is in another account is |
| + not minted (fresh registry, `Get` returns nil) and appears in neither |
| + side's siblings file. |
| + |
| +The steering rail can stay at the door only; a replica of an unsteered home |
| +serves custom domains and nothing else, which is not a safety question. |
| + |
| +--- |
| + |
| +## P2 |
| + |
| +### B2. The set-channel door accepts a replica, audits a move, bumps the serial, and the route never moves. |
| + |
| +Spec §2: "a replica runs its home's channel"; the create door refuses a |
| +caller-supplied channel and the pass forces the home's onto the route |
| +(`reconcile.go:373`, `:596-599`). Nothing refuses |
| +`POST /instances/{host}/channel` on a replica: `handleAPIInstanceChannel` |
| +reads the record, writes `rec.Channel = to` |
| +(`internal/console/channelmove.go:177`), audits "moved r7x2… from stable to |
| +canary" (`:187`), bumps the serial, and returns 200. The pass then converges |
| +the replica's route back to the home's channel every tick, so the route |
| +never changes while the record, the audit log and `carlos instances list` |
| +all say it did. Through the proposals path (`:224`) a quorum can approve |
| +the same nothing. The `handleAPIInstanceUpdate` PATCH does not carry |
| +`channel`, so this door is the only one. |
| + |
| +A related cosmetic consequence, worth a sentence in the spec: the replica |
| +record's `Channel` is a snapshot taken at create (`apiinstances.go:877`). |
| +After `carlos canary` on the home the replica's ROUTE follows (tested, |
| +`TestReconcileReplicaFollowsHomeChannel`) but its RECORD, and so the list |
| +and get views, keep showing the old channel. |
| + |
| +What to do: refuse in `handleAPIInstanceChannel` when `rec.Role == |
| +"replica"` with a message naming the home ("a replica runs its home's |
| +channel; move `<home>` instead"), and note the record-channel snapshot in |
| +§2 or have the list view render the home's channel for a replica. |
| + |
| +### B3. A home's password gate does not reach its replica: a gated public host is served ungated from the replica box. |
| + |
| +`routeFor` carries `Gate: rec.Gate` from each record onto its own route |
| +(`reconcile.go:365`). A replica record is created with no gate and the |
| +create door has no gate field; the gate is PATCH-only and per record. So an |
| +account whose home is behind `carlos gate` (the password-gate design) has |
| +its public host answered by the replica's edge with no gate at all, because |
| +the alias resolves to the replica's route and that route's `gate` column is |
| +empty. The buyer at the uk edge sees the mirror's checkout with no password; |
| +the buyer at the ie edge is asked for one. |
| + |
| +The spec does not mention the gate, so this is a gap between two features |
| +rather than a departure from §6. It should be decided one way: converge the |
| +home's gate onto the replica route the way the channel is converged (one |
| +more arm in `convergeOwned` reading `home.Gate`, and `rt.Gate = home.Gate` |
| +in `routeFor`), or refuse `role=replica` creates on a gated home and refuse |
| +gating a home with `Replicas`, and say which in §6. The first is the |
| +smaller change and matches the channel rule's shape. |
| + |
| +### B4. Wildcard and catch-all claims whose route is the home are not derived on the replica box; §3 says "every claim". |
| + |
| +Spec §3: "For every claim whose `Route` is the home's host, the same pass |
| +derives `claim.Host -> replica.Host` on the replica's box." The plain-alias |
| +half does that through `byReplicaOf` (`aliases.go:135`). The wildcard half |
| +does not: `rt, ok := byHost[primaryHost]; if !ok { continue }` |
| +(`aliases.go:358-361`), so a `Wildcard`/`Catchall` claim routed to the home |
| +derives neither its `wildcard_domains` row nor its `*.<host>` catch-all |
| +alias on the replica box. Buyers under such a domain still arrive at the |
| +replica's edge (the CNAME is the same) and are backhauled home rather than |
| +served locally, silently, for exactly the hosts the feature is for. |
| + |
| +The wildcard row also drives certificate issuance, which is a reason to |
| +leave it out deliberately. Either derive the catch-all alias (not the |
| +wildcard cert row) through `byReplicaOf` as the plain half does, or amend §3 |
| +to "every plain claim" and state that wildcard-claimed domains backhaul. |
| +Not a safety question; a scope statement. |
| + |
| +--- |
| + |
| +## P3 |
| + |
| +### B5. `TestReconcileProtectsReplicaWhoseHomeIsGone`'s first assertion cannot fail. |
| + |
| +`reconcile_test.go:2827` asserts the replica's channel is still "stable" |
| +after the home is deleted. Without the protect at `reconcile.go:900`, |
| +`convergeOwned` would receive a zero `home`, compute `channel = ""`, and the |
| +`channel != ""` guard (`:600`) would skip the converge anyway; the |
| +`replica_of` arm compares equal too. The second half (a fresh registry |
| +mints nothing) is the assertion that actually exercises the protect. Drop |
| +the first or reword its message so it does not claim to test the rail. |
| + |
| +### B6. `TestSiblingsPassWritesEachLayoutsDirectoryAt0600` asserts on a file nothing writes. |
| + |
| +`siblings_test.go:215` checks that `<dd>/<replica host>.env` does not exist. |
| +The siblings pass never writes a `.env`; the line is a copy from the env |
| +wiring tests. The intended assertion (the nested row's file is not ALSO |
| +written flat) is unmakeable in this fixture because the flat path is the |
| +home's file. Remove the line or restructure with two nested rows. |
| + |
| +### B7. `TestSiblingsPassRemovesAStaleFileWithoutFollowingASymlink` promises a removal it does not check. |
| + |
| +The comment at `siblings_test.go:283` says "the pass removes it again"; the |
| +code leaves a planted symlink in place by design (`siblings.go:371-377`), |
| +and the test asserts only that the canary survived. The test is right; the |
| +comment is wrong. Fix the comment, and consider asserting the symlink is |
| +still there so the "leave it" decision is pinned rather than accidental. |
| + |
| +### B8. The flat-collision refusal logs every pass; §7 says "logs once". |
| + |
| +`siblings.go:258` is inside the per-directory loop with no once-guard, so a |
| +box with two flat replica-bearing hosts logs the same line every five |
| +minutes forever. Harmless. Either a `sync.Map` keyed on the directory (the |
| +`withheldAliasSaid` shape in `edgesfile.go`) or amend §7. |
| + |
| +### B9. `siblingsFileName` is defined twice. |
| + |
| +`internal/host/siblings.go:31` and `internal/activator/backend_exec.go:1017` |
| +both say `"siblings.json"`. Plan correction A23 chose the stat-in-activator |
| +shape to avoid a cross-package PATH duplication and produced a NAME |
| +duplication instead. Export one (`activator.SiblingsFileName`; host already |
| +imports activator) so the two cannot drift. |
| + |
| +### B10. The rotation memory is per process. |
| + |
| +`siblingsWriter.last` lives on the loop's struct. An agent restart during |
| +the one-pass window after a promotion loses the retired derivative, and the |
| +file is rewritten with the new one only, reopening the window B-11 closed |
| +for that pass. Spec §7's "no restart" is about not needing one; it does |
| +not claim to survive one. Say so in §7, or persist `last` beside the file. |
| + |
| +### B11. When the fleet's last record goes, no stale siblings file is ever removed. |
| + |
| +`siblings.go:203` returns early on an empty record map, deliberately, so a |
| +bucket blip does not wipe every file. The cost is that a deployment whose |
| +last instance record is deleted keeps every `siblings.json` forever. Real |
| +deployments never reach zero records; note it in the comment. |
| + |
| +### B12. The CLI silently drops `--host` and `--channel` with `--role replica`; the docs say they are refused. |
| + |
| +`docs/site/cli.md:594`: "`--host` and `--channel` are refused alongside |
| +`--role replica`". `runInstancesCreate` checks `--host` only for a |
| +non-replica (`instances_cli.go:333`) and `instanceCreateReplica` sends |
| +neither flag (`:351`), so `--channel canary --role replica` produces a |
| +replica on the home's channel and prints nothing about it. The console |
| +would have refused both. Refuse them in `validateReplicaCreateArgs` |
| +(`:294`) so the CLI matches its own documentation. |
| + |
| +### B13. The delete door is no longer read-free for anyone. |
| + |
| +`handleAPIInstanceDelete` now does a `GetRecord` and, for every home, a |
| +`ListRecords`, and returns 502 on either failing |
| +(`apiinstances.go:1560-1590`). That is spec B-13's fail-closed ruling and is |
| +right, but it is a behaviour change for every account, not only ones with |
| +replicas: the door's own comment above it ("deletion has to outlive every |
| +version of it") argued the opposite for the manifest. Stated so the |
| +additivity claim is read as "byte-identical output", which holds, and not |
| +"identical behaviour at every door", which does not. |
| + |
| +### B14. Two comments cite the wrong revision, and the token comment was not added. |
| + |
| +`edgesfile.go:251` says custom domains came in "#162 revision 2"; the spec |
| +says revision 3. Plan Task 7 lists the token door as "comment only"; no |
| +comment was added at `handleAPIInstanceToken`. Neither matters to |
| +behaviour. |
| + |
| +### B15. The old-console rollout rule is in the spec but I could not see it in a handoff. |
| + |
| +Spec §6 makes "no replica record for an app until every console that can |
| +PATCH that app's records carries the fields" a rollout rule "in the branch |
| +handoff". The tip commit message does not carry it and there is no handoff |
| +file in the worktree; if the rule lives in `amadan branch handoff` it should |
| +be confirmed there before the roll, because Tito's console is pinned on an |
| +older build and a PATCH from it strips `role`/`home`/`replicas`. |
| + |
| +--- |
| + |
| +## Verified and sound, by spec section |
| + |
| +- **§2 record and rails.** All three fields `omitempty` |
| + (`provision.go:156-169`), `ValidRole` closed, `ValidReplicas` trims, |
| + refuses empty, dedupes, sorts, caps at 4; every §2 rail is in |
| + `ValidateShape` (`:467-501`) and each has a failing case in |
| + `TestValidateShapeReplicaRails`. `Role` and `Home` in |
| + `protectedFieldsEqual`, `Replicas` not (A12 honoured). |
| +- **§2 channel rule.** `all` filled after the shape and channel guards and |
| + before the scope check (`reconcile.go:781`, A6 honoured); both `routeFor` |
| + and `convergeOwned` read `home.Channel` for a replica; the protect at |
| + `:900` fires before `trashedFor`/`seedFor` (A4 honoured). |
| +- **§3 alias derivation.** Account host via `ReplicaOf` with route-beats- |
| + alias, custom domains via `byReplicaOf` inheriting the tenancy check; |
| + survives a second pass; swept when the route goes. Four tests, all real. |
| +- **§4 edges-file mark.** `byHost[a.Host].ReplicaOf != ""` (correction 1 |
| + honoured), published without a stamp, `role: "replica"`; the replica's |
| + own host stays `home`. `foldHomes` has the explicit case. The fold test |
| + and the e2e no-backhaul test both run and both would fail if the fold |
| + handed the replica the dial. |
| +- **§5/§5a.** Steering rail both ways at the PATCH door and in |
| + `ValidateShape`; nothing steers a pooled app (not reopened). |
| +- **§6 doors.** Every listed refusal is built and each has a test that |
| + isolates its rail (`replicaManifest()` with `Pool ""` and `de` allowed |
| + but not opted in, A13 honoured). Host minted before `ValidHost` (A10), |
| + CAS retry re-mints host and slug and re-runs the shape check (A17), |
| + `homeRegionOut` on both sides (A18), delete fails closed (A19), move |
| + rails in `moveGate` and the request handler (A16), placed app refused. |
| +- **§7 siblings file.** Per-pair HMAC-SHA256 under the mesh key with a |
| + label the hop-proof MAC cannot collide with (the hop message is |
| + `host + "\n" + ts`, `mesh.go:272-275`; the label has no newline). Written |
| + per directory, 0600, through `WriteAtomic`; the read and the removal go |
| + through `openat2` with `RESOLVE_NO_SYMLINKS` and `O_NONBLOCK` |
| + (`instancefs_linux.go:72`), so a planted symlink is refused and a planted |
| + FIFO cannot wedge the loop. Flat-layout collision refused per directory. |
| + Retired derivative republished for exactly one pass, memory bounded per |
| + pair. `CARLOS_SIBLINGS_FILE` set from an `Lstat` regular-file test and |
| + stripped otherwise, on both spawn paths. Edge accessors are nil-receiver |
| + safe (`homemap.go:626`, `:704`). `OnRecords` is set in |
| + `provisionTickConfig` so both reconcile legs carry it, and the callback |
| + copies under `reconcileMu`. |
| +- **§12 CLI.** `--role`, `--home`, `set-replicas` (nil-vs-empty split |
| + pinned), ROLE column, docs. |
| +- **Additivity.** `internal/edge/edge.go` is not in the diff; the request |
| + path takes no new branch. `ReplicaOf` is out of `Fingerprint()` and the |
| + alias row it produces is in it. `TestEdgesFileRawWireBodyMatchesSchema` |
| + and `TestReconcileMints` are untouched by the diff. `ReconcileAliases` |
| + adds one loop with no I/O unless a `ReplicaOf` row exists. |
| +- **Concurrency seams.** No pass other than the mint does a full-row |
| + `Upsert` on an instance route (`sidecar_reconcile.go:395` and |
| + `domains/reconcile.go:274,337` are other tables' rows), so A3's |
| + `replica_of` clearing is reachable only by `carlos route add`. `routeFor` |
| + and `convergeOwned` have no callers outside `reconcile.go`; all seven |
| + test call sites updated. `lastRecords` and `siblingSource` are atomics. |
diff --git a/docs/superpowers/specs/2026-09-21-replica-placements-design.md b/docs/superpowers/specs/2026-09-21-replica-placements-design.md| index b2b2d7ed7..7dda807c1 100644 |
| --- a/docs/superpowers/specs/2026-09-21-replica-placements-design.md |
| +++ b/docs/superpowers/specs/2026-09-21-replica-placements-design.md |
| @@ -328,7 +328,12 @@ enforce them and no reconcile pass will **(fable 4)**. |
| decides what it means **(fable B-5)**. |
| The channel is taken from the home record (§2) and a caller-supplied |
| -channel is refused. |
| +channel is refused. Note the record's `Channel` is a **snapshot taken at |
| +create**: after a `carlos canary` on the home the replica's ROUTE follows |
| +(the pass converges it every tick) but its RECORD, and so the list and get |
| +views, go on showing the channel it was created with. The channel door |
| +refuses a replica outright rather than letting someone "fix" that by hand, |
| +because the route would converge straight back. |
| **The host is minted by the platform** and a caller-supplied `host` is |
| refused on a `role=replica` create **(fable B-8)**. The name is one label |
| @@ -490,6 +495,27 @@ a lease call with it, which that pair's sibling would accept. So: |
| **No mesh key, no file.** A file whose `keys` list is empty reads as |
| "armed but unsignable"; the pass writes nothing until the edge has a key. |
| +**Two limits of the rotation memory**, found in the branch review: |
| + |
| +- It is **per process**. The retired derivative lives on the writer's |
| + struct, so an agent restart inside the one-pass window after a promotion |
| + loses it and the file is rewritten with the new key alone. §7's "no |
| + restart" means a rotation does not *require* one; it does not claim to |
| + survive one. A restart in that window costs the pairs one failed call |
| + each until both sides settle. |
| +- **A pass that learned nothing writes nothing.** An empty record map is |
| + treated as "the bucket did not answer", never as "every record is |
| + gone" — otherwise one blip would wipe every key file. The cost is that a |
| + deployment whose last instance record is deleted keeps its siblings |
| + files forever. No real deployment reaches zero records. |
| + |
| +**The delete door is no longer read-free.** It now reads the record and, |
| +for a home, lists the app's records, refusing on either failing (§6). That |
| +is a behaviour change at that door for **every** account, not only ones |
| +with replicas. The additivity claim in §1 is about byte-identical output |
| +for an account with no replicas, which holds; it was never a claim that |
| +every door behaves identically. |
| + |
| **The keys** are per *pair*: `HMAC(meshKey, "replica/" + account + "/" + |
| homeHost + "/" + replicaHost)` **(fable 5, adapted)**. Round 1 proposed |
| per-instance keys the home derives itself; the home cannot, because it must |