Reliability & hardening from multi-agent review (v3.7.24) - #69
Merged
Merged
Conversation
Multi-agent review surfaced five HIGH findings; this fixes all of them.
All 1013 tests pass.
- OpenIvExecutor (H1): write to a temp sibling then atomic File.Move on
overwrite, so a failed or interrupted copy never truncates an existing
game file in place. Adds an overwrite-failure regression test proving the
original survives and no .tmp is left behind.
- ChangeHistoryService (H4/H5): persist via JsonFileStore (logged, atomic
temp-write) instead of a bare catch{} that silently lost the audit trail;
guard the entry list with a lock and a Lazy singleton; Entries now returns
a snapshot. No more swallowed saves or torn cross-thread reads.
- ProfileManager: log skipped corrupt profiles instead of dropping them
silently, never seed stock defaults over unparseable user data, and write
profiles atomically through JsonFileStore.
- JobQueue (H3): serialize all job-state access under a lock and hand the
/jobs/{id} poller a detached snapshot, so it can never observe torn state
(e.g. State=Completed before ResultJson is visible).
- LocalApi (H2): add ApiErrors.Problem helper and route 21 catch handlers
through it, so exception text (absolute file paths) is logged server-side
with a correlation id rather than leaked to the client.
Second review pass: closes the remaining testable MEDIUM findings and cuts release 3.7.24. All 1021 tests pass (+8 new). - SafeModeEndpoints: contain restore to the GTA V root via new PathContainment.IsWithin helper, so a tampered/corrupt safe-mode manifest cannot move files to/from arbitrary locations. Adds PathContainmentTests (inside/sibling-prefix/traversal/null cases). - OpenIvExecutor: build a one-time key->entry archive lookup instead of re-enumerating per operation (O(n^2) -> O(n)); also avoids re-opening entries out of order on non-seekable streams. - RestorePointService: Lazy singleton + lock-guarded snapshot reads/writes, closing the torn-read race between UI and background saves. - Release 3.7.24: bump Version/AssemblyVersion/FileVersion in both csproj, update version assertions (VersionAndBrowseGuardTests, SetupWizardTests), add RELEASE_v3.7.24.md and a new CHANGELOG.md.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
🤖 Augment PR SummarySummary: This reliability release bumps the application and shared-library versions to v3.7.24.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0440c19a1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full multi-agent code review of the codebase surfaced five HIGH findings and several MEDIUM ones. This PR fixes all of the HIGH findings plus the testable MEDIUM ones, and cuts release v3.7.24. No new features — reliability, data-integrity, security, and concurrency hardening only.
Tests: 1012 → 1021 passing (+9), 0 failures. Every change was validated with
dotnet testbefore commit.HIGH findings fixed
OpenIvExecutornow writes each extracted file to a temp sibling and commits with an atomicFile.Move. Previously an overwrite truncated the existing file in place before the new bytes landed, so a crash/power-loss mid-copy could destroy a valid game file. Adds an overwrite-failure regression test.ChangeHistoryServicenow persists via the atomic, loggedJsonFileStoreinstead of a barecatch {}that discarded the audit trail; entry list is lock-guarded with aLazysingleton.JobEntrystate access is serialized and/jobs/{id}readers get a detached snapshot, so they can never observe torn state (e.g.State=CompletedbeforeResultJsonis visible).ApiErrors.Problemhelper; 21 handlers across 10 endpoint files no longer returnex.Message(absolute%APPDATA%/GTA paths). The exception is logged server-side with a correlation id; the client gets a generic message.MEDIUM findings fixed
PathContainment.IsWithinhelper; restore verifies each manifest path resolves inside the GTA V root before moving it, so a tampered/corrupt manifest can't touch arbitrary locations. NewPathContainmentTests.Lazysingleton + lock-guarded snapshot reads/writes, closing a torn-read race.Release v3.7.24
Version/AssemblyVersion/FileVersionin both csproj.VersionAndBrowseGuardTests,SetupWizardTests).RELEASE_v3.7.24.mdand a newCHANGELOG.md.Not in this PR (deliberately deferred)
BackupSchedulerlocking, god-method/file refactors (SetupDoctorService.RunAsync,InstallViewModel), installCancellationToken, and theDateTime.Now→UtcNowsweep. Real but each is its own change; bounded to what could be fully tested here.Test plan
dotnet test— 1021/1021 pass