carlos / platform Public

fix wave: spec corrections + stale comments + pinnable domains ceiling

Whole-branch review's tidy-up commit before PR, applying eight items
against docs/superpowers/specs/2026-08-18-service-principals-design.md
and the code:

- Spec §5: correct the legacy app-token reach to the true nine doors
  (five instance verbs, three domains, one logs) instead of the ten
  that wrongly included instances-manifest, and explain the
  registrations-vs-gates derivation error that put it there.
- Spec §4: domains attach/detach is exempt from the instance/write
  budgets for every principal kind, not just service — domainsGate
  never branches on Kind.
- Spec §2: drop the AccountSqid field that doesn't exist on Principal
  (principalKey keys on the numeric account id); add the fourth ALTER
  column, rotated_at, which landed after the other three.
- internal/console/apicli.go: cliRoute, not apiPrincipalAccount, is
  principalFrom's context-key writer; apiPrincipalAccount is a reader
  that 404s when the value is absent.
- internal/console/apidomains.go: attach/detach run through
  domainsGate now, with the convergence-ceiling budget, not
  instanceGate's workload budget verbatim.
- internal/console/svcroutes_test.go: fix the closed-door count
  message (12 -> 13, naming the routes door the rebase added).
- internal/console/authcli.go, apidomains_test.go: raise
  domainsCeilingBurst to 180 so it sits clear of svcWriteBurst (120),
  and derive TestWildcardEnableIsNotRateLimited's attempt count from
  both budgets plus the ceiling so a re-added write budget on
  domainsGate would fail the test instead of hiding behind an
  equal threshold.
- services.go: note beside the door list that store grant also needs
  CARLOS_STORE_OPERATORS membership, since reaching the door isn't
  sufficient on its own.

go build/vet/test -count=1 clean across all 27 packages; gofmt -l
clean for touched files; GOOS=darwin build clean.

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

Paul Campbell pushed by paul@keymail.dev 2 months ago e20ab77bd4df3986a20b0aebfb149668a9afa5d6 parent 2f9c7ab

7 files changed, +104 −43

docs/superpowers/specs/2026-08-18-service-principals-design.md +41 −12
internal/console/apicli.go +6 −4
internal/console/apidomains.go +16 −8
internal/console/apidomains_test.go +21 −12
internal/console/authcli.go +10 −6
internal/console/svcroutes_test.go +1 −1
services.go +9 −0

diff --git a/docs/superpowers/specs/2026-08-18-service-principals-design.md b/docs/superpowers/specs/2026-08-18-service-principals-design.md

index d4c7fa2d6..f2f0a7315 100644

--- a/docs/superpowers/specs/2026-08-18-service-principals-design.md

+++ b/docs/superpowers/specs/2026-08-18-service-principals-design.md

@@ -72,11 +72,17 @@ what every existing row already means — the idiom the `kind` columns used:

ALTER TABLE cli_tokens ADD COLUMN role TEXT NOT NULL DEFAULT '';

ALTER TABLE cli_tokens ADD COLUMN name TEXT NOT NULL DEFAULT '';

ALTER TABLE cli_tokens ADD COLUMN revoked_at INTEGER NOT NULL DEFAULT 0;

+ALTER TABLE cli_tokens ADD COLUMN rotated_at INTEGER NOT NULL DEFAULT 0;

```

`name` is the stable handle in `service:<sqid>/<name>`; it survives rotation,

which is what makes rate-limit keys and audit rows continuous across a secret

change. `revoked_at` lets rotate keep the name while retiring the secret.

+`rotated_at` landed later than the other three, once rotate needed to answer

+"how stale is the LIVE secret" separately from "how old is this credential":

+`created_at` keeps meaning when the credential was first granted, and

+`rotated_at` means when its current secret began (stamped equal to

+`created_at` at mint, advanced on every later rotation).

Three kinds, one resolver:

@@ -94,16 +100,19 @@ A new `Store.CLIPrincipal(raw, now)` returns a typed value:

```go

type Principal struct {

- Kind PrincipalKind // user | service

- Address string // person: the caller. service: minted_by, a fact only

- AccountID int64

- AccountSqid string

- App string // "" for account-scoped

- Role string // "" for a person

- Name string // "" for a person

+ Kind PrincipalKind // user | service

+ Address string // person: the caller. service: minted_by, a fact only

+ AccountID int64

+ App string // "" for account-scoped

+ Role string // "" for a person

+ Name string // "" for a person

}

```

+No `AccountSqid` field: `principalKey` (§4) keys on the numeric account id

+directly rather than resolving it to the sqid first — the string form is

+never shown to anyone, so there was nothing for a field to buy.

`CLIAppToken` becomes a thin wrapper over `CLIPrincipal` for the transition and

is then deleted.

@@ -267,7 +276,10 @@ func principalKey(p Principal) string {

if p.Kind == PrincipalUser {

return p.Address // unchanged for people, byte for byte

}

- return "service:" + p.AccountSqid + "/" + p.Name

+ if p.Name == "" {

+ return "app:" + strconv.FormatInt(p.AccountID, 10) + "/" + p.App

+ }

+ return "service:" + strconv.FormatInt(p.AccountID, 10) + "/" + p.Name

}

```

@@ -281,7 +293,14 @@ limits move.

| write | 30 / min | 120 / min |

| ship | 10 / hour (per token) | 60 / hour |

| instance | 60 / min | 60 / min (unchanged) |

-| domains attach/detach | instance budget | exempt (§4a) |

+| domains attach/detach | exempt (§4a) | exempt (§4a) |

+The domains row is exempt for every principal, person and service alike:

+`domainsGate` drops the instance budget and applies the convergence ceiling

+(§4a) keyed on `principalKey` without ever branching on kind. The exemption

+never depended on person-vs-service — the argument was always about the

+door (a hostname attach is not the per-repo call the instance budget was

+sized for) rather than about who was calling it.

The service numbers are a judgement calibrated on "100 repos enabling Pages in

a day" and "a push every 30 seconds sustained", not a derivation.

@@ -331,11 +350,21 @@ call sites rather than the name shows an app token today reaches more than the

instance verbs:

```

-instances manifest · list · create · get · update · delete apiinstances.go

-domains list · attach · detach apidomains.go:154, 391, 473

-logs read apilogs.go:48

+instances list · create · get · update · delete apiinstances.go

+domains list · attach · detach apidomains.go:154, 391, 473

+logs read apilogs.go:48

```

+Nine doors, not ten: `PUT .../instances-manifest` is NOT in the legacy reach.

+The first draft of this list was built from the route registrations filed

+under "instances" rather than from each handler's actual gate, and it

+wrongly included the enable door on that basis. `handleAPIInstanceEnable`

+has always gated on `apiAccountApp` directly, which refuses a non-person at

+`cliUser` with a 401 — no app token has ever reached it. It is registered

+`svcrole.Set{}` and stays closed; reopening it to match a binding-spec table

+that once (wrongly) listed it would hand every app token a door to rewrite

+its own app's manifest.

titogo's router uses its `CARLOS_INSTANCE_TOKEN` for the domains half — it

re-attaches every hostname on every reload pass. A tidy `instance` role meaning

"the instance verbs" would 403 that router on the next console deploy.

diff --git a/internal/console/apicli.go b/internal/console/apicli.go

index 3d4719ae0..64afe9ab2 100644

--- a/internal/console/apicli.go

+++ b/internal/console/apicli.go

@@ -572,10 +572,12 @@ func (s *Server) handleAPIPromote(w http.ResponseWriter, r *http.Request) {

}

// ok discarded here, and at every other write/create/ship/domains call

// site in this package that reads a principal back off the context:

- // apiAccountApp/apiAccount/apiFleet above already called

- // apiPrincipalAccount, principalFrom's only writer, so ok is already

- // true by the time this line runs — cliRoute stashed the principal on

- // the context before the handler ever started (Task 5 review, Risk A).

+ // apiAccountApp/apiAccount/apiFleet above already ran, and cliRoute —

+ // the ONLY writer of the context key — stashed the principal on the

+ // context before the handler ever started, so ok is already true by

+ // the time this line runs (Task 5 review, Risk A). apiPrincipalAccount

+ // is a reader, not the writer: it 404s if the key is ever absent,

+ // which cannot happen for a door that went through cliRoute.

// If that ordering were ever broken, the zero Principal reads as

// PrincipalUser with Address "", which keys the PERSON limiter on the

// empty string for every caller alike — turning, say, the create

diff --git a/internal/console/apidomains.go b/internal/console/apidomains.go

index df0236c1e..7d06eedcf 100644

--- a/internal/console/apidomains.go

+++ b/internal/console/apidomains.go

@@ -5,14 +5,22 @@

// the whole authorization; the agent's domains pass turns it into an alias

// row, on the box that actually owns the route.

-// The gate is the instance family's, verbatim (instanceGate, apiinstances.go):

-// the SAME two bearer shapes reach it — a member's user token, or an APP

-// token bound to one account and one app at mint time — and every refusal

-// before that proof is the same byte-identical 404. That reuse is deliberate:

-// titogo's router calls this door with its CARLOS_INSTANCE_TOKEN, the very

-// credential it already holds for instances, and a second hand-rolled gate

-// here would be a second place for the ordering (principal, budget,

-// readiness) to drift.

+// Attach and detach go through domainsGate, below — the instance family's

+// principal, binding and readiness checks, but with the instance-verb

+// WORKLOAD budget swapped for a CONVERGENCE-RATE ceiling (domainsCeilingBurst,

+// spec §4a): a Pages hub with wildcard DNS makes no per-repo hostname call at

+// all, so gating attach/detach on the per-minute instance budget would uplift

+// a call that isn't the workload that budget was sized for. The exemption

+// applies to every principal, person and service alike — it was never about

+// who is calling, only about what the door is. A debugging session chasing a

+// 429 here wants domainsCeilingBurst, not cliInstanceBurst.

+//

+// The rest of the gate IS the instance family's, verbatim: the same bearer

+// shapes reach it, and every refusal before the ceiling is the same

+// byte-identical 404. That reuse is deliberate: titogo's router calls this

+// door with its CARLOS_INSTANCE_TOKEN, the very credential it already holds

+// for instances, and a second hand-rolled gate here would be a second place

+// for the ordering (principal, budget, readiness) to drift.

// Two neutralities this file exists to keep:

// - a hostname another account holds answers 409 "hostname unavailable" and

diff --git a/internal/console/apidomains_test.go b/internal/console/apidomains_test.go

index 602bb51d2..df4fb58b9 100644

--- a/internal/console/apidomains_test.go

+++ b/internal/console/apidomains_test.go

@@ -1098,20 +1098,29 @@ func TestDelegationZoneInvalidAtConstruction(t *testing.T) {

// every repo's subdomain serves off the catchall. So the domains doors are

// EXEMPT from the write/instance WORKLOAD budgets rather than uplifted.

-// attempts is deliberately ABOVE cliInstanceBurst (the workload budget this

-// door must be exempt from) and BELOW domainsCeilingBurst (the convergence-

-// rate ceiling that still applies, see TestDomainsCeilingBoundsConvergence

-// below): a pass here means the instance budget is genuinely gone, not that

-// the loop merely stayed under it. Derived from both constants, not a

-// literal, so the claim cannot quietly stop being true if either one moves

-// — the assertion just below catches that instead of the test silently

-// starting to prove nothing (Task 5 review, round 3).

+// attempts is deliberately ABOVE both cliInstanceBurst (the instance workload

+// budget this door must be exempt from) and svcWriteBurst (the write budget

+// it must equally be exempt from) and BELOW domainsCeilingBurst (the

+// convergence-rate ceiling that still applies, see

+// TestDomainsCeilingBoundsConvergenceRate below): a pass here means BOTH

+// workload budgets are genuinely gone, not that the loop merely stayed under

+// one of them. domainsCeilingBurst is kept deliberately clear of

+// svcWriteBurst (see its comment, authcli.go) so that a volume satisfying

+// this inequality can exist at all — when the two constants were equal, no

+// attempts count could sit strictly between them, and a regression that put

+// the write budget back onto this door would have refused at the same

+// threshold this ceiling does, so this test would have kept passing anyway.

+// Derived from all three constants, not a literal, so the claim cannot

+// quietly stop being true if any of them moves — the assertion just below

+// catches that instead of the test silently starting to prove nothing

+// (Task 5 review, round 3; sharpened in the fix wave).

func TestWildcardEnableIsNotRateLimited(t *testing.T) {

const zone = "acme.example"

- const attempts = (cliInstanceBurst + domainsCeilingBurst) / 2

- if !(cliInstanceBurst < attempts && attempts < domainsCeilingBurst) {

- t.Fatalf("attempts (%d) must sit strictly between cliInstanceBurst (%d) and domainsCeilingBurst (%d) — as written, this test would no longer prove the exemption it is named for",

- attempts, cliInstanceBurst, domainsCeilingBurst)

+ const floor = max(cliInstanceBurst, svcWriteBurst)

+ const attempts = (floor + domainsCeilingBurst) / 2

+ if !(cliInstanceBurst < attempts && svcWriteBurst < attempts && attempts < domainsCeilingBurst) {

+ t.Fatalf("attempts (%d) must sit strictly above cliInstanceBurst (%d) and svcWriteBurst (%d) and strictly below domainsCeilingBurst (%d) — as written, this test would no longer prove the exemption it is named for",

+ attempts, cliInstanceBurst, svcWriteBurst, domainsCeilingBurst)

}

fx, _ := instanceFixture(t, "code-wc-exempt")

svc := serviceToken(t, fx.st, OpsAccountID, "hub", "titogo", svcrole.Publish)

diff --git a/internal/console/authcli.go b/internal/console/authcli.go

index 15d69e842..f7ef00374 100644

--- a/internal/console/authcli.go

+++ b/internal/console/authcli.go

@@ -155,12 +155,16 @@ const (

// on every move, so an unlimited exempt door would let one

// authenticated credential drive fleet-wide convergence at request

// rate — an alternating attach/detach loop on one host, for instance.

- // 120/min is svcWriteBurst's own scale and 2x

- // TestWildcardEnableIsNotRateLimited's per-minute volume, so it stays

- // "not rate limited" in every sense a real caller experiences; it

- // exists purely to cap how fast one credential can make every box look

- // at the world again, not to throttle the exempt workload.

- domainsCeilingBurst = 120 // per minute, the cliRateWindow, keyed on the principal

+ // 180/min is deliberately clear of svcWriteBurst (120) rather than

+ // merely above it: the two used to sit at the same number, which meant

+ // no test volume could land strictly between them, so a regression that

+ // put the write budget back onto this door (re-adding allowWrite to

+ // domainsGate) would refuse at the SAME threshold this ceiling does —

+ // nothing would fail. A real gap between the two numbers is what makes

+ // TestWildcardEnableIsNotRateLimited able to pick a volume that proves

+ // the write-budget exemption specifically, not just "some limiter or

+ // other let this through."

+ domainsCeilingBurst = 180 // per minute, the cliRateWindow, keyed on the principal

)

// rateLimiter is a fixed-window counter, keyed by whatever the caller thinks

diff --git a/internal/console/svcroutes_test.go b/internal/console/svcroutes_test.go

index 9faa6eb20..ff83d4b07 100644

--- a/internal/console/svcroutes_test.go

+++ b/internal/console/svcroutes_test.go

@@ -602,7 +602,7 @@ func TestEveryClosedDoorRefusesAServiceCredential(t *testing.T) {

seen++

}

if seen != len(closedDoors) || seen != 13 {

- t.Errorf("walked %d closed doors, want all %d (and 12 today: instance-token, instances-manifest, six fleets, four services)",

+ t.Errorf("walked %d closed doors, want all %d (and 13 today: instance-token, the routes door, instances-manifest, six fleets, four services)",

seen, len(closedDoors))

}

diff --git a/services.go b/services.go

index 44eac45c8..d7714e829 100644

--- a/services.go

+++ b/services.go

@@ -246,6 +246,7 @@ func printServiceCreated(acctSeg, name, role, app, token string, doors []string)

} else {

fmt.Fprintln(stdout, "it may:")

}

+ sawStoreGrant := false

for _, d := range doors {

method, path, ok := strings.Cut(d, " ")

if !ok {

@@ -253,6 +254,14 @@ func printServiceCreated(acctSeg, name, role, app, token string, doors []string)

continue

}

fmt.Fprintf(stdout, " %-6s %s\n", method, path)

+ if strings.HasSuffix(path, "/store/grant") {

+ sawStoreGrant = true

+ }

+ if sawStoreGrant {

+ fmt.Fprintln(stdout)

+ fmt.Fprintln(stdout, " (store grant also needs this principal listed on the console's")

+ fmt.Fprintln(stdout, " CARLOS_STORE_OPERATORS — being able to dial the door isn't enough)")

}

fmt.Fprintln(stdout)

fmt.Fprintln(stdout, "deliver it: "+deliverLine(app))