|
| 1 | +# Hosted memory governance contract — design (issue #6) |
| 2 | + |
| 3 | +Status: **proposed** — this document is the review surface for the contract |
| 4 | +between `coven-github` (the adapter) and `coven-code` (the runtime) for |
| 5 | +memory-backed familiars in hosted mode. Implementation follows in phased PRs, |
| 6 | +coordinated across both repositories, once it is agreed. |
| 7 | + |
| 8 | +It extends the [headless execution contract](headless-contract.md) and is |
| 9 | +governed by the same normative language (MUST / MUST NOT / SHOULD / MAY, per |
| 10 | +RFC 2119) and the same versioning discipline. Where prose in |
| 11 | +[`FAMILIAR-CONTRACT.md`](../FAMILIAR-CONTRACT.md), [`HOSTED.md`](../HOSTED.md), |
| 12 | +or [`docs/security.md`](security.md) disagrees, this file and the headless |
| 13 | +contract win for the wire shape. |
| 14 | + |
| 15 | +## Problem |
| 16 | + |
| 17 | +Familiar memory is the hosted product's differentiator, and also its sharpest |
| 18 | +data-boundary risk. The two sides know different things: |
| 19 | + |
| 20 | +- **The adapter** (`coven-github`) is the only component that knows the |
| 21 | + installation id, account, repository (id, owner/name, visibility, default |
| 22 | + branch), the actor who triggered the work, the trigger/task kind, and the |
| 23 | + customer's policy. |
| 24 | +- **The runtime** (`coven-code`) is the only component that loads, uses, and |
| 25 | + writes memory. |
| 26 | + |
| 27 | +Nothing today carries the adapter's knowledge across that boundary. Without an |
| 28 | +explicit contract, hosted memory silently defaults to whatever scoping the |
| 29 | +runtime happens to use locally — path-scoped, user-scoped, or globally |
| 30 | +familiar-scoped — instead of tenant/repo/policy-scoped. The failure modes are |
| 31 | +concrete: a familiar reads memory from the wrong installation or customer; |
| 32 | +untrusted pull-request content from a fork gets written into durable memory |
| 33 | +and influences later reviews; a maintainer cannot tell which memory shaped a |
| 34 | +review; a customer cannot inspect, revoke, or export what was learned about |
| 35 | +their repositories. |
| 36 | + |
| 37 | +## Goals |
| 38 | + |
| 39 | +Mapped from issue #6's acceptance criteria: |
| 40 | + |
| 41 | +- Every hosted `coven-code` invocation carries **explicit** memory policy — the |
| 42 | + runtime never infers scope. |
| 43 | +- Hosted mode **fails closed**: missing tenant/repo scope means no memory reads |
| 44 | + or writes, not "fall back to local behavior". |
| 45 | +- Untrusted PR/fork content **cannot** write durable memory under any policy. |
| 46 | +- A review can **cite** the memory entries that influenced its output. |
| 47 | +- Customers can **inspect and revoke** memory by installation and repository. |
| 48 | + |
| 49 | +## Non-goals |
| 50 | + |
| 51 | +- The memory storage engine, retrieval, and ranking inside `coven-code`. This |
| 52 | + contract governs *what crosses the boundary and under what authority*, not |
| 53 | + how the runtime indexes memory. |
| 54 | +- The inspect/revoke **API surface** in Cave. It builds on tenant-scoped task |
| 55 | + API auth (#3) and the durable store (#2); this contract only requires that |
| 56 | + memory be **keyed** so that surface is buildable (see [Addressability](#addressability-inspect-and-revoke)). |
| 57 | +- Usage metering of memory operations (#15) and audit/retention *tooling* |
| 58 | + (#12). This contract defines the retention *policy fields*; the enforcement |
| 59 | + tooling is separate. |
| 60 | +- Local/self-hosted memory behavior. Self-hosters own their own store; the |
| 61 | + fail-closed hosted rules apply only when a `memory_policy` is present. |
| 62 | + |
| 63 | +## Boundary model |
| 64 | + |
| 65 | +The adapter is the **policy authority**; the runtime is the **policy |
| 66 | +enforcer**. The adapter computes a `memory_policy` from installation policy, |
| 67 | +repository, actor, and trust, and stamps it into the session brief. The runtime |
| 68 | +MUST NOT read or write any memory outside what the policy grants, and MUST |
| 69 | +report what it did. The adapter then **validates** the result envelope against |
| 70 | +the same policy before publishing — defense in depth: a runtime bug or |
| 71 | +compromise cannot silently exceed the grant, because the adapter re-checks. |
| 72 | + |
| 73 | +``` |
| 74 | + installation policy ─┐ |
| 75 | + repository metadata ─┤ |
| 76 | + actor + trust ─┼──► adapter computes memory_policy ──► session brief |
| 77 | + trigger / task kind ─┘ │ |
| 78 | + ▼ |
| 79 | + coven-code loads/uses/writes memory |
| 80 | + strictly within memory_policy |
| 81 | + │ |
| 82 | + ▼ |
| 83 | + result.memory_used ◄── adapter VALIDATES |
| 84 | + (read / proposed / rejected / approval) │ |
| 85 | + ▼ |
| 86 | + adapter publishes; out-of-scope |
| 87 | + writes are refused, not persisted |
| 88 | +``` |
| 89 | + |
| 90 | +Two invariants make this safe even if the runtime misbehaves: |
| 91 | + |
| 92 | +1. **Deny by default.** Absence of a `memory_policy`, or `enabled: false`, means |
| 93 | + the runtime operates with no durable memory. A brief that omits the block is |
| 94 | + treated as memory-disabled, never as "unrestricted". |
| 95 | +2. **Adapter re-validation.** The runtime's self-report is not trusted on its |
| 96 | + own. The adapter rejects (does not persist / does not publish) any memory |
| 97 | + write the policy did not authorize, and records the rejection. |
| 98 | + |
| 99 | +## The `memory_policy` brief block |
| 100 | + |
| 101 | +Added to `session-brief.json` as an **optional** object. Per the headless |
| 102 | +contract's versioning rules an added optional field is backward-compatible, but |
| 103 | +because the brief schema is `additionalProperties: false` (and the Rust |
| 104 | +`SessionBrief` uses `deny_unknown_fields`), both sides MUST land the field in |
| 105 | +the same coordinated contract update before the adapter emits it — exactly how |
| 106 | +`review_context` and `audit_instruction` were introduced. |
| 107 | + |
| 108 | +```json |
| 109 | +{ |
| 110 | + "memory_policy": { |
| 111 | + "enabled": true, |
| 112 | + "tenant_scope": { |
| 113 | + "installation_id": 42, |
| 114 | + "account_id": 1001, |
| 115 | + "account_login": "acme-inc" |
| 116 | + }, |
| 117 | + "repo_scope": { |
| 118 | + "repo_id": 555, |
| 119 | + "owner": "acme-inc", |
| 120 | + "name": "billing", |
| 121 | + "visibility": "private", |
| 122 | + "default_branch": "main" |
| 123 | + }, |
| 124 | + "branch_scope": { |
| 125 | + "base_ref": "main", |
| 126 | + "head_ref": "fix/tax-rounding", |
| 127 | + "same_repo": true |
| 128 | + }, |
| 129 | + "trust_scope": "collaborator", |
| 130 | + "read_scopes": ["repo", "tenant_shared"], |
| 131 | + "write_scopes": ["repo"], |
| 132 | + "approval_required": true, |
| 133 | + "retention": { |
| 134 | + "max_age_days": 365, |
| 135 | + "delete_on_uninstall": true, |
| 136 | + "redact_secrets": true |
| 137 | + } |
| 138 | + } |
| 139 | +} |
| 140 | +``` |
| 141 | + |
| 142 | +| Field | Type | Meaning | |
| 143 | +|---|---|---| |
| 144 | +| `enabled` | bool | Master switch. `false` (or block absent) → no reads, no writes. | |
| 145 | +| `tenant_scope` | object | Installation id + account. **Required** when `enabled`. Missing → the runtime MUST refuse all memory. | |
| 146 | +| `repo_scope` | object | Repository identity and visibility. **Required** when `enabled`. | |
| 147 | +| `branch_scope` | object | Refs in play and whether the head is same-repo (vs a fork). Drives trust. | |
| 148 | +| `trust_scope` | enum | Actor trust: `maintainer` \| `collaborator` \| `external` \| `fork_pr` \| `scheduled`. See [Trust model](#trust-model). | |
| 149 | +| `read_scopes` | string[] | Memory namespaces the runtime MAY read: `repo`, `tenant_shared`, `familiar_global` (subset only when policy allows). Empty → read nothing. | |
| 150 | +| `write_scopes` | string[] | Namespaces the runtime MAY propose/write. Empty → propose nothing durable. | |
| 151 | +| `approval_required` | bool | When true, written memory is `pending` until a maintainer approves (ties to the `@familiar remember` command, #13). | |
| 152 | +| `retention` | object | `max_age_days`, `delete_on_uninstall`, `redact_secrets`. | |
| 153 | + |
| 154 | +### Scope enums |
| 155 | + |
| 156 | +- **`read_scopes` / `write_scopes`** name namespaces, never raw paths. The |
| 157 | + runtime resolves a namespace to concrete keys itself, but every resolved key |
| 158 | + MUST be prefixed by the tenant + repo coordinates (see |
| 159 | + [Addressability](#addressability-inspect-and-revoke)). `tenant_shared` is |
| 160 | + cross-repo memory *within one installation*; `familiar_global` is |
| 161 | + cross-tenant and is **never** granted in hosted mode by default. |
| 162 | +- A policy MUST NOT grant a write scope broader than its read scope for the |
| 163 | + same invocation. |
| 164 | + |
| 165 | +## Trust model |
| 166 | + |
| 167 | +`trust_scope` is computed by the adapter from the actor and the trigger — the |
| 168 | +runtime never decides trust. It gates the default read/write grants: |
| 169 | + |
| 170 | +| `trust_scope` | Who | Default read | Default durable write | |
| 171 | +|---|---|---|---| |
| 172 | +| `maintainer` | admin/maintain/write actor on the target repo (see #13 permission gate) | `repo`, `tenant_shared` | `repo` (subject to `approval_required`) | |
| 173 | +| `collaborator` | triage/read collaborator | `repo` | none (may *propose*, needs approval) | |
| 174 | +| `external` | non-collaborator commenter | none | none | |
| 175 | +| `fork_pr` | PR whose head is a fork (`branch_scope.same_repo == false`) | `repo` (read-only context) | **none — hard rule** | |
| 176 | +| `scheduled` | Branch Gardener / cron (#14) | `repo` | `repo` (system-authored, no untrusted input) | |
| 177 | + |
| 178 | +The load-bearing rule, restated because it is the issue's sharpest risk: |
| 179 | + |
| 180 | +> **Untrusted PR content can never write durable memory.** For `fork_pr` and |
| 181 | +> `external`, `write_scopes` MUST be empty regardless of other policy, and the |
| 182 | +> adapter MUST reject any memory write in the result envelope. Fork content may |
| 183 | +> *inform* a review but MUST NOT *persist* into memory that shapes future |
| 184 | +> reviews — otherwise a hostile fork PR is a memory-poisoning vector. |
| 185 | +
|
| 186 | +## Result envelope: `memory_used` |
| 187 | + |
| 188 | +The runtime reports memory activity in `result.json` as an **optional** object |
| 189 | +(same additive-field discipline as the brief). Non-memory runs omit it or set |
| 190 | +`enabled: false`. |
| 191 | + |
| 192 | +```json |
| 193 | +{ |
| 194 | + "memory_used": { |
| 195 | + "enabled": true, |
| 196 | + "read": [ |
| 197 | + { "id": "repo/acme-inc/billing/conventions/rounding", "scope": "repo" } |
| 198 | + ], |
| 199 | + "proposed": [ |
| 200 | + { "key": "repo/acme-inc/billing/conventions/tax-tables", |
| 201 | + "summary": "Tax tables live in src/tax/tables.rs, not the DB.", |
| 202 | + "scope": "repo", "approval": "pending" } |
| 203 | + ], |
| 204 | + "rejected": [ |
| 205 | + { "summary": "Reviewer's personal email is …", |
| 206 | + "scope": "repo", "reason": "pii" } |
| 207 | + ] |
| 208 | + } |
| 209 | +} |
| 210 | +``` |
| 211 | + |
| 212 | +| Field | Type | Meaning | |
| 213 | +|---|---|---| |
| 214 | +| `read` | array | Memory entries loaded, by id + scope. Powers **citation**. | |
| 215 | +| `proposed` | array | Candidate writes with `approval` ∈ `pending` \| `applied` \| `auto`. | |
| 216 | +| `rejected` | array | Candidates the runtime itself declined, with `reason` (`pii`, `secret`, `out_of_scope`, `low_confidence`). | |
| 217 | + |
| 218 | +The adapter uses `read` to cite influencing memory in the Check Run / PR |
| 219 | +surface (#13's marker-backed status comment is the natural home), and validates |
| 220 | +`proposed` against `write_scopes` before anything is persisted. |
| 221 | + |
| 222 | +## Addressability: inspect and revoke |
| 223 | + |
| 224 | +Every durable memory key MUST be prefixed with its owning coordinates: |
| 225 | + |
| 226 | +``` |
| 227 | +repo/<owner>/<name>/<namespace>/<key> |
| 228 | +tenant/<installation_id>/<namespace>/<key> |
| 229 | +``` |
| 230 | + |
| 231 | +This is what makes "inspect and revoke by installation and repository" |
| 232 | +mechanically possible: Cave (once #3 lands the auth) enumerates `repo/<o>/<n>/*` |
| 233 | +to show a customer what a familiar knows about their repo, and deletes by |
| 234 | +prefix to revoke. `delete_on_uninstall` is a prefix delete over |
| 235 | +`tenant/<installation_id>/*` and every `repo/*` the installation covered. A key |
| 236 | +that is not prefix-addressable is non-conformant and MUST be rejected. |
| 237 | + |
| 238 | +## Redaction |
| 239 | + |
| 240 | +Memory writes pass through the adapter's existing secret scrubbing (the |
| 241 | +`redact` module from #4) before persistence when `retention.redact_secrets` is |
| 242 | +true (always, in hosted mode). A proposed memory whose text still matches a |
| 243 | +token/credential pattern after scrubbing is rejected with `reason: "secret"`. |
| 244 | +This closes the loop with the token-leak boundary: credentials cannot enter |
| 245 | +durable memory any more than they can enter a brief or a comment. |
| 246 | + |
| 247 | +## Adapter enforcement points |
| 248 | + |
| 249 | +1. **Brief construction** (`crates/worker/src/brief.rs`): compute and stamp |
| 250 | + `memory_policy`. Deny-by-default — `enabled` only when the installation |
| 251 | + policy opts in **and** tenant + repo scope are known **and** `trust_scope` |
| 252 | + permits. `fork_pr`/`external` never get write scopes. |
| 253 | +2. **Result validation** (`crates/worker/src/lib.rs`, alongside the existing |
| 254 | + `validate_result_contract`): reject the envelope's memory writes that fall |
| 255 | + outside `write_scopes`, that target a non-prefixed key, or that fail |
| 256 | + redaction; record each rejection. Out-of-scope writes are a **hard fail** of |
| 257 | + the memory portion — the review still publishes, but the write does not, and |
| 258 | + the rejection is auditable. |
| 259 | +3. **Approval routing** (#13): `approval == pending` writes surface to the |
| 260 | + maintainer via the `@familiar remember` / `forget` commands, which today |
| 261 | + no-op with a "lands with #6" acknowledgement. This contract is the write |
| 262 | + path those commands will drive. |
| 263 | + |
| 264 | +## Configuration surface |
| 265 | + |
| 266 | +Installation memory policy extends the existing per-repo policy shape (the |
| 267 | +`[review]` precedent from #10): |
| 268 | + |
| 269 | +```toml |
| 270 | +[memory] |
| 271 | +enabled = false # hosted opt-in; off by default |
| 272 | +approval_required = true |
| 273 | +retention_days = 365 |
| 274 | +[memory.repos."acme-inc/billing"] |
| 275 | +enabled = true # per-repo override |
| 276 | +``` |
| 277 | + |
| 278 | +`doctor` validates that `[memory] enabled = true` is paired with a hosted |
| 279 | +deployment (it is inert for self-hosted single-tenant use) and that retention |
| 280 | +values are sane. |
| 281 | + |
| 282 | +## What changes where (implementation preview) |
| 283 | + |
| 284 | +| Component | Change | |
| 285 | +|---|---| |
| 286 | +| `docs/contracts/session-brief.schema.json` | add optional `memory_policy` (coordinated bump with coven-code) | |
| 287 | +| `docs/contracts/result.schema.json` | add optional `memory_used` | |
| 288 | +| `crates/config` | `[memory]` policy + per-repo overrides + doctor checks | |
| 289 | +| `crates/worker/src/brief.rs` | compute + stamp `memory_policy` (deny-by-default, trust-gated) | |
| 290 | +| `crates/worker/src/lib.rs` | validate `memory_used`; reject out-of-scope/unredacted writes | |
| 291 | +| `crates/github` | `trust_scope` derivation from actor permission (reuses #13's permission lookup) | |
| 292 | +| Cave (`#3` + `#18`) | inspect/revoke surface over prefix-addressable keys | |
| 293 | + |
| 294 | +## Test plan (maps to the issue's criteria) |
| 295 | + |
| 296 | +- **Explicit policy present:** every hosted invocation's brief carries |
| 297 | + `memory_policy`; a fake `coven-code` asserts the fields it received. |
| 298 | +- **Fail-closed:** brief with missing tenant or repo scope → `enabled` forced |
| 299 | + false; runtime performs no memory op. |
| 300 | +- **Fork cannot write:** a `fork_pr` result envelope that proposes a durable |
| 301 | + write → adapter rejects the write, publishes the review, records the |
| 302 | + rejection. Same for `external`. |
| 303 | +- **Cross-installation / cross-repo isolation:** a result whose `read`/`proposed` |
| 304 | + keys are not prefixed by the invocation's own tenant+repo → rejected. |
| 305 | +- **Citation:** a review that read memory surfaces those ids on the status |
| 306 | + comment. |
| 307 | +- **Redaction:** a proposed memory containing a token pattern → rejected |
| 308 | + `secret` after scrubbing. |
| 309 | +- **Memory-disabled:** policy `enabled=false` → no `memory_used` writes accepted. |
| 310 | + |
| 311 | +## Phased PRs (coordinated with coven-code) |
| 312 | + |
| 313 | +1. **Contract + schemas.** This doc, plus the additive `memory_policy` / |
| 314 | + `memory_used` schema fields and Rust types, landed in lockstep with the |
| 315 | + coven-code side that accepts/emits them. No behavior yet. |
| 316 | +2. **Adapter policy + enforcement.** `[memory]` config, deny-by-default brief |
| 317 | + stamping with trust gating, and result-envelope validation (reject |
| 318 | + out-of-scope/unredacted/non-prefixed writes). Wires the #13 `remember` / |
| 319 | + `forget` approval path. |
| 320 | +3. **Inspect / revoke.** Cave surface over prefix-addressable keys (needs #3); |
| 321 | + `delete_on_uninstall`; retention expiry. |
| 322 | + |
| 323 | +Each PR keeps `cargo check/clippy/test` green and lands separately mergeable; |
| 324 | +Phase 1's schema change is bilateral and MUST NOT ship on one side alone. |
0 commit comments