# v1.11.5 hardening pass — Episode A/B/C critic deferrals (ROUND 3)

Status: Draft round 3 (after plan-eng-critic round 1 + round 2 FAILs)
Branch: `fix/v1.11.5-hardening-pass`
Episode: `01KSB6S9116WGMXW44YFKB1HTR`
Target: PATCH release v1.11.5 (additive + backward-compatible)
Scope: 7-item bundle (item #5 deferred), single PR, single critic pass per stage

## Critic-round-2 findings + resolutions

| Critic finding | Resolution |
|---|---|
| **CRIT A**: `VALID_AUDIT_OPS` Sets at `src/cli.ts:4679-4687` and `src/server.ts:70-78` are exhaustive allow-lists; adding `'consolidate'` to AuditOp union alone leaves rows writable but unqueryable | **Both Sets updated.** Plus the error message at cli.ts:4703 is regenerated from `Array.from(VALID_AUDIT_OPS).join(' | ')` to prevent future drift (the existing message already missed `auth_revoke` — fixed as a side-effect). Round-trip test added. |
| **CRIT B**: pseudocode used `openLocalDb` / `localDb.close()` — those names don't exist. Real names are `openHippoDb` / `closeHippoDb`, already imported at api.ts:11 | **Pseudocode rewritten** with correct names. Also restructured per MED F: single DB handle opened at top of try, reused in finally (no second migration pass per invocation). |
| **HIGH C**: isLoopback test cases for `undefined`→true and `'0:0:0:0:0:0:0:1'`→true don't match current behavior | **Test cases corrected** to match the 3 exact strings the helper accepts: `'127.0.0.1'`, `'::1'`, `'::ffff:127.0.0.1'` → true; everything else (incl. `undefined`, `'0:0:0:0:0:0:0:1'`, public IPs) → false. NO behaviour change to isLoopback itself (extending it is a security-adjacent decision out of scope here). |
| **HIGH D**: snapshot determinism: `printContextMarkdown` calls `new Date()` for `resolveConfidence(e, now)` driving `[verified]/[stale]/[inferred]` tags | **Mechanism specified**: each snapshot test runs inside `vi.useFakeTimers({ now: new Date('2026-05-23T20:00:00Z') })`. Tests `afterEach` calls `vi.useRealTimers()`. Documented in the test file. |
| **MED E**: cap at server.ts:721 incurs auth cost before failing | **Cap moved** to lines 707-717 region, AFTER array validation, BEFORE `buildContextWithAuth(req)`. |
| **MED F**: `openHippoDb` in finally adds 10-100ms per sleep | **Fixed in CRIT B resolution** — single handle reused. |
| **LOW**: Python SDK doesn't break (`op: str` at python/src/hippo_memory/models.py:298) | **Stated explicitly** in CHANGELOG and "Risk" section. |

## Round 1 findings + resolutions (kept from round-2 plan)

| Round-1 critic finding | Resolution |
|---|---|
| CRIT #1: DECISION #5 admin-role gate assumes Context.actor object shape — it's a string. Schema migration is 12+ call sites | #5 **DEFERRED to v1.12.0**; TODOS.md entry added |
| CRIT #2: `op: 'consolidate'` not in AuditOp union | Union extended additively (see CRIT A resolution above for the rest of the wiring) |
| HIGH: #6 partial-failure paths | try/finally in api.sleep |
| HIGH: #8 cli.ts render helpers not exported | `export` added to `printContextMarkdown` (cli.ts:3416) + `renderSleepResult` (cli.ts:2131), flagged in CHANGELOG |

## Items

### #1 — HTTP DoS cap on `POST /v1/outcome` `ids.length`

**Where:** `src/server.ts`, AFTER the array validation loop (~line 717), BEFORE `buildContextWithAuth(req)` at line 719.
**Change:**
```ts
// (existing validation block: ensure ids is array of strings)
if (ids.length > 1000) {
  throw new HttpError(400, 'ids exceeds 1000-id cap');
}
// (then buildContextWithAuth, etc.)
```
**Why 1000:** /v1/outcome WRITES per id (~3 DB ops × N). At N=10000 within 1MB body cap, 30k sequential DB ops per request. 1000 keeps work bounded.
**Why placement-before-auth:** auth cost (api-key hash lookup + tenant resolve) is non-trivial. Cap-before-auth means malicious traffic costs O(parse+cap-check) not O(parse+auth+cap-check).
**Tests** (`tests/server-outcome-route.test.ts`):
- 1001 ids → 400
- 1000 ids → 200
- 1000 large-id payload (≥1MB) → 400 from BODY cap first (locks ordering)

### #2 — HTTP DoS cap on `GET /v1/context` `q` length

**Where:** `src/server.ts:737`, mirroring scope-cap pattern at line 758.
**Change:**
```ts
if (q !== undefined && q.length > 1024) {
  throw new HttpError(400, 'q exceeds 1024-character cap');
}
```
**Tests:** 1025 chars → 400; 1024 → 200.

### #3 — `/v1/context?q=foo` recall-audit-row test gap

**Where:** `tests/server-context-route.test.ts`.
**Change:** seed 3 memories, snapshot `audit_log` row-count before, call `GET /v1/context?q=foo`, assert response shape AND **exactly one new row** with `op='recall'` + caller's tenant (row-count delta, not absolute — robust against other tests' rows in shared tmp DBs).

### #4 — `isLoopback` helper unit tests

**Status:** `isLoopback` exported at `src/server.ts:240`. No production change.
**Where:** NEW `tests/server-isloopback-helper.test.ts`.
**Test cases** (corrected against actual helper behaviour at server.ts:240-246):
- TRUE: `'127.0.0.1'`, `'::1'`, `'::ffff:127.0.0.1'` (the 3 strings the helper accepts)
- FALSE: `'192.168.1.1'`, `'10.0.0.1'`, `'8.8.8.8'`, `'::ffff:192.168.1.1'`, `'fe80::1'`, `'0:0:0:0:0:0:0:1'` (NOT a form the helper recognises today), `undefined`, `''`
**No behaviour change to isLoopback.** Extending it to normalise fully-expanded IPv6 loopback would be a security-adjacent change requiring its own decision.

### #5 — Per-tenant `/v1/sleep` scoping — **DEFERRED**

NOT in this episode. v1.12.0 work alongside A5 v2. TODOS.md entry below.

### #6 — `audit_log` emission on sleep consolidation phases (REVISED-FINAL)

**Required wiring (5 sites, includes pre-existing `'outcome'` Set drift the round-3 critic surfaced):**
1. `src/audit.ts:130-138` — add `'consolidate'` to AuditOp union (additive enum extension)
2. `src/cli.ts:4679-4687` — add **both** `'consolidate'` and `'outcome'` to VALID_AUDIT_OPS Set (`'outcome'` is in the AuditOp union at audit.ts:138 and rows are emitted today per server.ts:699, but the Set rejects it — pre-existing drift the plan closes alongside `'consolidate'`)
3. `src/server.ts:70-78` — add **both** `'consolidate'` and `'outcome'` to VALID_AUDIT_OPS Set (same drift, same fix)
4. `src/cli.ts:4702-4705` — regenerate error message from `Array.from(VALID_AUDIT_OPS).join(' | ')` so the message can't drift again (current message already missed `auth_revoke` AND `outcome`; this fix-as-side-effect tightens hygiene)
5. `src/api.ts:1929` region — emit the row inside api.sleep (try/finally; single DB handle dedicated to the audit emit — phase helpers continue opening their own handles via hippoRoot)

**api.sleep pseudocode (corrected — uses real `openHippoDb` / `closeHippoDb`; handle is DEDICATED to the audit emit, phase helpers continue opening their own handles via hippoRoot as today):**
```ts
export async function sleep(ctx: Context, opts: SleepOpts): Promise<SleepResult> {
  let phaseCounters = { consolidationCount: 0, dedupCount: 0, auditDeletedCount: 0, ambientTotal: 0 };
  let phaseError: Error | null = null;
  let result: SleepResult | null = null;
  try {
    // ... existing phases unchanged: consolidate, deduplicateStore, auditMemories,
    //     autoShare, computeAmbientState — each opens its own DB handle via
    //     hippoRoot as today. SQLite single-writer means parallel handles are
    //     fine for the read-heavy phases. Accumulate into phaseCounters from
    //     each phase's return value.
    result = { /* assembled SleepResult */ };
    return result;
  } catch (err) {
    phaseError = err as Error;
    throw err;
  } finally {
    // Dedicated handle for the audit emit only. Opened in finally so it runs
    // on both success AND partial-failure paths.
    const db = openHippoDb(ctx.hippoRoot);
    try {
      appendAuditEvent(db, {
        tenantId: ctx.tenantId,
        actor: ctx.actor,
        op: 'consolidate',
        metadata: {
          ...phaseCounters,
          dryRun: opts.dryRun ?? false,
          noShare: opts.noShare ?? false,
          partial: phaseError !== null,
          ...(phaseError ? { errorMessage: phaseError.message } : {}),
        },
      });
    } finally {
      closeHippoDb(db);
    }
  }
}
```
Note: `AppendAuditOpts` is `{tenantId, actor, op, targetId?, metadata?}` (no `targetType`). `targetId` omitted because consolidate is host-wide.

**Tests:**
- `tests/api-sleep.test.ts`: assert 1 row with `op='consolidate'` + metadata counters after each invocation; mid-phase failure path (stub `auditMemories.deleteEntry` throws → audit row still emits with `partial: true`)
- `tests/server-audit-route.test.ts`: round-trip `GET /v1/audit?op=consolidate` returns rows seeded via api.sleep (locks the VALID_AUDIT_OPS update)
- CLI audit smoke: `hippo audit list --op=consolidate` lists rows (manual verify gate)

### #7 — Lock `api.recall` last_retrieval_ids divergence via test

**Decision:** document + lock via test. NOT add side-effect to api.recall.
**Verified twice now:** only `api.getContext` (api.ts:1851) and `cmdRecall` (cli.ts:1282) write last_retrieval_ids.
**Where:**
1. NEW `tests/api-recall-no-side-effects.test.ts`
2. JSDoc note on api.recall (api.ts:377 region): "Does NOT mutate index.last_retrieval_ids; use api.getContext if downstream code needs the last-recall outcome path"
3. `python/README.md` Limitations section updated

**Test (qualified per critic round 2 #7 — only assert `last_retrieval_ids`, not all fields):**
```ts
test('api.recall does NOT write last_retrieval_ids (cmdRecall divergence)', () => {
  // seed memories
  const before = loadIndex(ctx.hippoRoot).last_retrieval_ids;
  expect(before).toEqual([]);
  api.recall(ctx, { q: 'foo', limit: 5 });
  expect(loadIndex(ctx.hippoRoot).last_retrieval_ids).toEqual(before);
  await api.getContext(ctx, { q: 'foo', budget: 1000 });
  expect(loadIndex(ctx.hippoRoot).last_retrieval_ids).not.toEqual(before);
});
```

### #8 — CLI render snapshot tests (REVISED-FINAL)

**Verified:** `printContextMarkdown` (cli.ts:3416) + `renderSleepResult` (cli.ts:2131) are internal today. **Add `export` to both.** Flagged in CHANGELOG. JSDoc `@internal` keeps them unstable.

**Determinism mechanism (per critic round 2 HIGH D):**
- Snapshot test file uses `vi.useFakeTimers({ now: new Date('2026-05-23T20:00:00Z') })` in `beforeEach`
- `afterEach` calls `vi.useRealTimers()`
- Fixed memory ids in the seed data (no ULID generation)
- Fixed `created`/`updated` timestamps on memories
- Rationale: `resolveConfidence(e, now)` at cli.ts:3421 drives `[verified]/[stale]/[inferred]` tags; without fake timers, snapshots churn nondeterministically on every CI run

**Where:** NEW `tests/cli-context-render-snapshot.test.ts`.
**Snapshots (12):**
- pinnedOnly markdown
- markdown default (no q, no pinned)
- json format
- additional-context format
- framing observe (default)
- framing suggest
- framing assert
- query='*' fallback
- hybrid local-only
- hybrid with global
- renderSleepResult dry-run
- renderSleepResult full (note: consolidate metadata lives in audit_log row, NOT SleepResult; this snapshot is unaffected by #6 and only exercises the existing render output)

## Out of scope

- Item #5 per-tenant /v1/sleep scoping (deferred to v1.12.0)
- Non-loopback serving (`HIPPO_BIND_ALL`)
- Multi-role RBAC (A5 v2)
- Extending isLoopback to recognise additional IPv6 forms (security-adjacent decision)
- Adding side-effects to `api.recall` (rejected at #7)
- F9 hybrid retrieval

## Verify gates

- `npm run build` clean (TypeScript strict)
- `npm test` full suite green; ~27 new test cases across 4 new files + 5 edited files (1657 → ~1684)
- Manual smoke:
  - POST /v1/outcome with 1001 ids → 400 (BEFORE auth path)
  - GET /v1/context?q=<1025 chars> → 400
  - GET /v1/context?q=foo → 200 + 1 audit row `op=recall`
  - POST /v1/sleep → 200 + 1 audit row `op=consolidate` with phase counters
  - `hippo sleep` (CLI) → same audit row, `actor='cli'`
  - `hippo audit list --op=consolidate` → lists rows (locks Set wiring)
  - `GET /v1/audit?op=consolidate` → returns same rows

## Ship gates

- CHANGELOG entry under `## 1.11.5 (2026-05-23): Episode A/B/C critic-deferral hardening`. Calls out:
  - additive `AuditOp` extension: `'consolidate'` (Python SDK unaffected per python/src/hippo_memory/models.py:298 `op: str`)
  - additive cli.ts exports: `printContextMarkdown`, `renderSleepResult` (`@internal`, unstable)
  - 2 new HTTP 400 conditions on /v1/outcome and /v1/context
  - new audit_log emission on `hippo sleep` / POST /v1/sleep / cli `hippo sleep`
  - new filter value `--op=consolidate` on `hippo audit list` and `GET /v1/audit`
  - deferred #5 per-tenant sleep scoping with critic-round-1 rationale
- Version bump: **5 manifests per `src/version.ts:4-8` header** — `package.json`, `src/version.ts`, root `openclaw.plugin.json`, `extensions/openclaw-plugin/package.json`, `extensions/openclaw-plugin/openclaw.plugin.json` + lockfile. Version-parity guard test passes.
- ship-readiness-critic gate green.

## File map (FINAL — 14 files, 1 added since round 2 for server-audit-route round-trip test)

```
src/server.ts                                  +cap on /v1/outcome ids (pre-auth), +cap on /v1/context q, +'consolidate' in VALID_AUDIT_OPS
src/cli.ts                                     +export printContextMarkdown + renderSleepResult, +'consolidate' in VALID_AUDIT_OPS, regenerated error message
src/audit.ts                                   +'consolidate' in AuditOp union
src/api.ts                                     +try/finally appendAuditEvent in api.sleep (single openHippoDb handle reuse), +JSDoc on api.recall
tests/server-outcome-route.test.ts             +1001-ids → 400, +1MB body-cap interaction
tests/server-context-route.test.ts             +1025-char-q → 400, +q=foo recall-audit-row-count-delta test
tests/server-sleep-route.test.ts               (no change — #5 deferred)
tests/server-audit-route.test.ts               +consolidate round-trip test (locks VALID_AUDIT_OPS wiring)
tests/server-isloopback-helper.test.ts         NEW — 11 cases (corrected per critic round 2)
tests/api-sleep.test.ts                        +consolidate audit_log row + mid-phase failure test
tests/api-recall-no-side-effects.test.ts       NEW — locks divergence (only last_retrieval_ids field asserted)
tests/cli-context-render-snapshot.test.ts      NEW — 12 snapshots inside vi.useFakeTimers
python/README.md                               +Limitations note on api.recall divergence
TODOS.md                                       +note on #5 defer to v1.12.0 with critic-round-1 rationale
CHANGELOG.md                                   +1.11.5 entry
package.json                                   +1.11.5 version bump
src/version.ts                                 PACKAGE_VERSION → 1.11.5
openclaw.plugin.json (root)                    +1.11.5
extensions/openclaw-plugin/package.json        +1.11.5
extensions/openclaw-plugin/openclaw.plugin.json +1.11.5
package-lock.json                              +1.11.5
```

Total: 5 version-bearing manifests + lockfile.

## Risk

- **AuditOp + 2 Sets + error message**: 4 sites need to stay in sync. The new round-trip test (`GET /v1/audit?op=consolidate` returns api.sleep-seeded rows) locks this. The error-message regeneration from `Array.from(VALID_AUDIT_OPS)` prevents future drift on the message side.
- **`appendAuditEvent` in finally with shared `db` handle**: if the in-try code path opens additional handles or closes `db` early, the finally emit fails. Mitigation: api.sleep doesn't currently close its handles (other api functions own that); the new try opens ONE handle and the finally is sole owner of close.
- **`vi.useFakeTimers` interaction with async**: if a snapshot test awaits a promise that resolves on a real timer, `useFakeTimers` would hang. Mitigation: render helpers are synchronous; `getContext` (which the seed step uses) doesn't await timer-driven code.
- **Python SDK compatibility**: confirmed unaffected — `AuditEvent.op` is typed `op: str` at python/src/hippo_memory/models.py:298. No SDK update needed for this release.

## Effort

~2-3d wall time. ~600 lines total across 14 files (was 500 / 13; +1 file for the audit-route round-trip test).
