amadan
spec: instance-bound tokens rev 3, answering Astra round 2
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+133
−36
docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md+63 −36docs/superpowers/specs/CODEX-FINDINGS-INSTANCE-BOUND-TOKENS-R2.md+70 −0
CI cancelled — superseded by a newer push — run details
CI log
=== gate === cd launchctx && go vet ./... && go test ./... -count=1 ok amadan.net/carlos/platform.git/launchctx 0.011s ? amadan.net/carlos/platform.git/launchctx/controlname [no test files] ? amadan.net/carlos/platform.git/launchctx/wireid [no test files] go vet ./... gofmt -l .: clean go test ./... -count=1 -timeout 30m amadan: job cancelled — a newer push superseded it
diff --git a/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md b/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md| index 4e4b54f6b..277e84c5e 100644 |
| --- a/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md |
| +++ b/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md |
| @@ -1,6 +1,6 @@ |
| # Instance-bound tokens (#155, half 1) |
| -Status: draft rev 2, 2026-10-16 (Astra round 1 answered, §9). Discussion: amadan carlos/platform #155. |
| +Status: draft rev 3, 2026-10-16 (Astra rounds 1 and 2 answered, §9, §10). Discussion: amadan carlos/platform #155. |
| ## 1. The problem |
| @@ -124,28 +124,35 @@ A token is bound to one incarnation of its host: |
| is answered (§4.3). A host that is deleted and created again gets a new |
| `CreatedAt`, so the old tenant's token dies with the old record even if |
| no revoke ran. This is the guarantee; the revokes below are tidiness. |
| -- **`provision.CreateRecord` enforces a fresh `CreatedAt`** (codex 10). A |
| - zero `CreatedAt` is stamped with the clock; any other value outside |
| - [now − 15 min, now + 2 min] is refused with a new `ErrStaleCreatedAt`. |
| - It is the one boundary every record creation crosses (the console create, |
| - seed, import and alias paths and `provision.replica.go`), and each of them |
| - stamps the clock just before the write today, so none is refused. A |
| - record replayed from saved data therefore cannot carry its old |
| - incarnation. The seed path's longest gap between stamping and writing is |
| - its store checks, well inside the window. |
| +- **Every creation stamps `CreatedAt` from the clock.** That is true of |
| + every creation path today: the console create door (`apiinstances.go`, |
| + `rec.CreatedAt = time.Now()` and, in claim mode, `hostops.PrepareInstance`, |
| + which refuses a supplied timestamp and stamps its own, |
| + `hostops/record.go:159`), and the seed, import, alias and replica paths, |
| + which create from a record the create door stamped or stamp their own. |
| + No path deletes a record and re-creates it from saved data. Rather than a |
| + runtime rule that would still admit a replay inside its window (codex |
| + R2-2) and that breaks historical fixtures (codex R2-4), the invariant is |
| + pinned by an AST test listing every non-test caller of |
| + `provision.CreateRecord` and of `hostops.PrepareInstance`; a new caller |
| + fails the test until it is reviewed against this rule and added. |
| - `handleAPIInstanceDelete` revokes every live `kind='instance'` row for |
| (account, app, host) **before** deleting the record. If the revoke |
| fails, the delete answers 500 and deletes nothing. A delete that fails |
| after the revoke leaves a live instance with no token, which a member |
| fixes by minting again. |
| -- App trash/purge (`claim_app_identity.go`, `store.go:1736`) and the |
| - account-move and rename statements (`store.go:1928`, `:1970`, `:2074`, |
| - `:2089`) add `cliKindInstance` to their `kind IN (…)` lists, so an |
| - instance token follows the same rules as the app's other credentials. |
| -- Rows an older console left behind (§6) are swept by one statement at |
| - console start: `DELETE FROM cli_tokens WHERE kind='instance' AND NOT |
| - EXISTS (a live app row for its account and app)`. One indexed delete, |
| - bounded by the table, not by the bucket. |
| +- Purge (`claim_app_identity.go:182`, `store.go:1736`) deletes instance |
| + rows with the app's other credentials: its `kind IN (…)` list gains |
| + `cliKindInstance`. Trash does not touch them, as it does not touch app |
| + tokens, so a restore brings them back with the same records. |
| +- Account move and rename (`store.go:1970`, `:2089`) carry instance rows |
| + with the app: their `UPDATE … kind IN (…)` lists gain `cliKindInstance`. |
| +- The destination stale-credential checks (`store.go:1928`, `:2074`) do |
| + **not** count instance rows. A leftover instance row at the destination |
| + name, from an older console's purge (§6), is bound to a record |
| + incarnation that no longer exists, so it can only answer 404; it must |
| + not block the move, and it stays visible and revocable on the account |
| + pane. No start-up sweep (codex R2-1, R2-3). |
| - A record with a zero `CreatedAt` cannot be minted for (§5): there is |
| nothing to bind to. |
| @@ -162,7 +169,8 @@ instance branch **before** that test: |
| | `instanceGate`'s limiter (`apiinstances.go:87`) | keyed on `principalKey` for an instance principal, so every token for one host shares one budget and a sibling's is separate (codex 8); token hash for every other caller, unchanged | |
| | `CLIToken` (`authstore.go`) | gains `Host`; `IsInstance()`; `NeverExpires()` true (codex 9) | |
| | `scanCLITokenRows` and both listing queries | read `host`; role shown as `self` | |
| -| `serviceRowOf` / `/auth/cli/tokens/revoke` (`authcli.go:1543`, `:1618`) | takes the app-token branch: current membership of the row's account is required, then the account-scoped revoke. A former member cannot revoke it (codex 4) | |
| +| `serviceRowOf` / `/auth/cli/tokens/revoke` (`authcli.go:1543`, `:1618`) | takes the app-token branch: current membership of the row's account is required, then the same `RevokeCLITokenContext` deletion that branch already uses, which carries the request context and the claim admission (codex 4, R2-3). A former member cannot revoke it | |
| +| `serviceAccountSqids` (`authcli.go:1488`) | includes instance rows, so the settings page can link to the account pane | |
| | `serviceWhat` (`apiservices.go:441`) | names the host (copy review) | |
| | the services API projection (`apiservices.go:311`) | adds `"host"` | |
| | `templates/settings.html` | an instance row is labelled as bound to its host (copy review) | |
| @@ -276,7 +284,8 @@ func (b selfBinding) refuses(rec *provision.Record) bool |
| refuses them, or, if it does not, last-wins as `encoding/json` does; |
| the plan pins which. |
| 3. **GET then PUT:** a client that wants to send back what it read sends |
| - GET's explicit values (`settings`), never its effective ones. `channel` |
| + GET's `explicit` object (`apiupdates.go:74`) without its `channel` key, |
| + never the effective `settings` object (codex R2 table, 6). `channel` |
| stays refused on the PUT for everyone, even when unchanged |
| (`settings.console.channel-elsewhere`); the channel door is not a Self |
| door in half 1 (§8). |
| @@ -321,16 +330,17 @@ docs page section are copy review. |
| - Rolled-back console: kind `instance` does not resolve, so every |
| instance token is a 401. Nothing widens. |
| - What an older console does to these rows while it runs (codex 5): its |
| - purge, rename and account-move statements skip them, and its personal |
| - revoke path hard-deletes one for the address that minted it, member or |
| - not. None of these widens anything: a skipped row is bound to a record |
| - incarnation that the purge removed, so it can only 404, and a revoke by |
| - a former member only takes a token away. On roll-forward, the start-up |
| - sweep (§3.4) deletes rows whose app is gone, so a stranded row cannot |
| - block a later rename's stale-credential check. The rollback runbook |
| - line: "rolling back below #155 half 1 leaves instance tokens unusable |
| - until roll-forward; nothing to do by hand." |
| - |
| + purge skips them, its rename and account move leave them behind on the |
| + old name, and its personal revoke path deletes one for the address that |
| + minted it, member or not. None of these widens anything. A row left |
| + behind is bound to an incarnation, so after a purge it can only 404, and |
| + after an old rename it is bound to an app name whose records moved away, |
| + so it also 404s. A revoke by a former member only takes a token away. |
| + Leftover rows show on the account pane and are revoked there. They block |
| + nothing, because the stale-credential checks do not count them (§3.4). |
| + The rollback runbook line: "rolling back below #155 half 1 leaves |
| + instance tokens unusable until roll-forward; an app purged, renamed or |
| + moved meanwhile needs its instance tokens minted again." |
| - Old CLIs never send these tokens. The app token's reach is unchanged. |
| - The two `ALTER`s join authstore's tolerant list (a duplicate-column |
| error is ignored), and both default so old rows read unchanged. |
| @@ -361,8 +371,12 @@ docs page section are copy review. |
| install lands on neither record; A is replaced by a route of a |
| different kind before a move-status read, and the answer is the |
| canonical 404, not the kind refusal. |
| -- `CreateRecord` stamps a zero `CreatedAt`, refuses one older than 15 |
| - minutes, and every production caller passes. |
| +- The creation-call-site AST test (§3.4) lists exactly today's callers of |
| + `provision.CreateRecord` and `hostops.PrepareInstance`. |
| +- Purge deletes instance rows; trash then restore keeps them and the |
| + original token works again on the unchanged record; rename and account |
| + move carry them; a leftover instance row at the destination does not |
| + block either. |
| - Update settings: Self may change exactly `instance_control`'s keys; the |
| refusal names the first key in fixed order whatever the raw JSON order; |
| echoing GET's explicit values passes; echoing an inherited effective |
| @@ -374,8 +388,10 @@ docs page section are copy review. |
| revoke it from `/settings`; a current member can. |
| - Old-console SQL: the previous release's purge, rename and move |
| statements, run against a database holding instance rows, leave rows |
| - the new console's start-up sweep removes, and a rename afterwards is |
| - not refused. |
| + that resolve but answer the canonical 404 on every Self door, and a |
| + later rename or move by the new console is not refused. |
| +- Every test that writes `cli_tokens` runs against `OpenGated` with |
| + `HostClaim` on as well as the plain store. |
| - Delete revokes; a revoke failure aborts the delete. |
| - Purge, rename and account-move delete or carry instance rows the way |
| they do app rows. |
| @@ -403,9 +419,20 @@ Every finding was checked against the code and every citation held. |
| | 2 | P0 | Accepted. `Allow` refuses Self; `AllowSelf` admits it without Admin on the update doors (§3.3, §4.2). | |
| | 3 | P0 | Accepted. The binding travels into `readUpdatesTarget`, `requestInstall`, `applyUpdates` and `moveGate` and is checked at each read (§4.3). | |
| | 4 | P0 | Accepted. Instance rows take the app-token branch of the personal revoke, which requires current membership (§3.5). | |
| -| 5 | P1 | Accepted in part. An older console's writes are stated and shown not to widen; a start-up sweep reconciles on roll-forward (§3.4, §6). Refusing old writers outright is not possible: an old binary cannot be taught a rule after the fact. | |
| +| 5 | P1 | Accepted in part. An older console's writes are stated and shown not to widen (§6); rev 3 replaces the start-up sweep with stale checks that ignore instance rows (§3.4). Refusing old writers outright is not possible: an old binary cannot be taught a rule after the fact. | |
| | 6 | P1 | Accepted. Changes compare explicit values; the round trip is defined on GET's explicit settings (§4.4.2, §4.4.3). | |
| | 7 | P2 | Accepted. Fixed order `updates`, `major`, `pin` (§4.4.2). | |
| | 8 | P2 | Accepted. `instanceGate`'s limiter keys an instance principal on `principalKey` (§3.5). | |
| | 9 | P2 | Accepted. Every consumer is listed in §3.5's table; the plan's first task re-greps. | |
| -| 10 | P2 | Accepted. `CreateRecord` stamps or refuses `CreatedAt` (§3.4). | |
| +| 10 | P2 | Accepted in rev 2 with a freshness window; replaced in rev 3 (§10, R2-2). | |
| + |
| +## 10. Astra round 2 (CODEX-FINDINGS-INSTANCE-BOUND-TOKENS-R2.md) |
| + |
| +| # | Sev | Disposition | |
| +|---|---|---| |
| +| R2-1 | P1 | Accepted. The start-up sweep is gone; trash keeps instance rows, purge deletes them, and the destination checks ignore them (§3.4). | |
| +| R2-2 | P2 | Accepted by removal. The freshness window is dropped; no path replays a record, every path stamps the clock, and an AST test pins the creation call sites (§3.4). A per-incarnation nonce on the record was considered and declined: it needs a new record field at two creation boundaries plus a backfill write on every existing record, to guard a path that does not exist. | |
| +| R2-3 | P2 | Accepted. Personal revoke reuses `RevokeCLITokenContext`; with no sweep there is no start-up write; tests run on `OpenGated` (§3.5, §7). | |
| +| R2-4 | P2 | Moot: the freshness rule that broke the fixtures is gone. | |
| +| R1-6 (partly) | | Fixed: the round trip is GET's `explicit` object without `channel` (§4.4.3). | |
| +| R1-10 (partly) | | See R2-2. | |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-INSTANCE-BOUND-TOKENS-R2.md b/docs/superpowers/specs/CODEX-FINDINGS-INSTANCE-BOUND-TOKENS-R2.md| new file mode 100644 |
| index 000000000..8439653c8 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-INSTANCE-BOUND-TOKENS-R2.md |
| @@ -0,0 +1,70 @@ |
| +| Round 1 finding | Rev 2 closes it? | Reason | |
| +|---|---|---| |
| +| 1. PATCH can select a sibling’s environment | Closed | §4.4.1 restricts Self to `addrs` and `probe`, rejecting the other fields before side effects. | |
| +| 2. `Allow(Self)` also admits Admin | Closed | `AllowSelf(Set{})` explicitly excludes Admin; the existing sets remain unchanged. | |
| +| 3. Incarnation checks miss helper reads and retries | Closed | §4.3 carries `selfBinding` into record-reading helpers and checks every install attempt before record-dependent responses or writes. | |
| +| 4. Former members can revoke through personal settings | Closed | §3.5 requires current membership before instance-token revocation. The SQL execution gap is noted below. | |
| +| 5. Old lifecycle writers strand credentials | Partly | A startup sweep covers some rollback leftovers, but an old writer acting **after** that sweep can still leave rows that block a new console’s rename. Old personal revocation also remains available. | |
| +| 6. GET-to-PUT no-op contract differs from the API | Partly | Explicit-value comparison is correct, but §4.4.3 calls the explicit object `settings`. That object contains effective values; the explicit object is `explicit`. Both contain `channel`, which clients must omit. See [apiupdates.go:74](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/apiupdates.go:74). | |
| +| 7. Request order disappears during decoding | Closed | §4.4.2 specifies the deterministic order `updates`, `major`, `pin`. | |
| +| 8. Actual limiter remains per token | Closed | §3.5 changes `instanceGate` itself, and §7 tests two tokens sharing the endpoint’s budget. | |
| +| 9. Other consumers misclassify instance rows | Closed | §3.5 covers expiry, listings, API projection, settings presentation and revocation descriptions while preserving the actual app binding. | |
| +| 10. Fresh incarnation remains a caller convention | Partly | The new age check rejects old snapshots but accepts a replay within 15 minutes. `CreateRecord` also is not the sole production creation path. | |
| + |
| +**Round 2 review — `d0a6e506`, branch `instance-bound-tokens-155`** |
| + |
| +“Closed” above means addressed by the revised design. This commit contains no implementation. This was a static review; no files were changed and no tests were run. |
| + |
| +**Changes required. No new P0 finding. One new P1 finding follows, alongside three P2 findings.** |
| + |
| +**R2-1 — P1: The startup sweep deletes credentials for restorable apps** |
| + |
| +The proposed sweep deletes instance tokens without a **live** app row: [design:145](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md:145). |
| + |
| +A trashed app is not live, but its absence is temporary. The code explicitly defines live as `deleted_at = 0` at [store.go:1575](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/store.go:1575). Trashing changes the app row without deleting credentials at [store.go:1676](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/store.go:1676). Instance records survive trash and restore, as documented at [restore.go:28](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/restore.go:28). |
| + |
| +Consequently, trash → console restart → restore permanently loses the instance tokens even though the same records and incarnations return. Restore without a restart retains them. This contradicts the intended lifecycle parity with app credentials. |
| + |
| +**Fix:** Distinguish a missing app row from a retained, trashed row. Preserve credentials for restorable apps. Add a trash → startup sweep → restore test that proves the original token works again for the unchanged incarnation. |
| + |
| +**R2-2 — P2: The freshness window still permits incarnation replay** |
| + |
| +The new rule accepts any supplied timestamp within `[now − 15 min, now + 2 min]`, then concludes that saved records cannot retain their old incarnation: [design:127](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md:127). |
| + |
| +That conclusion does not follow. Create A at `t0`, mint a token, delete A without revoking it, and recreate the saved record at `t0 + 1 minute`. The timestamp passes the proposed check unchanged. Every `selfBinding` comparison then accepts the old token. |
| + |
| +The existing writer takes `Record` by value and serializes its timestamp directly at [provision.go:882](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/provision/provision.go:882). The proposed test checks only zero timestamps and timestamps older than 15 minutes: [design:364](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/docs/superpowers/specs/2026-10-16-instance-bound-tokens-design.md:364). Both tests could pass while recent replay revives a token. |
| + |
| +The claimed single creation boundary is also inaccurate. Claim-enabled creation bypasses `CreateRecord` at [apiinstances.go:1623](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/apiinstances.go:1623), eventually calling `WriteRecord` directly at [create_effects.go:103](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/hostops/create_effects.go:103). Its preparation already rejects supplied timestamps, but it must remain part of the creation-boundary inventory. |
| + |
| +**Fix:** Allocate a fresh incarnation for each new creation rather than treating timestamp recency as uniqueness. Keep retries of an already-prepared creation distinct from recreation. Test replay **inside** the acceptance window through both supported creation paths. |
| + |
| +This remains a creation-boundary gap, not a demonstrated exploit through the ordinary HTTP create endpoint. |
| + |
| +**R2-3 — P2: The new maintenance writes lack a specified claim-gated execution path** |
| + |
| +Rev 2 specifies a startup `DELETE` and switches personal instance-token revocation to the account-scoped revoke. Neither description identifies how these writes obtain SQL mutation authority. |
| + |
| +With `HostClaim` enabled, `cli_tokens` is an app-scoped gated table: [store_gate.go:191](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/store_gate.go:191). Its update and delete triggers require a matching permit: [store_gate.go:542](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/store_gate.go:542). `NewServer` requires the gate to be armed already: [server.go:1232](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/server.go:1232). |
| + |
| +`RevokeAccountToken` performs a raw update at [svcprincipal.go:379](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/svcprincipal.go:379). Existing account revocation supplies its authority through `serviceCredentialWrite` at [apiservices.go:627](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/apiservices.go:627). Personal revocation currently uses a context-aware admission wrapper at [authstore.go:628](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/authstore.go:628). |
| + |
| +Calling the account revoke directly would therefore fail for a current member on an enabled deployment. Running the proposed sweep through the ordinary startup store would also fail when it encounters a matching row. Moving cleanup before arming needs an explicit design decision; it is not equivalent to the existing schema migration. |
| + |
| +**Fix:** Specify the permitted execution path for personal revocation and orphan cleanup. Preserve request context and app admission for revocation. Define cleanup authority and startup ordering separately. Test both with `OpenGated`, not only the ordinary store. |
| + |
| +**R2-4 — P2: The timestamp change breaks existing historical-record fixtures** |
| + |
| +The freshness rule also changes the contract used by existing tests: |
| + |
| +- [seed_test.go:15](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/provision/seed_test.go:15) creates records through `CreateRecord` with a fixed `2026-10-08` timestamp. |
| +- [seed_test.go:368](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/provision/seed_test.go:368) does the same before testing import-abort protection. |
| +- [regiondrift_test.go:50](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/instance-bound-tokens-155/internal/console/regiondrift_test.go:50) deliberately creates a record two hours old to prove that records older than their app are excluded. |
| + |
| +Implementing the proposed guard without adapting these fixtures makes them fail during setup. Replacing every timestamp with `time.Now()` would erase the age relationship that the placement test exists to check. |
| + |
| +**Fix:** Include fixture migration in the design. Use a controlled creation clock or an explicit historical-record fixture mechanism, preserving the original assertions. Keep production creation enforcement intact. |
| + |
| +**Areas that remain sound** |
| + |
| +The revised `AllowSelf` contract preserves existing Admin reach without granting the update doors. Exact host matching, combined with the existing account and app checks, still prevents direct sibling requests. The proposed binding checks cover install retries and move refusals. PATCH’s field restriction closes the environment-binding escape, and the limiter change now targets the actual endpoint budget. |
| \ No newline at end of file |