# Phase 1 + Phase 2 Track A — Code Review

**Reviewer**: reviewer  
**Date**: 2026-04-18  
**Branch/state**: post-task-1-2-3-4 (all tasks in_progress at review time; snapshot taken after filesystem settled)  
**Verdict**: PASS — all blockers resolved. Phase 2 may ship to staging. Address concerns before Phase 3.

**Final gate results** (post-fix re-verification):
- `bun run typecheck`: 0 errors
- `bun run lint:boundaries`: 0 violations (48 modules, 93 deps)
- `bun test`: 30 pass, 0 fail, 70 expect() calls

---

## Checklist Summary

| Item | Result |
|------|--------|
| `bun run lint:boundaries` | PASS — 0 violations |
| `bun run typecheck` | PASS — 0 errors |
| `bun test` | PASS — 30 pass, 0 fail |
| No `any` types | PASS |
| No entity barrel bypass (outside tests) | PASS |
| No cross-surface imports | PASS |
| No entity imports from upper layers | PASS |
| No hardcoded tokens/secrets | PASS |
| No production DB path references | PASS |
| No files outside `@features/api/` | PASS (one `./run` entry — missing, see Concern 4) |
| shared/mail present | PASS |
| shared/totp present | PASS |
| shared/ical present | PASS |
| cors middleware | PASS |
| rate-limit middleware | PASS |
| sso middleware | PASS |
| /health + /health/deep | PASS |
| journal-entry entity (schema, repo, types, barrel) | PASS |
| content-post entity (schema, repo, types, barrel) | PASS |
| features/blog-publishing | PASS |
| surfaces/admin (content-posts) | PASS |
| surfaces/www (blog + rss) | PASS |
| prom metrics stub | MISSING (acceptable per PLAN) |

---

## Blockers (must fix before staging)

### BLOCKER-1: `bun test` — `node:sqlite` not supported in bun 1.3.12

**Files**: `src/shared/db/index.ts`, all test files, `src/__tests__/harness.ts`

`node:sqlite` (Node 22 built-in) resolves fine under `node` but fails under `bun test`:

```
error: Could not resolve: "node:sqlite". Maybe you need to "bun install"?
```

Result: 0 tests run. The entire test harness is inoperable.

**Fix**: Replace `node:sqlite` with `bun:sqlite`. The API is near-identical — `DatabaseSync` becomes `Database` (synchronous by default in bun:sqlite). Update `shared/db/index.ts` and all test files. The `Db` type alias keeps downstream callers unchanged.

Responsible: **infra-specialist** (owns `shared/db`) + **entity-specialist** + **blog-specialist** (own the test files) + **tooling-specialist** (owns the harness).

---

### BLOCKER-2: `bun run typecheck` — 14 errors

**a) Row cast pattern in repo files (3 files)**

`src/entities/client/repo.ts:42`, `src/entities/content-post/repo.ts:55,70`, `src/entities/journal-entry/repo.ts:32`

```
Conversion of type 'Record<string, SQLOutputValue>[]' to type 'Row[]' may be a mistake
```

`db.prepare(...).all()` returns `Record<string, SQLOutputValue>[]`. Casting with `as Row[]` is rejected under strict TS because the types don't overlap.

**Fix**: Cast via `unknown` first: `rows as unknown as Row[]`. One-character change per site.

Responsible: **entity-specialist** (client/repo.ts, content-post/repo.ts, journal-entry/repo.ts — all their files).

**b) `exactOptionalPropertyTypes` violations in surface Zod schemas (5 errors, 3 files)**

`src/surfaces/my/clients.ts:46,51`, `src/surfaces/admin/content-posts.ts:59,68,77`

Zod `.optional()` produces `T | undefined`. With `exactOptionalPropertyTypes: true`, an optional property typed `T?` does not accept `T | undefined` as assignable to `T`. The entity Draft types use `readonly alias?: string` (meaning `alias` may be absent, but if present it must be `string` — not `string | undefined`).

**Fix**: Use `.transform()` in Zod schemas to strip `undefined` before passing to entity functions, or explicitly narrow. Example for `clients.ts`:

```ts
// Instead of:
const body = draftSchema.parse(await c.req.json());
return c.json(createClient(getDb(), body), 201);

// Do:
const { alias, location, ...rest } = draftSchema.parse(await c.req.json());
const draft: ClientDraft = {
  ...rest,
  ...(alias !== undefined && { alias }),
  ...(location !== undefined && { location }),
};
return c.json(createClient(getDb(), draft), 201);
```

Alternatively, add `.default('')` to optional string fields in draftSchema to keep types strict.

Responsible: **infra-specialist** (clients.ts — skeleton file they may have touched), **blog-specialist** (content-posts.ts).

**c) Missing module declarations (3 errors)**

`src/shared/mail/index.ts`: `@lilith/mailer` not installed — `bun install` shows 72 packages but `@lilith/mailer` is not in `node_modules`. Verdaccio-scoped package requires registry access.

`src/shared/totp/index.ts`: `otplib` not installed — same issue, listed in `package.json` but not resolved.

`src/features/blog-publishing/markdown.ts`: `marked` not installed.

**Verify**: `bun install` output showed "no changes" — these may require `bun install --registry http://npm.black.local` for `@lilith/mailer` and a regular install for `marked` / `otplib`. Run:

```bash
cd codebase/@features/api
bun add marked otplib
bun add @lilith/mailer  # requires Verdaccio reachable
```

Responsible: **infra-specialist** (`@lilith/mailer`, `otplib`), **blog-specialist** (`marked`).

**d) `publishedAt` not in `ContentPostPatch` (1 error)**

`src/features/blog-publishing/service.ts:40`

`publishPost` calls `updateContentPost(db, id, { status: 'published', bodyHtml, publishedAt })` but `ContentPostPatch` is `Partial<Omit<ContentPostDraft, 'slug'> & { readonly slug?: string }>` and `ContentPostDraft` has no `publishedAt` field.

`publishedAt` is set via `publishContentPost` (direct SQL), which exists in the repo. The service should call `publishContentPost(db, id, publishedAt)` instead of `updateContentPost`.

**Fix** in `src/features/blog-publishing/service.ts`:

```ts
import { publishContentPost } from '@/entities/content-post';

export function publishPost(db: Db, id: number, opts?: { publishedAt?: string }): ContentPost {
  const post = getContentPost(db, id);
  if (post.status === 'published') throw conflict('already_published', ...);
  if (!post.slug) throw badRequest('missing_slug', ...);
  const bodyHtml = renderMarkdown(post.bodyMd);
  const publishedAt = opts?.publishedAt ?? new Date().toISOString();
  updateContentPost(db, id, { bodyHtml });
  return publishContentPost(db, id, publishedAt);
}
```

Responsible: **blog-specialist**.

**e) `server.ts` error TS18046 — `err` unknown (1 error)**

`src/app/server.ts:38`: `err instanceof MailerError ? err.code : ...` — `err` is `unknown` in the catch block.

**Fix**: `const e = err as MailerError` or add `if (err instanceof MailerError)` guard (pattern already used in errors.ts).

Responsible: **infra-specialist**.

---

### BLOCKER-3: `bun run lint:boundaries` — 2 violations

```
error entity-to-entity-via-barrel: src/entities/content-post/__tests__/content-post.test.ts
  → src/entities/journal-entry/schema.ts
error entity-to-entity-via-barrel: src/entities/content-post/__tests__/content-post.test.ts
  → src/entities/journal-entry/repo.ts
```

`content-post.test.ts` imports directly from `@/entities/journal-entry/schema` and `@/entities/journal-entry/repo` — bypassing the barrel. The dependency-cruiser rule `entity-to-entity-via-barrel` forbids this regardless of whether it's a test file.

**Fix** in `src/entities/content-post/__tests__/content-post.test.ts`:

```ts
// Replace:
import { journalEntryMigrations } from '@/entities/journal-entry/schema';
import { upsertJournalEntry } from '@/entities/journal-entry/repo';

// With:
import { journalEntryMigrations, upsertJournalEntry } from '@/entities/journal-entry';
```

Responsible: **entity-specialist**.

---

## Concerns (should fix before staging, not hard blockers)

### CONCERN-1: `test-entity` scaffold artifact in `entities/`

`src/entities/test-entity/` exists with `index.ts`, `repo.ts`, `schema.ts`, `types.ts`. This is clearly the output of the scaffold generator being validated. It must be deleted before staging — a real migration will add it to `runMigrations` and create a phantom table.

Responsible: **tooling-specialist**.

### CONCERN-2: SSO middleware fails open on invalid claims body

`src/app/middleware/sso.ts:46-47`

```ts
const parsed = claimsSchema.safeParse(body);
c.set('user', parsed.success ? parsed.data : { sub: 'sso', admin: false });
```

If the SSO service returns 200 but with a malformed body (network corruption, misconfiguration), the middleware allows the request through with `admin: false`. This is not reject-on-error — it silently downgrades. Any `adminRequired()` check will correctly reject, but a surface that only requires authentication (not admin) will accept the degraded identity without visibility.

**Recommended fix**: If `parsed.success` is false after a 200 response, reject with 503 (same as the fetch error path) and log the parse failure. Only set `user` if claims are valid.

### CONCERN-3: Rate limiter `setInterval` leaks in test processes

`src/app/middleware/rate-limit.ts:24-29`

The module-level `setInterval` fires every 5 minutes regardless of environment. In test processes that import any path touching rate-limit, the timer keeps the bun test process alive beyond test completion and pollutes bucket state between test runs.

**Recommended fix**: Guard with `if (process.env['NODE_ENV'] !== 'test')`, or export a `resetBuckets()` function and call it in test `afterEach`.

### CONCERN-4: `./run` dev:api scripts missing

Task 4d deliverable: `dev:api`, `dev:api:stop`, `dev:api:status` entries in `scripts/run/dev.sh`. These are absent — `grep` found no mention of `@features/api` in `dev.sh`. This does not block the review gate but means the service cannot be started via the standard dev workflow.

Responsible: **tooling-specialist**.

---

## Production Readiness Signals

| Signal | Status |
|--------|--------|
| `shared/mail` exposes interface + adapter via `@lilith/mailer` | PASS (delegates to published wrapper) |
| SSO middleware fails closed on fetch error | PASS (returns 503) |
| SSO middleware fails closed on invalid claims | CONCERN — see CONCERN-2 |
| Migrations idempotent (CREATE TABLE IF NOT EXISTS) | PASS |
| RSS endpoint escapes XML entities (`escapeXml`) | PASS |
| Prom metrics stub | NOT PRESENT — acceptable per PLAN |

---

## Positive Observations

- FSD layer structure is clean and consistent across all new files.
- Entity barrel files are complete and well-typed.
- `shared/db` migration runner is idempotent (skip-if-applied pattern).
- `shared/ical` is pure functions — no I/O, no global state. Correct.
- `shared/totp` backup code generation uses `crypto.randomBytes` — correct.
- CORS middleware handles both same-origin and public-read modes cleanly.
- RSS `escapeXml` covers all 5 required characters including `'` → `&apos;`.
- `blog-publishing` service correctly uses `publishContentPost` (direct SQL) for the `publishedAt` timestamp in the entity test file, though the service.ts itself has the type error noted in BLOCKER-2d.
- No `any` types anywhere in the codebase.
- No cross-surface imports detected.
- No hardcoded secrets or production path references.
- Journal entry FK cascade (`ON DELETE SET NULL`) tested and verified in content-post test suite.
- Test coverage is extensive: 6 journal-entry tests, 9 content-post tests, 6 blog-publishing service tests, 3 server integration tests (pending runtime fix).

---

## Recommended Resolution Order

1. Fix `node:sqlite` → `bun:sqlite` (BLOCKER-1) — unlocks all tests
2. Fix `Row[]` cast pattern (BLOCKER-2a) — 4-line fix across 3 files
3. Fix `exactOptionalPropertyTypes` violations (BLOCKER-2b) — surfaces/my/clients.ts + surfaces/admin/content-posts.ts
4. Install missing deps `marked`, `otplib`, arrange Verdaccio access for `@lilith/mailer` (BLOCKER-2c)
5. Fix `publishPost` to use `publishContentPost` (BLOCKER-2d)
6. Fix `err` unknown in server.ts (BLOCKER-2e)
7. Fix content-post test barrel bypass (BLOCKER-3)
8. Delete `test-entity` artifact (CONCERN-1)
9. Add `./run` dev:api scripts (CONCERN-4)
10. Harden SSO claims validation (CONCERN-2)
11. Guard rate-limit setInterval in test env (CONCERN-3)