Skip to content

fix(B-0272): accept hyphen-prefixed ROM option values#2168

Merged
AceHack merged 4 commits into
mainfrom
codex/b0272-rom-review-nits-followup
May 9, 2026
Merged

fix(B-0272): accept hyphen-prefixed ROM option values#2168
AceHack merged 4 commits into
mainfrom
codex/b0272-rom-review-nits-followup

Conversation

@AceHack
Copy link
Copy Markdown
Member

@AceHack AceHack commented May 9, 2026

Summary

  • accept hyphen-prefixed values such as --datfile -set.dat and --dir -roms by treating only absent option values as missing
  • return CLI argument parse errors through main() instead of calling process.exit inside parseArgs
  • add a focused regression test for hyphen-prefixed datfile and directory values

Tests

  • bun test tools/roms/canonicalize.test.ts
  • git diff --check

Coordination

Follow-up to the merged #2166 review thread. This intentionally keeps the older claim/b0272-rom-canonical-naming-smallest-slice-2026-05-08 single-quote parser branch out of scope so it can be handled separately or retired deliberately.

Copilot AI review requested due to automatic review settings May 9, 2026 02:39
AceHack and others added 3 commits May 8, 2026 22:40
Remove the startsWith("-") guard from readOptionValue so paths
like --dir -roms are parsed correctly instead of rejected.
Update test to use a truly missing value (trailing --datfile
with no argument).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
parseArgs now throws ArgError instead of calling process.exit,
so main() catches and returns the exit code. Tests no longer
need to monkeypatch process.exit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Codex <noreply@openai.com>
@AceHack AceHack force-pushed the codex/b0272-rom-review-nits-followup branch from 1f2de9d to 903333d Compare May 9, 2026 02:41
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the ROM canonicalizer CLI to correctly accept option values that begin with - (e.g., --datfile -set.dat, --dir -roms) by treating only an actually-absent value as missing, and routes parse errors back through main() so the module is more testable and composable.

Changes:

  • Adjust CLI arg parsing to accept hyphen-prefixed option values and propagate parse failures via main() return codes instead of process.exit inside parsing.
  • Implement bounded-memory SHA1 hashing using a fixed-size read buffer rather than reading whole files at once.
  • Add regression tests covering hyphen-prefixed values and clearer missing-value error reporting.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tools/roms/canonicalize.ts CLI parsing behavior updated; hashing switched to streaming reads; small path handling tweak for rename targets.
tools/roms/canonicalize.test.ts Adds focused CLI regression tests and updates scan expectations for extensionless/non-ROM files.
docs/backlog/P1/B-0272-atari-rom-canonical-naming-tosec-lookup-2026-05-08.md Updates last_updated date for the backlog item.
Comments suppressed due to low confidence (1)

tools/roms/canonicalize.test.ts:22

  • Test fixture MD5 value looks like 31 hex chars (MD5 should be 32). Even though the parser doesn’t validate MD5, keeping fixtures structurally valid avoids confusing future readers/debugging. Consider changing it to a 32-char hex string or omitting the md5 attribute entirely in the fixture.
  <game name="Combat (1977)(Atari)">
    <description>Combat (1977)(Atari)</description>
    <rom name="Combat (1977)(Atari).bin" size="2048" crc="4020b4fe" md5="b35e9442525e1a0b30dc0264c112233" sha1="da39a3ee5e6b4b0d3255bfef95601890afd80709" />
  </game>

Co-Authored-By: Codex <noreply@openai.com>
@AceHack AceHack merged commit 2cb890a into main May 9, 2026
25 checks passed
@AceHack AceHack deleted the codex/b0272-rom-review-nits-followup branch May 9, 2026 02:49
AceHack added a commit that referenced this pull request May 16, 2026
…th pure-drift this session) (#3820)

10th pure-drift close of the resume-session sequence. All 3
acceptance items verifiably shipped via PRs #2166 + #2168 + #2172:

- Script at tools/roms/canonicalize.ts (8953 bytes)
- TOSEC + No-Intro matching via Logiqx XML
- Rename + unmatched-hash reporting

Surfaced by peer Otto's audit-backlog-status-drift.ts (PR #3758);
per-acceptance verification confirmed pure-drift.

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants