---
phase: 03-server-documentation-schemas
reviewed: 2026-05-03T00:00:00Z
depth: standard
files_reviewed: 38
files_reviewed_list:
  - tools/asset-catalog/scripts/lint-adr.mjs
  - tools/asset-catalog/scripts/lint-parity-checklist.mjs
  - tools/asset-catalog/scripts/lint-subsystem-mds.mjs
  - tools/asset-catalog/scripts/lint-wiki-errata.mjs
  - tools/asset-catalog/src/autogen.ts
  - tools/asset-catalog/src/emit.ts
  - tools/db-schema/drizzle.config.ts
  - tools/db-schema/scripts/check-source-comments.mjs
  - tools/db-schema/src/index.ts
  - tools/db-schema/src/tables.ts
  - tools/db-schema/vitest.config.ts
  - tools/protocol-doc/cli.ts
  - tools/protocol-doc/output/protocol.ts
  - tools/protocol-doc/scripts/lint-protocol.mjs
  - tools/protocol-doc/src/autogen.ts
  - tools/protocol-doc/src/derive/xls-hints.ts
  - tools/protocol-doc/src/emit/build-table.ts
  - tools/protocol-doc/src/emit/index.ts
  - tools/protocol-doc/src/emit/json.ts
  - tools/protocol-doc/src/emit/markdown.ts
  - tools/protocol-doc/src/emit/typescript.ts
  - tools/protocol-doc/src/scanner/citations.ts
  - tools/protocol-doc/src/scanner/opcode-trace.ts
  - tools/protocol-doc/src/types.ts
  - tools/protocol-doc/vitest.config.ts
  - tools/save-format-doc/cli.ts
  - tools/save-format-doc/output/save-formats.ts
  - tools/save-format-doc/scripts/lint-save-formats.mjs
  - tools/save-format-doc/src/autogen.ts
  - tools/save-format-doc/src/emit/index.ts
  - tools/save-format-doc/src/emit/json.ts
  - tools/save-format-doc/src/emit/markdown.ts
  - tools/save-format-doc/src/emit/typescript.ts
  - tools/save-format-doc/src/scanner/citations.ts
  - tools/save-format-doc/src/scanner/format-trace.ts
  - tools/save-format-doc/src/types.ts
  - tools/save-format-doc/vitest.config.ts
  - scripts/verify-phase-3.mjs
findings:
  critical: 4
  warning: 7
  info: 4
  total: 15
status: issues_found
---

# Phase 3: Code Review Report

**Reviewed:** 2026-05-03
**Depth:** standard
**Files Reviewed:** 38
**Status:** issues_found

## Summary

Phase 3 produces three documentation tools (asset-catalog autogen extension, protocol-doc, save-format-doc), a Drizzle schema package, six lint scripts, and a composite verify gate. Determinism contracts (LF-only, sorted-keys JSON, no `Date.now`) are well-honoured across emitters. Lint scripts are thorough on schema invariants and provenance citations. However, the **generated TypeScript output files (`output/protocol.ts`, `output/save-formats.ts`) contain duplicate property names that will not compile under TypeScript strict mode**, blocking Phase 4 imports. Several scanner heuristics also have off-by-one or scope bugs that could mis-classify opcodes during future re-extraction.

## Critical Issues

### CR-01: Generated `output/protocol.ts` contains duplicate property names — will not compile

**File:** `tools/protocol-doc/output/protocol.ts:32-37, 105-130, 137-154, 165-177, 187-203, 234-243`
**Issue:** Multiple opcode interfaces emit duplicate property names that violate TypeScript's "Duplicate identifier" rule (TS2300/TS2393). Examples in the committed artifact:

- `OpS2CBroadcastSprite` (lines 28-37) declares `p_spr` twice (once as `number`, once as `string`).
- `OpS2CLoginResponseSuccess` (lines 105-130) declares `uinv_get` twice and `hxbridge` twice.
- `OpS2COnlineList` declares `p_uid`, `p_name` once but the pattern repeats across multi-write sites.
- `OpS2CRoomOccupants` (lines 157-177) declares `p_spr` twice.
- `OpS2CMbBoardListResponse` (lines 187-203) declares `mb_topic` three times.
- `OpS2CMbSummaryResponse` (lines 234-243) declares `mb_total` twice.

Phase 4 `packages/protocol/` imports this file verbatim per the file's own header (`// Phase 4 packages/protocol/ imports from here`). Under `strict: true` (the project's stated convention) TypeScript will refuse to compile any consumer.

Root cause is in `tools/protocol-doc/src/emit/typescript.ts:43-51` — `renderFieldsAsTsBody` walks `fields` in order and emits one TS property per `OpcodeField` with no de-duplication of `f.name`. The underlying `OpcodeField[]` correctly carries duplicate names because the GML wire layout legitimately writes the same source variable multiple times (e.g., `writeint(global.p_spr[uid,0])` followed by `writestring(global.p_spr[uid,1])` — same identifier, different element). The emitter must disambiguate.

**Fix:** Disambiguate names during TS emission. A minimal patch:

```typescript
function renderFieldsAsTsBody(fields: OpcodeField[]): string[] {
  const lines: string[] = [];
  const seen = new Map<string, number>();
  for (const f of fields) {
    const ts = TS_TYPE_FOR_GML[f.gml_type] ?? 'unknown';
    const count = seen.get(f.name) ?? 0;
    seen.set(f.name, count + 1);
    const uniqueName = count === 0 ? f.name : `${f.name}_${count + 1}`;
    lines.push(`  /** ${f.gml_type} (${f.byte_size === -1 ? 'variable' : `${f.byte_size}B`}) */`);
    lines.push(`  ${safeKey(uniqueName)}: ${ts};`);
  }
  return lines;
}
```

Add a unit test that asserts every emitted interface body has unique keys.

### CR-02: Generated `output/save-formats.ts` contains duplicate property names — will not compile

**File:** `tools/save-format-doc/output/save-formats.ts:23-31`
**Issue:** Same class of bug as CR-01. `SaveMbLogBnb.topic.topics` array elements have three properties all named `mb_topic`:

```typescript
topics: readonly Array<{
  /** write string (line 14) */
  mb_topic: string;
  /** write real (line 16) */
  mb_topic: number;
  /** write real (line 18) */
  mb_topic: number;
}>;
```

This is invalid TypeScript and will fail Phase 4 `SRV-10/11` imports. The `emitGrammarBody` loop in `tools/save-format-doc/src/emit/typescript.ts:87-92` also has no de-duplication.

**Fix:** Apply the same deduplication strategy as CR-01 inside `emitGrammarBody`, tracking seen names per scope (per object-literal body — reset between sections, between loop bodies, etc.):

```typescript
function emitGrammarBody(grammar: GrammarField[], depth: number): string[] {
  // ... existing setup ...
  const seen = new Map<string, number>();
  for (const f of flats) {
    if (f.kind !== 'flat') continue;
    const base = propName(f.name);
    const count = seen.get(base) ?? 0;
    seen.set(base, count + 1);
    const key = count === 0 ? base : `${base}_${count + 1}`;
    const ts = tsTypeForGmlType(f.type);
    lines.push(`${indent}/** ${f.op} ${f.type} (line ${f.line}) */`);
    lines.push(`${indent}${safeKey(key)}: ${ts};`);
  }
  // ... loops follow same pattern ...
}
```

Per-scope tracking is important: a `head` section's `mb_topic` should not collide with a `topic` section's `mb_topic`.

### CR-03: `lint-parity-checklist.mjs` exits successfully when synchronous validation errors exist if dynamic `import()` rejects

**File:** `tools/asset-catalog/scripts/lint-parity-checklist.mjs:257-296`
**Issue:** All success/failure exit code logic is inside `import('node:crypto').then(...)`. If the dynamic import rejects (rare but possible — e.g., custom Node loader policy, snapshot-mode workers), the rejection becomes an unhandled promise rejection and Node exits with code 0 by default in older versions or with whatever the runtime decides. Synchronous schema-validation errors accumulated in `errors++` would never be reported.

Additionally, there is no `.catch(...)` to convert a rejection into a non-zero exit. This means a transient I/O failure reading `protoPath`/`sfPath` for the SHA hash silently disappears.

This is a CI hygiene/correctness issue: the lint can pass with errors > 0 in pathological conditions.

**Fix:** Move the SHA-warning logic to a synchronous block (use `node:crypto` as a top-level import, not dynamic), and place all exit logic at the top level:

```javascript
import { createHash } from 'node:crypto';
// ... existing schema-validation loop ...

// Aggregate disposition log (D-22)
process.stdout.write(`Parity disposition counts: ${JSON.stringify(counts)}\n`);

// SHA drift warnings (non-blocking)
const livePr = createHash('sha256').update(readFileSync(protoPath)).digest('hex');
const liveSf = createHash('sha256').update(readFileSync(sfPath)).digest('hex');
if (checklist.inputs?.protocol_sha256 && checklist.inputs.protocol_sha256 !== livePr) {
  process.stderr.write(`WARN: parity-checklist.inputs.protocol_sha256 stale...\n`);
}
// ... and same for save_formats_sha256 ...

if (errors > 0) {
  process.stderr.write(`lint-parity-checklist: ${errors} error(s)\n`);
  process.exit(1);
}
process.stdout.write(`lint-parity-checklist: OK (${checklist.rows.length} rows validated)\n`);
process.exit(0);
```

### CR-04: `traceC2S` mutates the outer `for`-loop counter from inside an inner block — fragile control flow

**File:** `tools/protocol-doc/src/scanner/opcode-trace.ts:329, 394`
**Issue:** Inside a `for (let i = 0; i < lines.length; i++)` outer loop, line 394 does `i = j;` to skip past a switch-block we just consumed. Because `i` is declared with `let`, this re-assignment is permitted, but the outer-loop's post-increment (`i++`) immediately advances `i` to `j + 1` after the `break`. That is the apparent intent, but mutating an outer-loop counter from inside a nested loop is a maintenance hazard — any future refactor of the outer loop body could accidentally restore double iteration of the case bodies inside a switch (each case body would be reported twice as separate switches).

More importantly, **`i = j` only runs when `depth <= 0 && j > switchOpen`** (line 391). If the switch closes on the same line as `switchOpen` (one-liner switch — unlikely but possible) or if the closing `}` is missing entirely (truncated file), `i` is never reassigned and the outer loop will re-scan every line of the switch body looking for nested switches, potentially double-emitting traces.

**Fix:** Defensive reset — always set `i = j` after the inner walk, even when no close was found:

```typescript
for (let j = switchOpen; j < lines.length; j++) {
  // ... existing logic ...
  if (depth <= 0 && j > switchOpen) {
    flush();
    i = j;
    break;
  }
  // ... existing case detection ...
}
// Defensive: always advance past where we walked, even on EOF.
// (Use a separate variable instead of mutating outer i to avoid double-emission risk.)
```

A cleaner refactor: change the outer loop to a `while` with explicit counter advancement, or extract switch processing into a helper that returns the line to resume at.

## Warnings

### WR-01: Branch detection in `traceS2C` has off-by-one and ignores brace depth

**File:** `tools/protocol-doc/src/scanner/opcode-trace.ts:251-267`
**Issue:** Two distinct bugs in the branch-detection loop:

1. **Off-by-one:** `lines` is 0-indexed; `nearest` and `send.line` are 1-indexed. The loop `for (let ln = nearest; ln < send.line; ln++)` then references `lines[ln]`. For `nearest = 100` (1-indexed clearbuffer line), `lines[100]` is line 101 — the loop misses the clearbuffer line itself and reads one line past the intended range. Off-by-one.

2. **No brace-depth tracking:** The block comment at `opcode-trace.ts:21` states branch detection should fire only "at the same brace depth as clearbuffer," but the implementation just substring-matches `if(`/`else` on every line in the range. A nested helper call inside an `if` (e.g., `for(...){ if(condition) writeint(x); }`) will incorrectly trip `branches=true` and force `discriminator_hint='first_int_sign'`, which only `buildLoginResponseRow` actually consumes (since opcode 8 is special-cased at lines 251-258 of `build-table.ts`). For other opcodes the `branches` flag is mostly cosmetic, but if Phase 4+ ever uses it for codec selection, this is a pre-existing landmine.

**Fix:** Convert line numbers to 0-indexed at the loop boundary, and track depth:

```typescript
const lines = src.split('\n');
let branches = false;
let depth = 0; // depth relative to clearbuffer line
const startIdx = nearest; // 1-indexed → 0-indexed start (the line AFTER clearbuffer)
const endIdx = send.line - 1; // 0-indexed line of sendmessage (exclusive of writes after)
for (let ln = startIdx; ln < endIdx; ln++) {
  const lineText = lines[ln] ?? '';
  // Update depth from braces on this line.
  for (const c of lineText) {
    if (c === '{') depth++;
    else if (c === '}') depth--;
  }
  if (depth !== 0) continue; // only inspect siblings of the clearbuffer
  const t = lineText.trim();
  if (/^(if\b|else\b)/.test(t)) {
    branches = true;
    break;
  }
}
```

### WR-02: `xls-hints.ts` header-detection consumes a real data row when no header is present

**File:** `tools/protocol-doc/src/derive/xls-hints.ts:35-38`
**Issue:** The header-skip is gated on `dataLineCount === 0 && /opcode_byte/i.test(trimmed)`. If the CSV header line is missing AND the FIRST data row happens to contain the substring `opcode_byte` somewhere (e.g., a legacy_name like `set_opcode_byte` if the data ever has such a row), that data row is silently skipped as if it were a header. Conversely, if the header exists, `dataLineCount` increments after skipping the header — so subsequent data rows are processed correctly. The bug is narrow but real: a legacy_name accidentally containing `opcode_byte` is dropped.

Also, `dataLineCount` is incremented on every successfully-parsed data row but is never used after the header check. The variable is half-dead.

**Fix:** Use a dedicated boolean for header-tracking:

```typescript
let headerConsumed = false;
for (...) {
  // ... blank/comment skip ...
  if (!headerConsumed && /^opcode_byte\s*,/i.test(trimmed)) {
    headerConsumed = true;
    continue;
  }
  headerConsumed = true; // first non-blank, non-comment data row also "consumes" header slot
  // ... parse data ...
}
```

Or anchor the header detection to the start of the line: `/^opcode_byte\b/i`.

### WR-03: `lint-protocol.mjs` MVP names diverge from `lint-parity-checklist.mjs` MVP names — silent contract mismatch

**File:** `tools/protocol-doc/scripts/lint-protocol.mjs:46-54` vs `tools/asset-catalog/scripts/lint-parity-checklist.mjs:95-103`
**Issue:** `lint-protocol` enforces `MVP_NAMES = {movement, chat, login, login-response, room-join, room-leave, heartbeat}`. `lint-parity-checklist` enforces `MVP_REQUIRED = {movement, chat-public, login, login-response, room-join, room-leave, heartbeat}`. The `chat` vs `chat-public` divergence is potentially intentional (one is the wire-level opcode name, the other is the parity-feature name), but there is no comment in either lint asserting that distinction. A future maintainer renaming one will not realize the other lint independently encodes the canonical name.

This is a soft cross-contract issue: if Phase 4 codec generation pulls opcode name from protocol.json and parity name from parity-checklist.json, and those names are used as identifiers (e.g., method names), they'll silently diverge.

**Fix:** Either (a) align the names (rename the parity feature to `chat`, or rename the opcode to `chat-public`), or (b) add explicit comments in BOTH lint files cross-referencing the other and asserting the intentional divergence:

```javascript
// In lint-protocol.mjs:
// CLI-08 MVP names — MUST match lint-parity-checklist.mjs MVP_REQUIRED
// EXCEPT 'chat' here = 'chat-public' there (opcode level vs feature level).
// If you rename here, also rename there.
```

### WR-04: `lint-save-formats.mjs` PII regex misses common leaked-credential patterns

**File:** `tools/save-format-doc/scripts/lint-save-formats.mjs:166-177`
**Issue:** The PII guard catches three specific hardcoded passwords (`jarhead111`, `harrypotter`, `ilovepizza`) plus any `"password": "..."` key whose value isn't `<redacted...>`. This is fragile:

- Field names other than `password` (e.g., `pwd`, `pass`, `u_pwd` — and `u_pwd` is the actual GML field name from `User_DBUpdated.bnu` per `tables.ts:34`) are not checked.
- The hardcoded password list is a denylist; the next leaked password from a different account passes the lint.
- Any string field that happens to look like an email (`@`) or IP address has no guard.

A determined PII-leak prevention strategy would either (a) require all sample_records to be schema-validated with a dedicated `redacted: true` marker, or (b) match field name patterns inside sample_records and force them to `<redacted...>`.

**Fix:** Switch from value denylist to field-name allowlist of unredacted shapes:

```javascript
function findPiiLeak(samples) {
  const txt = JSON.stringify(samples);
  // Field names that MUST be redacted in any sample.
  const piiFields = ['password', 'pwd', 'u_pwd', 'pass', 'email', 'hash', 'legacyHash'];
  for (const field of piiFields) {
    const re = new RegExp(`"${field}"\\s*:\\s*"(?!<redacted)[^"]+"`, 'i');
    if (re.test(txt)) return `field '${field}' present unredacted`;
  }
  return null;
}
```

### WR-05: `runVerify` re-runs the full scanner on every invocation — verify output never compares the autogen-rewritten markdown

**File:** `tools/protocol-doc/src/emit/index.ts:126-184` and `tools/save-format-doc/src/emit/index.ts:294-356`
**Issue:** `runVerify` re-runs `scanGmlForOpcodes` / `scanGmlForSaveFormats` and re-emits markdown via `emitProtocolMd(expectedTable)` / `emitSaveFormatsMd(expectedTable)`. But `emitProtocolMd` writes the AUTOGEN markers with their refresh content already inlined (lines 96-114 of `markdown.ts`). The autogen rewrite step that runs during `runCatalog` (`runRegenAutogen` post-write) is NOT re-run during `runVerify`.

Result: if a maintainer hand-edits the AUTOGEN block content in `protocol.md` between `protocol-doc:catalog` and `protocol-doc:verify`, verify still reports OK because the byte-comparison is against `emitProtocolMd(table)` — which produces the SAME content the catalog wrote. But if any consumer (lint-protocol cross-checks) reads the actual markdown, drift goes undetected unless it also runs the autogen rewrite path.

This narrows to a minor verify-gate gap: the verify path tests `emit → byte-compare`, not `emit → autogen-refresh → byte-compare`. The autogen blocks are deterministic from the JSON, so in practice the discrepancy is small, but a test that validates the autogen-refreshed `.md` matches what `runCatalog` would write is missing.

**Fix:** In `runVerify`, after computing `expectedMd`, also rewrite autogen blocks against the expected JSON in a tmp copy and compare. Or, more simply, assert that re-running `runCatalog` against a tmp dir produces byte-identical output:

```typescript
// Snapshot: write expected outputs to tmp, run runRegenAutogen on tmp, compare.
const tmpDir = await fs.mkdtemp(join(tmpdir(), 'protocol-verify-'));
writeJsonDeterministic(join(tmpDir, 'protocol.json'), expectedTable);
writeMarkdownDeterministic(join(tmpDir, 'protocol.md'), emitProtocolMd(expectedTable));
await runRegenAutogen(tmpDir);
const tmpMd = readFileSync(join(tmpDir, 'protocol.md'), 'utf-8');
if (onDiskMd !== tmpMd) throw new Error('protocol.md drift after autogen refresh');
```

### WR-06: `extractFilenamePattern` builds `<expr:...>` placeholders that violate the `archived` PII assumption

**File:** `tools/save-format-doc/src/scanner/format-trace.ts:158-160`
**Issue:** When the parser encounters an unrecognised expression chunk in a filename argument, it emits `<expr:CHUNK>` verbatim into the `filename_pattern` field. If the original GML expression contains a string literal with PII (e.g., a hardcoded admin username embedded in a filename via `string("admin_"+account_name)`), that string flows into `protocol.json`/`save-formats.json`. Repo-private status mitigates this through Phase 7, but the lint provides no guard against exotic filename expressions leaking content.

A related issue: there is no upper-bound on the chunk length, so a multi-line concatenation resulting in a 500-byte expression would inflate the rendered filename_pattern in the markdown summary table.

**Fix:** Cap the chunk length and assert no string literals leak:

```typescript
} else if (chunk.length > 0) {
  // Defensive: any string literal in a runtime expression is suspicious.
  if (/"/.test(chunk)) {
    segments.push(`<expr:string-literal-redacted>`);
  } else {
    segments.push(`<expr:${chunk.slice(0, 64)}>`);
  }
}
```

### WR-07: `injectSamples` `archived_no_sample` branches collapse to identical behavior

**File:** `tools/save-format-doc/src/emit/index.ts:69-86`
**Issue:** Both branches of the inner `if/else if` set `next.archived_no_sample = true`:

```typescript
if (row.archived && !existsSync(samplesRoot)) {
  next.archived_no_sample = true;
} else if (row.archived) {
  next.archived_no_sample = true;
}
```

This is dead/duplicated code. The intent is unclear — possibly a placeholder for distinct behaviour if fixtures are missing vs. present-but-no-sample. As written, the conditions can be collapsed to `if (row.archived) { next.archived_no_sample = true; }`.

**Fix:** Collapse the redundant branches:

```typescript
if (row.archived) {
  next.archived_no_sample = true;
}
```

If the two branches are intended to diverge in the future, add a `// TODO` so the reviewer knows what differentiates them.

## Info

### IN-01: `inferRepoRootFromExtracted` in save-format-doc/emit/index.ts is dead code

**File:** `tools/save-format-doc/src/emit/index.ts:257-266`
**Issue:** `inferRepoRootFromExtracted` is defined but never referenced anywhere in the tool. Likely a leftover from an earlier draft of the resolver. Either wire it into `resolveExtractedDir` (and remove the duplicate logic) or delete it.

**Fix:** Delete the unused function.

### IN-02: `lint-subsystem-mds.mjs` filename-only regex fragile across path separators

**File:** `tools/asset-catalog/scripts/lint-subsystem-mds.mjs:206-208, 240-241`
**Issue:** Script-ID and object-ID enumeration uses `f.match(/^(\d+)-/)` against `readdirSync` results. On Windows this works because directory entries are filenames, not paths — but the comment "deterministic enumeration" elsewhere suggests cross-platform robustness was a goal. The regex is fine for filename-only entries, but a future reader could miss that the regex assumes a flat directory listing.

**Fix:** Add a comment clarifying the regex assumes filename-only entries (no path separators), or use `path.basename(f)` defensively.

### IN-03: `verify-phase-3.mjs` doesn't propagate per-step exit codes for diagnostic clarity

**File:** `scripts/verify-phase-3.mjs:50-57`
**Issue:** The script always exits 1 on any failure — but the underlying step's exit code (e.g., 2 for usage error, 1 for validation failure) is captured in `failed.status` for printing. Re-emitting the original status as the script's own exit code would let CI distinguish usage errors from validation failures.

**Fix:**

```javascript
process.exit(failed.status ?? 1);
```

### IN-04: `tools/db-schema/src/index.ts` re-exports without re-export-aware tooling

**File:** `tools/db-schema/src/index.ts:7`
**Issue:** `export * from './tables.js'` is fine, but the package has no explicit list of exported names. Phase 4 consumers will get every export from `tables.ts`, including any internal helpers added later (none today). For a published package surface this is a discipline issue; for an internal tool it's purely informational.

**Fix:** Consider switching to explicit named re-exports if/when the schema grows internal helpers:

```typescript
export {
  accounts, legacyCredentialsStaging, characters,
  inventoryItems, messageBoardTopics, messageBoardReplies,
  auditLog, sessions,
} from './tables.js';
```

---

_Reviewed: 2026-05-03_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_