lanspread/REVIEW_STEP_2.md

Review Step 2 — On-the-fly Decisions (IMPL_DECISIONS.md)

The implementor recorded six decisions made during implementation. Each is evaluated below.

---

1. Added just test recipe

Added a just test recipe so unit tests can be run through the repository's required just ... command surface instead of invoking cargo test directly.

Verdict: good.

CLAUDE.md explicitly says "Never use normal cargo ... commands, use the just ... commands instead." A just test recipe was a missing primitive, and adding it (justfile:22-23: cargo test --workspace) closes that gap cleanly. The implementation is the smallest possible — a one-line wrapper that runs the workspace tests. Nothing to argue with.

Tiny nit, not blocking: it doesn't take any args. If unit tests grow, you'll want just test [args] so the user can target a single test. That's a future polish.

---

2. Kept game-unpack-finished event name for compatibility

Kept the existing frontend game-unpack-finished event name for successful transactional installs. The peer now emits install lifecycle events, but the compatibility event still lets the UI reuse its existing "install complete" path.

Verdict: defensible, but worth a follow-up.

The plan said game-unpack-finished "fires only on true transactional install success (peer-driven). It does NOT fire on extract failure or commit-rename failure — those route to game-install-failed." Implementation matches this: PeerEvent::InstallGameFinished is bridged to game-unpack-finished in src-tauri/src/lib.rs:880-894, and InstallGameFailed is bridged to game-install-failed. So the contract is correct.

What's questionable is the name. There is no longer an "unpack" stage that's distinct from "install" — they're one transaction. A name like game-install-finished would be more honest. Keeping the old name makes the React side's setupUnpackListener (src/App.tsx:293-320) look stale: it's now an install-finished listener wearing an unpack-finished label.

The compatibility argument is thin because there is no external consumer of this event — it's an internal name shared between Rust and TS in this repo. The TS side could be renamed in the same change.

That said: this is a cosmetic decision and the implementor correctly noted it. Not a blocker. Plan said the event "fires on transactional install success" without prescribing a name. Recommend renaming in a follow-up, but the decision is acceptable as written.

---

3. Reused .lanspread/library_index.json for per-ID rescan

Implemented watcher rescans by reusing the existing .lanspread/library_index.json cache and updating a single game entry in that index. This satisfies the per-ID optimized rescan requirement without adding a second cache format.

Verdict: good call.

The plan said:

Optimized rescan (single ID)

1. Stat version.ini (existence + mtime), check local/ is-a-directory, stat any *.eti files at root level.
2. Compare to cached fingerprint.
3. Recursive walk only on fingerprint change.

It didn't prescribe a storage format — only behavior. Reusing the existing index file (local_games.rs:111-148) is the right call: one cache, one schema, one path to keep coherent. update_index_for_game (local_games.rs:394-449) stats the fingerprint, short-circuits on equality, and rebuilds only when the fingerprint differs. rescan_local_game (local_games.rs:568-589) is the single-ID entry point.

Two minor concerns:

• save_library_index (local_games.rs:141-148) does not use the atomic temp+rename pattern. The plan reserves atomic writes for the intent log, so this is consistent — but the index file is at risk of corruption on a crash mid-write. The blast radius is small: corrupt index → rescan rebuilds from scratch on next load. So this is acceptable, just worth knowing.
• The index file's revision is incremented inside both scan_local_library and rescan_local_game. If both paths race (fallback scan running while a watcher-triggered rescan finishes), the in-memory views could diverge briefly. They're each protected by reading and writing the same path with no global lock. In practice the gate in local_monitor.rs:32-36 prevents concurrent rescans for the same ID but not across IDs vs. fallback. Worth a thought; not a correctness bug because the next scan reconciles.

These caveats don't change the verdict.

---

4. Separate active_downloads cancellation-token map

Kept a separate active_downloads cancellation-token map next to the single active_operations table. The operation table is the authoritative state for gates; the token map is only cancellation plumbing for in-flight downloads.

Verdict: pragmatic.

The plan called for a single per-peer operation table. The implementation has it (active_operations) and additionally keeps active_downloads: HashMap<String, CancellationToken> for cancellation tokens (context.rs:38).

This is justifiable: CancellationToken doesn't fit naturally inside HashMap<String, OperationKind> because OperationKind is Copy. Storing the token alongside as a separate map keeps OperationKind simple and lets the gate-path (which only needs presence/kind) stay cheap. The download operation guard (OperationGuard::download, context.rs:142-153) clears both maps on drop.

Alternative: store OperationState { kind: OperationKind, cancel: Option<CancellationToken> }. Cleaner conceptually but invasive. The current shape is fine.

One thing to verify: the gate-path readers (local_download_available, watcher event handler, stream handlers) all check only active_operations. They don't need the token map. So the split doesn't leak. Good separation.

---

5. Downloaded-but-not-installed routes directly to InstallGame

Treated a downloaded-but-not-installed game as immediately installable from Tauri by sending PeerCommand::InstallGame directly. A not-downloaded game still uses GetGame, and the peer auto-installs after the sentinel commit.

Verdict: matches the plan.

This decision is actually prescribed by PLAN.md — see the install_game(id) routing rules:

• version.ini absent → send PeerCommand::GetGame { id } (fetch first; peer auto-fires InstallGame on download commit).
• version.ini present, local/ absent → send PeerCommand::InstallGame { id } directly (no re-download).
• version.ini present, local/ present → no-op.

Looking at src-tauri/src/lib.rs:88-128, the implementation matches the plan exactly. So this isn't really a "decision" — it's the plan. The implementor may have flagged it because they wanted to be explicit that they didn't add an intermediate download step. Either way, no concern.

---

6. LocalOnly badge derived from installed && !downloaded

Derived the UI's LocalOnly badge from installed && !downloaded because the UI-facing Game type does not carry the protocol-level Availability enum.

Verdict: acceptable, but a smell worth noting.

The protocol-level Availability is computed in build_game_summary (local_games.rs:352-356), then thrown away when game_from_summary (local_games.rs:370-387) converts to Game. The TS side has no availability field, so the badge is recomputed from installed && !downloaded in src/App.tsx:742-744.

This works because:

• Ready ⇔ downloaded == true
• LocalOnly ⇔ installed && !downloaded (because by construction we only emit LocalOnly when not downloaded)
• The third value Availability::Downloading is never emitted by build_game_summary.

So the predicate installed && !downloaded covers every case build_game_summary will produce. It's correct today.

The smell: if Availability::Downloading ever starts being emitted (or if a future state is added — e.g., Quarantined), the UI would silently miss it because the TS side reverse-engineers availability from two booleans. The single source of truth should flow through. A small, future-proofing follow-up is to add availability: string to the TS Game interface and serialize the protocol value end-to-end.

Not a blocker for this change.

---

Other implementation decisions NOT recorded in IMPL_DECISIONS.md but observed

These were made silently. The implementor may want to add them to the doc.

a. partition_download_descriptions keeps version.ini in transfer_descs

Plan says "partition" into two lists; implementation keeps both in one list and uses an in-memory version_buffer to route chunks. The end behavior is correct; the naming is misleading. See REVIEW_STEP_1 §1.

b. recover_on_startup re-runs on every load_local_library

Re-runs on SetGameDir and after every install/uninstall. Plan describes it as a one-shot startup. Likely intentional ("recovery is also a safety net for SetGameDir"), but never written down. See REVIEW_STEP_1 §3.

c. PeerCommand::UpdateGame is dead

The variant exists but has no sender path. The auto-install path uses RequestedInstallOperation::Auto and the Tauri update_game command sends GetGame. Either remove the variant or wire update_game to send it.

d. Auto/Install collapse in run_install_operation

Auto and Install both pick Updating when local/ exists and Installing otherwise (handlers.rs:356-363). That means an explicit PeerCommand::InstallGame against an already-installed game becomes an update. The user-facing semantics are surprising (calling "Install" produces an update) but the install_game Tauri command shields the UI from this by checking local/ first. Worth recording.

---

Summary

The six recorded decisions are all defensible. Two have follow-up smells worth fixing later (the game-unpack-finished legacy name and the UI's reverse-engineered LocalOnly predicate). The other four are either pragmatic minor refinements (just test, active_downloads map split) or directly implement the plan (InstallGame routing).

There are at least four additional decisions the implementor made but did not record. Recommend adding them to IMPL_DECISIONS.md so the rationale is preserved.