Fix internal map paths in coop mission .scmap files on deploy#328
Fix internal map paths in coop mission .scmap files on deploy#328TimMasalme wants to merge 3 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>
Two things could still go wrong silently. A path could be rewritten into something structurally valid but wrong - an off by one on the version, the wrong folder - and the .scmap round trip would happily accept it. And a failure while building the zip left a partial file behind in the served maps directory. verifyRelease() resolves every path the fixer rewrote against the files that actually end up in the zip. If one does not resolve, the mission is not deployed: no zip, no database update, the previous version stays in place. Checked against the current missions, this catches all 40 rewritten paths as broken when the rewriting is skipped, which is exactly the bug this branch fixes. References in text files are only reported, not enforced. Three of them have been broken for years - typos in comment headers, one stale hardcoded version - and failing on those would block releases for cosmetic reasons. The zip is now written to <name>.part and moved into place afterwards, so a failure cannot leave a half written archive where the game serves maps from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Compiles the deployment scripts and runs the .scmap path fixer over every mission in faf-coop-maps, in the same image the CronJobs use. Only runs when the scripts change. The load bearing assertion is that rewriting with a negative version reproduces the input byte for byte. It runs over all missions, not only the four that need the fix, so it also covers the old file formats - the coop missions use three of them (53, 56 and 60) and scoping a section to the wrong one breaks exactly those and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds SCMAP path rewriting and release verification to coop map deployment, introduces a Gradle verification task, and runs it in a GitHub Actions workflow against the coop maps repository. ChangesSCMAP deployment verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant CoopMaps
participant Gradle
participant ScmapPathFixerCheck
GitHubActions->>CoopMaps: checkout coop maps repository
GitHubActions->>Gradle: compile deployment scripts
GitHubActions->>Gradle: run verifyScmapFixer with MAPS_REPO
Gradle->>ScmapPathFixerCheck: execute checker
ScmapPathFixerCheck->>CoopMaps: scan .scmap files
ScmapPathFixerCheck-->>GitHubActions: exit status and verification summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/faf-legacy-deployment/scripts/CoopMapDeployer.kt (2)
235-267: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEach
.scmapis now parsed three-plus times per deploy.
getFileContentruns during checksum generation, again insideverifyRelease, and again increateZip(which itself regenerates checksums). For the larger missions this is a full parse + round-trip verification each time. Caching the rewritten bytes per file for the duration ofprocessCoopMapwould cut this down.Not blocking given the small mission count.
🤖 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/CoopMapDeployer.kt` around lines 235 - 267, Cache the processed `.scmap` bytes produced by `getFileContent` for each file throughout `processCoopMap`, and reuse that cache during checksum generation, `verifyRelease`, and `createZip` instead of reparsing and rewriting the same files. Thread the per-deployment cache through the relevant functions while preserving existing output and verification behavior.
269-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSide effects inside the lazy
checkmessage lambda.
broken.forEach { log.error(...) }executes only as a side effect of building the failure message. It works, but logging belongs outside the assertion.♻️ Proposed cleanup
+ broken.forEach { log.error("$map: $it") } check(broken.isEmpty()) { - broken.forEach { log.error("$map: $it") } "$map: ${broken.size} rewritten map path(s) do not resolve, not deploying this mission" }🤖 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/CoopMapDeployer.kt` around lines 269 - 272, Move the broken-path logging out of the lazy message lambda in the map deployment validation, while preserving the existing check failure message and deployment guard. In the surrounding CoopMapDeployer flow, log each entry in broken before invoking check, then keep the check block limited to constructing the "$map" summary message.apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt (1)
289-314: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded reads on malformed input produce cryptic index exceptions.
transferStringcan run past the buffer without finding a terminator (posends atsize + 1), andtransferSizedStringtrusts the length prefix. A corrupt or unsupported-version file surfaces asStringIndexOutOfBoundsExceptionrather than a message naming the field. Callers only seee.message, which will benull-ish for these.Cheap improvement: validate
lengthagainstsrc.size - posintransferSizedStringand require a terminator was actually found intransferString.🤖 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 289 - 314, Validate bounded input in transferString and transferSizedString: require transferString to find a null terminator before advancing past the buffer, and verify transferSizedString’s length is non-negative and does not exceed src.size - pos before constructing the string. Throw an exception with a clear field/context message when either validation fails so callers receive actionable error details.
🤖 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/coop-deployment-scripts.yml:
- Around line 22-28: Update both checkout steps in the workflow, including the
default checkout and the “Check out the coop missions” step, to set
persist-credentials to false. Add workflow-level permissions restricting the
token to contents: read, while preserving the existing checkout paths and
behavior.
In `@apps/faf-legacy-deployment/scripts/CoopMapDeployer.kt`:
- Around line 195-205: Update the scmap handling in getFileContent to require
bytes.isScmap() as well as referencesOwnMapFolder(...) before calling
fixScmapPaths. Preserve returning the original bytes for placeholders and other
files, matching the guard used by verifyRelease.
In `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt`:
- Around line 268-286: Update addVersion to preserve the input path’s original
separator and segment shape when inserting the version suffix, rather than
filtering empty segments and always rebuilding with a single leading slash.
Ensure the rewritten path changes only the intended map-name segment so
addedBytes remains equal to the actual length delta for paths with missing,
repeated, or trailing separators.
In `@apps/faf-legacy-deployment/scripts/ScmapPathFixerCheck.kt`:
- Around line 63-80: Add an expected folder allowlist or equivalent manifest
near the ScmapPathFixerCheck test data, and assert that the discovered folders
exactly match it before running rewrite validation. Keep
referencesOwnMapFolder() for per-folder processing, but ensure missing or
unexpected mission folders fail the check instead of silently skipping versioned
assertions.
---
Nitpick comments:
In `@apps/faf-legacy-deployment/scripts/CoopMapDeployer.kt`:
- Around line 235-267: Cache the processed `.scmap` bytes produced by
`getFileContent` for each file throughout `processCoopMap`, and reuse that cache
during checksum generation, `verifyRelease`, and `createZip` instead of
reparsing and rewriting the same files. Thread the per-deployment cache through
the relevant functions while preserving existing output and verification
behavior.
- Around line 269-272: Move the broken-path logging out of the lazy message
lambda in the map deployment validation, while preserving the existing check
failure message and deployment guard. In the surrounding CoopMapDeployer flow,
log each entry in broken before invoking check, then keep the check block
limited to constructing the "$map" summary message.
In `@apps/faf-legacy-deployment/scripts/ScmapPathFixer.kt`:
- Around line 289-314: Validate bounded input in transferString and
transferSizedString: require transferString to find a null terminator before
advancing past the buffer, and verify transferSizedString’s length is
non-negative and does not exceed src.size - pos before constructing the string.
Throw an exception with a clear field/context message when either validation
fails so callers receive actionable error details.
🪄 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: d86ffa63-e819-44b0-bb3e-50411d939017
📒 Files selected for processing (5)
.github/workflows/coop-deployment-scripts.ymlapps/faf-legacy-deployment/scripts/CoopMapDeployer.ktapps/faf-legacy-deployment/scripts/ScmapPathFixer.ktapps/faf-legacy-deployment/scripts/ScmapPathFixerCheck.ktapps/faf-legacy-deployment/scripts/build.gradle.kts
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Check out the coop missions | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: FAForever/faf-coop-maps | ||
| path: faf-coop-maps |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files matching coop =="
fd -a 'coop-deployment-scripts\.ya?ml$' .github/workflows 2>/dev/null || true
echo "== target workflow excerpt =="
if [ -f .github/workflows/coop-deployment-scripts.yml ]; then
cat -n .github/workflows/coop-deployment-scripts.yml
fi
echo "== checkout steps in target workflow =="
if rg -n "actions/checkout|permissions:|contents:|gradle|kotlin|run:" .github/workflows/coop-deployment-scripts.yml; then
true
fi
echo "== diff stat/name-only if available =="
git diff -- .github/workflows/coop-deployment-scripts.yml | sed -n '1,140p' || trueRepository: FAForever/gitops-stack
Length of output: 1860
🌐 Web query:
actions/checkout persist-credentials default true README
💡 Result:
The persist-credentials input in the actions/checkout GitHub Action currently has a default value of true [1][2][3]. When set to true (the default), the action configures the provided token or SSH key with the local git configuration, which allows your scripts to execute authenticated git commands (such as git push or git fetch) during the job [1][4][5]. These credentials are automatically removed by the action during post-job cleanup [1][4][5]. Although a pull request has been proposed to change this default to false to enhance security and reduce the risk of unintentional credential exposure, it remains set to true in the current official releases [6][1]. Users who wish to opt out of this behavior and prevent the action from configuring their credentials in the git config can explicitly set persist-credentials to false in their workflow configuration [1][4].
Citations:
- 1: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/cbb722410c2e876e24abbe8de2cc27693e501dcb/action.yml
- 3: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 4: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 5: https://github.com/marketplace/actions/checkout
- 6: Change the default value of persist-credentials to false actions/checkout#1687
🌐 Web query:
GitHub Actions contents: read checkout token persists Git config
💡 Result:
The GitHub Actions actions/checkout action persists authentication credentials (such as the GITHUB_TOKEN or a provided SSH key/PAT) by default to enable subsequent steps in your workflow to run authenticated Git commands [1][2][3]. Key technical details regarding this behavior include: Credential Storage Mechanism: Historically, actions/checkout configured these credentials directly in the local Git configuration (.git/config) [1][4]. However, beginning with version 6.0.0, the action was updated to store these credentials in a separate, isolated file located under the $RUNNER_TEMP directory [5][3][6]. This change uses Git's includeIf mechanism to conditionally include the credentials only when necessary, significantly improving security by separating them from the main repository configuration [4]. Persistence Lifecycle: The credentials remain persisted for the duration of the job [1][3]. A post-job cleanup step is responsible for removing these credentials once the job completes [1][2]. Opting Out: If you do not require authenticated Git commands in subsequent steps, you can prevent this behavior by setting the persist-credentials input to false [1][2]: - uses: actions/checkout@v4 with: persist-credentials: false Security Recommendations: Because the persist-credentials option defaults to true, any subsequent steps in the same job can potentially access these credentials [1][7]. To follow the principle of least privilege, it is recommended to: 1. Limit the permissions of your GITHUB_TOKEN to the minimum necessary, typically contents: read [8][9]. 2. Set persist-credentials: false if your workflow does not require further authenticated Git operations [4][7]. 3. Use fine-grained tokens or service accounts with specific scopes if you need to provide an alternative authentication method [2][10].
Citations:
- 1: https://github.com/actions/checkout/?tab=readme-ov-file
- 2: https://github.com/actions/checkout
- 3: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 4: Persist creds to a separate file actions/checkout#2286
- 5: actions/checkout@v5.0.1...v6.0.0
- 6: https://github.com/actions/checkout/tree/v6.0.0
- 7: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 8: actions/checkout@009b9ae
- 9: https://github.com/actions/checkout/tree/08c6903cd8c0fde910a37f88322edcfb5dd907a8
- 10: Submodules checkout "repository not found" error without PAT actions/checkout#2080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines count =="
wc -l .github/workflows/coop-deployment-scripts.yml
echo "== complete workflow =="
cat -n .github/workflows/coop-deployment-scripts.yml
echo "== matched script files =="
fd '(\.gradle|gradle|kotlin|build\.kt|build\.gradle|.*\.gradle)' apps/faf-legacy-deployment/scripts . 2>/dev/null | head -200 || true
echo "== gradle run/build files excerpts =="
if [ -d apps/faf-legacy-deployment/scripts ]; then
find apps/faf-legacy-deployment/scripts -maxdepth 3 \
\( -name 'build.gradle*' -o -name '*.gradle.kts' -o -name '*.gradle' -o -name '*.kt' \) -print -exec sed -n '1,180p' {} \;
fi
echo "== any git commands in scripts/graders =="
rg -n "git\\b|GITHUB_TOKEN|github::token|persist-credentials|actions/github-script|fetch|push|checkout" apps apps/faf-legacy-deployment .github 2>/dev/null || trueRepository: FAForever/gitops-stack
Length of output: 37004
🌐 Web query:
Eclipse JGit Git credentials provider includeIf runner temp GitHub token
💡 Result:
JGit does not currently support git config conditional includes (the includeIf directive) [1][2]. Consequently, you cannot use this mechanism to automatically inject configuration or credentials based on the file path or other conditions [3]. When working with GitHub Actions and the ephemeral GITHUB_TOKEN, attempting to use UsernamePasswordCredentialsProvider with just the token often fails [4]. The following approaches are recommended for authentication in this context: 1. Recommended Approach: Use TransportConfigCallback Modern versions of JGit (5.10+) often prioritize http.extraheader configuration, which can interfere with UsernamePasswordCredentialsProvider [5]. To ensure your credentials are used, implement a TransportConfigCallback to set the authentication explicitly [5]. For GitHub Actions, pass the repository owner as the username and the GITHUB_TOKEN as the password [4]: final String repoOwner = System.getenv("GITHUB_REPOSITORY_OWNER"); final String githubToken = System.getenv("GITHUB_TOKEN"); CredentialsProvider credentialsProvider = new UsernamePasswordCredentialsProvider(repoOwner, githubToken); TransportConfigCallback transportConfigCallback = transport -> { if (transport instanceof TransportHttp) { ((TransportHttp) transport).setPreemptiveBasicAuthentication(repoOwner, githubToken); } }; Git.cloneRepository.setURI("https://github.com/" + System.getenv("GITHUB_REPOSITORY") + ".git").setCredentialsProvider(credentialsProvider).setTransportConfigCallback(transportConfigCallback).call; 2. Limitations and Context - GITHUB_TOKEN: This token is automatically created for each workflow job and expires when the job finishes [6]. It is an installation access token, not a standard user PAT [6]. - Conditional Includes: Because JGit does not implement includeIf [1], any logic relying on conditional configuration files in your .gitconfig will be ignored. - Configuration Precedence: Be aware that if your environment contains http.extraheader settings (often injected in CI environments), they may override the CredentialsProvider [5]. Using the TransportConfigCallback is the most reliable way to bypass these overrides and force the use of your provided credentials [5].
Citations:
- 1: Conditional Include in gitconfig eclipse-jgit/jgit#9
- 2: https://bugs.eclipse.org/bugs/show_bug.cgi?id=543171
- 3: https://git-scm.com/docs/git-config
- 4: https://stackoverflow.com/questions/28073266/how-to-use-jgit-to-push-changes-to-remote-with-oauth-access-token
- 5: https://stackoverflow.com/questions/76158492/how-to-use-custom-credentials-while-using-jgit
- 6: https://docs.github.com/en/actions/concepts/security/github_token
Do not persist checkout credentials into the Gradle runtime.
Both actions/checkout@v6 steps leave the job token available for the rest of the job by default, and the Gradle tasks run repository-controlled Kotlin/JGit code that can read repo checkouts. Set persist-credentials: false on both checkout steps and limit the workflow token to contents: read.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 22-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 24-28: 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/coop-deployment-scripts.yml around lines 22 - 28, Update
both checkout steps in the workflow, including the default checkout and the
“Check out the coop missions” step, to set persist-credentials to false. Add
workflow-level permissions restricting the token to contents: read, while
preserving the existing checkout paths and behavior.
Source: Linters/SAST tools
| if (file.isScmapFile()) { | ||
| val bytes = file.readBytes() | ||
| // Only missions whose map references assets in their own folder need the version | ||
| // inserted. Everything else - including the placeholder .scmap files of the missions | ||
| // that use a base game map - is passed through and never parsed. | ||
| return if (bytes.referencesOwnMapFolder(map.folderName)) { | ||
| fixScmapPaths(bytes, map.folderName, version).bytes | ||
| } else { | ||
| bytes | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing isScmap() guard here, unlike verifyRelease — placeholder .scmap files can hard-fail deployment.
verifyRelease (Line 253) guards with bytes.isScmap() && bytes.referencesOwnMapFolder(...), but getFileContent only checks referencesOwnMapFolder. A placeholder .scmap (not a real map file) that happens to contain the text /maps/<folder> reaches fixScmapPaths, which throws IllegalArgumentException on the header check. That contradicts the comment right above claiming placeholders are "never parsed", and it breaks checksum generation and zip creation for that mission.
🐛 Proposed fix
- return if (bytes.referencesOwnMapFolder(map.folderName)) {
+ return if (bytes.isScmap() && bytes.referencesOwnMapFolder(map.folderName)) {
fixScmapPaths(bytes, map.folderName, version).bytes
} else {
bytes
}📝 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.
| if (file.isScmapFile()) { | |
| val bytes = file.readBytes() | |
| // Only missions whose map references assets in their own folder need the version | |
| // inserted. Everything else - including the placeholder .scmap files of the missions | |
| // that use a base game map - is passed through and never parsed. | |
| return if (bytes.referencesOwnMapFolder(map.folderName)) { | |
| fixScmapPaths(bytes, map.folderName, version).bytes | |
| } else { | |
| bytes | |
| } | |
| } | |
| if (file.isScmapFile()) { | |
| val bytes = file.readBytes() | |
| // Only missions whose map references assets in their own folder need the version | |
| // inserted. Everything else - including the placeholder .scmap files of the missions | |
| // that use a base game map - is passed through and never parsed. | |
| return if (bytes.isScmap() && bytes.referencesOwnMapFolder(map.folderName)) { | |
| fixScmapPaths(bytes, map.folderName, version).bytes | |
| } else { | |
| bytes | |
| } | |
| } |
🤖 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/CoopMapDeployer.kt` around lines 195 -
205, Update the scmap handling in getFileContent to require bytes.isScmap() as
well as referencesOwnMapFolder(...) before calling fixScmapPaths. Preserve
returning the original bytes for placeholders and other files, matching the
guard used by verifyRelease.
| 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 | ||
| } | ||
|
|
||
| addedBytes += suffix.length - (parts[1].length - segment.length) | ||
| val fixed = "/" + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part } | ||
| .joinToString("/") | ||
| rewritten += fixed | ||
| return fixed | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
addVersion normalizes path shape, which can desync addedBytes and hard-fail the delta check.
parts drops empty segments and the result is always rejoined with a single leading /. For any path that is not already in exactly /maps/x/y shape — e.g. maps/foo/env/x.dds (no leading slash), /maps/foo//env/x.dds, or a trailing slash — the emitted string length differs from the input by more than suffix.length, so the result.size - bytes.size == rewriter.addedBytes check in fixScmapPaths (Line 98) fails and the mission is refused. Failing safe is good, but it fails on legitimate input.
Consider preserving the original prefix/shape instead of rebuilding it.
♻️ Sketch: rebuild while preserving the leading separator
- addedBytes += suffix.length - (parts[1].length - segment.length)
- val fixed = "/" + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part }
- .joinToString("/")
+ val leading = if (lower.startsWith("/")) "/" else ""
+ val rebuilt = leading + parts.joinToString("/")
+ val fixed = leading + parts.mapIndexed { i, part -> if (i == 1) segment + suffix else part }
+ .joinToString("/")
+ addedBytes += fixed.length - lower.length + (lower.length - rebuilt.length)🤖 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 268 - 286,
Update addVersion to preserve the input path’s original separator and segment
shape when inserting the version suffix, rather than filtering empty segments
and always rebuilding with a single leading slash. Ensure the rewritten path
changes only the intended map-name segment so addedBytes remains equal to the
actual length delta for paths with missing, repeated, or trailing separators.
| if (bytes.referencesOwnMapFolder(folder)) { | ||
| try { | ||
| // delta accounting and the round trip are asserted inside fixScmapPaths | ||
| val fix = fixScmapPaths(bytes, folder, 4) | ||
| rewrittenMaps++ | ||
| notes += "${fix.rewritten.size} path(s) rewritten" | ||
|
|
||
| fix.rewritten.filterNot { it.contains(".v0004/") } | ||
| .forEach { failures += "$folder: not versioned: $it" } | ||
| fix.rewritten.filter { it != it.lowercase() } | ||
| .forEach { failures += "$folder: not lower cased: $it" } | ||
| fix.rewritten.filter { DOUBLE_VERSION.containsMatchIn(it) } | ||
| .forEach { failures += "$folder: version added twice: $it" } | ||
| } catch (e: Exception) { | ||
| failures += "$folder: ${e.message}" | ||
| notes += "FAILED: ${e.message}" | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the expected rewrite corpus independently.
This gate uses referencesOwnMapFolder() to decide whether to test versioned output, but never asserts the expected four missions. If that detection regresses to false, every versioned check is skipped and CI still succeeds. Maintain an expected folder allowlist (or equivalent manifest) and assert an exact match.
🤖 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/ScmapPathFixerCheck.kt` around lines 63 -
80, Add an expected folder allowlist or equivalent manifest near the
ScmapPathFixerCheck test data, and assert that the discovered folders exactly
match it before running rewrite validation. Keep referencesOwnMapFolder() for
per-folder processing, but ensure missing or unexpected mission folders fail the
check instead of silently skipping versioned assertions.
2 participants
The problem
getFileContent()rewrites/maps/<folder>/into/maps/<folder>.vNNNN/for text filesonly. The paths embedded in the
.scmapbinary are not touched, so after a release theystill point at the unversioned folder. The missions that ship their own terrain textures or
decals lose them — the map loads with missing textures.
Three commits: the fix, a check that the release is intact before it is written, and a CI
job so the path rewriting cannot break unnoticed later.
1. The fix
ScmapPathFixer.ktreads a.scmapand writes it back byte for byte, inserting therelease version into the map folder segment of every embedded path. It is a port of
fix_pathsfrom @speed2'ssc_map_parser.gd, which is also what produced the manuallycorrected
FAF_Coop_Operation_Blockade.v0004that is live today.CoopMapDeployer.getFileContent()calls it for.scmapfiles. Nothing else changes — theversion still comes from
coop_map.version + 1, change detection and zip building work asbefore, and
build.gradle.ktsneeds no new source entry becausekotlin.srcDirs(".")picks the file up.
Only the mission's own folder is versioned
A path is rewritten only when its folder segment equals the mission folder. This is not
cosmetic — versioning everything under
/maps/would break released missions:FAF_Coop_Fort_Clarke_Assaultusesmap = '/maps/X1CA_001/X1CA_001.scmap'. Those are exactly the missions whose repo.scmapis the 18 bytetest fake map fileplaceholder.FAF_Coop_Operation_Red_Revengereferences a vault map from inside its binary:/maps/seraphim outpost ep.v0002/skycube_tundra02a.dds. Blindly appending would produce…ep.v0002.v0003.Paths that lead somewhere else are left alone and logged at WARN. An already present
.vNNNNon the mission's own folder is replaced rather than appended to, so a re-runcannot stack suffixes.
Nothing is parsed without a reason
Before parsing, a raw case-insensitive byte scan looks for
/maps/<own folder>. Only mapsthat actually reference their own folder are parsed at all — 4 of 31 real map files. The
placeholders never reach the parser, and neither do the 27 maps that only use base game
textures.
2. Verification before the release is written
The rewriter checks itself: the byte delta has to match the bytes added to paths, and the
rewritten file has to parse again and reproduce itself. Both catch a structural mistake,
neither catches a semantic one — an off by one on the version, or the wrong folder name,
would pass both and ship a map with missing textures that nothing downstream notices.
verifyRelease()closes that gap. It resolves every path the fixer rewrote against thefiles that actually end up in the zip. If one does not resolve, the mission is not
deployed: no zip, no database update, the previous version stays in place, and the other
missions carry on.
Run against the current missions with the rewriting skipped — that is, the bug this PR
fixes — the check flags all 40 rewritten paths as broken. It would have caught the original
problem on its own.
References in text files are only reported, not enforced. Three of them have been broken
for years — two typos in comment headers, one stale hardcoded
scca_coop_r06.v0018— andfailing on those would block releases for cosmetic reasons.
The zip is also written to
<name>.partand moved into place afterwards, so a failuremidway cannot leave a half written archive in the directory maps are served from.
3. CI
gradle verifyScmapFixerruns the fixer over every mission in afaf-coop-mapscheckout,in the same
gradle:9.4-jdk21image the CronJobs use. The workflow only triggers whenapps/faf-legacy-deployment/scripts/**changes.The load bearing assertion is the identity check: rewriting with a negative version has to
reproduce the input byte for byte. It runs over all missions, not only the four that
need the fix, so it covers the old file formats too. The coop missions use three of them —
v53 (16 missions), v56 (14) and v60 (1) — and scoping a section to the wrong version breaks
exactly those and nothing else. That is not hypothetical: it is the mistake an earlier port
of this parser made, and only a test across all formats catches it.
Effect on the next release
FAF_Coop_Operation_BlockadeFAF_Coop_Operation_Tha_Atha_AezFAF_Coop_Operation_Golden_CrystalsFAF_Coop_Operation_Overlord_Surth_VelsokTheir checksums change, so these four get one version bump on the first run after this
lands. The other 38 missions are byte identical and are skipped as before. If a dry run
reports more than these four as changed, something is wrong — unless
MAP_DIRis empty, inwhich case everything counts as changed because there are no old zips to compare against.
One cross-check that is not in CI, because it needs the deployed artifact: the paths this
produces for
FAF_Coop_Operation_Blockadeare identical to the ones in the manuallycorrected
.v0004that is live right now — all 18 of them, including the lower casing.Out of scope
coopMapslist —FAF_Coop_Seabring_DefenseandFAF_Coop_Theban_Colonyare in the repo but not in the list.
FAF_Coop_Operation_Red_Revenge: its skycube points at another map while the.ddssitsin its own folder. Left as is here, since that is a decision about the map, not about the
deployment.
Summary by CodeRabbit
New Features
Bug Fixes
Tests