# v1.12.0 sub-episode 1: auth/role plumbing for A5 v2

Status: Draft round 2 (after plan-eng-critic round 1 FAIL — 2 CRITs + 4 HIGHs + 2 MEDs, all fixed below)

## Round 1 critic findings + resolutions

| Finding | Resolution |
|---|---|
| CRIT #1: migrations are in-code `MIGRATIONS[]` in `src/db.ts:33` (CURRENT_SCHEMA_VERSION=25), NOT a `src/migrations/` SQL-file directory | Rewrote §"Schema migration" to add `version: 26` entry to MIGRATIONS array. No SQL file. |
| CRIT #2: 5 missed Cat-A Context constructor sites; `actor: ctx.actor \|\| 'connector:slack'` fallback breaks under object actor (truthy object skips OR-fallback, connectors lose audit identity) | Enumerated all 5 sites + replaced the broken `\|\|` fallback pattern with explicit `subject` handling in §"Construction site enumeration". |
| HIGH #3: `McpContext` is a separate type at `mcp/server.ts:80-91` with 5 construction sites | Added §"McpContext decision" section: keep McpContext.actor as `string`, MCP construction sites pass `ctx.actor.subject` (avoids McpContext shape change, minimises blast radius). HTTP-MCP bridge at server.ts:1467 passes `.subject`. |
| HIGH #4: api.ts:1342 is the 13th `ctx.actor` site (not 12) | Added to Category C list. |
| HIGH #5: server.ts:1467 HTTP-MCP bridge | Resolved per HIGH #3 — passes `ctx.actor.subject`. |
| HIGH #6: 35 string-literal `actor:` sites across ~15 test files; plan hand-waved | §"Test surface enumeration" lists test files concretely from grep. |
| MED #7: Python SDK + audit.ts read-path notes | Added to §"Risk". |


Branch: `feat/v1.12.0-actor-role`
Episode: `01KSBCVZM8022ESHMPF70NQZDH`
Target: v1.12.0 MINOR (additive enum role on api_keys + Context.actor shape change = type-public-API addition, NOT breaking at data layer)
Scope: Sub-1 of 2 in the v1.12.0 A5 v2 cluster. Sub-2 (L9 background pipelines) deferred to a later session, depends on this Context.actor shape.

## Brainstorm carry-forwards (verbatim from episode brainstorm step)

Per the skill's resona-Phase-B2 rule: every concern surfaced in brainstorm must be either ADDRESSED in this plan or explicitly noted as out-of-scope.

1. **Legacy keys backfill role='admin'.** Single-tenant deployments today have one key = operator = admin by definition. Migration backfills with `DEFAULT 'admin'`. CLI `actor: 'cli'` + MCP `actor: 'mcp'` also build Context with `role: 'admin'` (process-local trusted invocations). **Addressed**: §"Type changes" + §"Schema migration".
2. **audit_log.actor column stays TEXT subject-only.** Role is request-time authorization, not historical accounting. Audit helpers continue accepting `actor: string` for the column. **Addressed**: §"Audit helper signatures unchanged".
3. **`hippo auth create-key --role` CLI flag DEFERRED.** Tests construct member keys via direct DB insert. Avoid v1.11.5-style scope creep on CLI surface. **Addressed**: §"Out of scope" + §"Test strategy".
4. **Role union = 'admin' | 'member' only.** Editor/viewer tiers in A5 v2 v2. **Addressed**: §"Type changes".
5. **Migration additive + reversible.** Column has DEFAULT; rollback to v1.11.x leaves the column orphaned but harmless. **Addressed**: §"Schema migration" + §"Risk".
6. **External Context constructors must be enumerated.** MCP server + connectors + CLI hardcoded `actor: 'cli'` strings — all need either Context construction updates OR audit helper subject-only passes. **Addressed**: §"Construction site enumeration" below.
7. **Branch:** `feat/v1.12.0-actor-role`. **Created** (verified at episode discover stage).
8. **No audit_log emission on /v1/sleep 403 path.** Rate-limiter already handles attack-traffic accounting. **Addressed**: §"Admin-role gate".

## Construction site enumeration (REVISED — round-1 critic surfaced 5 more Cat-A sites + 1 more Cat-C)

Three categories of `actor` reference, each handled differently:

**Category A: Context constructors (these change to build {subject, role}). 6 sites total.**
- `src/server.ts:306` `buildContextWithAuth` — Bearer path reads role from `ValidateResult`; loopback fallback `{subject: 'localhost:cli', role: 'admin'}`. **Update.**
- `src/server.ts:1039` slack-connector route: `actor: 'connector:slack'` → `actor: {subject: 'connector:slack', role: 'admin'}` (connectors are process-local = admin trust level).
- `src/server.ts:1306` github-connector route: same pattern as slack.
- `src/cli.ts:4735` CLI shared Context: `const ctx: api.Context = { hippoRoot: root, tenantId, actor: 'cli' }` → `actor: {subject: 'cli', role: 'admin'}`.
- `src/connectors/slack/ingest.ts:73` **CRITICAL — breaking logic bug**: `{ ...ctx, actor: ctx.actor || 'connector:slack' }`. The `||` fallback evaluates an object as truthy, so the new `ctx.actor` (object) skips the fallback and the connector loses its identity in the audit_log. **FIX (picked): drop the fallback entirely.** server.ts:1039 and 1306 always construct ctx and pass it in — verified — so the fallback is dead code. Replace with: `{ ...ctx }` (just the spread, ctx.actor already has the correct connector subject from the route Context construction at server.ts:1039/1306).
- `src/connectors/github/ingest.ts:167` same pattern + same fix.

**Category A.1: McpContext separate type — kept as string.**
- `src/mcp/server.ts:80-91` defines `interface McpContext { actor: string; ... }` independent of `api.Context`.
- **Decision: McpContext.actor stays string.** MCP server internal type, minimises blast radius. 5 MCP construction sites (mcp/server.ts:448, 553, 581, 621, 658) keep passing strings.
- The HTTP-MCP bridge at `src/server.ts:1467` translates `api.Context.actor` → `McpContext.actor` by passing `ctx.actor.subject` (string extraction at boundary).

**Category B: Audit-helper actor strings (these stay as strings).**
- `cli.ts:209,2201,2628,2657,3302,3809,4479,4506,4562,4656` and similar — pass `actor: 'cli'` to `appendAuditEvent({actor, ...})`. The helper's `actor: string` parameter is unchanged (per brainstorm decision #2). No edit needed.
- `audit.ts:143` `AppendAuditOpts.actor: string` — **unchanged**.
- `store.ts:37` `writeEntry`'s `actor` parameter — **unchanged** (continues accepting strings).
- `audit.ts:181, 208, 217` AuditLogEntry read path — **unchanged**, stays string-only.

## api.ts call site updates (14 sites — Category C reconciled from round-2 critic count discrepancy)

**Category C: api.ts `ctx.actor` reads (these change from `ctx.actor` to `ctx.actor.subject`). 14 sites total.**
- Lines: 198, 632, 1088, 1091, 1130, 1178, 1184, 1263, 1268, **1342**, 1485, 1807, 1820, 2063. (Round 1 missed 1342 — `who: ctx.actor` in the assemble drill path. Total was understated as 12 in v1; round-2 count = 14.)
- Each currently passes `actor: ctx.actor` (string→string) to a helper that expects string. **Update each to `actor: ctx.actor.subject`.** Audit-helper signatures unchanged; only the source-of-subject changes.

## McpContext decision (NEW — round-1 critic HIGH #3)

`src/mcp/server.ts:80-91` defines its own `interface McpContext` independent of `api.Context`. Two options:
- (a) Promote McpContext.actor to Actor object — symmetry with api.Context, but ripples through all MCP internals.
- (b) **Keep McpContext.actor as string** (PICKED). MCP doesn't enforce role-based auth itself (it's a stdio transport, not HTTP); the role gate only matters at /v1/sleep. Boundary translation: HTTP-MCP bridge at server.ts:1467 calls `ctx.actor.subject` to extract the string for McpContext.

5 MCP construction sites (mcp/server.ts:448, 553, 581, 621, 658) continue passing strings to McpContext. Zero change to MCP-side code.

## Type changes

```ts
// src/api.ts:71 (Context interface)
export interface Actor {
  subject: string; // 'cli' | 'localhost:cli' | 'api_key:<key_id>' | 'mcp'
  role: 'admin' | 'member';
}

export interface Context {
  hippoRoot: string;
  tenantId: string;
  actor: Actor;
}
```

`Actor` is exported so external Context constructors (test helpers, MCP server) can use it. Role union deliberately small for v1.12.0; richer RBAC is A5 v2 v2.

```ts
// src/auth.ts:64-68 (ValidateResult)
export interface ValidateResult {
  valid: boolean;
  tenantId?: string;
  keyId?: string;
  role?: 'admin' | 'member';  // NEW
}
```

`validateApiKey` extends its SELECT and return:
```ts
// src/auth.ts:80 region
const row = db
  .prepare(`SELECT key_hash, tenant_id, revoked_at, role FROM api_keys WHERE key_id = ?`)
  .get(keyId) as { key_hash: string; tenant_id: string; revoked_at: string | null; role: string } | undefined;
// ...
return { valid: true, tenantId: row.tenant_id, keyId, role: row.role as 'admin' | 'member' };
```

`createApiKey` extends its INSERT (auth.ts:59 region):
```ts
// BEFORE: INSERT INTO api_keys (key_id, key_hash, tenant_id, label, created_at) VALUES (?, ?, ?, ?, ?)
// AFTER:  INSERT INTO api_keys (key_id, key_hash, tenant_id, label, created_at, role) VALUES (?, ?, ?, ?, ?, ?)
//         + bind 'admin' as 6th param (or accept opts.role default 'admin')
```

**Boot-order guarantee (round-2 critic MED #3):** `openHippoDb` at `src/db.ts:905` runs `runMigrations(db)` synchronously BEFORE returning the db handle to callers. So when v1.12.0 binary opens a v1.11.x schema, migration 26 runs (adds the role column with DEFAULT 'admin') BEFORE the first `createApiKey` INSERT is attempted. The new 6-column INSERT is safe. If we instead kept the 5-column INSERT and relied on DEFAULT for new rows, that would also work — but explicit is better than implicit.

## Schema migration (REVISED — round-1 critic CRIT #1)

Migrations are **in-code** as objects in `src/db.ts:33` (`const MIGRATIONS: Migration[]`, type `{version, up}`). NOT a SQL-file directory. `CURRENT_SCHEMA_VERSION = 25` at db.ts:26. Last migration entry: `version: 25` at db.ts:819. The api_keys table is created in `version: 24` at db.ts:396-410 with no role column.

**Add a new entry at the end of the MIGRATIONS array:**

```ts
// src/db.ts (append to MIGRATIONS array)
{
  version: 26,
  up: (db) => {
    db.exec(`ALTER TABLE api_keys ADD COLUMN role TEXT NOT NULL DEFAULT 'admin'`);
  },
},
```

**Also update `CURRENT_SCHEMA_VERSION = 25` → `26`** at db.ts:26.

- Additive: existing rows get `role='admin'` via DEFAULT. Single-tenant operator = admin by definition (brainstorm decision #1).
- Old binaries (v1.11.x) silently ignore the column on SELECTs that don't name it; INSERTs without it use DEFAULT.
- New binaries on old data: schema version sentinel triggers the migration on next `openHippoDb`.
- Reversibility: this project's Migration type has no `down` function — rollback is "deploy old binary, ignore the column". `ALTER TABLE ... DROP COLUMN` is supported by SQLite 3.35+ but not part of the migration infrastructure.

## `buildContextWithAuth` changes

```ts
// src/server.ts:306 — Bearer path
if (auth.kind === 'bearer') {
  const db = openHippoDb(hippoRoot);
  try {
    const result = validateApiKey(db, auth.token);
    if (!result.valid || !result.tenantId || !result.keyId || !result.role) {
      throw new HttpError(401, 'invalid api key');
    }
    return {
      hippoRoot,
      tenantId: result.tenantId,
      actor: {
        subject: `api_key:${result.keyId}`,
        role: result.role,  // from validateApiKey's SELECT
      },
    };
  } finally {
    closeHippoDb(db);
  }
}

// Loopback fallback (existing)
return {
  hippoRoot,
  tenantId: resolveTenantId({}),
  actor: { subject: 'localhost:cli', role: 'admin' },  // process-local = admin
};
```

## Admin-role gate on POST /v1/sleep

```ts
// src/server.ts:791 region, AFTER the existing loopback guard
if (method === 'POST' && path === '/v1/sleep') {
  if (!isLoopback(req.socket.remoteAddress)) {
    throw new HttpError(403, '/v1/sleep is loopback-only ...');
  }
  const ctx = buildContextWithAuth(req, opts.hippoRoot);
  if (ctx.actor.role !== 'admin') {
    throw new HttpError(403, '/v1/sleep requires admin role');
  }
  // ... existing body parsing + api.sleep call ...
}
```

No audit_log emission on the 403 path per brainstorm decision #8.

## api.ts call site updates pattern (Category C — 14 sites detail)

Pattern (verified at line 198, applies identically at all 14):
- BEFORE: `writeEntry(ctx.hippoRoot, entry, { actor: ctx.actor, ... })`
- AFTER:  `writeEntry(ctx.hippoRoot, entry, { actor: ctx.actor.subject, ... })`

Same for `appendAuditEvent({actor: ctx.actor, ...})` → `{actor: ctx.actor.subject, ...}`.

Mechanical sed-able replacement: `ctx.actor` → `ctx.actor.subject` everywhere in api.ts. Verify each site with a Read pass (do NOT bulk-sed; sites where `ctx.actor` itself is an Actor — e.g. if we ever pass the whole Actor object — would break).

## Test strategy + test surface enumeration (REVISED — round-1 critic HIGH #6)

Real-DB per project convention. New tests:

1. `tests/auth-role-migration.test.ts` (NEW) — schema version-26 up on real DB; existing keys backfilled to admin; new column non-null; default 'admin' applied.
2. `tests/server-sleep-admin-gate.test.ts` (NEW) — extends `tests/server-sleep-route.test.ts` patterns. (a) admin Bearer → 200; (b) member Bearer → 403 with message; (c) loopback no-Bearer → 200 (existing behaviour); (d) non-loopback admin → 403 (existing loopback-only gate fires first). Member key constructed via direct DB insert (no CLI flag per brainstorm decision #3).
3. `tests/api-context-actor-shape.test.ts` (NEW) — small unit-style real-DB test that builds a Context, asserts `actor.subject` and `actor.role` are readable, and a helper still accepts `actor.subject` as a string.

**Existing tests requiring Context shape updates (enumerated from grep, ~15 files, ~35 sites):**

Per `grep -lrn "actor:.*'cli'|actor:.*'mcp'|actor:.*'localhost'" tests/`:
- `tests/api-context.test.ts`
- `tests/api-domain.test.ts`
- `tests/api-outcome-for-last-recall.test.ts`
- `tests/api-recall-no-side-effects.test.ts`
- `tests/api-remember.test.ts`
- `tests/api-sleep.test.ts`
- `tests/api-tenant-deny.test.ts`
- `tests/audit.test.ts`
- `tests/b3-goal-recall-log.test.ts`
- `tests/b3-outcome-end-to-end.test.ts`
- `tests/b3-outcome-propagation.test.ts`
- `tests/b3-recall-active-goals.test.ts`
- `tests/b3-retrieval-policy.test.ts`
- `tests/b3-review-fixes.test.ts`
- `tests/dag-v163-review-patch.test.ts`
- `tests/github-dlq.test.ts`
- `tests/goals-complete-no-propagate.test.ts`
- `tests/mcp-assemble.test.ts`
- `tests/mcp-context-scope.test.ts`
- `tests/mcp-drill.test.ts`
- (plus any further matches — execute stage greps fresh and updates all)

**Update pattern** for each site: `{ hippoRoot, tenantId, actor: 'cli' }` → `{ hippoRoot, tenantId, actor: { subject: 'cli', role: 'admin' } }`. Mechanical sed-able, but each site needs a Read to confirm no `actor: ctx.actor || '...'` fallback pattern (the connector-style bug class).

Defer (NOT in this episode):
- `hippo auth create-key --role` CLI flag.
- `hippo auth list` showing role column (cosmetic — could be follow-up).
- E2E test where a non-admin api_key tries to dedupe.

Defer (NOT in this episode):
- `hippo auth create-key --role` CLI flag.
- `hippo auth list` showing role column (cosmetic — could be follow-up).
- E2E test where a non-admin api_key tries to dedupe.

## File map (REVISED — round-1 critic findings #1, #2, #3, #4, #5, #6)

```
src/api.ts                                    +Actor interface, Context.actor shape, 13 ctx.actor.subject substitutions
src/auth.ts                                   +role in ValidateResult, +SELECT role in validateApiKey, +role in createApiKey insert (default 'admin')
src/db.ts                                     +MIGRATIONS entry version 26, CURRENT_SCHEMA_VERSION 25→26
src/server.ts                                 +buildContextWithAuth builds Actor object, +admin gate on /v1/sleep, +1467 HTTP-MCP bridge passes ctx.actor.subject, +1039/1306 connector route Context constructions
src/cli.ts                                    +4735 CLI Context constructor with Actor object
src/connectors/slack/ingest.ts                +73 fix broken ctx.actor||'connector:slack' fallback
src/connectors/github/ingest.ts               +167 same fix
src/mcp/server.ts                             (no type change; 5 construction sites unchanged per McpContext-stays-string decision)
tests/auth-role-migration.test.ts             NEW
tests/server-sleep-admin-gate.test.ts         NEW
tests/api-context-actor-shape.test.ts         NEW
tests/<existing>.test.ts × 15+                Update Context constructions per enumeration above
CHANGELOG.md                                  +1.12.0 entry
package.json + src/version.ts + 3 openclaw manifests + lockfile    +1.12.0 (5 manifests + lockfile per v1.11.5 lesson)
```

Total: ~25-30 files (8 src + ~18 test + CHANGELOG + 6 version bumps). Larger than v1.11.5's 21-file diff.

## Out of scope

- L9 background pipelines tenant-scoping (sub-episode 2).
- `hippo auth create-key --role` CLI flag.
- `hippo auth list` role column display.
- Multi-role RBAC beyond admin/member (editor/viewer/etc.).
- audit_log JSON-shape actor column.
- /v1/sleep audit emission on 403 path.
- Non-loopback serving (HIPPO_BIND_ALL).
- Per-tenant `/v1/sleep` semantics (tenant plumbing into deduplicateStore — A5 v2 v2 territory).

## Verify gates

- `npm run build` clean (TypeScript strict).
- `npm test` full suite green; expect ~10-15 new test cases plus updated Context constructions in existing tests.
- Migration up + down round-trip on real SQLite (open DB at v1.11.5 schema, run migration, verify role column added; revert, verify clean).
- Manual smoke:
  - `hippo serve` + admin Bearer to POST /v1/sleep → 200
  - same with member Bearer → 403
  - loopback POST /v1/sleep (no Bearer) → 200 (loopback-only fallback admin)
  - existing audit_log rows unchanged

## Ship gates

- CHANGELOG entry under `## 1.12.0 (2026-05-23 or later): A5 v2 auth/role plumbing`. Calls out: additive Actor type, additive role column on api_keys, admin-role gate on POST /v1/sleep, backward-compat for legacy keys (defaulted to admin).
- Version bump: 5 manifests + lockfile per v1.11.5 lesson.
- ship-readiness-critic green.

## Risk (REVISED)

- **api.ts 13-site mechanical replacement.** `ctx.actor` → `ctx.actor.subject`. TypeScript catches type errors. Each site Read'd in execute — no bulk-sed.
- **Connector fallback `||` pattern was a logic bug.** `actor: ctx.actor || 'connector:slack'` — under object actor, the OR-fallback NEVER fires (object is truthy). Connectors lose audit identity. Fix is per §"Construction site enumeration Category A": drop the fallback entirely (ctx is always provided in these sites) or use explicit subject check.
- **McpContext stays string.** Boundary translation at server.ts:1467 (`ctx.actor.subject`). MCP server internals unchanged.
- **Test surface (~15-20 files, ~35 sites).** Each Context construction in tests needs the new shape. Mechanical but tedious.
- **Migration: in-code MIGRATIONS array, NOT a SQL file.** version: 26 entry + CURRENT_SCHEMA_VERSION 25→26.
- **Python SDK unaffected.** `python/src/hippo_memory/models.py:297` types audit-log response `actor: str` — HTTP-only client, no Context construction. No SDK update needed.
- **audit.ts read-path AuditLogEntry stays string-only.** §"Out of scope" item; the listing/CLI read path is NOT touched by this episode.

## Effort

Single episode plausible but borderline. ~3-5d wall time on a 25-file diff. Plan-eng-critic decides if scope is right or if further decomposition (e.g. defer member-role tests to a separate small follow-up) is needed.
