Fix internal map paths in coop mission .scmap files on deploy#327
Fix internal map paths in coop mission .scmap files on deploy#327TimMasalme wants to merge 2 commits into
Conversation
getFileContent() only rewrote /maps/<folder>/ in text files, so the paths embedded in the .scmap binary kept pointing at the unversioned folder and the affected missions lost their custom textures after a release. ScmapPathFixer reads and rewrites the .scmap byte by byte, ported from speed2's sc_map_parser.gd, and inserts the release version into the map folder segment. Only paths that point at the mission's own folder are touched. Several missions deliberately reference a base game map (/maps/X1CA_001/X1CA_001.scmap) or a texture of another map, and versioning those would break them. A map file is only parsed at all when a raw byte scan finds "/maps/<own folder>", which also keeps the placeholder .scmap files of the missions using a base game map out of the parser. Affects 4 of the 42 deployed missions: Operation_Blockade, Tha_Atha_Aez, Golden_Crystals and Overlord_Surth_Velsok. Their checksums change, so they get one version bump on the first run after this lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not part of the pull request, only used to verify the Kotlin builds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThe legacy deployment scripts now detect and selectively rewrite embedded paths in Legacy SCMap deployment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CoopMapDeployer
participant ScmapPathFixer
participant ScmapRewriter
participant ChecksumAndZip
CoopMapDeployer->>ScmapPathFixer: detect SCMap and own map-folder reference
ScmapPathFixer->>ScmapRewriter: rewrite embedded map paths
ScmapRewriter-->>ScmapPathFixer: return serialized bytes
ScmapPathFixer-->>CoopMapDeployer: return validated content
CoopMapDeployer->>ChecksumAndZip: use content for checksums and ZIP output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
The problem
The change
Only the mission's own folder is versionedA path is rewritten only when its folder segment equals the mission folder. This is not
Paths that lead somewhere else are left alone and logged at WARN. An already present Nothing is parsed without a reasonBefore parsing, a raw case-insensitive byte scan looks for Self-check before shippingAfter rewriting, and before the result can end up in a zip, the fixer asserts that
If either fails it throws, Effect on the next release
Their checksums change, so these four get one version bump on the first run after this VerificationRun against every
Out of scope
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt (1)
66-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo automated tests for a complex binary format rewriter.
This PR's own goal is to "verify" the scmap fixer, and the runtime round-trip self-check here is a good safety net, but there's still no unit test exercising
fixScmapPaths/ScmapRewriteragainst real.scmapfixtures (own-folder path, base-game-map reference, decal sized-strings, different format versions). A fixture-based test asserting byte-for-byte output would also have caught the case-folding issue flagged below, since that bug slips past both the size-delta and round-trip checks.Want me to draft a JUnit test with a small synthetic
.scmapfixture covering these paths?🤖 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 `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt` around lines 66 - 93, Add automated fixture-based JUnit coverage for fixScmapPaths and ScmapRewriter using synthetic .scmap inputs that exercise own-folder paths, base-game-map references, decal sized-strings, and multiple format versions. Assert the complete output bytes, including the case-folding scenario, rather than relying only on the runtime size-delta and round-trip checks in fixScmapPaths.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/compile-legacy-deployment.yml:
- Around line 17-19: Harden the checkout step in the workflow by disabling
credential persistence via the checkout action’s persist-credentials setting,
and add explicit workflow permissions granting only contents read access. Update
the existing actions/checkout step; do not alter the compilation flow.
In `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt`:
- Around line 254-271: Update both early-return branches in addVersion to return
the original path rather than the lowercased value: the suffix-empty case and
the non-matching/invalid path case. Preserve lowercasing only for internal
comparisons and keep the version-rewrite path unchanged.
---
Nitpick comments:
In `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt`:
- Around line 66-93: Add automated fixture-based JUnit coverage for
fixScmapPaths and ScmapRewriter using synthetic .scmap inputs that exercise
own-folder paths, base-game-map references, decal sized-strings, and multiple
format versions. Assert the complete output bytes, including the case-folding
scenario, rather than relying only on the runtime size-delta and round-trip
checks in fixScmapPaths.
🪄 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 Plus
Run ID: 50b284da-fbc2-4f29-8463-5d6966c4a1e5
📒 Files selected for processing (3)
.github/workflows/compile-legacy-deployment.ymlapps/faf-legacy-deployment/scripts/CoopMapDeployer.ktapps/faf-legacy-deployment/scripts/ScmapPathFixer.kt
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Harden checkout: disable credential persistence.
zizmor flags this checkout for credential persistence (artipacked): the default leaves the GITHUB_TOKEN persisted for the rest of the job, even though this workflow only compiles code and never needs to push back. Also consider declaring explicit permissions: contents: read for least privilege.
🔒 Proposed fix
+permissions:
+ contents: read
+
jobs:
compile:
runs-on: ubuntu-latest
container:
image: gradle:9.4-jdk21
steps:
- uses: actions/checkout@v6
+ with:
+ persist-credentials: false📝 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.
| steps: | |
| - uses: actions/checkout@v6 | |
| permissions: | |
| contents: read | |
| jobs: | |
| compile: | |
| runs-on: ubuntu-latest | |
| container: | |
| image: gradle:9.4-jdk21 | |
| steps: | |
| - uses: actions/checkout@v6 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/compile-legacy-deployment.yml around lines 17 - 19, Harden
the checkout step in the workflow by disabling credential persistence via the
checkout action’s persist-credentials setting, and add explicit workflow
permissions granting only contents read access. Update the existing
actions/checkout step; do not alter the compilation flow.
Source: Linters/SAST tools
| private fun addVersion(path: String): String { | ||
| if (suffix.isEmpty()) return path | ||
| val lower = path.lowercase() | ||
| val parts = lower.split('/').filter { it.isNotEmpty() } | ||
| // expected: ["maps", "map_name", "env", ...] | ||
| if (parts.size < 3 || parts[0] != "maps") return lower | ||
|
|
||
| val segment = parts[1].replace(VERSIONED, "") | ||
| if (segment != folder) { | ||
| skipped += path | ||
| return lower | ||
| } | ||
|
|
||
| rewritten++ | ||
| addedBytes += suffix.length - (parts[1].length - segment.length) | ||
| return "/" + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part } | ||
| .joinToString("/") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
addVersion lowercases paths it's supposed to leave untouched.
The docstring here says "Everything else is returned untouched," and the caller's docstring (line 48) explicitly warns that base-game-map references like /maps/X1CA_001/X1CA_001.scmap must be left alone. But both early-return branches return lower instead of path, so every path that doesn't target the mission's own folder (base-game maps, shared textures, skyboxes, decals, etc.) still gets case-folded to lowercase.
This silently diverges from the documented contract and isn't caught by either safeguard: the size-delta check only detects length changes (lowercasing doesn't change length), and the round-trip re-verification reprocesses the already-lowercased output with suffix = "", which trivially matches itself. If any referenced asset (e.g. a base-game map file) has mixed-case path segments on disk, this rewrite would break that reference.
🐛 Proposed fix
private fun addVersion(path: String): String {
if (suffix.isEmpty()) return path
val lower = path.lowercase()
val parts = lower.split('/').filter { it.isNotEmpty() }
// expected: ["maps", "map_name", "env", ...]
- if (parts.size < 3 || parts[0] != "maps") return lower
+ if (parts.size < 3 || parts[0] != "maps") return path
val segment = parts[1].replace(VERSIONED, "")
if (segment != folder) {
skipped += path
- return lower
+ return path
}📝 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.
| private fun addVersion(path: String): String { | |
| if (suffix.isEmpty()) return path | |
| val lower = path.lowercase() | |
| val parts = lower.split('/').filter { it.isNotEmpty() } | |
| // expected: ["maps", "map_name", "env", ...] | |
| if (parts.size < 3 || parts[0] != "maps") return lower | |
| val segment = parts[1].replace(VERSIONED, "") | |
| if (segment != folder) { | |
| skipped += path | |
| return lower | |
| } | |
| rewritten++ | |
| addedBytes += suffix.length - (parts[1].length - segment.length) | |
| return "/" + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part } | |
| .joinToString("/") | |
| } | |
| private fun addVersion(path: String): String { | |
| if (suffix.isEmpty()) return path | |
| val lower = path.lowercase() | |
| val parts = lower.split('/').filter { it.isNotEmpty() } | |
| // expected: ["maps", "map_name", "env", ...] | |
| if (parts.size < 3 || parts[0] != "maps") return path | |
| val segment = parts[1].replace(VERSIONED, "") | |
| if (segment != folder) { | |
| skipped += path | |
| return path | |
| } | |
| rewritten++ | |
| addedBytes += suffix.length - (parts[1].length - segment.length) | |
| return "/" + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part } | |
| .joinToString("/") | |
| } |
🤖 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 `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt` around lines 254 - 271,
Update both early-return branches in addVersion to return the original path
rather than the lowercased value: the suffix-empty case and the
non-matching/invalid path case. Preserve lowercasing only for internal
comparisons and keep the version-rewrite path unchanged.
TimMasalme
changed the title
2 participants
Summary by CodeRabbit
New Features
.scmapfiles during legacy deployment packaging.Bug Fixes
.scmapfiles.Chores