fix(cli): make dungeon writes transactional

[scawful]

Summary

• add an explicit Rom::SaveSettings::require_backup mode while preserving legacy best-effort backup=true
• back up the actual existing save target (including Save As destinations), not the originally loaded source
• create required backups through a unique same-directory temporary file, best-effort durability sync, and rename only after the copy completes
• wrap dungeon sprite/object/collision serialization plus backup and disk commit in ScopedRomTransaction
• restore caller ROM bytes, size, filename, dirty state, and stream pointers on serializer, backup, or target-write failure
• replace the Windows remove-then-rename fallback with MoveFileExW(..., MOVEFILE_REPLACE_EXISTING) so a failed replacement does not first delete the target

Root cause

The dungeon edit handlers mutated their reusable in-memory Rom before calling the atomic disk save. A disk failure preserved the target file but left the caller's bytes and pointers mutated. Separately, backup=true logged and continued when backup creation failed, even for safety-critical writes.

Commands covered

• dungeon-place-sprite
• dungeon-remove-sprite
• dungeon-place-object
• dungeon-set-collision-tile

Other CLI callers that use only backup=true intentionally remain best-effort pending their own transaction audit.

Verification

• cmake --build --preset mac-ai --target yaze_test_unit --parallel 8
• focused ROM and dungeon edit suites: 44/44 passed
• object COW mutation followed by target temp-write failure restores memory, filename, dirty state, pointer, and disk bytes
• deterministic required-backup failure restores a second handler and leaves no temp/backup artifact
• Save As regression proves an existing destination's original bytes, rather than the loaded source, are stored in the required backup
• existing successful object COW write still creates one backup and reopens correctly
• YAZE_PREPUSH_BUILD_DIR=build/presets/mac-ai scripts/pre-push.sh
• pre-commit formatting and release/version checks passed

Test note: the broader local filter also selected the pre-existing environment-sensitive RomTest.LoadFromFile; it found the available 1 MiB local vanilla fixture while that test expects 2 MiB. The task-focused filter excludes only that unrelated fixture check.

Fixes #123

[claude]

Review: fix(cli): make dungeon writes transactional

Overview

This PR closes a real gap: the four dungeon-edit CLI commands (dungeon-place-sprite, dungeon-remove-sprite, dungeon-place-object, dungeon-set-collision-tile) mutated the shared in-memory Rom before the disk save, so a disk-level failure left the caller's Rom (bytes, filename, dirty flag) permanently diverged from the on-disk file. The fix:

• Adds Rom::SaveSettings::require_backup, a strict mode that fails before touching the save target if a full backup copy can't be made, alongside the existing best-effort backup.
• Implements the required backup via same-directory temp file + copy_file + rename, mirroring the existing temp-write-then-rename pattern already used for the ROM target itself.
• Wraps each handler's serializer + backup + disk commit in the existing ScopedRomTransaction (whole-buffer snapshot/restore), only committing after a fully successful disk write.
• Adds targeted regression tests for rollback on required-backup failure and on final-write failure, using a clever NAME_MAX-based fixture to deterministically force backup failure without mocking the filesystem.

This directly follows the "ROM writes as atomic disk operations" and ScopedRomTransaction conventions already documented in docs/internal/agents/rom-safety-guardrails.md, and the doc is updated in-place to describe the new contract — good practice for keeping that guide authoritative.

Correctness

• The commit/rollback ordering in MutateAndSaveRomWithBackup (src/cli/handlers/game/dungeon_edit_commands.cc:97-112) is sound: transaction.Commit() only runs after SaveRomWithBackup returns OK, so any failure — serializer, backup, or final rename — leaves the destructor to restore bytes/filename/dirty state. Good use of RAII here.
• One asymmetry worth flagging: because the required backup is created before the target temp-write/rename in Rom::SaveToFile (src/rom/rom.cc:388-395), a scenario where the backup succeeds but the subsequent target write fails leaves a real backup file on disk even though nothing changed. This is exercised and accepted by PlaceObjectDiskSaveFailureRollsBackCowPlanAndCallerRom (asserts CountBackupArtifacts == 1), so it's evidently intentional, but it means "required backup" can leave an orphaned artifact on a failed save — worth a one-line note in the guardrails doc so future readers don't mistake it for a bug.
• CreateRequiredBackup (src/rom/rom.cc:112-135) copies via copy_file + rename but, unlike the main ROM save path, does not call the equivalent of BestEffortFsyncFile/BestEffortFsyncParentDir before/after the rename. The primary save path treats fsync as important enough to do explicitly; the backup path (which the new require_backup mode is specifically supposed to make trustworthy) skips it. On a crash/power-loss right after the rename, the backup could theoretically be an unsynced/truncated file passed off as durable. Given the PR's stated goal is a guaranteed backup, consider reusing the same fsync helpers here for consistency.
• GetAvailableBackupPath (src/rom/rom.cc:97-111) has a check-then-act race between std::filesystem::exists and the later copy_file/rename (TOCTOU). For a single-user CLI tool this is low risk, but it's not literally guaranteed-unique the way the naming suggests.

Style / conventions

• The diff mixes the feature change with unrelated drive-by reformatting in rom.h (e.g. ReadTile16/WriteTile16 signature wrapping, ReadHelper one-liners split to multi-line, trailing-whitespace trims). None of it is wrong, but it inflates the diff and makes the actual behavioral change harder to review in isolation — consider splitting pure formatting into its own commit next time per repo convention of minimal, task-scoped edits.
• MutateAndSaveRomWithBackup's signature indentation looks like it was hand-aligned rather than clang-formatted (resources::OutputFormatter& formatter, under a name that no longer matches column width) — worth running the formatter pass before merge.
• The collision-tile lambda [&rom, &room]() mutable { ... std::move(room) ... } marks itself mutable despite capturing only by reference — harmless but unnecessary; mutable only matters for by-value captures.

Test coverage

Solid additions:

• RomTest.BestEffortBackupFailureStillSaves / RomTest.RequiredBackupFailureRollsBackTransactionAndLeavesNoArtifacts cover both backup modes at the Rom::SaveToFile level.
• DungeonEditCommandsTest.PlaceSpriteRequiredBackupFailureRollsBackCallerRom and PlaceObjectDiskSaveFailureRollsBackCowPlanAndCallerRom cover the handler-level rollback path end-to-end, including verifying disk bytes, in-memory bytes, dirty flag, filename, and absence of stray .tmp/backup artifacts.
• Good handling of platform differences (_WIN32 guard + GTEST_SKIP for the NAME_MAX fixture rather than silently failing on Windows/exotic filesystems).
• Gap: no test exercises GetAvailableBackupPath's suffix/collision logic directly (e.g., two required-backup saves against the same target within the same timestamp second) — low priority since it's a small pure function, but it's the one piece of new logic without direct coverage.

Security

No new attack surface — filenames come from already-trusted CLI/ROM state, and all filesystem operations use std::filesystem (no shell interpolation). The main hardening question is the fsync gap noted above, which is a durability concern rather than a security one.

Summary

Well-scoped fix with good test discipline for a real correctness bug (memory/disk divergence on partial failure). Main suggestions: reuse the fsync helpers in CreateRequiredBackup for durability parity with the primary save path, note the "backup can survive a failed save" behavior explicitly in the guardrails doc, and keep unrelated formatting changes out of behavioral PRs going forward.

[scawful]

Addressed the durability/documentation points in 251208e71: required backups now receive the same best-effort file/parent sync treatment as the primary save path, and the guardrail explicitly says a completed backup is retained after a later target-write failure. The follow-up also fixes strict Save As semantics by backing up the actual existing destination (with a source-A/target-B content regression) and replaces the Windows remove-then-rename fallback with MoveFileExW replacement. Focused coverage is now 44/44.

[claude]

Review: fix(cli): make dungeon writes transactional

Solid change — the ScopedRomTransaction + require_backup combo directly closes the gap described in the root cause (in-memory Rom staying mutated after a disk failure). The new tests (temp-write failure, deterministic required-backup failure, NAME_MAX-forced backup failure) exercise the actual failure paths rather than just the happy path, which is the right way to validate rollback logic.

Correctness

• ScopedRomTransaction (pre-existing, src/rom/transaction.h) snapshots vector(), filename(), dirty() before the mutation and restores them on scope-exit unless Commit() is called. Wiring MutateAndSaveRomWithBackup around mutation → required backup → disk write → Commit() means any failure in that chain now rolls back correctly, including partial in-memory mutation from a failing serializer. Good.
• CreateRequiredBackup's copy-to-temp + rename pattern (using copy_options::none, so a same-named leftover file causes an explicit failure rather than a silent overwrite) is a nice improvement over the legacy try/catch around std::filesystem::copy.
• The Windows rename fallback now uses MoveFileExW(..., MOVEFILE_REPLACE_EXISTING) instead of remove-then-rename, avoiding the window where a failed rename could leave the target file missing. Can't verify on this (Linux) box, but the logic reads correctly.

Potential issue — legacy backup=true path still backs up the wrong file

In Rom::SaveToFile, the else if (settings.backup) branch (still used by every CLI command other than the four dungeon handlers touched here) copies from filename_ — the ROM's originally loaded path — to a backup name derived from filename, the actual save target:

```cpp
} else if (settings.backup) {
std::string backup_filename =
absl::StrCat(filename, "backup", MakeSafeTimestamp(now_c));
std::filesystem::copy(filename_, backup_filename, ...); // wrong source
```

If settings.filename (a Save-As target) differs from filename_ and the target already has content on disk, this silently backs up the source ROM instead of the file about to be overwritten — so the real target's bytes can be lost with no usable backup. This looks pre-existing (confirmed against the base commit) rather than introduced here, and the PR body explicitly scopes the fix to the four dungeon commands ("other CLI writers ... remain best-effort pending their own transaction audit"), so not blocking on it. But the new test RomTest.BestEffortBackupFailureStillSaves formalizes this exact behavior as passing (backup silently uses the wrong source and the save proceeds anyway) — might be worth a tracking issue so this doesn't get lost, since it's a real data-loss footgun for any other command doing a Save-As over an existing file.

Minor nits

• dungeon_edit_commands.cc (collision-tile handler) — the lambda is declared mutable ([&rom, &room]() mutable { ... std::move(room) ... }), but room is captured by reference, so mutable has no effect here (it only matters for by-value captures). Harmless, but slightly misleading about capture semantics — could drop it.
• GetAvailableBackupPath/CreateRequiredBackup: the .tmp companion path isn't existence-checked before copy_file(..., copy_options::none, ...) — only the final backup path is probed for a free slot. A leftover <backup>.tmp from a prior crashed run at the exact same second-resolution timestamp+suffix would cause a spurious failure. Extremely unlikely in practice, not worth blocking on.

Test coverage

Good coverage of the new behavior: required-backup failure rollback, disk-write failure rollback with backup retained, and existing-success-still-backs-up-once. The Windows-specific MoveFileExW fallback and the legacy-backup-wrong-source behavior aren't covered by new/updated tests, but that's consistent with the PR's stated scope.

Security

No injection or traversal concerns — all paths are derived from already-trusted CLI-supplied filenames, consistent with the rest of the codebase. The switch from exception-based to std::error_code-based filesystem calls in the new required-backup path is a good defensive improvement (keeps exceptions from crossing into the absl::Status-based error handling).

Nothing here blocks merging; the legacy-backup-source nit is worth a follow-up issue rather than a change to this PR.

[scawful]

Tracked the pre-existing legacy backup=true Save As source/destination mismatch separately as #125. It remains outside this PR; #124’s new require_backup path already protects the actual destination and is covered by the A→B regression.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.