Fix case-sensitive filesystem creating duplicate directories#941
Merged
utkarshdalal merged 1 commit intoutkarshdalal:masterfrom Mar 28, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc case-insensitive path handling with centralized FileUtils helpers and a CaseInsensitiveFileSystem; updates Steam depot downloader to use the case-insensitive filesystem; applies case-insensitive resolution to Epic and GOG save/download paths; adds unit tests and bumps javasteam snapshot version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SteamService
participant DepotDownloader
participant CaseInsensitiveFS as FileSystem(rgba(100,150,250,0.5))
participant Disk as Disk(rgba(200,200,200,0.5))
SteamService->>DepotDownloader: construct(..., filesystem=CaseInsensitiveFS)
SteamService->>DepotDownloader: requestDepotExtraction(manifest, target)
DepotDownloader->>CaseInsensitiveFS: open/create path segments
CaseInsensitiveFS->>Disk: stat / list parent directory entries (case-insensitive match)
Disk-->>CaseInsensitiveFS: entries / metadata
CaseInsensitiveFS-->>DepotDownloader: resolved path with on-disk casing
DepotDownloader-->>SteamService: extraction complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt:526">
P2: Case-insensitive destination resolution fails for new files, allowing fallback path creation to reintroduce duplicate-case directories.</violation>
</file>
<file name="app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt:1259">
P1: Path normalization plus root-anchored resolution allows traversal-influenced save paths outside the intended Wine-prefix boundary.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt:1260">
P2: Case-insensitive resolution is bypassed when the leaf segment is missing, so fallback may create duplicate-case parent directories.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
jeremybernstein
force-pushed
the
jb/case-insensitive-paths
branch
from
b343ec7 to
e6665f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt`:
- Around line 1191-1195: The fallback for installDir uses
EpicConstants.getGameInstallPath(context, game.appName) but should use the
game's display/title so the install lookup matches how Epic stores installs;
change the fallback to EpicConstants.getGameInstallPath(context,
game.displayTitle) (or the actual display title property) and ensure the
computed installDir and the "{installdir}" entry in pathVars still use that
value (refer to the installDir variable, EpicConstants.getGameInstallPath(...)
call, and the pathVars map).
- Around line 1260-1264: The comparison uses the raw installDir string which can
have wrong casing; call FileUtils.resolveCaseInsensitive(File("/"), installDir)
to produce a case-corrected path (e.g., resolvedInstallDir) and use
resolvedInstallDir.absolutePath in the withinPrefix check instead of installDir
so that absPath is compared against the on-disk-cased install prefix; keep
existing checks for winePrefix (or resolve it similarly if needed) and replace
occurrences of installDir in the startsWith()/== comparisons with the
resolvedInstallDir.absolutePath.
In `@app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt`:
- Around line 21-29: The cache in CaseInsensitiveFileSystem (private val cache:
ConcurrentHashMap<Pair<Path, String>, Path>) is keyed by the raw segment string
so "Game" and "game" are cached separately; update onPathParameter to normalize
the segment when constructing the cache key (e.g., use
segment.lowercase(Locale.ROOT) or similar) both when reading (cache.getOrPut)
and when inserting so the Pair<Path, String> key uses the lowercased/normalized
segment consistently, preventing duplicate entries for differing case during
parallel operations.
- Around line 17-19: The constructor uses an unqualified SYSTEM symbol that is
undefined; update the default delegate in CaseInsensitiveFileSystem to reference
the companion constant via FileSystem.SYSTEM (or add the import
okio.FileSystem.Companion.SYSTEM if you prefer) so the delegate: FileSystem =
FileSystem.SYSTEM resolves correctly; ensure this change is applied in the
CaseInsensitiveFileSystem declaration that extends ForwardingFileSystem and
keeps the delegate parameter name unchanged.
In `@app/src/main/java/app/gamenative/utils/FileUtils.kt`:
- Around line 212-239: The code allows path traversal because
findFileCaseInsensitive/resolveCaseInsensitive accept relativePath with "." or
".."; normalize the relativePath into canonical segments before any filesystem
lookups and reject or clamp inputs that would escape baseDir. Specifically, in
findFileCaseInsensitive and resolveCaseInsensitive, first split and resolve
"."/".." (or compute a normalized relative path and the target canonical path)
and ensure the resolved path remains within baseDir.canonicalPath (fail-closed
or return null) before doing exact check or the case-insensitive segment
matching/append fallback; this prevents "../" from escaping baseDir while
preserving behavior for new files under on-disk casing differences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0cff235-7b88-42e6-b2c6-b120fc6058bf
📒 Files selected for processing (6)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (1)
- gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
Comment on lines
+34
to
+40
| // slow path: list parent and match case-insensitively. | ||
| // if multiple entries match (e.g. leftover _Work + _work from a | ||
| // prior bug), the first one returned by the filesystem wins — | ||
| // non-deterministic but unavoidable without deeper heuristics. | ||
| delegate.listOrNull(resolved) | ||
| ?.firstOrNull { it.name.equals(segment, ignoreCase = true) } | ||
| ?: exact |
Owner
There was a problem hiding this comment.
how slow is the "slow" path? Worried it might affect external storage badly. Having said that case sensitivitiy bugs are worse than slow SD reads/writes
Contributor
Author
There was a problem hiding this comment.
Slow = one readdir() syscall per cache miss
With the ConcurrentHashMap cache, each unique (parent, segment) pair only hits the slow path once per download session. Subsequent lookups are O(1). On SD cards the first list() might be a few ms slower than internal storage, but it only happens once per directory.
| maxDecompress = maxDecompress, | ||
| parentJob = coroutineContext[Job], | ||
| autoStartDownload = false, | ||
| filesystem = CaseInsensitiveFileSystem(), |
Contributor
Author
There was a problem hiding this comment.
the tiny DD change was made and the pointer updated in Gradle deps. works great.
Contributor
Author
There was a problem hiding this comment.
the CaseInsensitiveFileSystem is passed as the filesystem param to DepotDownloader, which uses Okio. So all file writes during download go through the case-insensitive resolver, which means that DD writes to the correctly-cased existing directory instead of creating a new one.
Contributor
Author
|
@utkarshdalal responded to your concerns, let me know if you have other things you want to talk out. I think this is a big improvement (for users experiencing problems, that is), though. |
phobos665
reviewed
Mar 27, 2026
phobos665
reviewed
Mar 27, 2026
| play-integrity = "1.6.0" # https://mvnrepository.com/artifact/com.google.android.play/integrity | ||
| hiltNavigationCompose = "1.2.0" # https://mvnrepository.com/artifact/androidx.hilt/hilt-navigation-compose | ||
| javasteam = "1.8.0.1-13-SNAPSHOT" # https://mvnrepository.com/artifact/in.dragonbra/javasteam | ||
| javasteam = "1.8.0.1-14-SNAPSHOT" # https://mvnrepository.com/artifact/in.dragonbra/javasteam |
Contributor
There was a problem hiding this comment.
Is this required for the current PR?
Contributor
Author
There was a problem hiding this comment.
yes, it adds the ability to pass in a custom FileSystem!
phobos665
reviewed
Mar 27, 2026
Comment on lines
+1257
to
+1264
| // resolve against on-disk casing to avoid creating duplicate dirs (e.g. locallow vs LocalLow) | ||
| // supersedes PR #701 | ||
| val joinedPath = normalizedParts.joinToString("/") | ||
| val resolved = FileUtils.resolveCaseInsensitive(File("/"), joinedPath) | ||
| // guard against path traversal escaping the wine prefix | ||
| val absPath = resolved.absolutePath | ||
| val withinPrefix = absPath.startsWith("$winePrefix/") || absPath == winePrefix || | ||
| (installDir.isNotEmpty() && (absPath.startsWith("$installDir/") || absPath == installDir)) |
Contributor
There was a problem hiding this comment.
Is this something that's worth extracting out of here into another place? I imagine it's probably similar in GOG?
Contributor
Author
There was a problem hiding this comment.
FileUtils.resolveCaseInsensitive() is already the centralized logic extraction. The surrounding context (path normalization & traversal limiting) is Epic-specific (GOG doesn't require it AFAICT, but tell me if I'm wrong). So I don't think there's anything worth generalizing, but let me know if you disagree.
Contributor
Author
agreed, added in force-push coming... now. |
jeremybernstein
force-pushed
the
jb/case-insensitive-paths
branch
from
e6665f4 to
035bdfb
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
1549-1559:⚠️ Potential issue | 🔴 Critical
CaseInsensitiveFileSystemstill leaves a mixed-case first-creation race here.
DepotDownloaderis doing parallel work here, butapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktresolves missing segments from current disk state and caches them by the original segment spelling. If two workers hitGame/...andgame/...before either directory exists, they can still choose different exact paths and create both trees, so the duplicate-directory bug survives on first creation. Please make the first resolution/creation atomic per parent + case-folded segment, and add a regression test for concurrent mixed-case writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 1549 - 1559, The CaseInsensitiveFileSystem currently races on first-creation because resolve/create logic can be invoked concurrently by DepotDownloader workers; modify CaseInsensitiveFileSystem (the resolution/creation code that looks up or creates path segments) to perform an atomic check-and-create per parent directory + case-folded segment (e.g., use a synchronized/locked map keyed by parent inode/path + normalizedSegment or a per-parent Mutex) so only one canonical on-disk segment is chosen/created for a given lowercase key, and update DepotDownloader usage if needed to rely on this atomic API; add a regression test that spawns concurrent tasks performing mixed-case writes (e.g., "Game/..." vs "game/...") to assert only one directory tree is created and no duplicate casings exist.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.kt (1)
91-99: Assert the resolved casing, not just existence.These cases are host-filesystem dependent right now: on a case-insensitive dev machine, returning the unresolved path would still pass. Compare against the exact file path you created so the test verifies normalization everywhere.
Proposed test tightening
`@Test` fun `findFile wrong casing returns file`() { val base = tmpDir.newFolder("game") - File(base, "CheckApplication.exe").createNewFile() + val file = File(base, "CheckApplication.exe") + file.createNewFile() val result = FileUtils.findFileCaseInsensitive(base, "checkapplication.exe") assertNotNull(result) - assertTrue(result!!.exists()) + assertEquals(file.absolutePath, result!!.absolutePath) } `@Test` fun `findFile nested wrong casing`() { val base = tmpDir.newFolder("game") val dir = File(base, "Data/SaveFiles") dir.mkdirs() - File(dir, "slot1.sav").createNewFile() + val file = File(dir, "slot1.sav") + file.createNewFile() val result = FileUtils.findFileCaseInsensitive(base, "data/savefiles/slot1.sav") assertNotNull(result) - assertTrue(result!!.exists()) + assertEquals(file.absolutePath, result!!.absolutePath) }Also applies to: 108-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.kt` around lines 91 - 99, Update the test `findFile wrong casing returns file` to assert the resolved path matches the exact file you created instead of only checking existence: after creating File(base, "CheckApplication.exe"), call FileUtils.findFileCaseInsensitive(base, "checkapplication.exe") and assert that result is not null and result!!.absolutePath (or result.canonicalPath) equals the created File's absolutePath/canonicalPath; do the same tightening for the other similar test that exercises `FileUtils.findFileCaseInsensitive` (the test covering the other filename case) so the tests validate correct case-normalization across filesystems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1549-1559: The CaseInsensitiveFileSystem currently races on
first-creation because resolve/create logic can be invoked concurrently by
DepotDownloader workers; modify CaseInsensitiveFileSystem (the
resolution/creation code that looks up or creates path segments) to perform an
atomic check-and-create per parent directory + case-folded segment (e.g., use a
synchronized/locked map keyed by parent inode/path + normalizedSegment or a
per-parent Mutex) so only one canonical on-disk segment is chosen/created for a
given lowercase key, and update DepotDownloader usage if needed to rely on this
atomic API; add a regression test that spawns concurrent tasks performing
mixed-case writes (e.g., "Game/..." vs "game/...") to assert only one directory
tree is created and no duplicate casings exist.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.kt`:
- Around line 91-99: Update the test `findFile wrong casing returns file` to
assert the resolved path matches the exact file you created instead of only
checking existence: after creating File(base, "CheckApplication.exe"), call
FileUtils.findFileCaseInsensitive(base, "checkapplication.exe") and assert that
result is not null and result!!.absolutePath (or result.canonicalPath) equals
the created File's absolutePath/canonicalPath; do the same tightening for the
other similar test that exercises `FileUtils.findFileCaseInsensitive` (the test
covering the other filename case) so the tests validate correct
case-normalization across filesystems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0deaca0a-1793-4ed5-8a9d-307f4808a7e8
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
- app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt
- gradle/libs.versions.toml
- app/src/main/java/app/gamenative/utils/FileUtils.kt
jeremybernstein
force-pushed
the
jb/case-insensitive-paths
branch
from
035bdfb to
ab8765a
Compare
Contributor
Author
|
nitpick commit is
The race condition concern: In practice this is a non-issue here. Can two concurrent The actual race window is tiny and |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
1549-1559: Please add the mixed-case download regression around this hook.This constructor path is now the Steam-side guard against recreating duplicate
Game/vsgame/directories on case-sensitive storage. A small automated fixture here would make future dependency bumps much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/SteamService.kt` around lines 1549 - 1559, Add a small automated mixed-case download regression around the DepotDownloader constructor in SteamService.kt: create a temporary test fixture that invokes DepotDownloader (the instance used in the shown constructor call) with two tiny test depots or paths whose target directories differ only by case (e.g., "Game/" and "game/"), run the downloader with autoStartDownload=false and a controlled filesystem (reuse CaseInsensitiveFileSystem or a temp real FS), then assert after the operation that the on-disk layout did not produce duplicate case-variant directories (only one canonical directory exists) to catch regressions that would recreate both "Game/" and "game/". Ensure the fixture is executed near the constructor call and fails the build/test when duplicates are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt`:
- Around line 1257-1264: The current withinPrefix check only validates the root
but not each manifest entry; before writing any manifest entry (where code uses
fileManifest.filename in the reconstruction loops), join the filename to
saveDir, resolve case-insensitively (using FileUtils.resolveCaseInsensitive with
normalizedParts/joinedPath pattern), then compute outputFile and assert
outputFile.absolutePath.startsWith(saveDir.absolutePath) or equals it; if not,
skip/log the entry and do not write. Apply this guard in both reconstruction
loops (the write sites referencing fileManifest.filename), ensuring you use the
same resolved path/casing as used for the root to prevent path traversal and
duplicate mixed-case directories.
In `@app/src/main/java/app/gamenative/utils/FileUtils.kt`:
- Around line 229-235: The loop inside for ((i, segment) in
segments.withIndex()) currently uses firstOrNull on current.listFiles() with
ignoreCase, which can pick an arbitrary case-only sibling; replace that logic by
collecting all candidates: val matches = current.listFiles()?.filter {
it.name.equals(segment, ignoreCase = true) } ?: emptyList(); if matches.size > 1
then fail fast (throw IllegalStateException or return null) to avoid ambiguous
case-only siblings; if matches.size == 1 set current = matches[0]; otherwise (no
match) fall back to the existing behavior that appends remaining segments
verbatim. Also guard against listFiles() returning null.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1549-1559: Add a small automated mixed-case download regression
around the DepotDownloader constructor in SteamService.kt: create a temporary
test fixture that invokes DepotDownloader (the instance used in the shown
constructor call) with two tiny test depots or paths whose target directories
differ only by case (e.g., "Game/" and "game/"), run the downloader with
autoStartDownload=false and a controlled filesystem (reuse
CaseInsensitiveFileSystem or a temp real FS), then assert after the operation
that the on-disk layout did not produce duplicate case-variant directories (only
one canonical directory exists) to catch regressions that would recreate both
"Game/" and "game/". Ensure the fixture is executed near the constructor call
and fails the build/test when duplicates are detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ff0e951-d46f-4b3a-8640-97e368102ed6
📒 Files selected for processing (7)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (2)
- app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
- app/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- gradle/libs.versions.toml
- app/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.kt
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The key points that make this safe:
Adding this to the long-term memory to avoid re-flagging in future reviews. ✏️ Learnings added
🧠 Learnings used |
Contributor
Author
|
@coderabbitai , your nitpick: app/src/main/java/app/gamenative/service/SteamService.kt (1) This suggestion is impractical, given what |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The simulated approach you describe — wiring a fake file-write response through ✏️ Learnings added
🧠 Learnings used |
jeremybernstein
force-pushed
the
jb/case-insensitive-paths
branch
from
ab8765a to
ab06af8
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt (1)
477-483:⚠️ Potential issue | 🟠 MajorResolve each manifest filename through the case-insensitive helper, and make the prefix check boundary-safe.
The raw
File(saveDir, fileManifest.filename)join still recreates mixed-case child directories under an already-correct root, andstartsWith(saveDir.canonicalPath)also accepts sibling escapes like../MyGameBackup/...because it is only a string-prefix test.Suggested fix
+ val saveDirCanonicalPath = saveDir.canonicalPath manifest.fileManifestList?.elements?.forEach { fileManifest -> try { - val outputFile = File(saveDir, fileManifest.filename) - if (!outputFile.canonicalPath.startsWith(saveDir.canonicalPath)) { + val outputFile = FileUtils.resolveCaseInsensitive( + saveDir, + fileManifest.filename.replace('\\', '/'), + ) + val outputCanonicalPath = outputFile.canonicalPath + if ( + outputCanonicalPath != saveDirCanonicalPath && + !outputCanonicalPath.startsWith("$saveDirCanonicalPath${File.separator}") + ) { Timber.tag("Epic").w("[Cloud Saves] Skipping path traversal: ${fileManifest.filename}") return@forEach }Apply the same change in both reconstruction loops.
Also applies to: 647-653
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt` around lines 477 - 483, Resolve each manifest filename through the existing case-insensitive path resolver before joining with saveDir (i.e., pass fileManifest.filename into the project's case-insensitive helper and use its returned relative path), then build outputFile from saveDir + resolved name; replace the naive startsWith(canonicalPath) check with a boundary-safe check (e.g., compare outputFile.canonicalPath to saveDir.canonicalPath + File.separator or compute a relative path and ensure it does not start with "..") to prevent sibling escapes; update both reconstruction loops that create outputFile, use outputFile.parentFile?.mkdirs() as before and keep the same skip/log behavior if the safety check fails.
🧹 Nitpick comments (2)
app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt (2)
27-31:createTempDiris deprecated; consider usingFiles.createTempDirectory.
createTempDirhas been deprecated since Kotlin 1.4. The modern replacement isjava.nio.file.Files.createTempDirectoryorkotlin.io.path.createTempDirectory.♻️ Suggested fix
+import java.nio.file.Files import java.io.File ... `@Before` fun setUp() { - tmpDir = createTempDir("depot_test") + tmpDir = Files.createTempDirectory("depot_test").toFile() fs = CaseInsensitiveFileSystem() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt` around lines 27 - 31, Replace the deprecated Kotlin createTempDir call in the setUp() method with the java.nio API: change tmpDir = createTempDir("depot_test") to create a temp directory using Files.createTempDirectory (or kotlin.io.path.createTempDirectory) and assign its path to tmpDir, ensuring tmpDir's type matches (convert to Path or File as needed) and keep fs = CaseInsensitiveFileSystem() unchanged; update imports to include java.nio.file.Files and handle any IOException if required.
22-31: Consider addingassumeTrueguard to skip on case-insensitive filesystems.The docstring correctly notes these tests pass trivially on macOS. Adding a runtime guard would skip the tests on case-insensitive filesystems, preventing false confidence when run locally on macOS.
♻️ Suggested guard
+import org.junit.Assume.assumeTrue import org.junit.Before ... + private fun isCaseSensitiveFilesystem(): Boolean { + val testDir = createTempDir("case_test") + try { + val lower = File(testDir, "test") + val upper = File(testDir, "TEST") + lower.createNewFile() + return !upper.exists() + } finally { + testDir.deleteRecursively() + } + } + `@Before` fun setUp() { tmpDir = createTempDir("depot_test") fs = CaseInsensitiveFileSystem() + assumeTrue("Test requires case-sensitive filesystem", isCaseSensitiveFilesystem()) }Based on learnings: "Such tests only pass on case-sensitive filesystems (Linux CI) and should guard with an
assumeTrue(isCaseSensitiveFilesystem())check."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt` around lines 22 - 31, Add a runtime guard in CaseInsensitiveFileSystemTest to skip on case-insensitive filesystems: in the setUp method of CaseInsensitiveFileSystemTest, call Assume.assumeTrue(isCaseSensitiveFilesystem()) (or equivalent helper) before creating tmpDir and fs so the test is skipped on macOS/other case-insensitive environments; ensure you import org.junit.Assume.assumeTrue and/or provide the isCaseSensitiveFilesystem() helper used to detect filesystem sensitivity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.kt`:
- Around line 477-483: Resolve each manifest filename through the existing
case-insensitive path resolver before joining with saveDir (i.e., pass
fileManifest.filename into the project's case-insensitive helper and use its
returned relative path), then build outputFile from saveDir + resolved name;
replace the naive startsWith(canonicalPath) check with a boundary-safe check
(e.g., compare outputFile.canonicalPath to saveDir.canonicalPath +
File.separator or compute a relative path and ensure it does not start with
"..") to prevent sibling escapes; update both reconstruction loops that create
outputFile, use outputFile.parentFile?.mkdirs() as before and keep the same
skip/log behavior if the safety check fails.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.kt`:
- Around line 27-31: Replace the deprecated Kotlin createTempDir call in the
setUp() method with the java.nio API: change tmpDir =
createTempDir("depot_test") to create a temp directory using
Files.createTempDirectory (or kotlin.io.path.createTempDirectory) and assign its
path to tmpDir, ensuring tmpDir's type matches (convert to Path or File as
needed) and keep fs = CaseInsensitiveFileSystem() unchanged; update imports to
include java.nio.file.Files and handle any IOException if required.
- Around line 22-31: Add a runtime guard in CaseInsensitiveFileSystemTest to
skip on case-insensitive filesystems: in the setUp method of
CaseInsensitiveFileSystemTest, call
Assume.assumeTrue(isCaseSensitiveFilesystem()) (or equivalent helper) before
creating tmpDir and fs so the test is skipped on macOS/other case-insensitive
environments; ensure you import org.junit.Assume.assumeTrue and/or provide the
isCaseSensitiveFilesystem() helper used to detect filesystem sensitivity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0bd8986-ff33-4ffa-9e07-1d60b268525f
📒 Files selected for processing (8)
app/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/service/epic/EpicCloudSavesManager.ktapp/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.ktapp/src/main/java/app/gamenative/utils/CaseInsensitiveFileSystem.ktapp/src/main/java/app/gamenative/utils/FileUtils.ktapp/src/test/java/app/gamenative/utils/CaseInsensitiveFileSystemTest.ktapp/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (3)
- app/src/main/java/app/gamenative/service/gog/GOGCloudSavesManager.kt
- gradle/libs.versions.toml
- app/src/test/java/app/gamenative/utils/CaseInsensitivePathResolutionTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/utils/FileUtils.kt
jeremybernstein
force-pushed
the
jb/case-insensitive-paths
branch
from
ab06af8 to
be7a2f0
Compare
Contributor
Author
|
@coderabbitai, but good catch -- those new tests confirmed that the race condition concern which I dismissed above (#941 (comment)) can in fact arise in practice. Fixed and fixed. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I'll remove the stale learning that declared the ✏️ Learnings added
⛔ Learnings removed🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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
3 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
CaseInsensitiveFileSystem(OkioForwardingFileSystemwrapper) that resolves each path component against on-disk casing before any I/O, with exact-match fast path and per-session cachefilesystemconstructor param (JavaSteam 1.8.0.1-14-SNAPSHOT)FileUtils.findFileCaseInsensitiveto Epic and GOG cloud save download pathsfilesystemparam)Test plan
Game/gameduplicate no longer created{appdata}/../locallowpath (Enter the Gungeon)Supersedes #701
Fixes #940
Summary by cubic
Fix duplicate directories on case-sensitive filesystems by resolving paths case-insensitively for Steam depot downloads and Epic/GOG cloud saves. Adds path traversal checks for Epic saves.
Bug Fixes
CaseInsensitiveFileSystem(OkioForwardingFileSystem) and pass it to DepotDownloader viafilesystemto resolve on-disk casing before I/O.FileUtils.findFileCaseInsensitivefor manifest/config lookups.FileUtils.resolveCaseInsensitivefor save writes to reuse on-disk casing and avoid duplicate dirs; Epic restricts resolved paths to the Wine prefix or install dir and skips any file that resolves outsidesaveDir.Dependencies
javasteamto1.8.0.1-14-SNAPSHOTto enable the injectablefilesystemparameter.Written for commit be7a2f0. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores