Skip to content

Core test suite + FAT12/BASIC/DMK/ROM/CLI correctness fixes - #2

Merged
VC-Anvil merged 8 commits into
masterfrom
claude/core-tests-hardening
Nov 4, 2026
Merged

Core test suite + FAT12/BASIC/DMK/ROM/CLI correctness fixes#2
VC-Anvil merged 8 commits into
masterfrom
claude/core-tests-hardening

Conversation

@VC-Anvil

@VC-Anvil VC-Anvil commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Summary

Adds the project's first automated tests and fixes the bugs they (and an independent review) turned up. No file-format changes; the GUI gains one button.

Tests & build

  • tests/: self-contained harness (no external framework), 67 core tests + cli_smoke.cmake, an end-to-end run of the msxforge binary. Run with ctest --test-dir build --output-on-failure.
  • CMake options MSXFF_BUILD_GUI / MSXFF_BUILD_TESTS: core + CLI + tests build without SDL2/OpenGL.
  • CI runs ctest after each build (skipped for the cross-compiled macOS x86_64 binary, which cannot run on the arm64 runner).

FAT12

  • Writes check free space first: a disk-full write/create no longer leaks clusters, truncates the file or leaves a 0-byte phantom entry; failed writes release what they allocated.
  • Entries in a subdirectory's 2nd+ cluster had the wrong offset whenever the clusters were not adjacent, so put/del/rename wrote into other files' data. Directories are now parsed cluster by cluster.
  • 8.3 names are validated (long names were silently truncated into duplicates); rename checks collisions; ./.. cannot be deleted/renamed.
  • Subdirectories grow past one cluster; new removeDir() and lookup(path); new/modified entries get a date/time.
  • Mount validates the BPB, falls back to the FAT media byte, and caps geometry at the image size.
  • fsck/cluster map terminate on directory loops and FAT chain loops; fsck reports size vs chain-length mismatches.

MSX-BASIC tokenizer

  • &B… hung the tokenizer; digits in variable names (A1) were crunched to constants (a syntax error on MSX); DATA is stored verbatim; ELSE / ' are stored as :ELSE / :REM' like the ROM does; names are upper-cased; !/E single vs #/D/reals double; exponent only when digits follow; RETURN/RESUME line numbers; lines sorted/replaced/deleted.
  • BCD is converted through decimal digits (exact, half-up rounding); detokenize → tokenize reproduces the bytes.

DMK

  • ID/data CRCs are actually verified now (the flags were never set); duplicate sector IDs are flagged.
  • Sectors per track come from the BPB / most common count: one protected track with an extra header no longer shifts every LBA.
  • Writing a sector recomputes its data CRC (deliberate CRC errors are preserved); sectors running past the track end are read-only.
  • dskToDmk() in core emits a proper IBM MFM layout with valid CRCs; tools/dsk2dmk wraps it.

ROM / tape

  • ASCII16 vs ASCII8 ties go to ASCII16 (0x6000/0x7000 games were reported as ASCII8); 0x77FF counts for ASCII16; plain 48/64K ROMs are "None", not "Unknown".
  • tsxToCas handles the remaining TZX block IDs and reports truncation.

CLI / GUI

  • File arguments accept DIR/NAME paths (only root files were reachable); del removes empty dirs; diff walks the whole tree (loop-safe) and exits 1 on differences; full usage text.
  • GUI: "Remove this folder" button for empty subdirectories.

Testing

  • ctest green locally (Linux, GCC 13, also under ASan/UBSan); Windows build cross-compiled with MinGW without warnings. GUI builds; the new button was not exercised interactively.
  • Every review fix has a regression test that fails on the previous code.
  • Not tested against a library of real commercial (copy-protected) dumps — DMK protection cases use constructed images.

Generated by Claude Code

claude added 8 commits November 4, 2026 13:11
@claude
fat12: harden writes and add a core test suite
821170d 
- Add tests/ (self-contained harness, `ctest`) and CMake options
  MSXFF_BUILD_GUI / MSXFF_BUILD_TESTS so core+CLI build without SDL2/GL.
- DiskImage::loadBytes() loads an image from memory.
- Mount: validate the BPB; fall back to the media byte in the FAT for
  boot sectors without a usable BPB; clamp clusters to FAT capacity.
- Writes check free space first: a disk-full write or create no longer
  leaks clusters, truncates the file or leaves a 0-byte phantom entry.
- Validate 8.3 names (too-long names were silently truncated, creating
  duplicates); rename checks for collisions.
- Subdirectories grow past one cluster; new removeDir() and lookup().
- New/modified entries get a date and time (fixed stamp for tests).
- fsck and the cluster map no longer recurse forever on directory
  loops; fsck reports size/chain-length mismatches and bad markers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
basic: crunch programs the way the MSX-BASIC ROM does
08192a6 
Tokenizer fixes (each covered by tests/test_basic.cpp):
- "&B1010" (and any '&' not followed by H/O/octal) hung forever.
- Digits inside variable names (A1, X12$) were crunched to numeric
  constants, which MSX-BASIC rejects as a syntax error; they stay ASCII.
- DATA items are stored verbatim up to ':' (they were crunched).
- ELSE is stored as ':' ELSE and ' as ':' REM ''', as on a real MSX;
  the detokenizer hides the colon again.
- Names are upper-cased outside strings, REM and DATA.
- '!' / 'E' give single precision (0x1D), '#' / 'D' / reals double
  (0x1F); an E/D only starts an exponent when digits follow ("1ELSE").
- RETURN and RESUME take line numbers; LIST/DELETE ranges keep both.
- Lines are sorted; a repeated number replaces, a bare number deletes.
- BCD is built from the decimal digits with half-up rounding instead of
  binary floating point, so constants are exact.

The detokenizer formats reals from the BCD digits (".5", "1.5!", "1#",
"1D+20", "2.5E-10"), so detokenize -> tokenize reproduces the bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
dmk: verify CRCs, flag duplicate IDs, keep LBAs stable on odd tracks
78d39be 
- ID and data CRCs (CRC-16/CCITT, MFM 0xA1 preset) are now actually
  checked; before, idCrcError/dataCrcError were never set although the
  inspector and README advertise them.
- Duplicate sector IDs on a track are flagged (and left unmapped).
- Sectors per track come from the BPB, else the most common count of
  standard sectors. One protected track with an extra sector header used
  to raise spt for the whole disk and shift every LBA, so the filesystem
  of such images was unreadable.
- Sectors with a data CRC error still map (an FDC returns their bytes).
- Writing a logical sector into a DMK recomputes its data CRC, so edited
  DMKs no longer reload as "protected"; deliberate CRC errors are kept.
- dskToDmk() moves into core and emits a proper IBM MFM layout (gaps,
  syncs, index mark, valid CRCs); tools/dsk2dmk is a thin wrapper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
rom, tape: fix ASCII16/ASCII8 tie, plain 48/64K ROMs, more TSX blocks
9ed3300 
- A game that only writes 0x6000/0x7000 tallies ASCII8 and ASCII16
  equally and was reported as ASCII8; ties now go to ASCII16, and
  writes to 0x77FF count for ASCII16.
- 48K/64K ROMs with no bank writes are plain, not "Unknown".
- tsxToCas skips the remaining TZX block types (0x15, 0x18, 0x19, 0x23,
  0x26-0x28, 0x2B) and reports a truncated block instead of reading
  past the end.
- Tests: ROM header/mapper detection on synthetic images; .cas parsing;
  CAS<->WAV (1200/2400 baud) and CAS<->TSX round trips; error paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
cli: paths into subdirectories, delete empty dirs, tree-wide diff
92ebf81 
- cat/detok/extract/put/del/rename/crc accept DIR/NAME paths; before,
  only root-directory files could be reached from the CLI.
- del removes an empty directory; protect shows duplicate-ID sectors.
- diff walks the whole tree and exits 1 when the images differ.
- save errors are reported instead of exiting silently.
- Usage lists every command.
- tests/cli_smoke.cmake drives the binary end to end (format, tok, add,
  mkdir, ls, cat, extract, rename, del, fsck, diff, dsk2dmk, cas, tsx)
  as a ctest that runs on every platform.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
ci, docs: run the test suites in CI, document headless build and tests
cca95e4 
- CI runs ctest after every build (the cross-compiled macOS x86_64
  binary cannot run on the arm64 runner, so it is skipped there).
- README: -DMSXFF_BUILD_GUI=OFF, the test suites, path arguments, and
  limitations updated (CRC checks and file creation now exist).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
gui: "Remove this folder" button for empty subdirectories
5403bd6 
Uses the new Fat12::removeDir(); shown while browsing a subdirectory,
goes back up to the parent on success.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@claude
fat12, dmk, cli: fix issues found in review
941c002 
- Entries in a subdirectory's 2nd+ cluster got the offset of the first
  cluster plus their index, i.e. the wrong place on disk whenever the
  clusters are not adjacent (normal once a directory grows). put, del,
  rename and rmdir then wrote into other files' data. Directories are
  now parsed cluster by cluster with each cluster's own offset.
- lookup() no longer resolves "." / ".."; removeDir, renameEntry and
  deleteFile refuse dot entries (del SUB/. freed SUB under its parent).
- A failed writeFile releases the clusters it allocated; mount caps
  the sector count at the image size (media-byte guesses on short
  dumps addressed sectors past the end).
- chainOf() stops at a cluster it already visited, so writing a file
  with a looping FAT chain repairs it instead of freeing it.
- createFile/makeDir count the extra cluster only when a full
  subdirectory must grow (makeDir refused with free slots available;
  a failed create still grew the directory).
- DMK sectors whose data runs past the track end stay readable but are
  not writable (a write clobbered the next track's IDAM table).
- cli diff walks each directory once (a self-referencing directory made
  it run for ever) and no longer drops files below depth 16.

Each fix has a regression test that fails without it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CtBtjm9PRm6sDhDVJbtciC
@VC-Anvil
VC-Anvil merged commit aca61ef into master Nov 4, 2026
4 checks passed
@VC-Anvil VC-Anvil mentioned this pull request Nov 4, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VC-Anvil @claude