fix(disk): Denibblize silently zero-fills a track's remaining sectors on any decode failure, on the flush path

[relmer]

Summary

NibblizationLayer::Denibblize abandons a track on the first sector it cannot
decode, leaves every remaining sector of that track as zeros, and returns
S_OK. It sits on the emulator's flush path for every sector-format image, so a
guest that leaves a track partially written loses the rest of that track on
eject — silently, with success reported all the way up.

This is independent of any spec in flight. Found while surveying the disk
substrate for specs/020-disk-file-access.

Mechanism

CassoEmuCore/Devices/Disk/DiskImage.cpp:429-435 — Serialize routes every
sector format straight through Denibblize:

case DiskFormat::Dsk:
case DiskFormat::Do:
case DiskFormat::Po:
    hr = NibblizationLayer::Denibblize (*this, m_format, outBytes);
    break;

CassoEmuCore/Devices/Disk/NibblizationLayer.cpp:741 — the output buffer starts
zeroed:

out.assign (kImageByteSize, 0);

CassoEmuCore/Devices/Disk/NibblizationLayer.cpp:762-769 — the per-sector loop:

for (sec = 0; sec < kSectorsPerTrack; sec++)
{
    HRESULT hrSector = DecodeOneSector (img, track, bitPos, outSector, data);

    if (FAILED (hrSector))
    {
        break;          // <-- abandons the whole track
    }
    ...
}

Denibblize then returns S_OK regardless.

Two distinct problems compound here:

1. Failure is not reported. A caller cannot distinguish "decoded cleanly"
   from "decoded nothing," because both return S_OK and a full-size buffer.
2. break rather than continue makes the blast radius the whole rest of the
   track. Sectors decoded before the bad one survive; every sector after it
   in scan order is zeroed, whether or not it was individually decodable.
   Because the loop walks the bit stream sequentially from bitPos, "after in
   scan order" is rotational position, not logical sector number — so which
   sectors are lost depends on where the head happened to be.

Reproduction path

Any guest that leaves a track in a state DecodeOneSector rejects:

• power-off or reset mid-sector-write, leaving a torn data field
• a program writing its own nibble format to part of a track
• a write that corrupts an address or data prologue

On the next flush — eject, motor spin-down, machine switch, or exit — the image
written back to disk has that track truncated from the failure point onward. The
user is told nothing.

Why it has not been noticed

Sector-format images are nibblized from valid sectors at load, so they decode by
construction. The failure needs a guest to write something malformed first, and
WOZ images take the WozLoader::Serialize path instead and are unaffected.

Suggested fix

• continue rather than break, so one unreadable sector costs one sector
  rather than the tail of a track. Advancing bitPos past the failed field is
  the fiddly part — resynchronizing on the next address prologue is the natural
  approach, and DecodeOneSector's existing prologue search already does that
  work.
• Report per-track decode results out of Denibblize instead of swallowing
  them, so callers can act. spec 020-disk-file-access needs exactly this
  report (its FR-017) to refuse writing to tracks that do not round-trip, so the
  two efforts should share one mechanism rather than each growing their own.
• Decide what a flush should DO on partial decode. Silently writing a damaged
  image is the current behavior and the worst option. Refusing the flush and
  telling the user preserves the file on disk; DiskImageStore::FlushEntry
  already routes genuine persist failures through the shared EHM notifier
  (CHRN/CBRN → EhmNotifyUser) for precisely this reason, so the plumbing
  exists.

Not in scope

Making Denibblize decode non-standard or copy-protected tracks. The goal is to
stop it destroying data it cannot read, and to say so when it cannot.

[relmer]

Two more silent zero-fill paths, neither going through the break

The report above named one mechanism. There are three. A fix targeting only the
break would ship looking complete while leaving two paths open.

From the same loop, NibblizationLayer.cpp:762-779:

for (sec = 0; sec < kSectorsPerTrack; sec++)
{
    HRESULT hrSector = DecodeOneSector (img, track, bitPos, outSector, data);

    if (FAILED (hrSector)) { break; }                  // (1) the original report

    if (outSector >= kSectorsPerTrack) { continue; }   // (2) line 771-774

    offset = (track * kSectorsPerTrack + interleave[outSector]) * kSectorByteSize;
    memcpy (&out[offset], data, kSectorByteSize);      // (3) no duplicate check
}

(2) Out-of-range sector number. A sector whose address field claims sector
200 is skipped by continue. The loop still runs only 16 times, so the track now
fills at most 15 logical sectors and one stays zero. Nothing failed and nothing
is reported.

(3) Duplicate sector numbers. Two physical sectors both claiming sector 5
both memcpy to the same offset. The second overwrites the first, and whichever
logical slot went unclaimed stays zero. Again, no failure.

Both arise from damaged or deliberately-formatted tracks, and both survive any
check based on "did every DecodeOneSector call succeed" — because every call
did succeed.

Consequence: completeness is a coverage property, not a loop property

"The loop ran sixteen times without failing" does not mean the track is intact.
The sound test is all 16 logical sectors written exactly once, which is a
16-bit coverage mask compared at end of track — cheaper than the decode work
already being done.

That subsumes the break case rather than sitting beside it: a track cut short
simply has incomplete coverage. It also means any future mechanism that loses a
sector is caught by the same check without anyone having anticipated it, which
is the property the current code most conspicuously lacks.

Suggested classification for the per-track report:

Outcome	Test
Blank	no address fields found anywhere on the track
Partial	address fields present, coverage incomplete or duplicated
Complete	all 16 logical sectors written exactly once

The Blank vs Partial discriminator has to be address-field presence, not
sector count — a track whose address fields are intact but whose data fields
all fail decodes zero sectors, yet is damaged rather than blank. Counting
sectors misclassifies precisely that case, and it is the case where a write path
must refuse.

Tests

UnitTest/EmuTests/NibblizationTests.cpp:308
(Denibblize_UnformattedTrack_ZeroFillsThatTrackAndKeepsOthers) already pins the
benign case, and its comment asserts the behavior is "intentional for sector
images (a blank disk is all zeros), not silent corruption of a valid track."
That is true of the wholly-unformatted track it wipes, and false as a general
claim — the comment needs narrowing alongside the fix, or it stands as a
documented argument for reverting it.

Missing siblings, one per mechanism:

• Denibblize_PartiallyDecodableTrack_ReportsDataLossAndDoesNotZeroTail
• Denibblize_OutOfRangeSectorNumber_ReportsIncompleteCoverage
• Denibblize_DuplicateSectorNumbers_ReportsIncompleteCoverage

All three are constructible the way the existing test builds its wiped track:
nibblize a valid image, then patch one address field's 4-and-4 sector value.

[relmer]

Recovery behavior: promote the mount, and follow it everywhere

Design decision for the follow-up work, recorded so it does not live only in
conversation.

What happens

At flush, when a track cannot be represented as standard sectors (Partial with
wholesale loss, or Unformatted where the guest wrote a non-standard format):

1. Do not write the sector-format file. It keeps its last good state.
2. Serialize the same in-memory image as WOZ beside it. No conversion step
   exists — a mounted .dsk is already per-track bit streams, and
   WozLoader::Serialize consumes those buffers verbatim. WOZ is the cheaper
   path; the sector path is the one doing a lossy transform and failing.
3. Promote the mount to the new file, so subsequent flushes succeed
   normally and the session keeps saving.
4. Tell the user once per mount, not once per flush — flush fires on motor
   spin-down, so the condition would otherwise notify dozens of times.

A single torn sector is NOT this case. After the continue+resync fix it costs
one sector, the user disclaimed it by resetting mid-write, and the write should
proceed with the lost sector named in the message. Promotion is for content a
sector format genuinely cannot hold.

The surfaces that must follow the promotion

If the mount moves and these do not, the UI lies about which file is being
written:

Surface	Site
Drive widget label	EmulatorShell.h:597 (mountedImagePath), EmulatorShell.cpp:2207
Write-protect menu item text	EmulatorShell.cpp:6165
Recent-disks MRU	EmulatorShell::RecordRecentDisk (EmulatorShell.cpp:2872, called at 2852)
Per-machine last-inserted disk	DiskSettings.cpp:185,269 (disk1Path / disk2Path)
Drive widget tooltip / write-protect cause	follows GetSourcePath
DiskImageStore::Entry path + format	the promotion itself

The persistence row is the one that turns this into data loss. If the mount
promotes to .woz but disk1Path still names the .dsk, the next launch
remounts the frozen sector image and the session's work appears to have
vanished — while sitting intact in a file next to it. Every other row is a
correctness-of-display issue; that one loses the user's work at the moment they
are most likely to assume it was saved.

CLI is a different case — refusal, not promotion

CassoCli disk operates on image files with no live session to preserve, so
there is nothing to promote and nothing at risk. A target track that cannot be
represented gets a refusal naming the track and the reason, and the file is left
untouched (all-or-nothing already requires this). Do not port the promotion
behavior to the CLI; it would silently redirect a build script's output file.

The sector-format case cannot arise there at all: a .dsk was nibblized from
valid sectors, so it decodes by construction. Only a WOZ source can carry an
unrepresentable track, and writing a WOZ back stays WOZ.

A manual counterpart is worth having

"Save disk as WOZ…" is a reasonable feature independently, and it makes the
automatic message actionable rather than merely informative. It cannot replace
the automatic path, though: the flush that matters most happens at exit, where
there is no opportunity to act on a suggestion.

Edges

• The WOZ write itself can fail (disk full, no permission). That is the one
  case where the session really is lost, and the message must be unmistakable
  rather than informational.
• Threading is already handled. FlushEntry runs on the CPU thread via
  Eject / PowerCycle / SoftReset, same as the existing flush write.
• Naming should overwrite one file, not accumulate timestamped copies. Each
  flush's state supersedes the last.

[relmer]

Correction to the CLI note above

The previous comment said promotion must not be ported to the CLI because it
"would silently redirect a build script's output file." That reads as though the
fix is to add a message. It is not, and the real argument is stronger.

Announcing it does not help, because nothing downstream reads the message.

casso disk put game.dsk PROG BIN prog.bin
Casso --disk1 game.dsk

If the first command promotes to game.woz and says so on stderr, the second
still mounts game.dsk — the stale one. The script boots yesterday's build
while the tool has technically told someone. In a script the contract is the
path and the exit code, not the prose: a non-zero exit stops the pipeline at
the point of failure, while promotion plus a message lets it continue with the
wrong file.

That is the asymmetry with the GUI. There, the downstream consumer is the
human reading the notification, so telling them is sufficient. In the CLI the
downstream consumer is the next command, and it cannot read.

The general principle stands and is worth stating separately: any write to a
path other than the one specified must be announced. It is simply not
sufficient on its own to make promotion safe for scripts.

Adjacent, and worth doing regardless

The CLI should report the output path it actually wrote on success, not only
when something surprising happens. DoAs65 currently prints it under -v only.
The tool already writes to paths the user did not type — extension inference
turns demo.a65 into demo.bin, and -s into demo.s19 — so the written path
is worth stating unconditionally. Cheap, and it makes the surprising case
legible against a baseline instead of being the only time a path appears.

[relmer]

A fourth mechanism, found during implementation — and why coverage caught it anyway

The issue enumerated three ways a sector goes silently missing: the break, the
out-of-range continue, and duplicate sector numbers. There is a fourth, found
while implementing the fix:

The decoder never validated the address field's checksum at all.

The DOS 3.3 address field carries a 4-and-4 encoded volume / track / sector plus
a checksum of volume ^ track ^ sector. Nothing verified it. So a corrupted
sector number in a header was accepted at face value, and the payload that
followed was filed under whatever slot the corrupted number named — silent
misplacement, not a reported failure. Sector A's data lands in slot B, and slot
A is never written at all.

That is worse than loss, because the buffer then contains real-looking data in
the wrong place rather than obvious zeros.

The point worth drawing out

Nobody anticipated this mechanism. It was not in the original report, not in the
follow-up comment enumerating the other two, and not in the plan.

Coverage catches it anyway. A misfiled sector shows up as one slot set twice
and another never set, which fails the "all 16 written exactly once" test
without anyone having reasoned about checksums. That is precisely the property
argued for when choosing coverage over a per-mechanism check:

any future mechanism that loses a sector is caught by the same check without
anyone having anticipated it

This is that claim being tested within a day, by a mechanism that already
existed in the code and had gone unnoticed through the entire analysis.

Validating the checksum is still the right fix — it identifies the cause
rather than only the symptom, and it lets the report say "damaged header" rather
than "coverage incomplete." But the safety net held before the specific fix
existed, which is the more useful fact about the design.

Also worth recording: a test that did nothing on half the disk

The first attempt at a damage model flipped one bit of the 4-and-4 encoded
checksum. That is a no-op for half the sectors on a track, because 4-and-4
encoding forces the odd bits to 1 — flipping a bit that is already set to the
value you are XOR-ing toward changes nothing. It silently did nothing on track 7
and worked on track 4.

A corruption test that fails to corrupt is the same class as everything else in
this issue: it reports success while doing nothing. Writing an explicitly wrong
checksum, rather than perturbing a correct one, is the reliable damage model.

[relmer]

Shipped on 020-disk-file-access — commit references, and what closing this needs

The fix and its recovery path are on the feature branch, not yet merged to
master. Recorded here rather than in a second issue.

The whole of it landed in one commit, which is why there is no separate
Refs/Closes pair: the decoder change and the recovery path are the same
change, and shipping the refusal without a way out would have traded silent
corruption for a dead end.

• c37d5e1c — fix(disk): report what each track yielded instead of zeroing
  what it could not read, carrying Refs #115. It covers all four mechanisms:
  the abandoned scan, an out-of-range sector number, duplicate sector numbers,
  and the unverified address checksum found during implementation. A track's
  outcome is decided by a sixteen-bit coverage mask — complete only when every
  slot was filled exactly once — so one rule catches all four rather than four
  patches catching one each. Blank and damaged stay apart on whether any
  address field was found, because zeros are correct for a track that never
  held anything. The three-argument Denibblize keeps its signature, forwards
  to the reporting form, and now fails on data loss, so no caller can pick
  the truncating overload by accident. A refused flush writes the session's
  writes beside the original as WOZ — which preserves the very track that
  caused the refusal — names that path in the notification, and never
  overwrites an earlier rescue.

• 2a71cbaf — feat(disk): prove a track is standard before rewriting it, and
  rewrite only what changed. The write half of the same guarantee:
  TrackWritability refuses the whole operation before anything is committed,
  rather than skipping the bad track and producing the half-written image the
  refusal exists to prevent.

Whether this closes is yours to decide, not mine. The defect is fixed, the
refusal has a recovery path, and both are covered by tests — the three damage
paths beside the existing unformatted-track case, plus store-level cases for
the rescue file. But nothing has reached master yet, and the branch still has
one manual check outstanding (an interrupted commit, which cannot be
unit-tested). Closing on merge seems right; closing now would be closing on a
branch.