feat(runtime): Wave 17.2 — Runtime Integrity#1
Conversation
…tion, asset hardening
Complete three runtime integrity tasks:
Task 1 — Suspension system completion:
- Zstd snapshot compression with magic-byte detection for backward compat
- Performance benchmark test verifies <500ms suspend, <1000ms resume targets
- 3 new regression tests (compressed roundtrip, size comparison, perf)
Task 2 — SDK panic elimination:
- Removed 5 expect("storage lock") calls via mutex recovery helper
- Removed 3 expect("rng lock") calls via mutex recovery helper
- All SDK functions now recover gracefully from mutex poison
Task 3 — Asset pipeline hardening:
- Replaced expect("texture_loader lock") with mutex recovery
- Replaced 12 expect("cache lock") calls with recovery helpers
- Added lock_entries/lock_id_map helpers for consistent mutex handling
All 79 tests pass. cargo fmt, clippy -D warnings clean.
📝 WalkthroughWalkthroughThis PR adds a new vibege-asset crate (caching, loading, package mounting) integrated into audio, renderer, and scene crates. It refactors vibege-audio (mixer/sound cache/playback handles), vibege-config (versioned schema/migration/profiles), vibege-core (diagnostics/services/state machine/event bus), vibege-input (action mapping/gamepad/mouse), vibege-ipc (TCP transport), vibege-renderer (batched pipeline), and vibege-sandbox (Windows Job Objects). vibege-scene gains a game runtime/lifecycle framework, expanded SceneManager (overlays/persistence/messaging), and new library/store platform modules, all wired through the runtime-app binary. ChangesAsset Management System
Audio Engine Refactor
Core Runtime Enhancements
Config System Refactor
Input System Refactor
IPC TCP Transport Rewrite
Renderer Batching Pipeline
Sandbox Windows Job Object Isolation
Game Runtime, Library & Store Framework (vibege-scene)
Runtime App Wiring
Sequence Diagram(s)sequenceDiagram
participant GameScene
participant GameRuntime
participant SessionController
participant AssetManager
participant PackageValidator
GameScene->>GameRuntime: load_from_package(data, game_name)
GameRuntime->>AssetManager: mount_package(key, data)
AssetManager-->>GameRuntime: PackageAsset
GameRuntime->>PackageValidator: validate(manifest, entry_data, engine_version)
PackageValidator-->>GameRuntime: ValidationReport
GameRuntime->>SessionController: mount / initialize / start
SessionController->>AssetManager: load_lua_source / load_texture
SessionController-->>GameRuntime: Running
GameRuntime-->>GameScene: &mut SessionController
sequenceDiagram
participant Renderer
participant TextureSlotManager
participant AssetManager
participant StagingBatch
participant GPUQueue
Renderer->>AssetManager: load_texture_asset(data)
AssetManager-->>Renderer: TextureAsset
Renderer->>TextureSlotManager: allocate bind group slot
Renderer->>StagingBatch: ensure(vertex/index capacity)
Renderer->>StagingBatch: upload(vertices, indices)
Renderer->>GPUQueue: draw_indexed per BindGroupId batch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/vibege-input/src/lib.rs (1)
438-473: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdvance
MouseStateeach frame before clearing mouse queries.
MouseState::tick()is never called, sodouble_click_timernever expires anddouble_clickis never cleared. After one click, any later second click can register as a double-click, andis_double_click()can remain true across frames.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-input/src/lib.rs` around lines 438 - 473, The mouse frame advance logic in end_frame currently clears deltas and button states but never advances MouseState, so double_click_timer can linger and double_click may stay set across frames. Update end_frame to call MouseState::tick() as part of the per-frame cleanup before clearing mouse queries, using the existing MouseState type and its tick method so double-click state expires correctly each frame.crates/vibege-scene/src/scenes/settings_scene.rs (1)
85-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the existing config instead of rebuilding sections from defaults.
This now resets unrelated saved preferences on every Settings save.
AudioConfig.muted/music_volume/sfx_volumeandGeneralConfig.themeare not copied here, so they silently fall back to defaults.💾 Suggested fix
fn save(&mut self, ctx: &mut SceneContext) { - ctx.config.set(vibege_config::VibegeConfig { - overlay: vibege_config::OverlayConfig { - hotkey_modifiers: self.hotkey_mod.clone(), - hotkey_key: self.hotkey_key.clone(), - position: self.position.clone(), - width: 800, - height: 600, - ..Default::default() - }, - audio: vibege_config::AudioConfig { - volume: self.volume, - ..Default::default() - }, - general: vibege_config::GeneralConfig { - startup_behavior: self.startup.clone(), - performance_mode: self.perf.clone(), - first_run_complete: true, - backend_url: "http://localhost:3000/api/v1".into(), - ..Default::default() - }, - ..Default::default() - }); + let mut config = ctx.config.get(); + config.overlay.hotkey_modifiers = self.hotkey_mod.clone(); + config.overlay.hotkey_key = self.hotkey_key.clone(); + config.overlay.position = self.position.clone(); + config.overlay.width = 800; + config.overlay.height = 600; + config.audio.volume = self.volume; + config.general.startup_behavior = self.startup.clone(); + config.general.performance_mode = self.perf.clone(); + config.general.first_run_complete = true; + config.general.backend_url = "http://localhost:3000/api/v1".into(); + config.general.theme = self.theme.clone(); + ctx.config.set(config); self.dirty = false; info!("Settings saved"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/scenes/settings_scene.rs` around lines 85 - 107, The save logic in settings_scene::SettingsScene::save is rebuilding VibegeConfig from defaults, which wipes unrelated persisted values. Update this method to start from the existing config in SceneContext and only overwrite the fields edited in the settings UI, especially preserving AudioConfig.muted/music_volume/sfx_volume and GeneralConfig.theme while still updating overlay, volume, startup_behavior, performance_mode, first_run_complete, and backend_url.crates/vibege-sandbox/src/lib.rs (3)
112-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against spawning over an existing child.
Line 112 allows
spawn()to be called again while a previous Unix child is still running; replacingself.childdrops the handle without killing the process.Suggested guard
pub fn spawn(&mut self) -> Result<()> { + if self.is_running() { + return Err(RuntimeError::new( + ErrorCode::INVALID_STATE_TRANSITION, + "Sandbox is already running", + )); + } let config = &self.config;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-sandbox/src/lib.rs` around lines 112 - 131, Guard against calling spawn() when a child process is already active, since replacing self.child can orphan a running Unix process. Add an early check in Sandbox::spawn (before spawn_unix()/spawn_windows()) that verifies self.child is None or the existing Child has exited, and return an error if a sandbox is already running. Keep the fix localized to spawn() and the child-management logic so the handle is never overwritten while a process is still alive.
335-339: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminate the Windows job during shutdown.
child.kill()only stops the main process; the Job Object stays open untilSandboxdrops, so descendant processes can keep running afterrequest_shutdown(). Close the job handle here or callTerminateJobObjectto stop the whole tree immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-sandbox/src/lib.rs` around lines 335 - 339, The shutdown path in request_shutdown only kills the child process and leaves the Windows Job Object alive, so descendant processes may continue running until Sandbox is dropped. Update the request_shutdown implementation to also terminate or close the job for the sandboxed process, using the existing Sandbox job-management logic/handle in this module, and keep the shutdown logging intact. Make sure the fix targets the same child/job state managed by Sandbox so the entire process tree stops immediately on shutdown.
249-277: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAssign the child in a suspended state first.
Command::spawn()starts the process beforeAssignProcessToJobObject, leaving a window where game code and any descendants can run outside the job. Use suspended creation on Windows, assign the job, then resume.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-sandbox/src/lib.rs` around lines 249 - 277, In the process startup flow in vibege-sandbox’s `lib.rs`, `Command::spawn()` starts the child before `AssignProcessToJobObject`, leaving it briefly outside the job. Change the Windows launch path to create the process suspended first, then call `AssignProcessToJobObject` on the child handle, and only after that resume the process. Keep the cleanup/error handling around the existing `cmd.spawn`, `child.id()`, and `AssignProcessToJobObject` steps consistent with the current `RuntimeError::with_cause` flow.
🟡 Minor comments (12)
crates/vibege-audio/src/mixer.rs-109-112 (1)
109-112: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRecover poisoned mixer locks instead of panicking.
lock().expect("mixer lock")is repeated throughoutmixer.rs; if one audio path panics while holding a mutex, the next mixer call will panic too. Recover the poisoned guard here so the audio path stays non-fatal, matching the crate’s error model.crates/vibege-audio/src/mixer.rs:111🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-audio/src/mixer.rs` around lines 109 - 112, The mixer lock handling in set_volume currently panics on a poisoned mutex via the mixer lock expect path, which makes subsequent audio calls fail non-fatally. Update the lock acquisition in set_volume to recover from a poisoned guard instead of panicking, and apply the same recovery pattern used by other mixer.rs methods that access self.channels so the mixer stays usable after a prior panic.crates/vibege-renderer/src/lib.rs-871-875 (1)
871-875: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck
ch.is_ascii()before converting tou8.char as u8truncates non-ASCII input (e.g.🙂becomesB), so some unsupported characters can render as a random printable glyph instead of the space fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-renderer/src/lib.rs` around lines 871 - 875, The text rendering loop in the draw-list code is converting each character with a direct `char as u8` cast before checking the ASCII range, which can turn non-ASCII input into a random printable glyph. Update the character handling in the `self.draw_list` text iteration so `ch.is_ascii()` is checked before any `u8` conversion, and only cast ASCII characters while sending all unsupported characters to the space fallback.crates/vibege-input/src/context.rs-93-100 (1)
93-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
find()/find_mut()search the wrong end of the stack.The docs say these return the top context, but
iter().find(...)returns the oldest matching entry. If the same context name is pushed more than once, reads and mutations hit the wrong layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-input/src/context.rs` around lines 93 - 100, The `ContextStack::find` and `ContextStack::find_mut` methods are scanning from the oldest entry, but they are documented to return the top context. Update both methods in `context.rs` to search from the end of `self.contexts` so the most recently pushed `InputContext` with the matching name is returned for both read and mutable access.crates/vibege-input/src/mouse.rs-36-38 (1)
36-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDouble-click timing needs to be per button.
double_click_timeris shared across all buttons, so a left-click can arm a right-click double-click. The public query is per button, so this creates false positives across unrelated mouse buttons.Also applies to: 75-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-input/src/mouse.rs` around lines 36 - 38, The double-click state in Mouse is currently shared across all buttons via double_click_timer, which can cause one button press to affect another. Update the mouse double-click tracking in Mouse and any related handling methods to store timing per MouseButton, matching the existing per-button double_click map. Make sure the logic that sets, checks, and clears double-click state uses the button identity so left/right clicks cannot arm each other.crates/vibege-scene/src/library/integrity.rs-82-102 (1)
82-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire real files for manifest and entry point checks.
exists()lets a directory named.vibege-install.jsonormain.luapass integrity, but launch later expects a readable source file. Useis_file()for both checks.Proposed fix
- if meta_path.exists() { + if meta_path.is_file() { ... - if entry_path.exists() { + if entry_path.is_file() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/library/integrity.rs` around lines 82 - 102, The integrity checks in check_manifest_exists and check_entry_point_exists currently accept directories because they use exists(), which can let a folder named .vibege-install.json or the entry point path pass validation. Update both checks to require real files by using is_file() on meta_path and entry_path, while keeping the existing IntegrityReport and IntegrityCheck behavior unchanged.crates/vibege-config/src/handle.rs-296-307 (1)
296-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNotify watchers after
create_profile().
create_profile()mutatesconfig.profilesand writes it to disk, but unlike the other mutators it never callsnotify(). Any UI/state driven byon_change()will miss newly created profiles until some unrelated config change happens later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-config/src/handle.rs` around lines 296 - 307, The create_profile mutator updates config.profiles and saves to disk but never notifies listeners, so on_change-driven UI/state can miss the new profile. Update create_profile in handle.rs to call notify() after the config is successfully mutated and persisted, matching the behavior of the other mutators that broadcast changes.crates/vibege-config/src/handle.rs-254-259 (1)
254-259: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
dirtyfalse after a successful profile save.These paths persist immediately, but they leave
dirty = true, sois_dirty()keeps reporting unsaved changes even after the write succeeded. That breaks the contract documented on Lines 81-88.Also applies to: 275-280, 304-307, 320-325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-config/src/handle.rs` around lines 254 - 259, The profile save paths are marking the config as dirty before writing and never clearing it after a successful save, so `is_dirty()` stays true even though persistence succeeded. Update the save flow in `save_profile` and the other immediate-save branches referenced by the same pattern to only keep `dirty` set on failure, and explicitly reset `guard.dirty` to false after `save_config_file` returns successfully while preserving the existing error handling and state cloning around `guard`, `path`, and `cfg`.crates/vibege-asset/src/statistics.rs-38-46 (1)
38-46: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMerge the per-type breakdown as well.
merge()updates the totals but dropsasset_type_breakdown, so merged stats can no longer reconcile with their per-type detail.Suggested fix
pub fn merge(&mut self, other: &AssetStatistics) { self.total_assets += other.total_assets; self.total_memory_bytes += other.total_memory_bytes; self.total_cache_hits += other.total_cache_hits; self.total_cache_misses += other.total_cache_misses; self.total_loads += other.total_loads; self.total_releases += other.total_releases; self.total_failed_loads += other.total_failed_loads; + + for (&asset_type, stats) in &other.asset_type_breakdown { + let entry = self.asset_type_breakdown.entry(asset_type).or_default(); + entry.count += stats.count; + entry.memory_bytes += stats.memory_bytes; + entry.cache_hits += stats.cache_hits; + entry.cache_misses += stats.cache_misses; + entry.loads += stats.loads; + entry.releases += stats.releases; + entry.failed_loads += stats.failed_loads; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-asset/src/statistics.rs` around lines 38 - 46, The AssetStatistics::merge method currently combines only the aggregate counters and leaves asset_type_breakdown behind, so merged statistics lose their per-type detail. Update merge to also merge the breakdown map for each asset type, ensuring counts are accumulated rather than overwritten, while keeping the existing total fields behavior unchanged.crates/vibege-asset/src/cache.rs-100-106 (1)
100-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount misses in
get_data()too.This accessor updates
hitsbut never updatesmisses, so cache hit-rate reporting is wrong for callers that probe throughget_data().Suggested fix
pub fn get_data(&self, key: &str) -> Option<T> { let entries = lock_entries(&self.entries); - entries.get(key).map(|e| { - self.hits.fetch_add(1, Ordering::Relaxed); - e.data.clone() - }) + match entries.get(key) { + Some(e) => { + self.hits.fetch_add(1, Ordering::Relaxed); + Some(e.data.clone()) + } + None => { + self.misses.fetch_add(1, Ordering::Relaxed); + None + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-asset/src/cache.rs` around lines 100 - 106, The get_data method in Cache only increments hits on a successful lookup and never records misses, so probe-based access skews hit-rate reporting. Update Cache::get_data to increment the misses counter when entries.get(key) returns None, keeping the existing hits increment on successful clones, and preserve the current locking and Ordering::Relaxed usage around the entries and counters.crates/vibege-core/src/event.rs-150-180 (1)
150-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPriority is only enforced within each subscriber list.
The API says higher-priority subscribers receive events first, but
publish()always runssubscribersbeforefiltered. That means aLowglobal subscriber still beats aHighfiltered subscriber for the same event. Either merge dispatch by priority or narrow the public contract/docs.Also applies to: 185-209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-core/src/event.rs` around lines 150 - 180, The event dispatch order in RuntimeEvent handling is inconsistent because `publish()` processes the `subscribers` list before the `filtered` list, so a lower-priority global subscriber can run ahead of a higher-priority filtered subscriber. Fix this by either merging the two subscription paths into a single priority-ordered dispatch in `publish()` (using the existing `subscribe_with_priority` and `subscribe_filtered_with_priority` priority values) or by updating the public contract/docs to clearly state that priority only applies within each list.crates/vibege-scene/src/store/download.rs-103-107 (1)
103-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
all()doesn't include the active download.The doc says this returns queued and active tasks, but it only clones
queue. Any UI pollingall()will lose the currently downloading item as soon asnext()moves it out of the deque.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/store/download.rs` around lines 103 - 107, The DownloadStore::all method currently returns only the queued items from queue and omits the active download, so it does not match its documented behavior. Update all() to include the currently active task as well, using the store’s active/current download state together with queue, so callers polling all() can still see the in-progress item after next() removes it from the deque.crates/vibege-scene/src/store/cache.rs-54-60 (1)
54-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset cache metadata when the cache contents are refreshed or cleared.
The fallback path in
StoreManager::fetchmarks the cache offline, but a later successfulcache_listings()call never clears that flag.clear()also leaveslast_fetchuntouched, sois_offline()/has_recent_data()can report stale state after recovery or after a manual cache clear.Proposed fix
pub fn cache_listings(&self, listings: Vec<GameListing>, ttl_secs: u64) { let mut cache = self.listings.lock().expect("cache lock"); let ttl = Duration::from_secs(ttl_secs); for listing in listings { cache.insert(listing.id.clone(), CachedEntry::new(listing, ttl)); } *self.last_fetch.lock().expect("cache lock") = Some(Instant::now()); + *self.offline.lock().expect("cache lock") = false; } @@ pub fn clear(&self) { self.listings.lock().expect("cache lock").clear(); self.search_results.lock().expect("cache lock").clear(); self.sections.lock().expect("cache lock").clear(); + *self.last_fetch.lock().expect("cache lock") = None; + *self.offline.lock().expect("cache lock") = false; }Also applies to: 149-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/store/cache.rs` around lines 54 - 60, Reset the cache metadata whenever the cache is refreshed or cleared: `cache_listings()` should also clear the offline/stale state that `StoreManager::fetch` sets, and `clear()` should reset `last_fetch` along with the cached listings. Update the `Cache` methods `cache_listings` and `clear` so `is_offline()` and `has_recent_data()` reflect the current cache contents after recovery or a manual clear. Make sure the reset happens in the same place the listings map is updated, using the existing `last_fetch` and any offline flag fields tracked by `Cache`.
🧹 Nitpick comments (5)
crates/vibege-ipc/src/lib.rs (1)
455-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the size-limit error path.
This only proves the fixture is large; it does not prove
write_message_torejects oversized messages.🧪 Proposed test tightening
fn test_message_size_limit() { let transport = create_test_transport(); let conn = IpcConnection::new(transport).with_max_message_size(10); let msg = IpcMessage::new(MessageKind::Ping, "x".repeat(100).as_str()); - let json = serde_json::to_vec(&msg).unwrap(); - assert!(json.len() as u64 > conn.max_message_size); + + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = std::thread::spawn(move || { + let _ = listener.accept(); + }); + + let mut stream = TcpStream::connect(addr).unwrap(); + let result = write_message_to(&mut stream, &msg, conn.max_message_size, conn.stats()); + assert!(result.is_err()); + handle.join().unwrap(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-ipc/src/lib.rs` around lines 455 - 461, The current test only checks that a serialized `IpcMessage` is larger than `IpcConnection::max_message_size`, but it does not exercise the actual rejection path in `write_message_to`. Update `test_message_size_limit` in `crates/vibege-ipc/src/lib.rs` to send an oversized message through `IpcConnection::write_message_to` (using the existing `create_test_transport`, `IpcConnection::new`, and `IpcMessage::new` setup) and assert that the size-limit error is returned instead of only comparing lengths.crates/vibege-runtime-app/src/main.rs (1)
371-377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDebounce the overlay hotkey
GetAsyncKeyStateonly reports the current key-down state here, so holding the combo re-armsrequest_toggle()on everyAboutToWaitpass and can flip the overlay repeatedly. Track the previous pressed state and toggle only on the up→down edge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-runtime-app/src/main.rs` around lines 371 - 377, The overlay hotkey handling in main’s key-check loop currently triggers request_toggle() on every AboutToWait pass while the combo remains held. Update the hotkey logic to remember the previous pressed state for the matched key combo and only call vibege_tray::request_toggle() when it transitions from not pressed to pressed. Use the existing key-state checks around GetAsyncKeyState, and add a small debouncing state near the hotkey handling in main.crates/vibege-scene/src/library/collections.rs (1)
28-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLock poisoning will permanently break
CollectionManagerafter any panic.Every method does
self.collections.lock().expect("collections lock"). If any critical section ever panics (even from an unrelated bug), the mutex becomes poisoned and every subsequent call torebuild,all,add_to_collection, etc. will panic forever, taking down the whole collections subsystem. Recovering withunwrap_or_else(|e| e.into_inner())avoids this cascade.♻️ Example fix pattern
- pub fn all(&self) -> Vec<Collection> { - self.collections.lock().expect("collections lock").clone() - } + pub fn all(&self) -> Vec<Collection> { + self.collections + .lock() + .unwrap_or_else(|e| e.into_inner()) + .clone() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/library/collections.rs` around lines 28 - 133, The `CollectionManager` methods are using `self.collections.lock().expect("collections lock")`, which turns any mutex poisoning into a permanent panic cascade across `rebuild`, `all`, `get`, `add_custom`, `remove_custom`, `add_to_collection`, `remove_from_collection`, `is_favorite`, and `pinned_games`. Update each lock acquisition in `CollectionManager` to recover from poisoning by taking the inner value from the poisoned lock instead of expecting success, so the subsystem keeps working after an unrelated panic.crates/vibege-scene/src/library/registry.rs (1)
107-120: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse poison recovery for registry locks.
This new runtime path still uses
.expect("registry lock"), so one poisoned mutex turns later library operations into panics instead of recovering withinto_inner()like the PR’s lock-hardening direction.Also applies to: 124-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-scene/src/library/registry.rs` around lines 107 - 120, The registry still panics on poisoned mutexes because `Registry::all`, `Registry::get`, and the other lock sites in `crates/vibege-scene/src/library/registry.rs` use `.expect("registry lock")`. Update these methods and the scan/update path to recover from poison by taking the inner value with `into_inner()` (matching the PR’s lock-hardening approach) instead of unwrapping, so `games`, `by_name`, and `last_scan` continue operating after a prior panic.crates/vibege-config/src/migration.rs (1)
10-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead/fragile version-handling logic.
The post-loop
config.version < MIN_SUPPORTED_VERSIONcheck (lines 25-33) is currently unreachable: any version entering thewhileloop that doesn't match an explicit migration arm (including versions belowMIN_SUPPORTED_VERSION) is already caught by the wildcard arm at line 17-21, which resets to defaults and returns immediately. The only way to reach line 25 isconfig.version >= CONFIG_VERSION, which can never be< MIN_SUPPORTED_VERSIONgivenMIN_SUPPORTED_VERSION <= CONFIG_VERSION. Additionally, whenCONFIG_VERSIONis bumped beyond 2 in the future, a v1 config would migrate to v2 via the explicit arm, then hit the wildcard arm for2(since there's no explicit2 => ...case yet) and get reset to defaults instead of continuing the migration chain — easy to overlook when adding new versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vibege-config/src/migration.rs` around lines 10 - 33, The version migration flow in migrate_config is redundant and fragile because the wildcard branch already resets unsupported versions, making the post-loop MIN_SUPPORTED_VERSION check unreachable. Remove that dead check and rework the match in migrate_config so future versions are handled explicitly in a forward migration chain (for example, add a dedicated arm for each supported config version instead of relying on v =>), ensuring CONFIG_VERSION bumps do not accidentally reset already-migrated configs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60d55617-fbc7-4e7b-b2da-c0eb1fc74ac7
📒 Files selected for processing (110)
Cargo.tomlcrates/vibege-asset/Cargo.tomlcrates/vibege-asset/src/cache.rscrates/vibege-asset/src/handle.rscrates/vibege-asset/src/lib.rscrates/vibege-asset/src/loader.rscrates/vibege-asset/src/metadata.rscrates/vibege-asset/src/package.rscrates/vibege-asset/src/statistics.rscrates/vibege-asset/src/types.rscrates/vibege-audio/Cargo.tomlcrates/vibege-audio/src/engine.rscrates/vibege-audio/src/handle.rscrates/vibege-audio/src/lib.rscrates/vibege-audio/src/mixer.rscrates/vibege-audio/src/sound_cache.rscrates/vibege-config/Cargo.tomlcrates/vibege-config/src/config/audio.rscrates/vibege-config/src/config/developer.rscrates/vibege-config/src/config/graphics.rscrates/vibege-config/src/config/input.rscrates/vibege-config/src/config/mod.rscrates/vibege-config/src/handle.rscrates/vibege-config/src/lib.rscrates/vibege-config/src/migration.rscrates/vibege-config/src/profile.rscrates/vibege-config/src/validation.rscrates/vibege-config/tests/integration.rscrates/vibege-core/src/diagnostics.rscrates/vibege-core/src/error.rscrates/vibege-core/src/event.rscrates/vibege-core/src/lib.rscrates/vibege-core/src/lifecycle.rscrates/vibege-core/src/metrics.rscrates/vibege-core/src/services.rscrates/vibege-core/src/state_machine.rscrates/vibege-input/Cargo.tomlcrates/vibege-input/src/action.rscrates/vibege-input/src/context.rscrates/vibege-input/src/gamepad.rscrates/vibege-input/src/lib.rscrates/vibege-input/src/mouse.rscrates/vibege-ipc/Cargo.tomlcrates/vibege-ipc/src/lib.rscrates/vibege-renderer/Cargo.tomlcrates/vibege-renderer/src/lib.rscrates/vibege-runtime-app/Cargo.tomlcrates/vibege-runtime-app/src/main.rscrates/vibege-sandbox/Cargo.tomlcrates/vibege-sandbox/src/lib.rscrates/vibege-scene/Cargo.tomlcrates/vibege-scene/src/lib.rscrates/vibege-scene/src/library/collections.rscrates/vibege-scene/src/library/history.rscrates/vibege-scene/src/library/integrity.rscrates/vibege-scene/src/library/manager.rscrates/vibege-scene/src/library/mod.rscrates/vibege-scene/src/library/models.rscrates/vibege-scene/src/library/registry.rscrates/vibege-scene/src/library/search.rscrates/vibege-scene/src/library/updates.rscrates/vibege-scene/src/runtime/context.rscrates/vibege-scene/src/runtime/error.rscrates/vibege-scene/src/runtime/lifecycle.rscrates/vibege-scene/src/runtime/mod.rscrates/vibege-scene/src/runtime/orchestrator.rscrates/vibege-scene/src/runtime/session.rscrates/vibege-scene/src/runtime/state.rscrates/vibege-scene/src/runtime/validator.rscrates/vibege-scene/src/scene/kind.rscrates/vibege-scene/src/scene/manager.rscrates/vibege-scene/src/scene/message.rscrates/vibege-scene/src/scene/mod.rscrates/vibege-scene/src/scene/state.rscrates/vibege-scene/src/scene/tests.rscrates/vibege-scene/src/scenes/error_scene.rscrates/vibege-scene/src/scenes/first_run_scene.rscrates/vibege-scene/src/scenes/game_manager.rscrates/vibege-scene/src/scenes/game_scene.rscrates/vibege-scene/src/scenes/home_scene.rscrates/vibege-scene/src/scenes/library_scene.rscrates/vibege-scene/src/scenes/mod.rscrates/vibege-scene/src/scenes/settings_scene.rscrates/vibege-scene/src/scenes/store_scene.rscrates/vibege-scene/src/store/cache.rscrates/vibege-scene/src/store/discovery.rscrates/vibege-scene/src/store/download.rscrates/vibege-scene/src/store/manager.rscrates/vibege-scene/src/store/metadata.rscrates/vibege-scene/src/store/mod.rscrates/vibege-scene/src/store/models.rscrates/vibege-scene/src/store/search.rscrates/vibege-scene/src/ui_helper.rscrates/vibege-sdk/Cargo.tomlcrates/vibege-sdk/src/assets.rscrates/vibege-sdk/src/audio.rscrates/vibege-sdk/src/input.rscrates/vibege-sdk/src/lib.rscrates/vibege-sdk/src/render.rscrates/vibege-sdk/src/runtime.rscrates/vibege-sdk/src/storage.rscrates/vibege-sdk/src/util.rscrates/vibege-suspension/Cargo.tomlcrates/vibege-suspension/src/lib.rscrates/vibege-tray/src/lib.rscrates/vibege-window/Cargo.tomlcrates/vibege-window/src/display.rscrates/vibege-window/src/dpi.rscrates/vibege-window/src/lib.rscrates/vibege-window/src/overlay.rs
💤 Files with no reviewable changes (2)
- crates/vibege-ipc/Cargo.toml
- crates/vibege-input/Cargo.toml
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct VibegeConfig { | ||
| /// Schema version. Missing (v1 files) default to 1. | ||
| #[serde(default = "version_v1")] | ||
| pub version: u32, | ||
|
|
||
| /// Overlay configuration (originated in v1). | ||
| pub overlay: OverlayConfig, | ||
|
|
||
| /// Audio configuration. | ||
| pub audio: AudioConfig, | ||
|
|
||
| /// General / runtime configuration. | ||
| pub general: GeneralConfig, | ||
|
|
||
| /// Graphics / display configuration (added in v2). | ||
| #[serde(default)] | ||
| pub graphics: GraphicsConfig, | ||
|
|
||
| /// Input / mouse configuration (added in v2). | ||
| #[serde(default)] | ||
| pub input: InputConfig, | ||
|
|
||
| /// Developer options (added in v2). | ||
| #[serde(default)] | ||
| pub developer: DeveloperConfig, | ||
|
|
||
| /// Active profile name (added in v2). | ||
| #[serde(default = "default_profile_name")] | ||
| pub active_profile: String, | ||
|
|
||
| /// Named profiles (added in v2). | ||
| #[serde(default)] | ||
| pub profiles: ProfileMap, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does the toml crate (0.8.x) still require non-table struct fields to be serialized before table fields, or has the ValueAfterTable restriction been lifted?
💡 Result:
The ValueAfterTable restriction in the toml crate is a fundamental limitation of the TOML specification itself, which requires that all non-table fields (primitive values) must be emitted before any tables [1][2][3]. Because the toml crate uses Serde for serialization, it follows the order dictated by the Rust struct's field definition or the iteration order of maps [2]. This restriction has not been lifted in the 0.8.x version of the toml crate [4]. If your data structure attempts to serialize a table (a nested struct, map, or array of tables) before a primitive value, the serializer will return a ValueAfterTable error [1][5][2]. To resolve this, you have three primary approaches: 1. Reordering: Manually reorder the fields in your Rust struct so that all simple, non-table fields appear before any fields that serialize as tables [1][6]. 2. Using toml_edit: If reordering is impractical (e.g., when dealing with dynamic maps), the toml_edit crate can be used instead [7][8][4]. It provides a more flexible API that does not enforce the same strict serialization ordering requirements as the standard toml crate [7][8]. 3. Serialization helpers: For maps, you can use the toml::ser::tables_last helper function, which explicitly ensures that table-like entries are serialized only after primitive values [2][9][3].
Citations:
- 1: https://users.rust-lang.org/t/why-toml-to-string-get-error-valueaftertable/85903
- 2: values must be emitted before tables / ValueAfterTable error when using a HashMap toml-rs/toml-rs#142
- 3: https://docs.diesel.rs/2.0.x/toml/ser/index.html
- 4: Struct with Multiple Tables (HashMap)-> Err(ValueAfterTable) toml-rs/toml-rs#407
- 5: ValueAfterTable error when not using maps toml-rs/toml-rs#145
- 6: Unresolvable ValueAfterTable errors toml-rs/toml-rs#258
- 7: Why error ValueAfterTable caused? toml-rs/toml#395
- 8: https://epage.github.io/blog/2023/01/toml-vs-toml-edit/
- 9: https://docs.rs/toml/0.5.11/toml/ser/fn.tables_last.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target module and related tests first.
git ls-files crates/vibege-config/src/config crates/vibege-config/tests | sed 's#^`#FILE` #'
printf '\n== Outline: crates/vibege-config/src/config/mod.rs ==\n'
ast-grep outline crates/vibege-config/src/config/mod.rs --view expanded || true
printf '\n== Outline: crates/vibege-config/tests/integration.rs ==\n'
ast-grep outline crates/vibege-config/tests/integration.rs --view expanded || true
printf '\n== Relevant field order and Default impl ==\n'
sed -n '1,180p' crates/vibege-config/src/config/mod.rs | cat -n
printf '\n== Integration test relevant section ==\n'
sed -n '1,220p' crates/vibege-config/tests/integration.rs | cat -nRepository: millsydotdev/vibege-runtime
Length of output: 16851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for TOML save/roundtrip usage and serializer helpers.
rg -n "to_string_pretty|to_string\(|tables_last|ValueAfterTable|save\(" crates/vibege-config -SRepository: millsydotdev/vibege-runtime
Length of output: 3317
Move active_profile before the table fields
toml::to_string_pretty(&VibegeConfig) will hit ValueAfterTable while active_profile sits after overlay/audio/general/graphics/input/developer. Reorder the scalar fields before any table-typed fields here, and keep Default::default() in the same order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-config/src/config/mod.rs` around lines 31 - 65, The TOML
serialization order in VibegeConfig is causing a ValueAfterTable error because
the scalar active_profile is declared after table fields like overlay, audio,
general, graphics, input, and developer. Reorder VibegeConfig so active_profile
comes before any table-typed fields, and make sure the Default implementation
lists fields in the same order to keep serde and default construction aligned.
| impl Validate for VibegeConfig { | ||
| fn validate(&self) -> Result<(), Vec<String>> { | ||
| let mut errors = Vec::new(); | ||
| errors.extend(self.overlay.validate().err().unwrap_or_default()); | ||
| errors.extend(self.audio.validate().err().unwrap_or_default()); | ||
| errors.extend(self.graphics.validate().err().unwrap_or_default()); | ||
| errors.extend(self.input.validate().err().unwrap_or_default()); | ||
| errors.extend(self.developer.validate().err().unwrap_or_default()); | ||
| if errors.is_empty() { | ||
| Ok(()) | ||
| } else { | ||
| Err(errors) | ||
| } | ||
| } | ||
|
|
||
| fn sanitize(&mut self) { | ||
| self.overlay.sanitize(); | ||
| self.audio.sanitize(); | ||
| self.graphics.sanitize(); | ||
| self.input.sanitize(); | ||
| self.developer.sanitize(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
VibegeConfig::validate()/sanitize() never touch general.
GeneralConfig implements Validate (lines 326-380) with real checks (startup_behavior/performance_mode/theme/backend_url enums and URL prefix), but VibegeConfig::validate() (lines 92-104) and sanitize() (lines 106-112) never call self.general.validate()/self.general.sanitize(). As a result, an invalid general section (e.g. a malformed backend_url or unknown theme) silently passes top-level validation and validate_and_fix(), even though tests/integration.rs exercises these exact failure modes directly on GeneralConfig (e.g. test_general_validate_backend_url, test_general_validate_theme) but never through VibegeConfig::validate(), so the gap isn't caught by the test suite.
🐛 Proposed fix
fn validate(&self) -> Result<(), Vec<String>> {
let mut errors = Vec::new();
errors.extend(self.overlay.validate().err().unwrap_or_default());
errors.extend(self.audio.validate().err().unwrap_or_default());
+ errors.extend(self.general.validate().err().unwrap_or_default());
errors.extend(self.graphics.validate().err().unwrap_or_default());
errors.extend(self.input.validate().err().unwrap_or_default());
errors.extend(self.developer.validate().err().unwrap_or_default());
if errors.is_empty() {
Ok(())
} else {
Err(errors)
}
}
fn sanitize(&mut self) {
self.overlay.sanitize();
self.audio.sanitize();
+ self.general.sanitize();
self.graphics.sanitize();
self.input.sanitize();
self.developer.sanitize();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Validate for VibegeConfig { | |
| fn validate(&self) -> Result<(), Vec<String>> { | |
| let mut errors = Vec::new(); | |
| errors.extend(self.overlay.validate().err().unwrap_or_default()); | |
| errors.extend(self.audio.validate().err().unwrap_or_default()); | |
| errors.extend(self.graphics.validate().err().unwrap_or_default()); | |
| errors.extend(self.input.validate().err().unwrap_or_default()); | |
| errors.extend(self.developer.validate().err().unwrap_or_default()); | |
| if errors.is_empty() { | |
| Ok(()) | |
| } else { | |
| Err(errors) | |
| } | |
| } | |
| fn sanitize(&mut self) { | |
| self.overlay.sanitize(); | |
| self.audio.sanitize(); | |
| self.graphics.sanitize(); | |
| self.input.sanitize(); | |
| self.developer.sanitize(); | |
| } | |
| } | |
| impl Validate for VibegeConfig { | |
| fn validate(&self) -> Result<(), Vec<String>> { | |
| let mut errors = Vec::new(); | |
| errors.extend(self.overlay.validate().err().unwrap_or_default()); | |
| errors.extend(self.audio.validate().err().unwrap_or_default()); | |
| errors.extend(self.general.validate().err().unwrap_or_default()); | |
| errors.extend(self.graphics.validate().err().unwrap_or_default()); | |
| errors.extend(self.input.validate().err().unwrap_or_default()); | |
| errors.extend(self.developer.validate().err().unwrap_or_default()); | |
| if errors.is_empty() { | |
| Ok(()) | |
| } else { | |
| Err(errors) | |
| } | |
| } | |
| fn sanitize(&mut self) { | |
| self.overlay.sanitize(); | |
| self.audio.sanitize(); | |
| self.general.sanitize(); | |
| self.graphics.sanitize(); | |
| self.input.sanitize(); | |
| self.developer.sanitize(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-config/src/config/mod.rs` around lines 91 - 113,
VibegeConfig::validate() and sanitize() are skipping the general section, so
invalid GeneralConfig values can pass top-level checks unnoticed. Update the
VibegeConfig impl in validate() and sanitize() to include self.general alongside
overlay, audio, graphics, input, and developer, using GeneralConfig::validate
and GeneralConfig::sanitize so backend_url, theme, startup_behavior, and
performance_mode are enforced consistently.
| event_bus.publish(&vibege_core::RuntimeEvent::ShuttingDown); | ||
| .run(move |event, elwt| match event { | ||
| Event::WindowEvent { event: we, .. } => { | ||
| input.lock().expect("Input lock").handle_window_event(&we); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Recover poisoned input locks instead of panicking.
These .expect("Input lock") calls reintroduce lock-poison panic paths in the runtime loop. Use the same poison-recovery pattern as the rest of this runtime-integrity PR.
Proposed fix
- input.lock().expect("Input lock").handle_window_event(&we);
+ lock_input(&input).handle_window_event(&we);
...
- input.lock().expect("Input lock").end_frame();
+ lock_input(&input).end_frame();Add a helper outside main:
fn lock_input(input: &Mutex<InputManager>) -> std::sync::MutexGuard<'_, InputManager> {
input.lock().unwrap_or_else(|e| {
warn!("Input lock poisoned — recovering");
e.into_inner()
})
}Also applies to: 332-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-runtime-app/src/main.rs` at line 231, Replace the remaining
Input lock panic paths in main with poison recovery. The
`input.lock().expect("Input lock")` call in the window-event handling path, and
the similar call at the other listed site, should use the same recovery approach
as the rest of the runtime integrity changes. Add a small helper such as
`lock_input` near `main` that returns a `MutexGuard<InputManager>` via
`unwrap_or_else`, logs a warning on poison, and recovers with `into_inner()`,
then use that helper wherever `InputManager` is locked.
| by_name.insert(name.clone(), games.len()); | ||
| games.push(game); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| games.sort_by(|a, b| a.name.cmp(&b.name)); | ||
|
|
||
| *self.games.lock().expect("registry lock") = games.clone(); | ||
| *self.by_name.lock().expect("registry lock") = by_name; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Rebuild by_name after sorting games.
by_name stores pre-sort indices, then games is sorted. If disk iteration returns B, A, get("B") can return A, which can cascade into updating or uninstalling the wrong game.
Proposed fix
- by_name.insert(name.clone(), games.len());
games.push(game);
}
}
}
}
games.sort_by(|a, b| a.name.cmp(&b.name));
+ by_name = games
+ .iter()
+ .enumerate()
+ .map(|(idx, game)| (game.name.clone(), idx))
+ .collect();
*self.games.lock().expect("registry lock") = games.clone();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| by_name.insert(name.clone(), games.len()); | |
| games.push(game); | |
| } | |
| } | |
| } | |
| } | |
| games.sort_by(|a, b| a.name.cmp(&b.name)); | |
| *self.games.lock().expect("registry lock") = games.clone(); | |
| *self.by_name.lock().expect("registry lock") = by_name; | |
| games.push(game); | |
| } | |
| } | |
| } | |
| } | |
| games.sort_by(|a, b| a.name.cmp(&b.name)); | |
| by_name = games | |
| .iter() | |
| .enumerate() | |
| .map(|(idx, game)| (game.name.clone(), idx)) | |
| .collect(); | |
| *self.games.lock().expect("registry lock") = games.clone(); | |
| *self.by_name.lock().expect("registry lock") = by_name; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-scene/src/library/registry.rs` around lines 98 - 108, The
registry rebuild logic in `registry.rs` is keeping `by_name` indices from before
`games.sort_by`, so name lookups can point to the wrong entry after sorting.
Update the `LibraryRegistry` rebuild flow so `games` is sorted first, then
reconstruct `by_name` from the final sorted `games` vector using each game’s
`name` and its new position. Make sure the fix is applied in the same rebuild
path that currently updates `self.games` and `self.by_name`.
| /// Validate the package. | ||
| pub fn validate(&self, engine_version: &str) -> ValidationReport { | ||
| let entry_data = Some(self.ctx.source.as_bytes()); | ||
| PackageValidator::validate(&self.ctx.manifest, entry_data, &[], engine_version) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Advance the controller to Validated on success.
GameRuntime::load_from_source() calls mount() -> validate() -> initialize(). Because Line 92 only returns a report, the controller stays Mounted, and Line 99 then rejects Mounted -> Initialized against RuntimeState::valid_transitions(). That makes the orchestrated runtime path fail immediately after a passing validation.
Suggested direction
- pub fn validate(&self, engine_version: &str) -> ValidationReport {
+ pub fn validate(&mut self, engine_version: &str) -> Result<ValidationReport, RuntimeError> {
let entry_data = Some(self.ctx.source.as_bytes());
- PackageValidator::validate(&self.ctx.manifest, entry_data, &[], engine_version)
+ let report =
+ PackageValidator::validate(&self.ctx.manifest, entry_data, &[], engine_version);
+ if report.passed {
+ self.transition(RuntimeState::Validated)?;
+ }
+ Ok(report)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Validate the package. | |
| pub fn validate(&self, engine_version: &str) -> ValidationReport { | |
| let entry_data = Some(self.ctx.source.as_bytes()); | |
| PackageValidator::validate(&self.ctx.manifest, entry_data, &[], engine_version) | |
| } | |
| /// Validate the package. | |
| pub fn validate(&mut self, engine_version: &str) -> Result<ValidationReport, RuntimeError> { | |
| let entry_data = Some(self.ctx.source.as_bytes()); | |
| let report = | |
| PackageValidator::validate(&self.ctx.manifest, entry_data, &[], engine_version); | |
| if report.passed { | |
| self.transition(RuntimeState::Validated)?; | |
| } | |
| Ok(report) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-scene/src/runtime/session.rs` around lines 91 - 95, Advance the
session controller to Validated inside validate() after
PackageValidator::validate succeeds, so GameRuntime::load_from_source() can
proceed from Mounted to Initialized without tripping
RuntimeState::valid_transitions(). Update the validate method on the
session/controller path to perform the state transition after a successful
report is produced, keeping the existing validation call and preserving the
returned ValidationReport.
| fn call_scene<F>( | ||
| scene: &mut dyn Scene, | ||
| f: F, | ||
| ctx: &mut SceneContext, | ||
| pending: &mut VecDeque<SceneAction>, | ||
| error_fallback: &mut Option<Box<dyn Scene>>, | ||
| ) where | ||
| F: FnOnce(&mut dyn Scene, &mut SceneContext) -> SceneResult, | ||
| { | ||
| match f(scene, ctx) { | ||
| Ok(SceneAction::Continue) => {} | ||
| Ok(action) => { | ||
| pending.push_back(action); | ||
| } | ||
| Err(e) => { | ||
| let id = scene.id(); | ||
| warn!(?id, error = %e, "Scene lifecycle error"); | ||
| if let Some(fallback) = error_fallback.take() { | ||
| let mut node = SceneNode::new(fallback); | ||
| let fid = node.id(); | ||
| let _ = node.scene.on_create(ctx); | ||
| let _ = node.scene.on_enter(ctx); | ||
| let _ = node.scene.on_activate(ctx); | ||
| info!(?fid, "Error fallback displayed"); | ||
| // We can't push to overlays here without access to self. | ||
| // Instead, queue a PushModal action. | ||
| pending.push_back(SceneAction::PushModal(node.scene)); | ||
| } else { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Stop advancing the transition after a lifecycle error.
call_scene() turns Err into queued actions and returns (), so callers keep running later hooks and still insert the scene even when on_create() failed. In the fallback branch it also runs on_create/on_enter/on_activate immediately, then PushModal runs them again. Propagate failure back to the caller and queue the raw fallback scene without pre-initializing it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-scene/src/scene/manager.rs` around lines 748 - 775, The
call_scene() helper is swallowing lifecycle failures and converting them into
queued actions, which lets callers continue the transition and can double-run
fallback initialization. Update call_scene() and its callers to propagate Err
back so the transition stops immediately after a lifecycle error, and in the
error_fallback branch queue the fallback scene itself with
SceneAction::PushModal without calling on_create, on_enter, or on_activate
beforehand.
| let inp = InputState::new( | ||
| &ctx.input, | ||
| &[ | ||
| "up", "down", "enter", "escape", "r", "f", "delete", "c", "u", | ||
| ], | ||
| ); | ||
|
|
||
| if inp.pressed(4) | ||
| /* esc */ | ||
| { | ||
| match &self.view_mode { | ||
| ViewMode::CollectionView(_) => { | ||
| self.view_mode = ViewMode::Collections; | ||
| self.selection = 0; | ||
| } | ||
| ViewMode::Collections => { | ||
| self.view_mode = ViewMode::List; | ||
| self.selection = 0; | ||
| } | ||
| ViewMode::List => { | ||
| return Ok(SceneAction::Pop); | ||
| } | ||
| } | ||
| return Ok(SceneAction::Continue); | ||
| } | ||
| if r { | ||
| self.games = scan_games(); | ||
|
|
||
| if inp.pressed(5) | ||
| /* r */ | ||
| { | ||
| self.manager.refresh(); | ||
| self.game_names = self | ||
| .manager | ||
| .games() | ||
| .into_iter() | ||
| .map(|g| g.name.clone()) | ||
| .collect(); | ||
| self.selection = 0; | ||
| return Ok(SceneAction::Continue); | ||
| } | ||
|
|
||
| if self.games.is_empty() { | ||
| if inp.pressed(9) /* c */ && matches!(self.view_mode, ViewMode::List) { | ||
| self.view_mode = ViewMode::Collections; | ||
| self.selection = 0; | ||
| return Ok(SceneAction::Continue); | ||
| } | ||
|
|
||
| if up && self.selection > 0 { | ||
| self.selection -= 1; | ||
| } | ||
| if down && self.selection + 1 < self.games.len() { | ||
| self.selection += 1; | ||
| if inp.pressed(8) /* u */ && matches!(self.view_mode, ViewMode::List) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Fix the shifted input indices before this can delete games accidentally.
The binding order is escape=3, r=4, f=5, delete=6, c=7, u=8, but the handlers use 4/5/6/7/9. As written, pressing c can run the uninstall path, and the collections shortcut is unreachable.
Proposed fix
+ const UP: usize = 0;
+ const DOWN: usize = 1;
+ const ENTER: usize = 2;
+ const ESCAPE: usize = 3;
+ const REFRESH: usize = 4;
+ const FAVORITE: usize = 5;
+ const DELETE: usize = 6;
+ const COLLECTIONS: usize = 7;
+ const UPDATES: usize = 8;
+
- if inp.pressed(4)
+ if inp.pressed(ESCAPE)
...
- if inp.pressed(5)
+ if inp.pressed(REFRESH)
...
- if inp.pressed(9) /* c */ && matches!(self.view_mode, ViewMode::List) {
+ if inp.pressed(COLLECTIONS) && matches!(self.view_mode, ViewMode::List) {
...
- if inp.pressed(8) /* u */ && matches!(self.view_mode, ViewMode::List) {
+ if inp.pressed(UPDATES) && matches!(self.view_mode, ViewMode::List) {
...
- if inp.pressed(0) && self.selection > 0 {
+ if inp.pressed(UP) && self.selection > 0 {
...
- if inp.pressed(1) && self.selection + 1 < collections.len() {
+ if inp.pressed(DOWN) && self.selection + 1 < collections.len() {
...
- if inp.pressed(2) {
+ if inp.pressed(ENTER) {
...
- if inp.pressed(6)
+ if inp.pressed(FAVORITE)
...
- if inp.pressed(7)
+ if inp.pressed(DELETE)Also applies to: 183-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-scene/src/scenes/library_scene.rs` around lines 105 - 151, The
input key indices in LibraryScene::update are shifted from the binding order,
causing the wrong actions to fire. Update the inp.pressed(...) checks in the
keyboard handler block so they match the declared keys in InputState::new
(escape, r, f, delete, c, u), especially the shortcuts for collections and
uninstall/delete logic. Use the existing ViewMode branching and SceneAction
return flow, but correct the numeric indices consistently in this handler and
the related section referenced by the comment.
| pub fn next(&self) -> Option<DownloadTask> { | ||
| let mut queue = self.queue.lock().expect("queue lock"); | ||
| let mut active = self.active.lock().expect("active lock"); | ||
| if active.is_some() { | ||
| return None; | ||
| } | ||
| let task = queue.pop_front()?; | ||
| *active = Some(task.clone()); | ||
| Some(task) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Unify mutex acquisition order before this queue is used concurrently.
next() / clear() lock queue then active, while fail() / cancel() do the reverse. Two threads entering those paths concurrently can deadlock the entire download system. Guard both fields with one mutex or enforce one lock order everywhere.
Also applies to: 60-83, 124-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vibege-scene/src/store/download.rs` around lines 43 - 52, The download
queue logic has inconsistent mutex acquisition order across
`DownloadStore::next`, `clear`, `fail`, and `cancel`, which can deadlock when
accessed concurrently. Fix this by either guarding both `queue` and `active`
with a single mutex or making every path acquire the locks in the same order
everywhere. Update the affected methods in `DownloadStore` so their locking
strategy is identical and safe under concurrent use.
|
Superseded by PR #2 |
1 participant
Summary
Three runtime integrity improvements:
Task 1 — Suspension Compression
Task 2 — SDK Panic Elimination
Task 3 — Asset Pipeline Hardening
Validation
Summary by CodeRabbit
New Features
Bug Fixes