diff --git a/docs/plans/tools-catalogue-room-grants.md b/docs/plans/tools-catalogue-room-grants.md index aabfda5e5..f2d6ea4ce 100644 --- a/docs/plans/tools-catalogue-room-grants.md +++ b/docs/plans/tools-catalogue-room-grants.md @@ -17,7 +17,7 @@ Sam's review note on order is taken: **one first-party tool works end to end in | 3 | Approval for parked calls — the existing `propose-action` consent path (`ApprovalAction`) as the pending-call envelope | Kai | 1–2 d | a person with authority consents to the exact call before the broker executes it, once; nothing new is invented for it | | 4 | The page — the Tools list with per-member rows first, then the grant aside and the trail; option A | Kai builds, UX Lead gates | 3–4 d | the page draws what the server enforces at the time it draws it, and only that | | 5 | The manifest parser — `.claude-plugin` / `.cursor-plugin` → Installable, with the validation in ADR-001 §1–§2 | Kai | 2–3 d | a third-party server becomes a catalogue row | -| 6 | Per-person Connections — GitHub OAuth (and Gmail after it) as the credential a member grants; proposal in §10, HOLD for Sam's read | unsized | — | the "granted by {member}" row when the member is not the admin | +| 6 | Per-person Connections — GitHub OAuth (and Gmail after it) as the credential a member grants; proposal in §10, HOLD for Sam's read | Kai, after Sam's read | 12–13 d in seven ordered pieces (§10.7); not scheduled | the "granted by {member}" row when the member is not the admin | | 7 | HTTP hook endpoint + CLI hooks-config writer | Kai | 5–7 d ingress/claim only | ADR-028 claims advise at `PreToolUse` (ruled 2026-09-12, 67958): the kernel refuses nothing (D7); the hook denies only when the path is under another seat's active claim and the caller holds none covering it, the reason names the holder and the claim that lifts it, and everything else, including any transport failure at the CLI-written hook, allows. Replay decisions are ledger events keyed by pod + agent + `eventId`, not a side store (D10), so no agent reads another's decision (Vera 67956). Transport failure at the hook is fail-open with a short timeout (≤5s), because the only deny is a positive fact and the absence of an answer is not one (67964). The route checks membership in `:podId` after `agentRuntimeAuth`, resolves realpaths under the repo root before the claim match, and never logs `tool_input` or the bearer. Hook bodies carry the tool name, an args digest and the resolved paths, never raw args — so no payload, including `Write` contents, reaches the server or the replay ledger event (Vera 67967). Separate lane. | Pieces 1 → 2 → 3 are strictly ordered. **4 is not gated on the broker** (Sam 67413): per-member installs ship before it, so the Tools list ships first with per-member rows — "installed by you · your agents may use it" — from the catalogue's existing `InstallableInstallation` rows, and room-grant rows, the aside's grant, and the trail appear as 1–3 land. The page work can start the day this plan merges. 5 has no dependency on 1–4 and goes after them only because there is one builder; a second builder takes it in parallel. 6 and 7 are after the slice and not in this sprint. @@ -186,7 +186,7 @@ The contract is ADR-001 §1–§2 as amended and Kai's 67390/67395/67399; nothin ## 10. Per-person Connections (piece 6) — proposal, HOLD for Sam's read -**Status:** proposed, not scheduled, not sized. Direction from Sam (67645, 2026-09-11); the four security constraints from Vera (67647) are folded in where they bite, not listed separately. Nothing here changes pieces 1–5; every sentence below extends a thing the slice already has. **Sam reads this before anything in it is built.** +**Status:** proposed, not scheduled; **sized in 10.7** (Sam 68231, 2026-09-12), still HOLD. Direction from Sam (67645, 2026-09-11); the four security constraints from Vera (67647) are folded in where they bite, not listed separately — 10.7 names the piece that satisfies each. Nothing here changes pieces 1–5; every sentence below extends a thing the slice already has. **Sam reads this before anything in it is built.** Sam's boundary, which this section is built around: *a credential should reach only the rooms and agents it is granted to.* Grok's per-user model gives every agent a person runs all of that person's credentials; a key file the agent "can use but not read" still sits in the agent's environment. Here the credential never enters an environment at all — the broker holds it, the grant names who may cause it to be used, and the trail names who did. @@ -220,7 +220,7 @@ Same model, same owner field, same `owner`/`repo` config the slice's App row car `connectorSecrets` is reused as it is (AES-256-GCM, key ring, `rewrap-connector-secrets.ts`; ADR-025 D6, as Slack's `botTokenRef` already uses it): the access token goes in with `connectorSecrets.put(integrationId, provider, material)` and the row stores the returned opaque reference at `config.credentialRef`; an OAuth refresh token goes behind `refreshTokenRef` the same way. Both keys join `INTEGRATION_SECRET_CONFIG_KEYS` (`models/integrationPublicConfig.ts`), so `toPublicIntegrationConfig` strips them from every response the way #1673 made it strip `accessToken` and `botTokenRef`; `credentialHint` (last four characters, computed once at intake) is the only trace and is what the row shows after saving. -The material is decrypted in exactly one place: the broker's `resolveConnection` grows a `credentialFor(connection)` that returns `{ token, expiresAt }` — the same shape `githubAppService.getInstallationToken` returns for the App row — and the executing tool never sees which kind it got. GitHub's refresh tokens are single-use: using one kills both it and the old access token, so two broker calls that find the access token expired and both refresh would leave one of them holding a dead pair on a working Connection (Vera 67895). `credentialFor` therefore refreshes behind a fence: a conditional update that bumps `refreshGeneration` from the value the caller read — exactly one caller wins it. The winner refreshes at the provider, `put`s the new pair, writes the new references, and only then `revoke`s the old ones; a failed provider call leaves the old pair and the row untouched apart from the bumped generation, and the next caller tries again. Every loser re-reads the row and **never sets `status: 'error'` and never revokes anything**. A loser that re-reads before the winner has written finds the old reference, and the old access token is already dead the moment the winner used the refresh token (Vera 67904), so the re-read is not the fence's end: if the references it finds are the ones it started with, it waits out the winner's provider call (bounded, a second or two) and re-reads once more; the winner writing the new references is what the loser is waiting for, not the generation bump. A loser still holding the old reference after that retry answers the tool call with a retryable `credential_refreshing` and touches nothing, and the next call finds the row written. Only the generation holder may mark the row `status: 'error'`, and only on a definitive refusal from the provider (`invalid_grant`, the refresh token itself expired or revoked); the next call on that row is refused `connection_mismatch`, as a disconnected App row is today. No agent environment holds anything: the ADR-008 `mcp[]` entry stays the broker URL placeholder, and the agent's only credential stays `${COMMONLY_AGENT_TOKEN}` (§4). Two tests extend rather than appear: `never returns the credential` (§4) runs over a `github-user` Connection, and `no integration response carries a credential` (#1673) gains a row with both references set. One is new: `a loser that reads the old reference retries once after the winner writes`. +The material is decrypted in exactly one place: the broker's `resolveConnection` grows a `credentialFor(connection)` that returns `{ token, expiresAt }` — the same shape `githubAppService.getInstallationToken` returns for the App row — and the executing tool never sees which kind it got. GitHub's refresh tokens are single-use: using one kills both it and the old access token, so two broker calls that find the access token expired and both refresh would leave one of them holding a dead pair on a working Connection (Vera 67895). `credentialFor` therefore refreshes behind a fence: a conditional update that bumps `refreshGeneration` from the value the caller read — exactly one caller wins it. The winner refreshes at the provider, `put`s the new pair, writes the new references, and only then `revoke`s the old ones; a failed provider call leaves the old pair and the row untouched apart from the bumped generation, and the next caller tries again. Every loser re-reads the row and **never sets `status: 'error'` and never revokes anything**. A loser that re-reads before the winner has written finds the old reference, and the old access token is already dead the moment the winner used the refresh token (Vera 67904), so the re-read is not the fence's end: if the references it finds are the ones it started with, it waits out the winner's provider call (bounded, a second or two) and re-reads once more; the winner writing the new references is what the loser is waiting for, not the generation bump. A loser still holding the old reference after that retry answers the tool call with a retryable `credential_refreshing` and touches nothing, and the next call finds the row written. Only the generation holder may mark the row `status: 'error'`, and only on a definitive refusal from the provider (`invalid_grant`, the refresh token itself expired or revoked); the next call on that row is refused `connection_mismatch`, as a disconnected App row is today. No agent environment holds anything: the ADR-008 `mcp[]` entry stays the broker URL placeholder, and the agent's only credential stays `${COMMONLY_AGENT_TOKEN}` (§4). One test extends: `no integration response carries a credential` (#1673) gains a row with both references set. Two are new: `never returns the credential` — §4 item 5 names it, but it was never written (Vera 68233), so 6b writes it, over a `github-user` Connection — and `a loser that reads the old reference retries once after the winner writes`. ### 10.4 How a grant scopes it — the record does not change @@ -243,3 +243,31 @@ Named tests: `removing a Connection revokes every grant on it`, `the provider re - **The granter leaves the room.** Options: the grant survives to its expiry (it always has one), or it is revoked on leave. Wren's recommendation: revoke on leave, because the person who could revoke it can no longer see the trail that would tell them to. - **A token that does not expire.** Accepted and marked in v1 (10.2), or refused outright. Recommendation: accept and mark; refusing pushes people to the classic PAT with an expiry they set to a year. - **Gmail's place.** After GitHub proves 10.2–10.5 on one real person's Connection, not in parallel; the Google consent-screen review is calendar time no sequencing shortens. + +### 10.7 Sizing — seven ordered pieces, the gate for each, and the question that comes first + +Sam's rule (68231): nothing in §10 is built until it is sized and read. This is the sizing; the read is still owed. Estimates are mine and Kai confirms, the same as §1's table. One builder, strictly in this order — each piece is a PR that lands on its own with its named tests, and each has a gate Vera clears at the exact head (Wren gates Vera's own). **Order principle:** close the gap the slice already has (6a), then teach the mint and the broker the new row type before any intake exists (6b), then the intake that needs nothing from anyone outside the repo (6c), then the removal that intake makes necessary (6d), then OAuth, which waits on two things only Sam can do at the App (6e), then the refresh fence OAuth makes necessary (6f), then the page (6g). Nothing reaches a member's screen before 6g, so no half-built path is ever the only path a person sees. + +| # | piece | size | what lands | gate — Vera's constraint it satisfies, and the tests the head must carry | +|---|---|---|---|---| +| 6a | **Removal cascades on the row the slice has** — `RoomGrant.revokeByConnection(connectionId)` (every root grant on the row → `revokeCascade`), and `DELETE /api/integrations/:id` runs 10.5's steps 1–2 (grants, then `status: 'disconnected'` + `revokedAt`) before it deletes. Applies to the App row today; no new type. | 1 d | 10.5 steps 1–2 | **Constraint 3, first half** (every grant on a removed Connection dies through #1661's cascade). Tests: `removing a Connection revokes every grant on it`; the broker refuses a grant whose row is `disconnected` before the delete completes. | +| 6b | **The record, the mint and the broker learn `github-user`** — 10.1's row shape; `credentialRef` and `refreshTokenRef` join `INTEGRATION_SECRET_CONFIG_KEYS`; `credentialHint` is the only trace; `connectionOwnerId` and the `github-app` type check in `routes/grants.ts` extend by type; the mint's `providerRole` ceiling (`400 insufficient_credential_scope`) and the `expiresAt` cap (10.4); `resolveConnection` grows `credentialFor(connection)` that returns `{ token, expiresAt }` from `connectorSecrets.get` for the new type — **no refresh yet** (a stored token is used as-is until 6f); the catalogue's `connections[]` lists the person's own rows beside the admin's. Rows exist only from fixtures until 6c. | 2 d | 10.1, 10.4, the read half of 10.3 | **Constraint 4** (only the row's owner can grant it: `grantedBy` from `createdBy`, never the body; an admin minting on a member's row is `403 access_denied`) and **constraint 1, the "no API returns it" half**: the B1 leak matrix gains a `github-user` row with both references set and `KNOWN_EXPOSURES` stays `[]`; `never returns the credential` is written here — §4 item 5 names it and it was never written — and runs over a `github-user` Connection; `no integration response carries a credential` gains the row. | +| 6c | **Pasted-token intake** — `POST /api/integrations` accepts `github-user` with `intake: 'token'`; the three provider checks before anything is stored (`GET /user`, `GET /repos/{owner}/{repo}` → `providerRole`, the expiration header → `expiresAt`, `null` accepted and marked), refusal with a reason code and nothing stored on any failure; `connectorSecrets.put`, the row keeps only the reference and the hint; the paste field write-only from the first save (#1674's pattern). | 2 d | 10.2 path 3, the write half of 10.3 | **Constraint 2** (checked with the provider at intake, whose it is and what it can reach, scopes recorded, expiring preferred) and **constraint 1, the "by reference" half**. Tests: a pasted token the provider refuses stores nothing; the row after intake carries a reference and a hint and no material; `providerRole` is what the provider said, not what the body said. | +| 6d | **Removal finishes** — 10.5 steps 3–5 for the paste row: no provider API, so the response carries `revokeAt`; `connectorSecrets.revoke` on the references only after that; delete only after that; a `disconnected` row with references is the sweep's "not finished". | 1 d | 10.5 steps 3–5 | **Constraint 3, second half** (the material is gone by reference; the provider is told where it can be). Tests: `a removed Connection's secret is gone by reference`; `a failed provider revoke leaves the row and its references for a retry`. The OAuth-specific tests wait for 6f. | +| 6e | **OAuth intake through the App's user authorization** — `GET /api/integrations/connect/github/start` and `/callback`, the single-use nonce bound to the caller with a TTL (the Slack shape in `routes/installables.ts`), rate-limited like `oauthLimiter` and ahead of auth (B3's rule); the access token and the refresh token stored as two references; `expiresAt` = the refresh token's six months; `providerRole ∩ App permissions` as the ceiling. **Blocked on Sam at the App, not on code:** `GITHUB_APP_CLIENT_ID` / `GITHUB_APP_CLIENT_SECRET` into GCP SM through the `api-keys` ExternalSecret, and "Request user authorization during installation" switched on. | 2 d code; Sam's two App actions are calendar time | 10.2 path 1 | **Constraint 2** for the OAuth row (what the token can do is the App's permission set, recorded at intake) and **constraint 1** for the refresh token (by reference, never returned). Tests: the callback refuses a nonce that is used, expired, or bound to another caller; the login OAuth app's client id is never sent; both references set and neither serialised. | +| 6f | **The refresh fence** — 10.3's `refreshGeneration` conditional update, one winner, losers wait once and answer `credential_refreshing`, only the generation holder marks `status: 'error'` and only on `invalid_grant`; removal calls `credentialFor` first so the provider revoke carries a live token, `invalid_grant` there counts as revoked. The riskiest piece; it is last of the server work so the whole loop exists before it. | 2 d | 10.3 refresh, 10.5 step 3 for OAuth | **Constraint 1 under concurrency** (the single decrypt point holds when two calls refresh at once; no loser revokes anything) and **constraint 3 at the provider** (`DELETE /applications/{client_id}/grant` with a live token). Tests: `a loser that reads the old reference retries once after the winner writes`; `removal refreshes before it revokes at the provider`; `an authorization already gone at the provider counts as revoked`; `the provider revoke runs before the material is gone`. | +| 6g | **The page** — "Connect GitHub" as the primary control, the paste field under a disclosure with the fine-grained-token link, "does not expire" on a `null` expiry, the hint after saving, "granted to {room} by {member}" for a non-admin granter, Revoke for the granter only, `revokeAt` shown once after removal, "cannot write here" after a `provider_denied` trail row. Reads only what 6b–6f put on main. | 2–3 d | §1's row 6 — "granted by {member}" | **UX Lead at 1440 and 390**, the page gate every §1 piece has; Vera's constraints are all server-side and are already cleared by then — the page test is the standing rule that it renders no control the server does not enforce (no "Connect GitHub" until 6e is on main). | +| 6h | **Gmail** | not sized | 10.6 | After GitHub proves 6b–6g on one real person's Connection; the consent-screen review is calendar time and starts whenever Sam wants it to, independent of this order. | + +**Total: 12–13 d of build** (1 + 2 + 2 + 1 + 2 + 2 + 2–3) for GitHub, one builder, plus Sam's two App actions before 6e. 6a can start the day Sam's read lands, because it changes nothing §10 proposes and closes a gap the slice has today. 6b–6d need no one outside the repo. 6e is the first piece with an external dependency, and it is the reason the order puts the paste path first: the paste path proves the whole loop — intake, reference, broker, trail, removal — with nothing to wait for, while OAuth waits on the App. + +**Where each of Vera's four lands, in one place:** + +| Vera 67647 | satisfied by | +|---|---| +| 1. `connectorSecrets` by reference, decrypted only on the broker path, no API returns it, masked hint | 6b (strips, hint, leak-matrix row), 6c (stored by reference), 6e (refresh token the same way), 6f (the single decrypt point holds under concurrent refresh) | +| 2. Provider check at intake — whose it is, what it can do — scopes recorded, expiring preferred | 6c (paste: `GET /user`, `GET /repos`, the expiration header), 6e (OAuth: the App's permission set ∩ the person's role) | +| 3. Removing a Connection revokes every grant through #1661's cascade, and the token at the provider where it can | 6a (the cascade, on the App row too), 6d (material by reference; `revokeAt` where there is no API), 6f (the provider revoke with a live token, refresh-first) | +| 4. Only a connection's owner can grant it, as the mint already requires | 6b (the existing rule, restated for the new type; a test that an admin cannot mint on a member's row) | + +**The one question a build needs Sam to answer first.** *Does the first per-person Connection a member can create come from the paste path, or does nothing reach a member until OAuth (6e) exists?* Everything else in 10.6 can wait for Sam's read of the recommendations already written there (revoke on leave; accept and mark a non-expiring token; Gmail after GitHub), and none of those changes the order above. This one does: if the answer is "paste is fine as the first path a member sees", 6g moves ahead of 6e and ships after 6d, and a member has a working Connection about a week earlier — but for that window the fallback §10.2 says is "never the default" is the only control on the form, which is the Grok shape Sam called not optimal. **Wren's recommendation, and what the table assumes:** the order as written. The paste path is built first because it proves the loop with no external dependency, but the page (6g) waits for OAuth, so no member ever sees a form whose only path is the fallback. The cost is that the two App actions (client secret into GCP SM, user authorization switched on) sit on the critical path from the day 6d lands; they take Sam minutes and can be done any time before then.