Fix Skyrim deployment pipeline integrity - #105
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements target-aware deployment identity and splits mixed Root/Data mods into separate deployment batches during collection installation. It also introduces durable mod-level and per-file deploy target metadata to survive manifest purges, and ensures zero-file deployments fail instead of reporting false success. However, several critical issues were identified: the durable deploy target and base path are not persisted to the database for collection-installed mods; rolling back a mod version with a 'mixed' target incorrectly deploys root files to the data directory; mixed mod deployment does not short-circuit if the first batch fails; and unvalidated paths are checked on the filesystem before path safety validation, posing a path traversal probing risk.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| db.set_deploy_file_targets_for_mod(mod_id, &deploy_file_targets) | ||
| .map_err(|e| InstallError::Failed(e.to_string()))?; |
There was a problem hiding this comment.
The durable deploy_target and deploy_base_path are never persisted to the database for collection-installed mods, leaving them as the default 'data'. This breaks the ability to recover the correct target (like "root", "custom", or "mixed") during a full redeploy after a manifest purge. We should call db.set_deploy_target_for_mod_with_base to persist this metadata.
db.set_deploy_file_targets_for_mod(mod_id, &deploy_file_targets)
.map_err(|e| InstallError::Failed(e.to_string()))?;
db.set_deploy_target_for_mod_with_base(
mod_id,
deploy_target_str,
vortex_dir.as_ref().and_then(|p| p.to_str()),
)
.map_err(|e| InstallError::Failed(e.to_string()))?;| let deploy_base = if mod_target == "root" { | ||
| &game_path | ||
| } else if mod_target == "custom" { | ||
| rollback_base = db | ||
| .get_deploy_base_path_for_mod(mod_id) | ||
| .map_err(|e| e.to_string())? | ||
| .map(PathBuf::from) | ||
| .ok_or_else(|| { | ||
| "Cannot safely roll back custom deploy target without durable base path metadata".to_string() | ||
| })?; | ||
| &rollback_base | ||
| } else { | ||
| &data_dir | ||
| }; |
There was a problem hiding this comment.
When rolling back a mod version, if mod_target is "mixed", the code falls back to using &data_dir as the single deploy_base. This is incorrect because mixed mods contain both root files (which must go to game_path) and data files (which go to data_dir). Using &data_dir as the single base will incorrectly deploy the root files to the data directory. Consider handling "mixed" target rollback by splitting files into batches or returning an error if mixed rollback is not supported via this path.
| let mut results = Vec::new(); | ||
| if !root_files.is_empty() { | ||
| let mappings: Vec<deployer::DeployFileMapping> = root_files | ||
| .iter() | ||
| .map(|f| deployer::DeployFileMapping { | ||
| source_relative_path: f.source_relative_path.clone(), | ||
| relative_path: f.relative_path.clone(), | ||
| }) | ||
| .collect(); | ||
| results.push(deployer::deploy_mod_atomic_mapped_with_progress( | ||
| &db_c, | ||
| &gid, | ||
| &bn, | ||
| mod_id, | ||
| &sp, | ||
| &gp, | ||
| &mappings, | ||
| &progress_cb, | ||
| &gp, | ||
| "root", | ||
| )); | ||
| } | ||
| if !data_files.is_empty() { | ||
| results.push(deployer::deploy_mod_atomic_with_progress( | ||
| &db_c, | ||
| &gid, | ||
| &bn, | ||
| mod_id, | ||
| &sp, | ||
| &data_effective_dir, | ||
| &data_files, | ||
| &progress_cb, | ||
| &gp, | ||
| &data_target, | ||
| )); | ||
| } | ||
| results |
There was a problem hiding this comment.
When deploying mixed mods, if the first batch (root_files) fails, the code still proceeds to attempt deployment of the second batch (data_files). This is inefficient and can leave the filesystem in an inconsistent state before rollback. The deployment should short-circuit immediately if the first batch fails.
let mut results = Vec::new();
if !root_files.is_empty() {
let mappings: Vec<deployer::DeployFileMapping> = root_files
.iter()
.map(|f| deployer::DeployFileMapping {
source_relative_path: f.source_relative_path.clone(),
relative_path: f.relative_path.clone(),
})
.collect();
let res = deployer::deploy_mod_atomic_mapped_with_progress(
&db_c,
&gid,
&bn,
mod_id,
&sp,
&gp,
&mappings,
&progress_cb,
&gp,
"root",
);
let is_err = res.is_err();
results.push(res);
if is_err {
return results;
}
}
if !data_files.is_empty() {
results.push(deployer::deploy_mod_atomic_with_progress(
&db_c,
&gid,
&bn,
mod_id,
&sp,
&data_effective_dir,
&data_files,
&progress_cb,
&gp,
&data_target,
));
}
results| if !root_dir.join(&rel_norm).is_file() { | ||
| continue; | ||
| } | ||
| if !crate::staging::is_safe_relative_path(&rel_norm) | ||
| || !crate::staging::is_safe_relative_path(&source_norm) | ||
| { | ||
| log::warn!( | ||
| "Skipping unsafe Root/ file during collection deploy: {}", | ||
| rel_norm | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In split_collection_root_data_files, the code performs a filesystem check root_dir.join(&rel_norm).is_file() before validating that rel_norm and source_norm are safe relative paths. Performing filesystem operations on unvalidated paths is a security risk (potential path traversal probing). The validation checks should be performed first.
if !crate::staging::is_safe_relative_path(&rel_norm)
|| !crate::staging::is_safe_relative_path(&source_norm)
{
log::warn!(
"Skipping unsafe Root/ file during collection deploy: {}",
rel_norm
);
continue;
}
if !root_dir.join(&rel_norm).is_file() {
continue;
}![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| let mod_target = db | ||
| .get_deploy_target_for_mod(mod_id) | ||
| .unwrap_or_else(|_| "data".to_string()); | ||
| let effective_dir = match mod_target.as_str() { | ||
| "root" => game.game_path.clone(), | ||
| "custom" => { | ||
| // Recompute from staged file shape — we don't store | ||
| // the custom path itself, only the kind. | ||
| let (dir, _) = resolve_effective_deploy_dir( | ||
| db.set_enabled(mod_id, true).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Bug: For mixed-target mods, toggle_mod calls db.set_enabled(mod_id, true) before deployment finishes, risking an inconsistent state if a crash occurs mid-deploy.
Severity: MEDIUM
Suggested Fix
Move the db.set_enabled(mod_id, true) call to execute only after all deployment operations for the mixed-target mod have successfully completed. This will ensure the database state is only updated once the file system is in a consistent state, matching the logic for other mod types.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src-tauri/src/commands/mods.rs#L752-L755
Potential issue: In the `toggle_mod` function, when enabling a "mixed" target mod, the
database is updated to mark the mod as enabled via `db.set_enabled(mod_id, true)` before
the file deployment operations are complete. If a crash or power loss occurs after this
database update but before deployment finishes, the application will be left in an
inconsistent state. Upon restart, the mod will appear as enabled in the UI but will have
incomplete or no files deployed, causing it to have no effect in-game. The pre-launch
self-healing mechanism does not correct this issue as it relies on a deployment manifest
that would not be fully created.
Did we get this right? 👍 / 👎 to inform future reviews.
| let (count, fallback) = deploy_staged_file( | ||
| &native.game_data_root, | ||
| &detected.data_dir, | ||
| &m.name, | ||
| &staging, | ||
| rel, | ||
| &bepinex_status, | ||
| )?; |
There was a problem hiding this comment.
Bug: The deploy_native function lacks a rollback mechanism. If a file deployment fails mid-process, already-deployed files from other mods are not cleaned up, leaving an inconsistent state.
Severity: MEDIUM
Suggested Fix
Implement an atomic deployment mechanism for deploy_native, similar to the deploy_mod_atomic function used for Wine deployments. On a file deployment error, the function should catch the error, remove all files deployed during the current operation, and restore any backed-up files before returning the error. This ensures the deployment is an all-or-nothing operation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src-tauri/src/plugins/paralives_native.rs#L184-L191
Potential issue: The `deploy_native` function for native Paralives deployments iterates
through mods and deploys their files. If an error occurs during this process, for
example when `deploy_staged_file` fails because a BepInEx plugin is found without
BepInEx being installed, the function returns immediately. However, it does not roll
back any files that were successfully deployed from previous mods in the same operation.
This leaves the game installation in an inconsistent state, where some mods are
partially installed. This contrasts with the Wine deployment path, which uses an atomic
deployment with full rollback on failure.
1 participant
Summary
Verification
cargo test deployer --lib→ 40 passedcargo test --lib→ 1628 passed, 0 failed, 1 ignorednpx svelte-check --threshold error→ 0 errors, 271 warningsgit diff --check→ cleanReview gates