---
phase: 01-extraction
fixed_at: 2026-05-02T19:55:00Z
review_path: .planning/phases/01-extraction/01-REVIEW.md
iteration: 1
findings_in_scope: 15
fixed: 14
skipped: 1
status: partial
---

# Phase 1: Code Review Fix Report

**Fixed at:** 2026-05-02T19:55:00Z
**Source review:** `.planning/phases/01-extraction/01-REVIEW.md`
**Iteration:** 1

**Summary:**
- Findings in scope: 15 (0 critical, 6 warning, 9 info)
- Fixed: 14
- Skipped: 1 (IN-02, deliberately documentation-only per its own Fix section)

All 128 unit tests + 29 integration tests pass after fixes.

## Fixed Issues

### WR-01: Inline `inflateSync` in `sounds.ts` bypasses `BinaryReader.decompress()` post-cap recheck

**Files modified:** `tools/extract-gmd/src/reader/sounds.ts`
**Commit:** 6e60be7
**Applied fix:** Replaced both v440 and v600 inline `inflateSync(compressed, { maxOutputLength: ... })` call sites with `r.decompress()` so all ZLIB streams flow through the same 64 MiB-capped path used by `fonts.ts` (datafile branch) and `BinaryReader.readZlibImage()` (sprites/backgrounds). Also switched the v800 raw-bytes branch from signed `readInt32LE()` to `readUint32LE()` so a high-bit-set length yields a clear out-of-bounds diagnostic instead of "readBytes negative length …". Removed now-unused `inflateSync` import + local `MAX_INFLATE_BYTES` constant.

### WR-02: `extract.ts` `readFileSync(inputPath)` has no pre-read size cap

**Files modified:** `tools/extract-gmd/src/extract.ts`
**Commit:** a8369dd
**Applied fix:** Added `MAX_GMD_BYTES = 256 * 1024 * 1024` constant + `statSync(inputPath).size` pre-check that throws before `readFileSync` runs. Real BNO files top out <30 MiB, so the cap is generous; pathological inputs (5 GiB blob, /dev/zero stream) are rejected before they OOM Node. Comment cites threat T-01-02 (extension of the post-read ZLIB-bomb defense to the pre-read I/O boundary).

### WR-03: `readFontsOrDataFiles` `r.skip(skipLen)` accepts negative length without explicit guard

**Files modified:** `tools/extract-gmd/src/reader/fonts.ts`
**Commit:** cf29119
**Applied fix:** Added explicit `if (skipLen < 0 || skipLen > 1024 * 1024) throw` before `r.skip(skipLen)` in the datafile skip-string read. The diagnostic now names the datafile block + id, matching the style of `settings.ts:144` (`iconLen` cap) instead of falling through to the lower-level "skip negative length …" message.

### WR-04: `readSettings` uses signed `readInt32LE` for `compressedLen` in `skipZlibImage`

**Files modified:** `tools/extract-gmd/src/reader/settings.ts`
**Commit:** e22e1b1
**Applied fix:** `skipZlibImage` now rejects `compressedLen < 0` or `> 64 MiB` with a settings-specific diagnostic. The cap mirrors the inflate ceiling — anything larger cannot inflate legally either, so this is a strict tightening of the existing post-read defense.

### WR-05: `readBackgrounds`/`readSprites` `marker` value other than `-1` or `10` is silently accepted

**Files modified:** `tools/extract-gmd/src/reader/sprites.ts`, `tools/extract-gmd/src/reader/backgrounds.ts`
**Commit:** 879cc58
**Applied fix:** Both readers now throw `unexpected ... presence marker {marker}` (with cursor position) for any value other than `-1` (sentinel: no data) or `10` (LateralGM presence flag). Previously, garbage markers fell through to `readZlibImage()` and could produce nonsense images. Comments updated to match.

### WR-06: `transcompile` `@N` substitution treats `argValues[i]` `undefined` as empty string silently

**Files modified:** `tools/extract-gmd/src/dnd/transcompile.ts`
**Commit:** 3f4e451
**Applied fix:** `formatArg(undefined)` now returns `'/* MISSING_ARG */'` instead of `''`. A template like `instance_create(@0, @1, @2)` with only 2 supplied `argValues` now renders as `instance_create(arg0, arg1, /* MISSING_ARG */)` — visibly malformed in diff review and discoverable by a downstream GML linter, rather than silent.

### IN-01: `readBackgrounds` writes pre-bbox state into output even when `image` is absent

**Files modified:** `tools/extract-gmd/src/reader/backgrounds.ts`
**Commit:** a6d1ba4
**Applied fix:** Documentation-only comment block above the `Background` constructor noting that `width`/`height` may be 0 or wire-stream values when `image` is undefined; consumers should treat them as meaningful only when `bg.image` is present. Faithful-to-LateralGM behavior preserved per the canonical-truth philosophy.

### IN-03: `eventTypeName` `names` array is `noUncheckedIndexedAccess`-protected, but `names[eventType]` returns `undefined` for `eventType >= 12`

**Files modified:** `tools/extract-gmd/src/emit/tree.ts`
**Commit:** df2bacb
**Applied fix:** Expanded JSDoc on `eventTypeName` to explain that indices 0..11 are GM 5.x stock event types (per Plan-07 Bug-8 `noEvents = read4 + 1`) and that the `Event{N}` fallback is forward-compat for hypothetical GM 8.x extensions.

### IN-04: `actionLookup.ts` uses module-level mutable `_diskReadCount` for testing instead of a DI hook

**Files modified:** `tools/extract-gmd/src/dnd/actionLookup.ts`
**Commit:** 220e306
**Applied fix:** Tagged `_diskReadCount`, `_resetCacheForTests`, and `_diskReadsForTests` with `@internal` JSDoc so documentation tooling treats them as internal. The original underscore-prefix + Test-only signaling is preserved.

### IN-05: `cli.ts` direct-invocation detection is heuristic

**Files modified:** `tools/extract-gmd/cli.ts`
**Commit:** 841387c
**Applied fix:** Replaced the three-clause heuristic (`file://...` raw compare + two `endsWith` fallbacks) with the canonical pattern from `build-fixtures.ts:988`: `process.argv[1] !== undefined && fileURLToPath(import.meta.url) === process.argv[1]`. Portable across POSIX and Windows; no false-positive on a hypothetical `not-our-cli.ts`.

### IN-06: `verifyManifest` regex requires LITERAL two spaces; CRLF line in manifest would silently mis-parse

**Files modified:** `tools/extract-gmd/src/emit/manifest.ts`
**Commit:** fedaed2
**Applied fix:** Strip trailing `\r` from each line before regex match, so a Windows-edited `MANIFEST.sha256` (CRLF-converted) doesn't capture the `\r` into the relPath group and fail every line as drift.

### IN-07: `port-action-ids.ts` `clone()` uses `execSync` with template-string interpolation

**Files modified:** `tools/extract-gmd/scripts/port-action-ids.ts`
**Commit:** 331c599
**Applied fix:** Migrated both `execSync` call sites (`git clone …` and `git -C … sparse-checkout`) to `execFileSync('git', […])` with structured argv arrays. Removed `execSync` import (now unused). Defense-in-depth — the variables are module-level constants so this is preventative against a future "parameterize from CLI" change.

### IN-08: `tests/fixtures/*.gmd` binary blobs are committed but redundant

**Files modified:** `tools/extract-gmd/tests/fixtures/build-fixtures.ts`, deleted `tests/fixtures/tiny-script.gmd` and `tests/fixtures/tiny-sprite.gmd`
**Commit:** f5bca02
**Applied fix:** Took option 1 from the review: removed the `buildTinyScript()` and `buildTinySprite()` stub functions (both were `return buildTinyEmpty()`), removed their write-out from `main()`, and deleted the two stub-output binaries. End-to-end script + sprite coverage is provided by `buildTinyFull()` (per Plan 02-04 block-specific writers) which is consumed in-memory by `tests/reader/readProjectFile.test.ts`. No grep references to `tiny-script.gmd` / `tiny-sprite.gmd` outside the deleted code, so this is a clean removal.

### IN-09: `actionLookup.ts` `LibAction` interface is duplicated in `port-action-ids.ts`

**Files modified:** `tools/extract-gmd/src/dnd/actionLookup.ts`, `tools/extract-gmd/scripts/port-action-ids.ts`
**Commit:** 3eacf60
**Applied fix:** Took the lightweight option from the review (added cross-reference comments rather than extracting to a shared module) since the porter is intentionally outside `tsconfig.include` and cross-import would entangle the two builds. Each `LibAction` declaration now points to the other and notes the CI guard catches JSON drift but not TS shape drift.

## Skipped Issues

### IN-02: `_libId` and many other "discarded" fields are read+ignored without surfacing into the canonical record

**Files:** `tools/extract-gmd/src/dnd/readAction.ts:55,84-85`, `tools/extract-gmd/src/reader/rooms.ts:84-86,87-88,94-96`
**Reason:** skipped: the finding's own Fix section explicitly states "No code change required for Phase 1. Note for Phase 2 planning: enumerate what fields would be useful to surface in `raw` (rooms, actions, paths) once Phase 2/3 readers exist as consumers and define what they need." This is documentation guidance for Phase 2 planning, not a Phase 1 code change. Recorded here so the Phase 2 planner is reminded to revisit `raw` catch-all surfacing for rooms / DnD actions / paths.
**Original issue:** Throughout the readers, fields are read with `const _foo = r.readBool(); void _foo;` to satisfy `noUnusedLocals`. The canonical record is therefore lossy. For Phase 2/3 forensic record, these fields may matter (e.g. `_isometric`, `_snapX/_snapY`, `_locked` per instance, `_libId` in actions).

---

_Fixed: 2026-05-02T19:55:00Z_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 1_