268 lines
12 KiB
Markdown
268 lines
12 KiB
Markdown
# 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 `'` → `'`.
|
|
- `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)
|