# v1.7.3 Review-Tail Implementation Plan

> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task.

**Goal:** Close the four lower-confidence items deferred from the v1.7.2 review chain. Hygiene release. No public API change. No schema change. No behaviour change.

**Architecture:** Two TDD test additions, one internal-only parameter rename, one README docs backfill. Each slice is independent and lands as a single atomic commit. Ship as v1.7.3 once all four are green.

**Tech Stack:** TypeScript 5, vitest, real SQLite (better-sqlite3) — no mocks per repo rule.

**Branch:** `feat/v1.7.3-review-tail` off `master`. Do not commit on master.

---

## Pre-flight (run once before Task 1)

```bash
git status --short
git switch -c feat/v1.7.3-review-tail
npm install
npx vitest run --reporter=dot 2>&1 | tail -10
```

Expected:
- Clean working tree (the only untracked items should be unrelated to this work).
- New branch created.
- All tests green on baseline. **If anything is red, STOP and report — do not start v1.7.3 on a red baseline.**

---

## Task 1: Module-load assertion runtime test (codex P1-3)

**Why:** `src/store.ts:226-230` throws if `RECALL_DEFAULT_DENY_SCOPES` is ever blanked. Existing test `tests/api-recall-deny-scopes-source-of-truth.test.ts:39` only pins `length > 0` — never exercises the throw. A future maintainer who emptied the array would not be caught.

**Approach:** Extract a small `assertNonEmpty<T>` helper from the inline guard and unit-test the helper directly. Module-load-time throws are awkward to mock cleanly across vitest workers — extracting the helper is the simpler, lower-blast-radius path. Helper stays internal (`@internal`, NOT re-exported from `src/index.ts`).

**Files:**
- Modify: `src/store.ts:222-230` — extract `assertNonEmpty`, replace inline guard with a call
- Create: `tests/store-assert-non-empty.test.ts` — direct unit test on the helper

### Step 1: Write the failing test

Create `tests/store-assert-non-empty.test.ts`:

```ts
/**
 * v1.7.3 — runtime test for the module-load throw path on
 * RECALL_DEFAULT_DENY_SCOPES. Codex P1-3 from v1.7.2 review.
 *
 * The inline guard fires on module import; direct testing of the constant
 * is impossible (the throw runs before any test code). The guard is
 * extracted as `assertNonEmpty(arr, name)` and tested here directly.
 */

import { describe, it, expect } from 'vitest';
import { assertNonEmpty } from '../src/store.js';

describe('assertNonEmpty (v1.7.3 review-tail)', () => {
  it('throws when array is empty', () => {
    expect(() => assertNonEmpty([], 'TEST_CONST')).toThrow(
      /TEST_CONST cannot be empty/,
    );
  });

  it('does not throw when array has at least one element', () => {
    expect(() => assertNonEmpty(['x'], 'TEST_CONST')).not.toThrow();
  });

  it('handles readonly arrays without widening at the call site', () => {
    const arr = ['unknown:legacy'] as const;
    expect(() => assertNonEmpty(arr as readonly string[], 'TEST')).not.toThrow();
  });
});
```

### Step 2: Run the test to verify it fails

```bash
npx vitest run tests/store-assert-non-empty.test.ts
```

Expected: FAIL — `assertNonEmpty is not exported from '../src/store.js'`.

### Step 3: Implement helper + replace inline guard

Edit `src/store.ts`. The current block at lines 222-230 is:

```ts
// Cast to readonly string[] — `as const` makes this `readonly ['unknown:legacy']`
// with literal length 1, so a direct `.length === 0` is "unreachable" per TS
// even though the assertion is a real runtime guard against future maintainers
// blanking the array. Cast widens the type so the check compiles.
if ((RECALL_DEFAULT_DENY_SCOPES as readonly string[]).length === 0) {
  throw new Error(
    'RECALL_DEFAULT_DENY_SCOPES cannot be empty — would silently allow quarantine scopes',
  );
}
```

Replace with:

```ts
/**
 * @internal v1.7.3 — runtime guard against a future maintainer blanking a
 * load-bearing literal array. Extracted from the inline guard so the throw
 * path is directly testable. `as const` arrays widen via `readonly T[]` at
 * the call site so the empty case is reachable at runtime.
 */
export function assertNonEmpty<T>(arr: readonly T[], name: string): void {
  if (arr.length === 0) {
    throw new Error(
      `${name} cannot be empty — would silently allow quarantine scopes`,
    );
  }
}

assertNonEmpty(
  RECALL_DEFAULT_DENY_SCOPES as readonly string[],
  'RECALL_DEFAULT_DENY_SCOPES',
);
```

### Step 4: Run tests to verify they pass

```bash
npx vitest run tests/store-assert-non-empty.test.ts tests/api-recall-deny-scopes-source-of-truth.test.ts
```

Expected: PASS — 3 new tests + all pre-existing pass. The existing `length > 0` assertion still passes (constant unchanged).

### Step 5: Type-check + full test sweep

```bash
npx tsc --noEmit
npx vitest run --reporter=dot 2>&1 | tail -5
```

Expected: zero TS errors, full suite green.

### Step 6: Commit

```bash
git add src/store.ts tests/store-assert-non-empty.test.ts
git commit -m "test: assertNonEmpty throw-path test for RECALL_DEFAULT_DENY_SCOPES (v1.7.3)"
```

**Success criteria:**
- `assertNonEmpty<T>` exported from `src/store.ts`, `@internal`, NOT in `src/index.ts`.
- 3 new test cases pass.
- Existing tests untouched and still green.
- One atomic commit.

---

## Task 2: `summarize_overflow=0` (false path) thin-client test (codex P2-3)

**Why:** `tests/client-recall-opts-parity.test.ts` covers two cases — all four params present (`summarize_overflow=1`) and all four undefined (omitted). It does NOT cover `summarizeOverflow: false`, which should serialize `summarize_overflow=0` (not omitted, not `=1`). A future regression that treated `false` as "undefined" would silently drop the off-signal.

**Approach:** Add a third `it()` block to the existing file. If the test passes immediately, it's a pure pin (existing behaviour is correct). If it fails, that itself is the regression we want to know about — Step 4 branches into pin-or-fix.

**Files:**
- Modify: `tests/client-recall-opts-parity.test.ts` — add third `it` block after line 73
- (Conditional) Modify: `src/client.ts` — only if the test fails, indicating a real bug

### Step 1: Verify current `client.ts` serialization shape (sanity check)

```bash
grep -n "summarize_overflow\|summarizeOverflow" src/client.ts
```

Read the matching lines. Confirm the serialization branch shape (e.g. `if (opts.summarizeOverflow !== undefined) params.set('summarize_overflow', opts.summarizeOverflow ? '1' : '0')` vs the foot-gun `if (opts.summarizeOverflow) params.set('summarize_overflow', '1')`). Note finding for use in Step 4.

### Step 2: Write the test

In `tests/client-recall-opts-parity.test.ts`, after the second `it()` block (closes around line 73), insert before the final `});`:

```ts
  it('serializes summarize_overflow=0 when summarizeOverflow is false (not omitted)', async () => {
    originalFetch = globalThis.fetch;
    const fetchSpy = vi.fn(() =>
      Promise.resolve(
        new Response(JSON.stringify({ results: [], total: 0, tokens: 0, windowSize: 200 }), {
          status: 200,
          headers: { 'content-type': 'application/json' },
        }),
      ),
    );
    globalThis.fetch = fetchSpy as unknown as typeof fetch;

    await recall('http://localhost:9999', undefined, {
      query: 'alpha',
      summarizeOverflow: false,
    });

    const url = String(fetchSpy.mock.calls[0]![0]);
    expect(url).toContain('summarize_overflow=0');
    expect(url).not.toContain('summarize_overflow=1');
  });
```

### Step 3: Run

```bash
npx vitest run tests/client-recall-opts-parity.test.ts
```

Two outcomes:

**A) PASS:** `client.ts` already serializes `false` as `=0` correctly. The test pins existing behaviour. Skip Step 4. Commit message stays as `test: ...`.

**B) FAIL:** Real bug — `false` is being treated as undefined and dropped. Proceed to Step 4.

### Step 4 (conditional, only if Step 3 was FAIL): Fix `src/client.ts`

Find the `summarize_overflow` serialization site. Change from a truthy check to an explicit `!== undefined` check:

```ts
// Before (foot-gun):
if (opts.summarizeOverflow) params.set('summarize_overflow', '1');

// After:
if (opts.summarizeOverflow !== undefined) {
  params.set('summarize_overflow', opts.summarizeOverflow ? '1' : '0');
}
```

Re-run Step 3. Expected: PASS.

### Step 5: Full suite check

```bash
npx vitest run tests/client-recall-opts-parity.test.ts
npx vitest run --reporter=dot 2>&1 | tail -5
```

Expected: 3 cases in this file, full suite green.

### Step 6: Commit

If Step 4 was skipped (Step 3 passed):

```bash
git add tests/client-recall-opts-parity.test.ts
git commit -m "test: pin summarize_overflow=0 thin-client serialization on explicit false (v1.7.3)"
```

If Step 4 ran (Step 3 failed):

```bash
git add tests/client-recall-opts-parity.test.ts src/client.ts
git commit -m "fix: serialize summarize_overflow=0 on explicit false (v1.7.3)"
```

**Success criteria:**
- 3 test cases pass in `client-recall-opts-parity.test.ts`.
- No regression in any other test file.
- One atomic commit (1 file unless Step 4 surfaced a bug, then 2 files).

---

## Task 3: Rename `recallScope` parameter to `scopeFilter`

**Why:** Internal-only readability nit from the v1.7.2 maintainability review. `recallScope: RecallScopeFilter` reads as "the scope (a value)" but the value is a *filter shape* with `mode` and an optional `value`. `scopeFilter` matches the type name and reads as "the filter being applied". Pure rename — no behaviour change, no signature shape change.

**Files:**
- Modify: `src/store.ts:649` — `loadSearchRows` parameter
- Modify: `src/store.ts:672, 676, 677, 689` — usage sites inside `loadSearchRows`
- Modify: `src/store.ts:1562, 1566` — `loadRecallSearchEntries` local + call site

No callers outside `src/store.ts` — verified pre-flight.

### Step 1: Pre-flight grep to confirm scope of rename

```bash
grep -rn "recallScope" src/ tests/
```

Expected: matches only inside `src/store.ts`. If `tests/` reference `recallScope` symbolically, the rename scope expands to those files — handle them in the same commit.

### Step 2: No new test needed

Existing tests `tests/store-recall-scope-filter-type.test.ts` and `tests/api-recall-deny-scopes-source-of-truth.test.ts` reference the *type* `RecallScopeFilter` (unchanged) and the runtime *behaviour* (unchanged). They will keep passing.

### Step 3: Rename in `loadSearchRows` signature

In `src/store.ts:644-650`, change:

```ts
function loadSearchRows(
  db: ReturnType<typeof openHippoDb>,
  query: string,
  limit: number,
  tenantId: string | undefined,
  recallScope?: RecallScopeFilter,
): MemoryRow[] {
```

to:

```ts
function loadSearchRows(
  db: ReturnType<typeof openHippoDb>,
  query: string,
  limit: number,
  tenantId: string | undefined,
  scopeFilter?: RecallScopeFilter,
): MemoryRow[] {
```

### Step 4: Rename usage sites inside `loadSearchRows` body

Inside the function body (lines 672-696), replace every `recallScope` with `scopeFilter`. Use Edit's `replace_all` against a unique substring scoped to that function only (do NOT use a global replace_all on the file — `loadRecallSearchEntries` uses the same name as a local variable; handle that separately in Step 5).

Specifically rewrite:
- `if (recallScope !== undefined) {` → `if (scopeFilter !== undefined) {`
- `if (recallScope.mode === 'default-deny') {` → `if (scopeFilter.mode === 'default-deny') {`
- `scopeParams.push(recallScope.value);` → `scopeParams.push(scopeFilter.value);`

### Step 5: Rename local in `loadRecallSearchEntries`

In `src/store.ts:1562-1566`, change:

```ts
const recallScope: RecallScopeFilter =
  requestedScope && requestedScope !== ''
    ? { mode: 'exact', value: requestedScope }
    : { mode: 'default-deny' };
return loadSearchRows(db, query, limit, tenantId, recallScope).map(rowToEntry);
```

to:

```ts
const scopeFilter: RecallScopeFilter =
  requestedScope && requestedScope !== ''
    ? { mode: 'exact', value: requestedScope }
    : { mode: 'default-deny' };
return loadSearchRows(db, query, limit, tenantId, scopeFilter).map(rowToEntry);
```

### Step 6: Verify no `recallScope` symbol remains

```bash
grep -rn "recallScope" src/ tests/
```

Expected: zero matches.

### Step 7: Type-check + full test sweep

```bash
npx tsc --noEmit
npx vitest run --reporter=dot 2>&1 | tail -5
```

Expected: zero TS errors, full suite green.

### Step 8: Commit

```bash
git add src/store.ts
git commit -m "refactor: rename loadSearchRows recallScope param to scopeFilter (v1.7.3)"
```

**Success criteria:**
- Zero `recallScope` matches under `src/` and `tests/`.
- TypeScript clean.
- Full test suite green.
- One atomic commit, internal-only (no public API surface touched).

---

## Task 4: README "What's new" backfill for v1.7.0 + v1.6.5

**Why:** The publish-repo skill (and project convention) mandate a README "What's new" section per release. v1.6.5 and v1.7.0 ships skipped this; v1.7.1 + v1.7.2 are present. Backfill so the README reflects the actual chronological release history.

**Files:**
- Modify: `README.md` — insert two new sections between v1.7.1 (line 99 close) and v1.6.4 (line 101 open)

Final order: v1.7.2 → v1.7.1 → **v1.7.0 (new)** → **v1.6.5 (new)** → v1.6.4 → v1.6.3 → ...

**Source for content:** `CHANGELOG.md` v1.7.0 (lines 67-119) and v1.6.5 (lines 136-168). Each section distils to a tight 3-5 bullet summary.

### Step 1: No test needed (docs only).

### Step 2: Insert the two sections

In `README.md`, find the closing line of `### What's new in v1.7.1` (the bullet ending with "...never poisons the on-disk FTS index"). After that bullet, before the `### What's new in v1.6.4` heading, insert:

```markdown
### What's new in v1.7.0

- **`MemoryEntry.bm25_score?: number`.** Raw FTS5 `bm25()` score surfaced as provenance metadata on the FTS path of `loadSearchEntries`. `undefined` on every other path (empty query, FTS unavailable, LIKE fallback, full-store fallback, `readEntry`, `loadAllEntries`, deserialize). NOT a drop-in for the JS-side BM25 scorer in `src/search.ts` — different tokenizer, scale, sign convention. Provenance only.
- **`RecallOpts.scorerWindow?: number`.** Decouples scorer candidate pool from `limit`. Default `undefined` preserves the existing 200-row store-internal default. Useful when `summarizeOverflow=true` and you want a wider candidate pool to detect more level-2 parent clusters.
- **`RecallResult.windowSize?: number`.** Reports the scorer window actually used so callers can introspect "did the scorer see enough candidates?" without re-deriving the value.
- **API contract fix (CRITICAL).** `RecallContractError` HTTP serialization aligned to `{error: <message>, code: <code>}` to match every other v1/* error. The v1.6.5 one-off shape (`{error: <code>, message: <text>}`) was a public-contract drift caught by the api-contract specialist in `/review`. **Breaking for v1.6.5 callers reading `body.error` for the typed code value** — migrate to `body.code`.
- Three review-chain rounds (`/plan-eng-review`, `/codex review --model gpt-5.5`, `/review`) shaped this release: 4 P0s killed mk1 (including a fabricated `bm25_score` column), 2 P0s killed mk2 (including an MCP cap addressing a non-existent contract), and the 5-specialist `/review` pass added the api-contract fix and 4 INFO-level test improvements.

### What's new in v1.6.5

- **`RecallContractError` exported class with `.code` field.** Thrown by `api.recall` when `HIPPO_REQUIRE_SESSION_SCOPED_FRESH_TAIL=1` AND `freshTailCount > 0` AND `freshTailSessionId` is unset. HTTP returns 400 with the typed error; MCP propagates via `-32603`; CLI exits 1. Default env unset preserves v1.6.x tenant-wide back-compat.
- **Timestamp invariant documented** in `src/memory.ts`: all in-process `MemoryEntry` and session-state timestamps are canonical `Date.prototype.toISOString()` (24 chars, UTC, ms, trailing `Z`). Importers preserving local-time offsets MUST normalize on write.
- **`assemble` ISO sort uses byte compare** instead of `localeCompare` — ~50× faster on canonical UTC ISO with no semantic change given the in-process invariant. Caveat documented: `deserializeEntry` / `rebuildIndex` round-trip frontmatter timestamps as-is, so legacy markdown with non-canonical offsets propagates without normalization.
- **`loadFreshRawMemories` JSDoc-deprecated** for tenant-wide use (no `sessionId`). NO runtime `console.warn` — codex C9 rejected library-level stderr noise. Direct callers bypass the `api.recall` guard, so the JSDoc is the only nudge at that layer.
```

### Step 3: Verify chronological order

```bash
grep -n "What's new in" README.md | head -10
```

Expected: v1.7.2 → v1.7.1 → v1.7.0 → v1.6.5 → v1.6.4 → v1.6.3 → ... in descending version order.

### Step 4: Commit

```bash
git add README.md
git commit -m "docs: README What's new backfill for v1.7.0 + v1.6.5 (v1.7.3)"
```

**Success criteria:**
- Both new sections present in correct chronological order.
- v1.7.1 and v1.6.4 sections untouched.
- Each new section is a tight 3-5 bullet summary.
- One atomic commit.

---

## Final phase: ship

After all 4 tasks committed, run the ship pipeline:

### S1: Self-review

```bash
git log --oneline master..HEAD
git diff master..HEAD --stat
```

Invoke `/self-review`. Verify each commit traces directly to a TODOS.md item. No drift, no scope creep, no edits outside the planned files.

### S2: Pre-landing review

Invoke `/review` against the diff vs `master`. Address any P0/P1 surfacing. Defer P2/P3 with one-line note in the v1.7.4 section of `TODOS.md`.

### S3: Ship-check

Invoke `/ship-check`. Confirm:
- 4 items closed
- Full test suite green
- TypeScript clean
- README + CHANGELOG entries present (CHANGELOG entry is added in S4)
- Worth shipping (yes — closes v1.7.2 review-tail block)

### S4: Bump + CHANGELOG

Edit `package.json`: `"version": "1.7.2"` → `"version": "1.7.3"`.

Add to top of `CHANGELOG.md` (above the v1.7.2 entry at line 3):

```markdown
## 1.7.3 (YYYY-MM-DD)

Hygiene release closing the four lower-confidence items deferred from the v1.7.2 review chain. No public API change. No behaviour change. No schema change.

### Tests

- Module-load assertion runtime test for `RECALL_DEFAULT_DENY_SCOPES` (codex P1-3 from v1.7.2). Extracted `assertNonEmpty` helper from inline guard so the throw path is directly testable.
- `summarize_overflow=0` thin-client serialization on explicit `false` (codex P2-3 from v1.7.2). Pins that `false` produces `=0` rather than omission.

### Refactored (no behaviour change)

- Renamed `loadSearchRows` parameter `recallScope` → `scopeFilter` for readability (v1.7.2 maintainability INFO). Internal-only.

### Documented

- README "What's new" backfill for v1.7.0 and v1.6.5 (skipped at ship time, restored for chronological completeness).
```

Add to `README.md` above the v1.7.2 section:

```markdown
### What's new in v1.7.3

- **Hygiene release.** Closes the v1.7.2 review-tail: module-load assertion runtime test, `summarize_overflow=0` thin-client pin, internal `scopeFilter` rename, and a README "What's new" backfill for v1.7.0 and v1.6.5.
- No public API change. No behaviour change. No schema change.
```

Commit:

```bash
git add package.json CHANGELOG.md README.md
git commit -m "chore: bump to v1.7.3 + CHANGELOG + What's new"
```

### S5: `/ship`

Invoke `/ship`. Pushes branch, opens PR.

### S6: `/land-and-deploy`

Merge → npm publish → tag.

### S7: `hippo capture` + close TODOS

```bash
hippo capture --stdin <<< 'v1.7.3 shipped: 4 review-tail items from v1.7.2. Pure hygiene. assertNonEmpty extracted, summarize_overflow=0 pinned, scopeFilter renamed, README backfilled for v1.7.0 + v1.6.5.'
```

Edit `TODOS.md` — strikethrough or remove the four items in the `v1.7.3 — review-tail from v1.7.2` block (lines 5-10). Commit:

```bash
git add TODOS.md
git commit -m "docs: close v1.7.3 review-tail in TODOS"
```

Push the post-merge cleanup commit to `master`.

---

## Risk register

| Risk | Likelihood | Mitigation |
|------|-----------|------------|
| Pre-existing test failures on `master` baseline | Low | Pre-flight check at top of plan. STOP if red. |
| Task 2 Step 3 reveals real `client.ts` bug | Medium | Step 4 conditional path branches into pin-or-fix. Commit message updates accordingly. |
| Task 3 rename hits an unexpected caller in tests/ | Low | Step 1 grep at start of Task 3. If found, scope expands to those files in the same commit. |
| `assertNonEmpty` shadows an existing helper | Very low | Pre-flight: `grep -rn "assertNonEmpty" src/`. If a helper already exists, reuse it instead of creating. |
| README markdown structure drift between v1.7.1 and v1.6.4 sections | Low | Step 3 grep verifies chronological order after insert. |

---

## Constraints (from CLAUDE.md + project memory)

- **Real DB for tests** where tests touch the DB. Tasks 1, 2, 3 use no DB (pure helper, fetch spy, rename). Task 4 is docs.
- **Never `--no-verify`** on commits. If a hook fails, fix the underlying issue and create a NEW commit.
- **Atomic commit per task.** No squashing across tasks.
- **TDD: failing test first** on Tasks 1 + 2.
- **Branch off master.** Do not commit directly to master.
- **No top-level repo doc gets rewritten** without explicit user "apply" — applies to PLAN.md, ROADMAP.md, RESEARCH.md, TODOS.md, README.md, CLAUDE.md, MEMORY.md. README.md edit in Task 4 is an additive insertion (not a rewrite); explicit user approval of this plan IS the "apply" for that one section.

## Estimated time

| Task | Time |
|------|------|
| Pre-flight | 2 min |
| Task 1 (assertNonEmpty + test) | 15 min |
| Task 2 (summarize_overflow=0 test) | 5 min (10 min if Step 3 surfaces a real bug) |
| Task 3 (recallScope → scopeFilter rename) | 10 min |
| Task 4 (README backfill) | 10 min |
| Ship phase (S1-S7) | 20 min |
| **Total** | **~60-65 min** |
