carlos / movement Public

Review 3: the environment is fixed at exec, and other corrections

The third pass cleared everything but one blocker and two corrections.

The blocker: "a release build refuses while placeholders remain" was
unbuildable. There is one build, and the canary serves the same binary
production would, so refusing at build time would have made plan 1
impossible to review on a canary as the cadence requires. The check moved
to boot, against the origin — refuse on the canonical host, banner on a
canary.

And a mechanism that does not exist: a process's environment is fixed at
exec, so the middleware could not re-read MOVEMENT_ADMINS. What actually
makes removal fast is carlos env convergence bouncing the instance. Gate
10 now models a restart; the test it would have grown otherwise would have
passed while proving nothing.

The spec is ready to implement against.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0175aLTHqLrYkGe1skMV1rvq

Paul Campbell 1 month ago b90d500ab87243da03bd69e31fe82084acb36e65 parent d9e1383

2 files changed, +104 −38

AGENTS.md +11 −6
docs/superpowers/specs/2026-09-01-movement-design.md +93 −32

diff --git a/AGENTS.md b/AGENTS.md

index 84117c7..55f0a8e 100644

--- a/AGENTS.md

+++ b/AGENTS.md

@@ -93,10 +93,13 @@ emails. This app *is* its copy — the words are the mechanism that sorts people

into buckets, not decoration on top of one.

Templates carry **no literal user-facing text**. Every string resolves through

-`internal/copy`, keyed by ID, and unreviewed entries are marked as

-placeholders that a release build refuses to ship. That is what makes it

-possible to build the whole engine before the words exist, without ever

-putting unreviewed copy on a canary.

+`internal/copy`, keyed by ID, and unreviewed entries are marked placeholders.

+With any placeholder remaining the app **refuses to boot on the canonical

+origin**, and boots behind a banner on a canary or loopback. There is only one

+build — the canary serves the same binary production would — so the check is at

+boot against the origin, not at build time. That is what lets the whole engine

+be built and canary-reviewed before the words exist, while keeping unreviewed

+copy off production.

---

@@ -156,8 +159,10 @@ putting unreviewed copy on a canary.

`/signin` included. Fail closed, and indistinguishable from an app with no

admin at all.

- `Authorize` runs once, when a magic link is redeemed. Re-check the allowlist

- on **every** admin request, reading the environment rather than a boot-time

- slice, or a removed admin keeps access until something cycles the instance.

+ on **every** admin request, or a removed admin keeps access for the life of

+ their session. The list itself is read at boot — a process's environment is

+ fixed at exec, so nothing re-reads it; `carlos env set` bounces the instance

+ within seconds, and the per-request check is what then kills the session.

- **Never mount `auth.Begin` directly at `POST /signin`.** It sends the mail

before `Authorize` ever runs, which makes this app a mailer anyone can point

at any address from our verified identity. Check the allowlist first and

diff --git a/docs/superpowers/specs/2026-09-01-movement-design.md b/docs/superpowers/specs/2026-09-01-movement-design.md

index e60dbaa..d00bd35 100644

--- a/docs/superpowers/specs/2026-09-01-movement-design.md

+++ b/docs/superpowers/specs/2026-09-01-movement-design.md

@@ -1,15 +1,20 @@

# 🤖 Movement — design

-**Status:** design, revised twice on 2026-09-01. Approved in brainstorming by

-Paul on 2026-09-01; revised the same day against two review passes dispatched

-to **Fable** (22 findings, then 14 against the revision — the second pass

-returned "not ready", and rulings 13, 14, 19 and 20 changed mechanism as a

-result). Every ruling is dated and attributed, with the rejected alternative

+**Status:** design, revised three times on 2026-09-01, and ready to implement

+against. Approved in brainstorming by Paul on 2026-09-01; revised the same day

+against three review passes dispatched to **Fable** — 22 findings, then 14

+against the first revision ("not ready"; rulings 13, 14, 19 and 20 changed

+mechanism), then one blocker and two corrections against the second, all

+applied. Every ruling is dated and attributed, with the rejected alternative

beside it.

-**Reviewing model, per §11:** dispatched to Fable, 2026-09-01, twice.

+**Reviewing model, per §11:** dispatched to Fable, 2026-09-01, three times.

*(Recorded as what was dispatched, not what an agent self-reported — models

-are unreliable about their own identity.)* Rulings 12–26 postdate Paul's

-approval and carry only the review passes, not his.

+are unreliable about their own identity.)*

+**Rulings 12–26 postdate Paul's approval** and carry only the review passes,

+not his. Several reverse or materially change what he approved — most of all

+ruling 12 (the instance key moved out of the database and into the

+environment) and ruling 15 (confirmation moved from GET to POST). They need his

+eyes before plan 2 puts anything in front of a visitor.

**Scope:** the Movement recruitment app: a public signup surface, a confirmed

email list, and an admin view over it. It does not define a mailing-list

sender; §13 names the trigger for that.

@@ -75,7 +80,7 @@ enumerate who did.

| 23 | **`POST /signin` is app-owned: it checks `MOVEMENT_ADMINS` before delegating to `auth.Begin`, and answers the same redirect either way** | Mounting `auth.Begin` directly, which sends a magic link *before* `Authorize` ever runs — making the app a mailer for any address on the internet, from our verified identity. `sheets` hit this in production | This session, 2026-09-01 |

| 24 | **The send happens after the write transaction commits, in a goroutine bounded by a deadline** | A send inside the transaction. `mail.Send` discards its context and `smtp.SendMail` dials with no deadline, so a hung SES connection would hold the single writer and stall every other signup and the tick | This session, 2026-09-01 |

| 25 | **The `mail.Logged` fallback is detected by checking `CARLOS_SMTP_HOST`/`CARLOS_SMTP_FROM` before calling `FromEnv`, and `Mailer` is always passed explicitly** | Detecting the fallback by type — `loggedSender` is unexported, so it cannot be. `auth.New` also substitutes `Logged` for a nil `Mailer`, which is the same leak by another door | This session, 2026-09-01 |

-| 26 | **Every user-facing string is drawn from an `internal/copy` table keyed by ID; templates contain no literal copy** | Placeholder text written into templates and cleaned up later — which AGENTS.md forbids, and which would put unreviewed copy on a public canary during plan 1's branch review | This session, 2026-09-01 |

+| 26 | **Every user-facing string is drawn from an `internal/copy` table keyed by ID; templates contain no literal copy. Unreviewed entries are marked placeholders, and the app refuses to boot with any placeholder remaining when `MOVEMENT_ORIGIN` is the canonical host — canaries and loopback boot, showing a banner** | Placeholder text written into templates and cleaned up later (AGENTS.md forbids it); **and** "a release build refuses while placeholders remain", which was unbuildable — there is one build, `carlos ship` publishes it, and the canary serves it, so refusing at build time would make plan 1 impossible to review on a canary as the cadence requires | This session, 2026-09-01, extended after the third review |

| 21 | **`/admin` lists confirmed rows; pending appears only as a count** | Listing pending rows — an address nobody has proven owns it, joined to a stance, which is exactly the pair §7 protects | This session, 2026-09-01 |

| 22 | **Pinned: Go 1.25, rastrillo v0.23.0; the app is scaffolded with `rastrillo new movement`** | Hand-writing the app shape, which redoes the scaffold's work and skips its pin test, CI files and browser harness | This session, 2026-09-01 |

@@ -181,7 +186,8 @@ passed explicitly.

`TokenExpiry` through an explicit `.Select` allowlist that **excludes the

budget columns and `CreatedAt`**, so an attacker cannot reset a budget by

re-submitting;

- - **confirmed** → no write and no send at all.

+ - **confirmed** → no write to the row and no send at all. The nonce is

+ still spent, as it is for every accepted submission.

6. After the transaction commits (ruling 24), and subject to the budgets in

§9, the confirmation mail is sent.

7. The response is the same "check your inbox" page in every case — including

@@ -382,14 +388,45 @@ excludes the budget columns from its `.Select` allowlist (§4 step 5), and the

sweep age is longer than the per-address window (§10) — otherwise an attacker

re-submits, or simply waits for the sweep, and the counter starts again.

+**The counter is bumped in one statement** —

+`INSERT … ON CONFLICT(hour_bucket) DO UPDATE SET count = count + 1` — inside

+the signup transaction. A read-then-write across two statements is a lost

+update waiting for concurrency, even behind a single writer. Fixed hour buckets

+let a burst straddle a boundary and send up to twice the ceiling in one rolling

+hour; at these numbers that is acceptable and it is written down rather than

+discovered.

**Admin sign-in mail draws on the same global budget** (ruling 23). It is sent

by `rastrillo/auth` rather than by our own handler, so the `mail.Sender` handed

to `auth.New` is wrapped to pass through the counter. A budget that counts only

-the mail we remember sending is not a budget.

+the mail we remember sending is not a budget. Three things the wrapper must

+get right, because `auth.Begin` calls `Send` synchronously and with no deadline

+of its own:

+- commit the count **before** attempting the send;

+- apply its own deadline, since nothing upstream will;

+- **return `nil` when it suppresses for budget.** Returning an error makes

+ `Begin` redirect to `?err=` instead of `?sent=1` and log the failure with the

+ address attached — a different response, which is the thing ruling 23 exists

+ to avoid.

+One residual, accepted: signin's own per-address limiter can answer `?err=rate`,

+and only an allowlisted address ever reaches it. That is a narrow oracle for

+admin membership, not list membership, so it sits outside the one rule — but

+"the same redirect either way" is true of our check, not of every path beneath

+it.

+**The budget counts attempts, not successes.** Three failed sends during SES's

+post-`enable` propagation window will lock an honest address out for 24 hours,

+with re-submission (§10) unavailable until it clears. Conservative on purpose,

+and stated so it is a choice rather than a surprise.

The in-memory per-address limiter from `password/limit.go`'s shape stays as a

-cheap first filter, but §5 and this section both state that it is not the

-defence.

+cheap filter, but §5 and this section both state that it is not the defence.

+**It is checked after proof of work, not before.** Keyed by address and

+consulted first, it would let a stranger trip a victim's budget for free and

+lock them out of signing up; behind the solve, tripping someone else's costs

+real work.

---

@@ -415,6 +452,12 @@ the scheduled tick. The send therefore happens **after commit**, in a

goroutine with its own deadline. A sender that never returns must not be able

to block a second submission, and that is a test.

+That deadline must be **well under the platform's idle window** — seconds

+against minutes — because a goroutine outliving the response is exactly the

+shape §11a rejects for the tick. It is safe here only because it is short. A

+`SIGTERM` landing mid-send loses that one mail after its budget bump; §10's

+re-submit path is the recovery, and it is not worth more machinery than that.

**One malformed-address exception.** A syntactically invalid address is

rejected visibly, with an error naming the field. That leaks nothing: it is a

property of the string the visitor just typed, not of any address's state on

@@ -477,12 +520,17 @@ step-up mechanism for sensitive routes layered on a longer session, not a way

to shorten one. A surface listing named people's political positions does not

want a month-long cookie.

-Removal takes effect when the middleware next runs against a reloaded

-allowlist. `MOVEMENT_ADMINS` is read at boot, and platform config converges in

-seconds but does not restart the process, so **the middleware re-reads the

-environment** rather than closing over a boot-time slice. Otherwise "loses

-access on the next request" is only true after something happens to cycle the

-instance.

+**How removal actually takes effect.** `MOVEMENT_ADMINS` is read once at boot.

+A process's environment is fixed at exec — re-reading `os.Getenv` per request

+sees nothing new, and an earlier draft of this section claimed otherwise. What

+makes removal fast is the platform: `carlos env set` convergence re-renders and

+**bounces** socket-exec instances within a few seconds, and hibernation

+respawns the process on the next wake anyway. The per-request allowlist check

+is what then invalidates an existing session, rather than leaving it live until

+its 8 hours are up.

+Gate 10 must therefore model a restart. A test that calls `t.Setenv` mid-run

+would pass while proving nothing, which is worse than no test.

`/admin` lists **confirmed rows only**, newest first, with a count of pending

(ruling 21). A pending row is an address nobody has proven owns it joined to a

@@ -510,7 +558,11 @@ instance hibernating mid-job — and does three things:

1. Deletes unconfirmed rows older than **30 days**. Longer than the 24-hour

per-address mail budget on purpose (§9): a sweep shorter than the budget

- window is a way to reset the budget by waiting.

+ window is a way to reset the budget by waiting. The cost is that a pending

+ row outlives its own 24-hour token by 29 days, purely to hold a counter —

+ junk rows and Litestream replica size are the price of ruling 20. If that

+ ever matters, moving the per-address budget to its own table keyed by an

+ address hash lets this drop back to about 48 hours.

2. Deletes `spent_nonces` past `ExpiresAt`.

3. Calls `auth.Sweep(now)` for expired magic links and sessions, which nothing

else in the app does.

@@ -519,9 +571,10 @@ All three are idempotent, so at-least-once delivery needs no dedupe and

`carlos.TickOccurrence` is not used. Confirmed rows are never touched, and

neither is a pending row with a live token.

-A tick that 403s is not silent — `carlos schedule ls` records "404 (app

-refused)" — but nothing alerts on it, and the platform reads 4xx as "do not

-retry". Gate 11 stands in front of that.

+A tick that 403s records `app refused` in the schedule's last status, but

+nothing alerts on it and `next` still advances, so `carlos schedule ls` reads

+much like success at a glance. The platform treats 4xx as "do not retry". Gate

+11 stands in front of that.

---

@@ -563,8 +616,10 @@ manual step before each deploy.

unknown one.

9. **Admin fails closed.** With `MOVEMENT_ADMINS` empty, all seven admin

routes answer 404 — iterated, not spot-checked.

-10. **A removed admin loses access on the next request**, not on session

- expiry.

+10. **A removed admin loses access on the next request after the instance

+ cycles**, not on session expiry. The test restarts the app with a changed

+ allowlist and re-uses the old session cookie — `t.Setenv` mid-run would

+ pass without proving anything, since the environment is fixed at exec.

11. **The tick reaches the sweep.** A POST with no browser headers and a valid

bearer must not 403 — the CSRF trap in ruling 18, which fails silently and

permanently.

@@ -575,7 +630,8 @@ manual step before each deploy.

14. **Logging containment.** No `slog` import outside the logging package, and

a failing sender's address never reaches captured output.

14a. **Templates carry no literal user-facing text** — every string resolves

- through `internal/copy`, and no placeholder survives into a release build.

+ through `internal/copy`. With a placeholder remaining, the app refuses to

+ boot on the canonical origin and boots with a banner on a canary.

15. **Difficulty is calibrated**, p95 and p99 recorded on real hardware.

---

@@ -602,12 +658,17 @@ Two plans, split at the copy gate.

**Plan 1 is copy-independent only because of ruling 26.** Every user-facing

string resolves through an `internal/copy` table keyed by ID, so plan 1 fills

-that table with marked placeholders and never writes copy into a template. A

-release build refuses while any placeholder remains, and routes are mounted by

-internal stance key until copy review settles the slugs. Without that table,

-"placeholder copy throughout" would mean unreviewed words in templates — which

-AGENTS.md forbids, and which the shipping cadence would then put on a public

-canary during plan 1's own branch review.

+that table with marked placeholders and never writes copy into a template.

+Routes are mounted by internal stance key until copy review settles the slugs.

+**Where the placeholder check runs matters**, because there is only one build:

+`make release` produces it, `carlos ship` publishes it, and the canary serves

+that same binary. A build-time refusal would make plan 1 impossible to review

+on a canary, which the shipping cadence requires. So the check is at **boot,

+against the origin**: with placeholders remaining, the app refuses to start

+when `MOVEMENT_ORIGIN` is the canonical host, and starts with a visible banner

+on a canary or loopback origin. Unreviewed words can be reviewed; they cannot

+reach production.

**Plan 1 — the engine.** Every gate in §12 except 15 green at its end.