ISO 9660: non-zero-origin coverage completes ADR-0005 per-backend - #51
Merged
Conversation
Finishes the ADR-0005 per-backend origin coverage. ISO 9660 was the one filesystem backend without a synthetic non-zero-origin test, and it is the exact hybrid-disc case the ADR names. The test prepends a 150-sector zeroed pregap to a make_iso9660 image, resolves with find_origin, and asserts the origin, the volume/listing, and -- the part that matters -- that read_file returns the payload byte for byte from the resolved origin. A listing alone is not enough: an extent-vs-origin bug can list every file and still read pregap zeros, which is how the EMU3 bug in #49 hid behind a whole-looking listing. The ISO 9660 path was already offset-clean: probe, the descriptor scan, the directory walk and read_file all read origin + extent * SECTOR_SIZE, so no fix was needed -- the test proves it. Confirmed the test discriminates by temporarily dropping the origin in read_file (listing passed, read-back failed), then reverting. docs/formats/iso9660.md now records the LBAs-relative-to-origin assumption and flags the untreated case: a real hybrid disc whose ISO LBAs count from the physical disc start rather than the track would double-count and need its own treatment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this finishes
ADR-0005 — "probe for the filesystem origin; never assume byte 0." The failure it exists to prevent is silent: a parser assuming byte 0 reads a zeroed pregap, finds no volume, and reports an empty disc, not an error.
#49 added synthetic non-zero-origin tests for EMU3 and Roland (and, in doing so, uncovered and fixed a real EMU3 bug where scans returned file-relative addresses, so at a non-zero origin every bank listed and read back empty). ISO 9660 was the one filesystem backend left without such a test — and it is the exact hybrid-disc case the ADR names: an ISO track ahead of the sampler partition, or a pregap left inside the cooked stream.
What the test now proves
test_origin_resolves_when_the_pregap_is_inside_the_cooked_streamprepends a 150-sector zeroed pregap to a syntheticmake_iso9660image, resolves withfind_origin, and asserts:origin.offset == 150 * 2048andbackend.name == "iso9660"(not an assumed byte 0);read_filereturns the payload byte for byte from the resolved origin.The read-back is the assertion that matters. A listing alone passes even when the reads are wrong — an extent-vs-origin bug can list every file and still return pregap zeros, which is exactly how the EMU3 bug hid behind a whole-looking listing. This is the discriminating property the pre-#49 tests lacked.
Was the ISO 9660 path offset-clean? Yes — no fix needed
Every read the backend does is
origin + extent * SECTOR_SIZE:probe, the descriptor scan, the directory walk, andread_fileall add the record's LBA to the resolved origin rather than reading it absolutely. So unlike EMU3, this backend needed no code change — the test proves the existing behaviour.Confirmed the test genuinely discriminates: temporarily dropped the origin in
read_file(image.read(entry.start_block * SECTOR_SIZE, ...)), and the listing still passed while the read-back failed on pregap zeros; reverted, and it passes. Source is unchanged in this PR.Docs
docs/formats/iso9660.mdnow records the LBAs-relative-to-origin assumption and flags the one untreated case as a watch-for: a real hybrid disc whose ISO LBAs count from the physical disc start rather than the track would double-count underorigin +and needs its own treatment and fixture. None is known in the collection, so it is a caveat, not a case handled today.Scope
Test + docs only (source untouched). Synthetic fixtures only (ADR-0008). Branched off
mainand touches only ISO 9660 files, so there is no overlap with the open #49.Verify
uv run ruff check .— cleanuv run ruff format --check .— cleanuv run pytest -q— 340 passed, 112 skippeduv tool install --editable . && samplerdisc --version—0.3.0🤖 Generated with Claude Code