lanspread/REVIEW_STEP_1.md

Review Step 1 — Faithfulness to PLAN.md

Scope: latest three commits (6c8a2bb, c5dfbf9, fce34c7) against PLAN.md.

Verdict

The implementation is substantially faithful to PLAN.md. The major design pillars — version.ini as the download sentinel, local/ as the install predicate, the per-game intent log, transactional install/update/uninstall, version.ini-only download transaction, operation table gating, catalog filtering, notify-based watcher with per-ID gating, and the move of install logic from Tauri into lanspread-peer::install — are all present and structurally match the plan.

There are a handful of small deviations and a few items that don't quite line up with the wording in PLAN.md. None are correctness regressions; some are worth following up.

What lines up cleanly

State model

• local_games::version_ini_is_regular_file (crates/lanspread-peer/src/local_games.rs:40) and local_games::local_dir_is_directory (local_games.rs:32) implement the plan's two independent predicates. is_committed_install from the plan isn't named explicitly, but local_dir_is_directory enforces the "is-a-directory, not a path that exists" rule.
• Availability::Ready and Availability::LocalOnly are emitted from build_game_summary (local_games.rs:328-368) exactly as the plan specifies. installed && !downloaded correctly maps to LocalOnly.
• is_version_ini is tightened to a strict <game_id>/version.ini match in crates/lanspread-db/src/db.rs:181-184, with a test (db.rs:240) that rejects nested aoe2/local/version.ini.

Intent log

• .lanspread.json is written atomically through .lanspread.json.tmp + rename + parent-dir fsync on Unix (crates/lanspread-peer/src/install/intent.rs:86-100).
• Schema mismatch / corruption / wrong ID all fall back to None (intent.rs:68-83), which matches the plan's "treat missing/unparseable as None" rule.

Transaction primitives

• install::install, install::update, install::uninstall, install::recover_on_startup in crates/lanspread-peer/src/install/transaction.rs follow the plan's step ordering (intent → FS work → commit rename → intent None → cleanup).
• prepare_owned_empty_dir / prepare_backup_slot refuse to touch markerless reserved dirs, matching the ownership-marker rule for the None intent case (transaction.rs:354-375).
• Recovery dispatches by recorded intent (transaction.rs:148-156) and matches every named row of the install matrix; unlisted combinations fall through to _ => {}, which is exactly the plan's "write intent matching FS observation; do not mutate FS" default.

Download transaction

• begin_version_ini_transaction parks an existing version.ini as .version.ini.discarded and clears stale scratch files (crates/lanspread-peer/src/download.rs:193-206).
• commit_version_ini_buffer writes .version.ini.tmp, fsyncs, renames to version.ini, fsyncs the parent on Unix, and sweeps .version.ini.discarded (download.rs:223-240).
• prepare_game_storage skips is_version_ini() entries (download.rs:148-191).
• Rollback never restores the previous sentinel (download.rs:208-221), matching the plan's explicit "downloads are destructive to prior downloaded state" rule.
• Recovery for download transients lives in transaction::recover_download_transients and is invoked both per-root in recover_game_root and once unconditionally over the game_dir top level (transaction.rs:118-156).

Catalog injection and gating

• start_peer receives Arc<RwLock<HashSet<String>>> (crates/lanspread-peer/src/lib.rs:189-215); the Tauri side populates it from the bundled game.db in ensure_bundled_game_db_loaded (crates/lanspread-tauri-deno-ts/src-tauri/src/lib.rs:711-721).
• All four enforcement points the plan calls out are present:
  • Scanner: update_index_for_game skips non-catalog IDs (local_games.rs:394-449).
  • local_download_available checks catalog membership (local_games.rs:48-66).
  • File-stream handlers in services/stream.rs:293-362 route through can_serve_game and also reject any path that resolves under local/.
  • Install/Update/Uninstall command handlers reject non-catalog IDs (handlers.rs:342-345, 411-414).

Operation table

• A single active_operations: HashMap<String, OperationKind> carries all four states (crates/lanspread-peer/src/context.rs:17-44).
• update_and_announce_games preserves the previous summary for IDs under an active operation (handlers.rs:521-538), and handle_list_games filters them out (services/stream.rs:155-163).
• Watcher events for IDs with an active operation are dropped (services/local_monitor.rs:229-236).

Aggregation (peer_db)

• get_all_games, peers_with_game, peers_with_latest_version, get_latest_version_for_game, and even majority_game_size filter on availability == Ready via game_is_ready (peer_db.rs:268, 322, 380, 394, 420, 450, 726). The plan's three explicit aggregation rules all hold.

Scanner + watcher

• notify 7.x is added to Cargo.toml and consumed in services/local_monitor.rs.
• Non-recursive watches on game_dir and each game root, plus a reconcile pass on every tick (local_monitor.rs:107-173).
• Per-ID gating uses a RescanGate { running, pending } structure with no time-based debounce (local_monitor.rs:32-36, 238-287).
• Fallback scan runs every LOCAL_GAME_FALLBACK_SCAN_SECS; verified 300 in config.rs would be the expected value (not re-read here but the README and ARCHITECTURE both call out 300s).
• should_ignore_game_child covers .local.*, .version.ini.*, .lanspread, .lanspread.json, .sync, .softlan_game_installed (local_monitor.rs:322-330).
• Watcher init failure is non-fatal — build_watch_state returns None and logs a warning (local_monitor.rs:71-105); the fallback scan continues to run.

Tauri / UI

• unpack_game / do_unrar standalone command is removed; do_unrar survives only as the body of SidecarUnpacker::unpack (src-tauri/src/lib.rs:58-66, 643-684).
• The ___TO_BE_DELETE___{id} whole-folder backup scheme is gone.
• update_game_installation_state (src-tauri/src/lib.rs:348-389) no longer requires downloaded to flag a game as installed.
• New events game-install-begin, game-install-failed, game-uninstall-begin, game-uninstall-finished, game-uninstall-failed are wired both Rust-side (src-tauri/src/lib.rs:860-953) and TS-side (src/App.tsx:390-457).
• InstallStatus.Installing and InstallStatus.Uninstalling added (src/App.tsx:22-29); LocalOnly badge rendered for installed-but-not-downloaded (src/App.tsx:742-744); secondary uninstall button is present and only shown when installed and idle (src/App.tsx:766-778).
• install_game Tauri command routes by local state (src-tauri/src/lib.rs:88-128): missing version.ini → GetGame; downloaded but no local/ → InstallGame; already installed → no-op. Matches the plan.
• update_game(id) is a thin wrapper around PeerCommand::GetGame (src-tauri/src/lib.rs:131-157).
• New uninstall_game Tauri command forwards to PeerCommand::UninstallGame (src-tauri/src/lib.rs:159-187).

Where it deviates from PLAN.md

1. Download partition is structural, not enforced

Plan: "Partition game_file_descs into version.ini and non-version.ini."

Implementation (download.rs:110-137): partition_download_descriptions keeps version.ini inside transfer_descs and just additionally extracts a version_desc. The version.ini path then rides through build_peer_plans and download_chunk like any other file, where the runtime check version_buffer.matches(&chunk.relative_path) routes its chunks to memory instead of disk. Functionally equivalent, but the implementation does not actually partition; the variable name transfer_descs is misleading. prepare_game_storage is what actually keeps the sentinel off disk.

This is fine, but a future maintainer reading the plan and looking for "two plans, one for archives, one for the sentinel" won't find it.

2. Auto-install via Auto/Install collapsing

Plan: "Peer auto-fires InstallGame { id } (see install transaction)" after a successful download commit.

Implementation: handle_download_game_files_command (handlers.rs:268-281) calls run_install_operation(..., RequestedInstallOperation::Auto) directly — it doesn't actually re-enter the command loop with a PeerCommand::InstallGame. The Auto variant then collapses to Installing or Updating depending on whether local/ exists (handlers.rs:336-372). End behavior is correct, but the dispatch path is different from "fire a command." Worth noting because:

• PeerCommand::UpdateGame (lib.rs:163) exists but never has a sender — Tauri's update_game always sends GetGame, and the auto-install path uses RequestedInstallOperation::Auto. The variant is technically dead code today.

3. recover_on_startup is re-run on every load_local_library

load_local_library runs both at startup and on every SetGameDir, install, update, and uninstall (handlers.rs:487-497). Each call re-sweeps .version.ini.tmp/.version.ini.discarded across every game root and re-reads every intent file. Plan describes recovery as a startup pass.

This is not a correctness issue (recovery is idempotent and operation-table gating prevents touching games mid-operation), but on a 100-game directory it adds an O(N) read-dir + a pile of intent reads on every state-changing command. Worth flagging.

4. recover_on_startup doesn't filter local_only recovery from announcements

PLAN: "The intent log and recovery still operate on it (so we don't corrupt on-disk state for an unknown ID)."

Implementation: recover_on_startup (transaction.rs:118-143) walks every dir under game_dir and runs the matrix for each ID, ignoring catalog membership. That matches the plan. ✓

5. Order of write_intent vs rollback in uninstall

PLAN order:

1. Write intent Uninstalling
2. rename(local, .local.backup); drop marker
3. rm -rf .local.backup
4. Write intent None
5. Emit success event

Implementation (transaction.rs:93-116) on step-3 failure calls restore_backup before writing None. Plan says "Rollback (step 3 fails): attempt rename(.local.backup, local); surface error." It does not specify intent ordering. The implementation does write None after the restore — but only on the success branch of the rollback. On the error-of-rollback path, the original error is wrapped and returned without writing None. That leaves the intent log saying Uninstalling if rollback also fails, which is conservative (next startup will retry uninstall). This is consistent with the plan's "never delete unless we have positive proof of ownership" bias.

6. download_chunk truncate guard

The chunk.length == 0 && chunk.offset == 0 branch in download.rs:377-380 truncates the file to zero. This isn't from the plan but was inherited from earlier code. With the new sentinel-last contract it's still correct, because version.ini chunks never reach this branch (they take the in-memory path above).

7. update_game_installation_state still exists

Plan didn't mandate full deletion. The function is now uncoupled (good) but the comment about archives-without-sentinel still emits a log line and resets local_version to None when not downloaded. Tauri's UI-facing Game still gets downloaded and installed from this function on every refresh_games_list. Acceptable.

Items the plan called for but I didn't independently verify

These are implemented but I am not 100% certain without running the suite:

• The 300s constant (LOCAL_GAME_FALLBACK_SCAN_SECS) — only seen indirectly.
• mDNS/QUIC pipeline behavior unchanged in practice (no protocol bytes touched).

Summary

PLAN.md is honored in spirit and almost entirely in letter. The deviations above are either cosmetic (partition naming), minor (recovery re-run cost), or vestigial (PeerCommand::UpdateGame dead variant). The core invariants — sentinel-last download, marker-or-intent-proves-ownership recovery, catalog-and-operation-gated serve, watcher-as-trigger with rescan gating, and the install state machine living in the peer crate — all hold.