lilith-platform.live/codebase/@features/api/REVIEW.md

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:

// 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:

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:

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:

// 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

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 ' → '.
• 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)