--- phase: 01-extraction reviewed: 2026-05-02T00:00:00Z depth: standard files_reviewed: 67 files_reviewed_list: - .gitignore - decomp/TOOLS.md - package.json - tools/extract-gmd/.gitignore - tools/extract-gmd/README.md - tools/extract-gmd/cli.ts - tools/extract-gmd/data/README.md - tools/extract-gmd/data/action-ids.json - tools/extract-gmd/package.json - tools/extract-gmd/scripts/port-action-ids.ts - tools/extract-gmd/src/dnd/actionLookup.ts - tools/extract-gmd/src/dnd/readAction.ts - tools/extract-gmd/src/dnd/readActions.ts - tools/extract-gmd/src/dnd/transcompile.ts - tools/extract-gmd/src/emit/index.ts - tools/extract-gmd/src/emit/json.ts - tools/extract-gmd/src/emit/manifest.ts - tools/extract-gmd/src/emit/png.ts - tools/extract-gmd/src/emit/tree.ts - tools/extract-gmd/src/emit/unknown-actions.ts - tools/extract-gmd/src/extract.ts - tools/extract-gmd/src/reader/BinaryReader.ts - tools/extract-gmd/src/reader/backgrounds.ts - tools/extract-gmd/src/reader/datafiles.ts - tools/extract-gmd/src/reader/fonts.ts - tools/extract-gmd/src/reader/header.ts - tools/extract-gmd/src/reader/objects.ts - tools/extract-gmd/src/reader/paths.ts - tools/extract-gmd/src/reader/readProjectFile.ts - tools/extract-gmd/src/reader/rooms.ts - tools/extract-gmd/src/reader/scripts.ts - tools/extract-gmd/src/reader/settings.ts - tools/extract-gmd/src/reader/sounds.ts - tools/extract-gmd/src/reader/sprites.ts - tools/extract-gmd/src/reader/timelines.ts - tools/extract-gmd/src/types.ts - tools/extract-gmd/src/verify.ts - tools/extract-gmd/tests/dnd/readAction.test.ts - tools/extract-gmd/tests/dnd/transcompile.test.ts - tools/extract-gmd/tests/emit/json.test.ts - tools/extract-gmd/tests/emit/manifest.test.ts - tools/extract-gmd/tests/emit/png.test.ts - tools/extract-gmd/tests/emit/tree.test.ts - tools/extract-gmd/tests/emit/unknown-actions.test.ts - tools/extract-gmd/tests/fixtures/README.md - tools/extract-gmd/tests/fixtures/build-fixtures.ts - tools/extract-gmd/tests/integration/cli-extract.test.ts - tools/extract-gmd/tests/integration/cli-verify.test.ts - tools/extract-gmd/tests/integration/extract-client.test.ts - tools/extract-gmd/tests/integration/extract-server.test.ts - tools/extract-gmd/tests/integration/no-plaintext-creds.test.ts - tools/extract-gmd/tests/integration/reproducibility.test.ts - tools/extract-gmd/tests/integration/tree-shape.test.ts - tools/extract-gmd/tests/reader/BinaryReader.test.ts - tools/extract-gmd/tests/reader/backgrounds.test.ts - tools/extract-gmd/tests/reader/datafiles.test.ts - tools/extract-gmd/tests/reader/fonts.test.ts - tools/extract-gmd/tests/reader/header.test.ts - tools/extract-gmd/tests/reader/objects.test.ts - tools/extract-gmd/tests/reader/paths.test.ts - tools/extract-gmd/tests/reader/readProjectFile.test.ts - tools/extract-gmd/tests/reader/rooms.test.ts - tools/extract-gmd/tests/reader/scripts.test.ts - tools/extract-gmd/tests/reader/settings.test.ts - tools/extract-gmd/tests/reader/sounds.test.ts - tools/extract-gmd/tests/reader/sprites.test.ts - tools/extract-gmd/tests/reader/timelines.test.ts findings: critical: 0 warning: 6 info: 9 total: 15 status: issues_found --- # Phase 1: Code Review Report **Reviewed:** 2026-05-02 **Depth:** standard **Files Reviewed:** 67 **Status:** issues_found ## Summary The Phase 1 extractor is a careful, well-documented LateralGM port. Discipline is high across the board: - **LateralGM source-faithfulness markers** are pervasive (`// Source: org.lateralgm.file.GmFileReader.read{X}` headers + line citations in nearly every reader; the 13 wire-format Plan-07 bugs each carry a `// Bug N` comment with rationale). - **ISO-8859-1 string decoding** is correctly pinned in `BinaryReader.readStrBody` (`buf.toString('latin1', ...)`) with a load-bearing test (`BR-08`) that asserts `0xA9 → ©` survives without UTF-8 replacement-char corruption. The same `latin1` pin appears in the porter's LGL reader. - **Bounds checks and 64 MiB ZLIB-bomb cap** are present on every primitive in `BinaryReader` and on the central `decompress()` helper. - **Determinism** is well-engineered: sorted-keys JSON (`emit/json.ts`), LF-only line endings (`normalizeLineEndings` in `emit/tree.ts`), POSIX-normalized manifest paths sorted after cross-OS rewrite (`emit/manifest.ts`), no `Date.now()` in any emit module, sharp+libvips versions surfaced in MANIFEST header. - **Path traversal** is defended in `sanitizeName` (strips `/`, `\\`, `..`, control chars, dot-only names; caps length at 100). The original name is preserved in `meta.json`, only the path component is sanitized. - **TypeScript strict hygiene** is solid: `strict`, `noUncheckedIndexedAccess`, `exactOptionalPropertyTypes` all on; the codebase does not contain `as any` (only narrowly-justified `as unknown as { versions: ... }` for the sharp internals). - **Test discipline** is mostly real-behavior: golden-fixture round-trips for each block, plus `tests/integration/extract-client.test.ts` / `extract-server.test.ts` parse the actual BNO `.gmd` files and assert the BMP magic, path-prefix discipline, no replacement chars in resource names, and embedded-audio-file presence. Reproducibility is asserted across two full-tree extracts in `reproducibility.test.ts`. The findings below are mostly hardening and consistency items, not correctness defects. No critical issues. The most worth-doing items are **WR-01** (consolidate ad-hoc `inflateSync` usage in `sounds.ts` / `fonts.ts` to flow through `BinaryReader.decompress()` for uniform 64 MiB enforcement and uniform error wording) and **WR-02** (defensive pre-read file-size cap in `extract.ts`). --- ## Warnings ### WR-01: Inline `inflateSync` in `sounds.ts` bypasses `BinaryReader.decompress()` post-cap recheck **Files:** `tools/extract-gmd/src/reader/sounds.ts:77`, `tools/extract-gmd/src/reader/sounds.ts:91` **Issue:** `BinaryReader.decompress()` (line 158-161) applies `maxOutputLength: 64 MiB` AND a defensive post-inflate `if (out.length > MAX_INFLATE_BYTES) throw` recheck. `sounds.ts` instead calls `inflateSync(compressed, { maxOutputLength: MAX_INFLATE_BYTES })` directly with the same cap but no post-check. In Node's current implementation `maxOutputLength` is enforced and the post-check is redundant; nevertheless having two ZLIB call sites with subtly different defenses is a maintainability hazard — a future ZLIB-cap audit will see `decompress()` and miss these. Also, `compressedLen` is read via signed `readInt32LE()` (lines 75, 89) then handed to `readBytes(n)`; a corrupted/malicious `.gmd` with the high bit set would cause `readBytes` to reject as "negative length" with an unrelated error message ("readBytes negative length …") instead of a ZLIB framing diagnostic. **Fix:** Have `sounds.ts` call `r.decompress()` to align with `fonts.ts:82` (datafile branch) and `BinaryReader.readZlibImage()` (sprites/backgrounds). Read `compressedLen` via `readUint32LE` everywhere it is used as a length prefix. ```ts // In sounds.ts ver2 === 440 branch: audioBytes = r.decompress(); // In sounds.ts ver2 === 600 branch: audioBytes = r.decompress(); ``` The current ver2-not-440 branch's "raw bytes" path (line 92-94) should use `readUint32LE` for `dataLen`. ### WR-02: `extract.ts` `readFileSync(inputPath)` has no pre-read size cap **File:** `tools/extract-gmd/src/extract.ts:17` **Issue:** Threat T-01-02 (ZLIB bomb) is mitigated post-read via the 64 MiB inflate cap, and T-01-01 (length-prefix attack) is mitigated inside `BinaryReader.readBytes`. But the very first I/O — `readFileSync(inputPath)` — accepts any file size. A 5 GiB malicious or accidental input (or an FS-mount pointing at `/dev/zero` lookalike) would OOM the Node process before any header validation runs. Real BNO `.gmd` files top out at <30 MiB; a generous cap of e.g. 256 MiB would still admit any plausible v530-era project. **Fix:** ```ts import { statSync, readFileSync } from 'node:fs'; const MAX_GMD_BYTES = 256 * 1024 * 1024; // 256 MiB; BNO files are <30 MiB export async function extract(inputPath: string, outDir: string): Promise { const size = statSync(inputPath).size; if (size > MAX_GMD_BYTES) { throw new Error(`Input file ${size} bytes exceeds ${MAX_GMD_BYTES}-byte cap`); } const buf = readFileSync(inputPath); // ... } ``` ### WR-03: `readFontsOrDataFiles` `r.skip(skipLen)` accepts negative length without explicit guard **File:** `tools/extract-gmd/src/reader/fonts.ts:68-69` **Issue:** `skipLen = r.readInt32LE()` is signed; `r.skip(skipLen)` does throw on negative (line 28-30 of BinaryReader correctly checks `n < 0`). Functionally safe — but the diagnostic ("skip negative length …") will hide the underlying root cause (corrupted datafile-block header). Same pattern recurs in `settings.ts:144` (`iconLen` read via signed `readInt32LE`, capped by an explicit `iconLen < 0 || iconLen > MAX_ICO_BYTES` check, which is the better pattern). For consistency the datafile skip-string read should match. **Fix:** ```ts const skipLen = r.readInt32LE(); if (skipLen < 0 || skipLen > 1024 * 1024) { throw new Error(`readFontsOrDataFiles: datafile skip-string length ${skipLen} out of bounds at id ${id}`); } r.skip(skipLen); ``` ### WR-04: `readSettings` uses signed `readInt32LE` for `compressedLen` in `skipZlibImage` **File:** `tools/extract-gmd/src/reader/settings.ts:42-44` **Issue:** `skipZlibImage` reads `const compressedLen = r.readInt32LE()` then `r.skip(compressedLen)`. A negative value would be rejected by `r.skip` but with a non-specific error. Worse, an attacker-supplied positive value up to `INT32_MAX` (2 GiB) would be accepted by `skip` until it hits the buffer-size check, but the error wouldn't communicate "ZLIB image header was implausible." Cap it. **Fix:** ```ts function skipZlibImage(r: BinaryReader): void { const compressedLen = r.readInt32LE(); if (compressedLen < 0 || compressedLen > 64 * 1024 * 1024) { throw new Error(`readSettings.skipZlibImage: compressed length ${compressedLen} out of bounds`); } if (compressedLen > 0) r.skip(compressedLen); } ``` ### WR-05: `readBackgrounds`/`readSprites` `marker` value other than `-1` or `10` is silently accepted **Files:** `tools/extract-gmd/src/reader/sprites.ts:139-142`, `tools/extract-gmd/src/reader/backgrounds.ts:94-95` **Issue:** Both readers document that `marker` is a presence flag with `-1` meaning "no data" and `10` meaning "present" (per LateralGM `readZlibImage`). The current code only branches on `marker === -1`; any other value (including `0`, `7`, `42`, …) silently falls through to `readZlibImage()` as if it were `10`. The sprites comment even acknowledges this ("other values are file format mysteries — log via cursor in error rather than throw") but does not actually log or throw. A corrupted input could pass parsing and produce nonsense images. **Fix:** Treat unexpected markers as a parse error so drift is visible (cursor position is preserved in the message): ```ts const marker = r.readInt32LE(); if (marker === -1) { /* sentinel */ continue; } if (marker !== 10) { throw new Error(`readSprites: unexpected frame presence marker ${marker} at id=${id} f=${f} cursor=${r.cursor}`); } const inflated = r.readZlibImage(); ``` ### WR-06: `transcompile` `@N` substitution treats `argValues[i]` `undefined` as empty string silently **File:** `tools/extract-gmd/src/dnd/transcompile.ts:44-48`, `tools/extract-gmd/src/dnd/transcompile.ts:60` **Issue:** `formatArg(undefined) → ''`. Combined with `noUncheckedIndexedAccess` returning `T | undefined` for `action.argValues[i]`, this means a template like `instance_create(@0, @1, @2)` with only 2 supplied `argValues` will render as `instance_create(arg0, arg1, )` — syntactically invalid GML. Phase 1 deliberately treats `.dnd.json` as canonical truth and `.gml` as best-effort (D-09/D-10), so this is not a correctness bug, but it produces silently malformed output that a Phase 6+ consumer may run through a GML linter. Worth flagging in the rendered comment so it surfaces in diff review. **Fix:** When a template `@N` resolves to `undefined`, emit a visible placeholder so the malformedness is auditable: ```ts function formatArg(v: string | number | boolean | undefined): string { if (v === undefined) return '/* MISSING_ARG */'; // ...rest unchanged } ``` Alternative: if `argValues.length < expectedArgCount` (from `lib.argCount`), bail out to the unknown-style stub `// PARTIAL ACTION_ID=; see .dnd.json`. --- ## Info ### IN-01: `readBackgrounds` writes pre-bbox state into output even when `image` is absent **File:** `tools/extract-gmd/src/reader/backgrounds.ts:109-115` **Issue:** When `hasImage === false` (or `marker === -1`), `width` and `height` are still emitted as the just-read int32s, but `image` is omitted. Consumers reading `bg.width`/`bg.height` for an image-less background get the wire-stream values (which may have been zero or garbage depending on how the source IDE wrote an empty layer). This is faithful to LateralGM but semantically loose. Document or set width/height to 0 when no image present. Low priority — there are no observed image-less backgrounds in BNO. **Fix:** Add an inline comment noting `width`/`height` may be zero when `image` is absent, OR clamp explicitly when no image: leaving as-is is also fine given the canonical-truth philosophy. ### 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` (many `void _xxx;`) **Issue:** Throughout the readers, fields are read with `const _foo = r.readBool(); void _foo;` to satisfy `noUnusedLocals`. This is the correct Plan-07 pattern, but the data is discarded silently. The canonical record (`meta.json` etc.) is therefore lossy. For Phase 2/3 forensic record, these fields may matter (e.g. `_isometric`, `_snapX/_snapY`, `_locked` per instance, `_libId` in actions). Several such fields are surfaced into `settings.raw` already; consider doing the same for rooms (a `raw` catch-all) and DnD actions. **Fix:** 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. ### IN-03: `eventTypeName` `names` array is `noUncheckedIndexedAccess`-protected, but `names[eventType]` returns `undefined` for `eventType >= 12` **File:** `tools/extract-gmd/src/emit/tree.ts:83-93` **Issue:** `noUncheckedIndexedAccess` makes `names[eventType]` `string | undefined`. The `?? \`Event${eventType}\`` fallback handles the out-of-range case. Good. But Bug 8 (`noEvents = read4 + 1`) means real BNO objects iterate `eventType` 0..11 (12 types); a future GM 8.x file with more event types would exercise the `Event{N}` fallback silently. Add a brief comment explaining the fallback exists for forward-compat. **Fix:** Comment-only — note in the function header that the array indexes 0..11 are the GM 5.x stock event types and `Event{N}` covers GM 8.x extensions discovered later. ### IN-04: `actionLookup.ts` uses module-level mutable `_diskReadCount` for testing instead of a DI hook **File:** `tools/extract-gmd/src/dnd/actionLookup.ts:24,31,44-52` **Issue:** Test-only counter and reset hook live in production module surface (`_resetCacheForTests`, `_diskReadsForTests`). The header comment in `transcompile.test.ts` explains that Vitest 4 cannot `vi.spyOn` ESM-imported `fs.readFileSync`, which makes this defensible. Underscore-prefix convention plus the explicit "Test-only" JSDoc are fine signaling. Low-impact — just be aware tree-shaking will retain these symbols. An alternative would be to inject the loader function via a default export, but the current approach is pragmatic. **Fix:** None required. Consider tagging with `/** @internal */` for documentation tooling. ### IN-05: `cli.ts` direct-invocation detection is heuristic **File:** `tools/extract-gmd/cli.ts:78-81` **Issue:** ```ts const invokedDirectly = import.meta.url === `file://${process.argv[1]}` || process.argv[1]?.endsWith('cli.ts') || process.argv[1]?.endsWith('cli.js'); ``` The first comparison is correct for POSIX but fragile on Windows (drive-letter case differences, `file:///C:/...` form vs `C:\...`). The fallback `endsWith('cli.ts')` works but would also match a hypothetical `not-our-cli.ts`. The `build-fixtures.ts` `main()` guard at line 988 uses `fileURLToPath(import.meta.url) === process.argv[1]`, which is the more portable pattern. **Fix:** Align with the build-fixtures pattern: ```ts import { fileURLToPath } from 'node:url'; const invokedDirectly = process.argv[1] !== undefined && fileURLToPath(import.meta.url) === process.argv[1]; ``` ### IN-06: `verifyManifest` regex `^([0-9a-f]{64})\s\s(.+)$` requires LITERAL two spaces; CRLF line in manifest would silently mis-parse **File:** `tools/extract-gmd/src/emit/manifest.ts:74-78` **Issue:** Manifest is written with `\n` join (line 56), so on POSIX this is fine. But if a user opens `MANIFEST.sha256` on Windows in a CRLF-converting editor and saves, the trailing `\r` becomes part of the captured `relPath`, so every line will report drift. Worth either rejecting `\r` explicitly with a clearer error, or stripping `\r` before regex. **Fix:** ```ts for (const rawLine of raw.split('\n')) { const line = rawLine.replace(/\r$/, ''); if (!line || line.startsWith('#')) continue; // ... } ``` ### IN-07: `port-action-ids.ts` `clone()` uses `execSync` with template-string interpolation but `LATERALGM_REPO`/`LATERALGM_TAG` are constants **File:** `tools/extract-gmd/scripts/port-action-ids.ts:274-280` **Issue:** `execSync(\`git clone --depth 1 --branch v${LATERALGM_TAG} ... ${LATERALGM_REPO} "${dir}"\`)` — the variables are module-level constants so this is not a command injection vector today, but the pattern would break the moment someone parameterizes `LATERALGM_TAG` from CLI. `dir` comes from `mkdtempSync` so it is a system-chosen path. Consider switching to `execFileSync('git', ['clone', '--depth', '1', ...])` for defense-in-depth. **Fix:** ```ts import { execFileSync } from 'node:child_process'; execFileSync('git', [ 'clone', '--depth', '1', '--branch', `v${LATERALGM_TAG}`, '--filter=blob:none', '--sparse', LATERALGM_REPO, dir, ], { stdio: ['ignore', 'inherit', 'inherit'] }); ``` ### IN-08: `tests/fixtures/*.gmd` binary blobs are committed but their byte sizes aren't asserted **Files:** `tools/extract-gmd/tests/fixtures/tiny-empty.gmd`, `tiny-script.gmd`, `tiny-sprite.gmd` (all binary) **Issue:** These three fixtures are committed binary blobs (per `tests/fixtures/build-fixtures.ts` `main()` and the README at `tests/fixtures/README.md`). `tiny-script.gmd` and `tiny-sprite.gmd` are currently stubs that return `buildTinyEmpty()` (line 908-915) — they are byte-identical to `tiny-empty.gmd`. Either delete the redundant fixtures and the stub functions, or make them distinct (extend `buildTinyScript` to actually exercise a script block, etc.). As-is, they create the impression of test coverage that does not exist. **Fix:** Either: 1. `rm tiny-script.gmd tiny-sprite.gmd` and remove the stub functions; rely on `buildTinyFull()` for end-to-end coverage. 2. Implement real script/sprite content per the original plan-02/plan-03 intent. ### IN-09: `actionLookup.ts` `LibAction` interface is duplicated in `port-action-ids.ts` **Files:** `tools/extract-gmd/src/dnd/actionLookup.ts:12-20`, `tools/extract-gmd/scripts/port-action-ids.ts:69-77` **Issue:** The `LibAction` shape is declared in both files. They agree today, but a future field rename would silently desynchronize (CI catches the JSON drift via `--check`, but not the TS shape drift). Extract to a shared type module if the porter is considered part of the build (it currently is — `tsconfig.json:14` includes `tests/**/*.ts` but not `scripts/**/*.ts`, so the porter is essentially independent — fine to leave as-is, but worth documenting). **Fix:** Optional — extract to `src/dnd/LibAction.ts` and import in both, OR add a comment in each declaration referencing the other. --- _Reviewed: 2026-05-02_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_