From f2ebe24e297701084bf24a88e70e9efa62d2dd99 Mon Sep 17 00:00:00 2001 From: Aaron Stainback Date: Fri, 8 May 2026 22:23:52 -0400 Subject: [PATCH] fix(B-0272): address ROM tool review nits Tighten the ROM canonicalizer review surface by documenting the actual JSON fields, hashing files with bounded memory, ignoring extensionless files by default, using dirname for rename targets, validating option values, bumping the backlog timestamp, and keeping the DAT fixture realistic. Focused checks: bun test tools/roms/canonicalize.test.ts; git diff --check. Local typecheck remains blocked because tsc is not installed in this worktree. Co-Authored-By: Codex --- ...anonical-naming-tosec-lookup-2026-05-08.md | 2 +- tools/roms/canonicalize.test.ts | 36 ++++++++++++++- tools/roms/canonicalize.ts | 44 +++++++++++++++---- 3 files changed, 71 insertions(+), 11 deletions(-) diff --git a/docs/backlog/P1/B-0272-atari-rom-canonical-naming-tosec-lookup-2026-05-08.md b/docs/backlog/P1/B-0272-atari-rom-canonical-naming-tosec-lookup-2026-05-08.md index 87ac963323..7424bd492c 100644 --- a/docs/backlog/P1/B-0272-atari-rom-canonical-naming-tosec-lookup-2026-05-08.md +++ b/docs/backlog/P1/B-0272-atari-rom-canonical-naming-tosec-lookup-2026-05-08.md @@ -4,7 +4,7 @@ priority: P1 status: open title: "Atari 2600 ROM canonical naming via TOSEC/No-Intro hash lookup" created: 2026-05-08 -last_updated: 2026-05-08 +last_updated: 2026-05-09 parent: B-0083 depends_on: [] classification: buildable-now diff --git a/tools/roms/canonicalize.test.ts b/tools/roms/canonicalize.test.ts index f7472d63f9..7f9e851afb 100644 --- a/tools/roms/canonicalize.test.ts +++ b/tools/roms/canonicalize.test.ts @@ -2,7 +2,13 @@ import { describe, expect, test } from "bun:test"; import { existsSync, mkdtempSync, writeFileSync, readFileSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { parseDatfile, hashFileSha1, scanRomFiles, matchAndReport } from "./canonicalize.ts"; +import { + main, + parseDatfile, + hashFileSha1, + scanRomFiles, + matchAndReport, +} from "./canonicalize.ts"; const FIXTURE_DATFILE = ` @@ -12,7 +18,7 @@ const FIXTURE_DATFILE = ` Combat (1977)(Atari) - + Adventure (1980)(Atari) @@ -92,6 +98,8 @@ describe("scanRomFiles", () => { const tmp = mkdtempSync(join(tmpdir(), "rom-scan-")); writeFileSync(join(tmp, "notes.txt"), "data"); writeFileSync(join(tmp, "save.sav"), "data"); + writeFileSync(join(tmp, "README"), "data"); + writeFileSync(join(tmp, ".DS_Store"), "data"); const files = scanRomFiles(tmp); expect(files.length).toBe(0); @@ -179,3 +187,27 @@ describe("matchAndReport", () => { expect(results[0]?.renamed).toBe(false); }); }); + +describe("main", () => { + test("reports missing datfile value clearly", () => { + const originalStderrWrite = process.stderr.write; + const originalExit = process.exit; + let stderr = ""; + + process.stderr.write = ((chunk: string | Uint8Array) => { + stderr += String(chunk); + return true; + }) as typeof process.stderr.write; + process.exit = ((code?: number) => { + throw new Error(`exit:${code}`); + }) as typeof process.exit; + + try { + expect(() => main(["--datfile", "--dir", "/tmp"])).toThrow("exit:64"); + expect(stderr).toContain("missing value for --datfile"); + } finally { + process.stderr.write = originalStderrWrite; + process.exit = originalExit; + } + }); +}); diff --git a/tools/roms/canonicalize.ts b/tools/roms/canonicalize.ts index 82e9b7b3d9..01e920ac78 100644 --- a/tools/roms/canonicalize.ts +++ b/tools/roms/canonicalize.ts @@ -6,17 +6,21 @@ // bun tools/roms/canonicalize.ts --datfile --dir // bun tools/roms/canonicalize.ts --datfile --dir --apply // -// Output (default dry-run): JSON array of { file, sha1, match, canonicalName }. +// Output (default dry-run): JSON array of +// { file, sha1, matched, canonicalName, renamed }. // --apply: renames matched files to their canonical names. import { createHash } from "node:crypto"; import { + closeSync, + openSync, readdirSync, readFileSync, + readSync, renameSync, existsSync, } from "node:fs"; -import { basename, extname, join } from "node:path"; +import { basename, dirname, extname, join } from "node:path"; // --- Datfile parsing (Logiqx XML) --- @@ -132,8 +136,21 @@ export function parseDatfile(xml: string): ReadonlyMap { // --- File hashing --- export function hashFileSha1(path: string): string { - const data = readFileSync(path); - return createHash("sha1").update(data).digest("hex"); + const hash = createHash("sha1"); + const fd = openSync(path, "r"); + const buffer = Buffer.allocUnsafe(1024 * 1024); + try { + let bytesRead = 0; + do { + bytesRead = readSync(fd, buffer, 0, buffer.length, null); + if (bytesRead > 0) { + hash.update(buffer.subarray(0, bytesRead)); + } + } while (bytesRead > 0); + } finally { + closeSync(fd); + } + return hash.digest("hex"); } // --- Directory scanning --- @@ -143,7 +160,7 @@ export function scanRomFiles(dir: string): readonly string[] { for (const entry of readdirSync(dir, { withFileTypes: true })) { if (!entry.isFile()) continue; const ext = extname(entry.name).toLowerCase(); - if (ROM_EXTENSIONS.has(ext) || ext === "") { + if (ROM_EXTENSIONS.has(ext)) { files.push(join(dir, entry.name)); } } @@ -189,7 +206,7 @@ export function matchAndReport( let renamed = false; if (apply && !alreadyCorrect) { - const dir = filePath.slice(0, filePath.length - currentName.length); + const dir = dirname(filePath); if (!isSafeCanonicalName(canonicalName)) { process.stderr.write( `skip: unsafe canonical name from datfile: ${canonicalName}\n`, @@ -241,12 +258,23 @@ function parseArgs(argv: readonly string[]): Args { let dir: string | undefined; let apply = false; + function readOptionValue(index: number, flag: string): string { + const value = argv[index + 1]; + if (value === undefined || value.startsWith("-")) { + process.stderr.write(`missing value for ${flag}\n`); + process.exit(64); + } + return value; + } + for (let i = 0; i < argv.length; i++) { const arg = argv[i]; if (arg === "--datfile" || arg === "-d") { - datfile = argv[++i]; + datfile = readOptionValue(i, arg); + i++; } else if (arg === "--dir") { - dir = argv[++i]; + dir = readOptionValue(i, arg); + i++; } else if (arg === "--apply") { apply = true; } else if (arg === "--help" || arg === "-h") {