Encryption key generation failure in web export#607
Open
ikostan wants to merge 102 commits into
Open
Conversation
Removed the ai_autonomous_agent entry from the enabled editor plugins in project.godot. Updated globals.gd to use OS.has_feature("web") instead of comparing OS.get_name(), preventing OS.get_unique_id() from crashing on Web builds. Added workspace/test_injection.sh — a local CI/CD simulation script that injects a test salt into project.godot (with backup), runs a Godot web export (expects godot CLI and a 'Web' preset), verifies the injection, packages the export, and starts a Python server that sets COOP/COEP headers for SharedArrayBuffer/thread support. The script includes basic verification output and usage notes for running locally.
Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/davidanson/markdownlint-cli2-action) from 23.0.0 to 23.2.0. - [Release notes](https://github.com/davidanson/markdownlint-cli2-action/releases) - [Commits](DavidAnson/markdownlint-cli2-action@ce4853d...ded1f94) --- updated-dependencies: - dependency-name: DavidAnson/markdownlint-cli2-action dependency-version: 23.2.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [release-drafter/release-drafter](https://github.com/release-drafter/release-drafter) from 7.2.0 to 7.3.0. - [Release notes](https://github.com/release-drafter/release-drafter/releases) - [Commits](release-drafter/release-drafter@5de9358...c2e2804) --- updated-dependencies: - dependency-name: release-drafter/release-drafter dependency-version: 7.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps the pip group with 1 update in the / directory: [urllib3](https://github.com/urllib3/urllib3). Updates `urllib3` from 2.6.3 to 2.7.0 - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst) - [Commits](urllib3/urllib3@2.6.3...2.7.0) --- updated-dependencies: - dependency-name: urllib3 dependency-version: 2.7.0 dependency-type: direct:production dependency-group: pip ... Signed-off-by: dependabot[bot] <[email protected]>
…nson/markdownlint-cli2-action-23.2.0 Bump DavidAnson/markdownlint-cli2-action from 23.0.0 to 23.2.0
…e-drafter/release-drafter-7.3.0 Bump release-drafter/release-drafter from 7.2.0 to 7.3.0
Bump urllib3 from 2.6.3 to 2.7.0 in the pip group across 1 directory
…ps://github.com/ikostan/SkyLockAssault into encryption-key-generation-failure-in-web-export
ikostan
added
web
testing
dependencies
Contributor
Reviewer's GuideRefactors save encryption to use a CI‑injected GDScript salt and SHA‑256 key while hardening web exports, adding CI utilities for salt/feature-flag injection, improving corrupted-save recovery, and stabilizing browser and GUT tests; updates CI workflows and dependencies accordingly. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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:
📝 WalkthroughWalkthroughThis PR updates Godot web export salt handling and platform detection, adds a salt-injection and export test script, updates three GitHub Actions action pins, and bumps ChangesWeb Export Encryption and Salt Management
Workflow and Dependency Maintenance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In workspace/test_injection.sh, consider adding a trap or final cleanup step to automatically restore project.godot from project.godot.backup so local runs don't leave the project file permanently modified.
- The build/export section in workspace/test_injection.sh is duplicated (two "Executing Build & Export Pipeline" headers and multiple mkdir/rm commands); you can simplify this by consolidating the logic into a single block.
- The salt-escaping pipeline in workspace/test_injection.sh is a bit hard to follow (ESCAPED_SALT, GODOT_ESCAPED, SED_ESCAPED); consider reducing the number of intermediate variables or adding brief comments at each stage to clarify the purpose of each transformation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In workspace/test_injection.sh, consider adding a trap or final cleanup step to automatically restore project.godot from project.godot.backup so local runs don't leave the project file permanently modified.
- The build/export section in workspace/test_injection.sh is duplicated (two "Executing Build & Export Pipeline" headers and multiple mkdir/rm commands); you can simplify this by consolidating the logic into a single block.
- The salt-escaping pipeline in workspace/test_injection.sh is a bit hard to follow (ESCAPED_SALT, GODOT_ESCAPED, SED_ESCAPED); consider reducing the number of intermediate variables or adding brief comments at each stage to clarify the purpose of each transformation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
May 15, 2026 5:14a.m. | Review ↗ |
| JavaScript | |
May 15, 2026 5:14a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![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: 2
🧹 Nitpick comments (2)
workspace/test_injection.sh (2)
11-11: ⚡ Quick winAdd comment explaining the intentional special character test case.
The raw secret
'T3st_S@lt!_2026#"\'intentionally includes special characters (quotes and backslashes) to test the escaping logic. While syntactically correct, this is non-obvious.📝 Suggested clarification
-# 1. Hardcoded Secret (Mimicking GitHub Secrets) -RAW_SECRET='T3st_S@lt!_2026#"\' +# 1. Hardcoded Secret (Mimicking GitHub Secrets) +# Note: Intentionally includes special characters (", \, ') to test escaping edge cases +RAW_SECRET='T3st_S@lt!_2026#"\'🤖 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 `@workspace/test_injection.sh` at line 11, Add an inline comment above the RAW_SECRET assignment clarifying that the value RAW_SECRET='T3st_S@lt!_2026#"\' intentionally contains quotes and a backslash to exercise/verify escaping logic in tests, and that the characters must remain exactly as written (do not sanitize or change them) so future readers know this is a deliberate test case.
30-37: ⚡ Quick winRemove redundant
GODOT_ESCAPEDvariable.Line 34 creates
GODOT_ESCAPEDwith identical logic toESCAPED_SALT(line 30), then line 35 immediately uses it to createSED_ESCAPED. This adds no value and makes the escaping chain harder to follow.♻️ Proposed simplification
# 2. Mimic the GitHub Action Injection Step ESCAPED_SALT=$(printf '%s' "$PRODUCTION_SALT" | sed 's/\\/\\\\/g; s/"/\\"/g') echo "⚙️ Running sed injection script..." -GODOT_ESCAPED=$(printf '%s' "$PRODUCTION_SALT" | sed 's/\\/\\\\/g; s/"/\\"/g') -SED_ESCAPED=$(printf '%s' "$GODOT_ESCAPED" | sed 's/\\/\\\\/g; s/&/\\&/g; s/|/\\|/g') +SED_ESCAPED=$(printf '%s' "$ESCAPED_SALT" | sed 's/\\/\\\\/g; s/&/\\&/g; s/|/\\|/g') sed -i "s|^[[:space:]]*security/save_salt=.*|security/save_salt=\"$SED_ESCAPED\"|g" project.godot🤖 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 `@workspace/test_injection.sh` around lines 30 - 37, GODOT_ESCAPED is redundant because it duplicates ESCAPED_SALT; remove the GODOT_ESCAPED assignment and build SED_ESCAPED directly from ESCAPED_SALT (use ESCAPED_SALT in the printf/sed chain that creates SED_ESCAPED), then keep the sed -i replacement using SED_ESCAPED to update security/save_salt in project.godot; ensure variable names remain ESCAPED_SALT and SED_ESCAPED so the final sed line still references SED_ESCAPED.
🤖 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 `@project.godot`:
- Line 57: Document the breaking change in the PR description and add migration
guidance: explain that changing save_salt to "dev_fallback_salt" will make
previously plaintext dev configs unreadable and advise developers to delete
local user://config.cfg (or equivalent) before running; additionally modify
safe_load_config() to detect legacy plaintext configs (e.g., by checking for
absence of expected salt/version/marker or validating MD5 before decryption) and
implement a graceful migration path or clear error message prompting deletion,
and include a version/check flag written when creating encrypted configs so
future loads can distinguish formats.
In `@workspace/test_injection.sh`:
- Line 144: The simple cd export/web command can fail; update the script in
workspace/test_injection.sh to check the directory change and handle errors
explicitly by testing the result of cd export/web (or verifying the directory
exists beforehand), and on failure print a clear error message and exit non‑zero
so subsequent steps don't run; ensure the error handling is adjacent to the
existing cd export/web invocation so it's easy to locate.
---
Nitpick comments:
In `@workspace/test_injection.sh`:
- Line 11: Add an inline comment above the RAW_SECRET assignment clarifying that
the value RAW_SECRET='T3st_S@lt!_2026#"\' intentionally contains quotes and a
backslash to exercise/verify escaping logic in tests, and that the characters
must remain exactly as written (do not sanitize or change them) so future
readers know this is a deliberate test case.
- Around line 30-37: GODOT_ESCAPED is redundant because it duplicates
ESCAPED_SALT; remove the GODOT_ESCAPED assignment and build SED_ESCAPED directly
from ESCAPED_SALT (use ESCAPED_SALT in the printf/sed chain that creates
SED_ESCAPED), then keep the sed -i replacement using SED_ESCAPED to update
security/save_salt in project.godot; ensure variable names remain ESCAPED_SALT
and SED_ESCAPED so the final sed line still references SED_ESCAPED.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: be56e06d-73ad-4cc8-b46a-da75a129206d
📒 Files selected for processing (7)
.github/workflows/lint_readme.yml.github/workflows/release_drafter.yml.github/workflows/release_drafter_pr.ymlproject.godotrequirements.txtscripts/core/globals.gdworkspace/test_injection.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-28T02:11:45.806Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 588
File: .github/workflows/deploy_to_itch.yml:44-56
Timestamp: 2026-04-28T02:11:45.806Z
Learning: When a CI workflow edits Godot's `project.godot` (INI) to inject custom ProjectSettings values, insert the setting key under the correct section header that matches the `game/` (or other) root in the ProjectSettings path. For example, `ProjectSettings.get_setting("game/security/save_salt", ...)` expects the INI entry under `[game]` with key `security/save_salt` (i.e., `[game]` then `security/save_salt=...`), not under `[application]`. Otherwise the lookup will fall back to the default value at runtime.
Applied to files:
.github/workflows/release_drafter_pr.yml.github/workflows/release_drafter.yml.github/workflows/lint_readme.yml
🪛 GitHub Actions: Pull Request Pipeline / 1_GUT Unit Tests _ unit-test.txt
scripts/core/globals.gd
[error] 520-520: safe_load_config triggered ERR_FILE_CORRUPT due to encrypted file MD5 mismatch.
🪛 Shellcheck (0.11.0)
workspace/test_injection.sh
[info] 11-11: Want to escape a single quote? echo 'This is how it'''s done'.
(SC1003)
[warning] 144-144: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🔇 Additional comments (8)
requirements.txt (1)
34-34: LGTM!.github/workflows/release_drafter_pr.yml (1)
19-19: LGTM!.github/workflows/release_drafter.yml (1)
29-29: LGTM!.github/workflows/lint_readme.yml (1)
24-24: LGTM!project.godot (1)
53-53: LGTM!workspace/test_injection.sh (2)
46-79: LGTM!
88-116: LGTM!scripts/core/globals.gd (1)
483-483: ⚡ Quick winRemove the unsupported claim about changing from
OS.get_name()toOS.has_feature().The encryption key function in
_get_encryption_key()usesOS.has_feature("web"), but git history shows no evidence that it ever usedOS.get_name() == "Web". The concern about platform detection equivalence is valid in principle, but the specific claim about this particular change cannot be verified.However, the existing code already includes safeguards for this scenario: it detects legacy plaintext files (via
is_legacy = trueinsafe_load_config()) and logs clear warnings when decryption fails. The migration path is documented through these error messages and the fallback logic.If platform detection semantics differ between
OS.get_name()andOS.has_feature(), users with existing encrypted configs will see the decryption error message and can manually migrate by decrypting with the old key. Consider adding documentation about this potential compatibility issue or adding a tool/script for safe key rotation if this is a known risk.> Likely an incorrect or invalid review comment.
The cd command can fail if the directory doesn't exist or permissions are denied. While the export should have created export/web, it's safer to handle potential failures explicitly.
…ps://github.com/ikostan/SkyLockAssault into encryption-key-generation-failure-in-web-export
Python and Playwright tests (audio_flow_test.py and test_ci_flag_injection.py) are fully up to date and require no changes. The Playwright assertions don't explicitly hunt for the IS_AUTOMATED_TEST log, so dropping it to DEBUG won't break anything. However, workspace/test_injection.sh represents a gap. Because it acts as your local CI/CD simulation, it needs to perfectly mirror the actual deployment pipeline. Right now, it handles the salt injection but entirely skips the new ci flag injection step we just built. To keep everything synchronized, we need to add the Python script execution to the simulation and ensure export_presets.cfg is properly backed up and restored so your local repository stays clean.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Owner
Author
Addressed |
Owner
Author
The Unscoped sed Call (browser_test.yml)In test_injection.sh, we correctly scoped the plugin-disabling command to just the [editor_plugins] block. But in our GitHub Actions workflow, it is still a greedy regex that will disable any array named enabled= anywhere in project.godot. The FixUpdate lines 25-26 in .github/workflows/browser_test.yml to match the scoped version from our shell script: - name: "Disable Editor Plugins (Prevent Headless Crash)"
run: |
# Prevents Signal 11 crashes during headless export on Ubuntu runners
sed -i '/^\[editor_plugins\]/,/^\[/ s/^enabled=PackedStringArray.*/enabled=PackedStringArray()/' project.godot |
In test_injection.sh, you correctly scoped the plugin-disabling command to just the [editor_plugins] block. But in your GitHub Actions workflow, it is still a greedy regex that will disable any array named enabled= anywhere in project.godot. The Fix: Update lines 25-26 in .github/workflows/browser_test.yml to match the scoped version from your shell script
Owner
Author
macOS / BSD
|
macOS / BSD sed compatibility (test_injection.sh & inject_salt.sh) Both scripts are currently hardcoded to use sed -i "...", which is standard for Linux/GNU but will fail with an error on macOS (which requires sed -i '' "..."). To make these scripts portable for any developer testing locally, we need to add a small wrapper.
…y secret to reduce brittleness suggestion (testing): Relax assertion on exact error message for empty secret to reduce brittleness Asserting on the exact string in stdout makes this test fragile to harmless wording changes in the script. Instead, rely on behavior: assert the non‑zero return code, that the file content remains unchanged, and optionally that some error output is produced, without checking this specific phrase.
Owner
Author
Addressed. |
Owner
Author
I just read through the exact Our script defines the variables at the top: GODOT_CMD="godot"
RAW_SECRET='T3st_S@lt!_2026#"\'
export PRODUCTION_SALT="$RAW_SECRET"
# Cross-platform sed for in-place editing (macOS vs Linux)
sedi() {
if [ "$(uname)" = "Darwin" ]; then
sed -i '' "$@"
else
sed -i "$@"
fi
}
And then immediately jumps into: echo "=========================================="
echo " Starting Local CI/CD Simulation"
echo "=========================================="
if [ ! -f "scripts/core/globals.gd" ] || [ ! -f "project.godot" ] || [ ! -f "export_presets.cfg" ]; then
echo "❌ ERROR: Required project files not found!"
exit 1
fi
It checks if the files exist ( To finally resolve this issue, we need to paste this exact block into echo "=========================================="
echo " Checking System Dependencies"
echo "=========================================="
for cmd in $GODOT_CMD python3 bash sed; do
if ! command -v $cmd >/dev/null 2>&1; then
echo "❌ ERROR: Required command '$cmd' is not installed or not in your PATH."
exit 1
fi
done
|
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
.github/scripts/inject_salt.shyou buildGODOT_ESCAPED/SED_ESCAPEDand definesedi, but never actually invokesedto replace the placeholder in the target file—right now the script logs and exits without modifying the file; add the in‑place replacement step. workspace/test_injection.shdefines thesedihelper function twice with identical implementations; drop the duplicate definition to avoid confusion and future drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.github/scripts/inject_salt.sh` you build `GODOT_ESCAPED`/`SED_ESCAPED` and define `sedi`, but never actually invoke `sed` to replace the placeholder in the target file—right now the script logs and exits without modifying the file; add the in‑place replacement step.
- `workspace/test_injection.sh` defines the `sedi` helper function twice with identical implementations; drop the duplicate definition to avoid confusion and future drift.
## Individual Comments
### Comment 1
<location path="tests/ci/test_ci_flag_injection.py" line_range="164-167" />
<code_context>
+ assert content.count('custom_features="ci"') == 1
+
+
+def test_inject_ci_flag_multiple_presets(repo_tmp):
+ """Verifies that the script updates all available presets in the file."""
+ root = Path(repo_tmp)
+ config = root / "export_presets.cfg"
+
+ multi_preset_content = (
+ "[preset.0]\n" 'name="Web"\n\n' "[preset.1]\n" 'custom_features=""\n'
+ )
+ config.write_text(multi_preset_content, encoding="utf-8")
+
+ result = run_ci_injection(root)
+ assert result.returncode == 0
+
+ content = config.read_text(encoding="utf-8")
+
+ # Both preset sections should contain the CI feature flag and remain intact
+ assert "[preset.0]" in content
+ assert "[preset.1]" in content
+ assert content.count('custom_features="ci"') == 2
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case where `custom_features` appears under `[preset.*.options]` to assert it is not touched by the injector.
Given the nuance that only root preset headers should be updated (not `[preset.X.options]` sections), please add a test with `custom_features` inside a `[preset.0.options]` block and assert it remains unchanged, while the root `[preset.0]` entry is still injected/normalized. This will lock in the scoping behavior and guard against future regex changes affecting option-level keys.
```suggestion
content = config.read_text(encoding="utf-8")
assert content.count('custom_features="ci"') == 1
def test_inject_ci_flag_does_not_touch_preset_options(repo_tmp):
"""Ensures option-level `custom_features` keys are not modified."""
root = Path(repo_tmp)
config = root / "export_presets.cfg"
preset_with_options = (
"[preset.0]\n"
'custom_features=""\n\n'
"[preset.0.options]\n"
'custom_features="foo,bar"\n'
)
config.write_text(preset_with_options, encoding="utf-8")
result = run_ci_injection(root)
assert result.returncode == 0
updated_content = config.read_text(encoding="utf-8")
# Root preset gets the CI flag
assert "[preset.0]" in updated_content
assert 'custom_features="ci"' in updated_content
# Options section remains unchanged
assert "[preset.0.options]" in updated_content
assert 'custom_features="foo,bar"' in updated_content
# Ensure we only injected at the root preset level
assert updated_content.count('custom_features="ci"') == 1
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Modified the test to assert that invalid custom_features keys inside option blocks are actively scrubbed, maintaining Godot configuration integrity.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
Owner
Author
We can completely ignore that feedback—the bot is hallucinating or reviewing an older version of our file! If we look at the exact At the very bottom of the file (lines 36-37), we have: # 3. Replace the safe placeholder with the real secret
sedi "s|\"CI_INJECT_SALT_HERE\"|\"$SED_ESCAPED\"|g" "$TARGET_FILE"
We are properly invoking the Sometimes these AI code review bots (like Sourcery or DeepSource) cache older commits or lose context when you make rapid changes across multiple files. We have already fully addressed this, and the script is completely functional. We can dismiss that specific comment on GitHub! |
Owner
Author
The bot caught a classic copy-paste glitch! If we look at the test_injection.sh file we just uploaded, starting right around line 24, we accidentally pasted the exact same sedi() function twice in a row. While bash will technically just let the second one overwrite the first one and run fine, it is messy and confusing for anyone reading the script. Furthermore, the system dependency check we talked about in the last step is still missing from this file! Let's clean up both issues in one shot. The FixReplace everything from line 20 (GODOT_CMD="godot") down to the Starting Local CI/CD Simulation echo block with this perfectly clean, single-function version that also includes your dependency guards |
The bot caught a classic copy-paste glitch! If you look at the test_injection.sh file you just uploaded, starting right around line 24, you accidentally pasted the exact same sedi() function twice in a row. While bash will technically just let the second one overwrite the first one and run fine, it is messy and confusing for anyone reading the script. Furthermore, the system dependency check we talked about in the last step is still missing from this file! Let's clean up both issues in one shot. The Fix Replace everything from line 20 (GODOT_CMD="godot") down to the Starting Local CI/CD Simulation echo block with this perfectly clean, single-function version that also includes your dependency guards
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- You now have multiple places that manipulate editor plugins and export presets (deploy_to_itch.yml, browser_test.yml, workspace/test_injection.sh, inject_ci_flag.py); consider consolidating this logic (or at least reusing the Python helper consistently) to avoid future drift between CI and local workflows.
- The
sedihelper function for cross‑platformsedappears in bothinject_salt.shandworkspace/test_injection.sh; factoring this into a single shared implementation (or at least keeping one canonical version) would make it easier to maintain escaping behavior reliably. - In
globals.gdyou rely on the literal placeholderCI_INJECT_SALT_HEREto decide whether to crash in production; if that string ever changes in one place and not the other you’ll get subtle behavior changes, so it may be safer to centralize the sentinel value (e.g., a const) or assert onGlobals.save_encryption_pass.is_empty()instead of a string literal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You now have multiple places that manipulate editor plugins and export presets (deploy_to_itch.yml, browser_test.yml, workspace/test_injection.sh, inject_ci_flag.py); consider consolidating this logic (or at least reusing the Python helper consistently) to avoid future drift between CI and local workflows.
- The `sedi` helper function for cross‑platform `sed` appears in both `inject_salt.sh` and `workspace/test_injection.sh`; factoring this into a single shared implementation (or at least keeping one canonical version) would make it easier to maintain escaping behavior reliably.
- In `globals.gd` you rely on the literal placeholder `CI_INJECT_SALT_HERE` to decide whether to crash in production; if that string ever changes in one place and not the other you’ll get subtle behavior changes, so it may be safer to centralize the sentinel value (e.g., a const) or assert on `Globals.save_encryption_pass.is_empty()` instead of a string literal.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
We must absolutely reject this suggestion because the bot completely failed to understand how your pipeline actually works, and following its advice will literally destroy your game. 🚨 Reject Feedback #3 (
|
Owner
Author
Zero new bugs is a massive victory! That means the core logic, the Python scripts, and the shell pipelines are all perfectly stable. At this point, Sourcery is just playing "architect" and nitpicking your project structure. ✅ Resolve Feedbacks #1 & #2 (The Clean Refactor)The bot is right that Step A: Create a shared utils script Create a new file at #!/bin/bash
# .github/scripts/ci_utils.sh
# Cross-platform sed for in-place editing (macOS vs Linux)
sedi() {
if [ "$(uname)" = "Darwin" ]; then
sed -i '' "$@"
else
sed -i "$@"
fi
}
# Shared function to safely disable Godot editor plugins before headless exports
disable_editor_plugins() {
local target_file="${1:-project.godot}"
echo "🔌 Disabling editor plugins in $target_file to prevent headless crashes..."
sedi '/^\[editor_plugins\]/,/^\[/ s/^enabled=PackedStringArray.*/enabled=PackedStringArray()/' "$target_file"
}
**Step B: Clean up Remove the # Source the shared utilities
source "$(dirname "$0")/ci_utils.sh"
Step C: Clean up `test_injection.sh Remove the # ... (around line 21, right after your dependency check) ...
source .github/scripts/ci_utils.sh
echo "=========================================="
echo " Starting Local CI/CD Simulation"
echo "=========================================="
# ... (skip down to the plugin section) ...
# Disable editor plugins using the shared function
disable_editor_plugins "project.godot"
Step D: Update your YAML workflows Finally, anywhere in our GitHub Actions (like - name: "Disable Editor Plugins (Prevent Headless Crash)"
run: |
source .github/scripts/ci_utils.sh
disable_editor_plugins "project.godot"
By doing this, we've centralized our cross-platform logic and our Godot environment prep into one neat file. If we ever need to change how plugins are disabled, we only have to change it in |
Introduce .github/scripts/ci_utils.sh with cross-platform sedi and disable_editor_plugins helpers. Update inject_salt.sh to source the shared utilities instead of redefining sedi, and refactor workspace/test_injection.sh to source the utilities, add dependency checks and improved logging, and call disable_editor_plugins to avoid headless Godot crashes. This centralizes CI-related functions and ensures consistent behavior across macOS/Linux environments.
Replace inline sed snippets with sourcing .github/scripts/ci_utils.sh and calling disable_editor_plugins("project.godot") in both browser_test.yml and deploy_to_itch.yml. This centralizes the editor-plugin disabling logic, removes duplicated sed commands, and makes the behavior reusable and easier to maintain across CI workflows.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
.github/scripts/inject_salt.shyou currently computeGODOT_ESCAPED/SED_ESCAPEDand sourceci_utils.shbut never actually runsed/sedito replace theCI_INJECT_SALT_HEREplaceholder, so the script is effectively a no-op; add the in-place substitution step to perform the intended injection. - In
workspace/test_injection.shtheRAW_SECRETassignment uses a single-quoted string containing an unescaped single-quote/backslash sequence ('T3st_S@lt!_2026#"\'), which will break shell parsing; switch to double quotes or escape the embedded quote properly so the variable is set as intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.github/scripts/inject_salt.sh` you currently compute `GODOT_ESCAPED`/`SED_ESCAPED` and source `ci_utils.sh` but never actually run `sed`/`sedi` to replace the `CI_INJECT_SALT_HERE` placeholder, so the script is effectively a no-op; add the in-place substitution step to perform the intended injection.
- In `workspace/test_injection.sh` the `RAW_SECRET` assignment uses a single-quoted string containing an unescaped single-quote/backslash sequence (`'T3st_S@lt!_2026#"\'`), which will break shell parsing; switch to double quotes or escape the embedded quote properly so the variable is set as intended.
## Individual Comments
### Comment 1
<location path="tests/ci/test_salt_injection.py" line_range="106-113" />
<code_context>
+ ), f"Artifact pollution detected. Unexpected files found: {unexpected}"
+
+
+def test_injection_multiple_placeholders(repo_tmp):
+ """Ensures global replacement updates all occurrences, leaving no partial leftovers."""
+ dummy_rel = f"{repo_tmp}/dummy_multi.gd"
+ dummy_abs = Path(PROJECT_ROOT) / dummy_rel
+
+ dummy_abs.write_text(
+ "extends Node\n"
+ 'var security = {"save_salt": "CI_INJECT_SALT_HERE"}\n'
+ 'var another = {"save_salt": "CI_INJECT_SALT_HERE"}\n',
+ encoding="utf-8",
+ )
+
+ result = run_injection(dummy_rel, "multi-placeholder-salt")
+
+ assert result.returncode == 0
+ content = dummy_abs.read_text(encoding="utf-8")
+ assert "CI_INJECT_SALT_HERE" not in content
+ assert content.count("multi-placeholder-salt") == 2
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a case where the placeholder appears multiple times on the same line
The current test covers multiple occurrences across lines. To better validate the `sed` behavior, please also add a case where `CI_INJECT_SALT_HERE` appears multiple times on a single line (e.g., a long JSON-like literal) to confirm it performs a truly global substitution per line.
```suggestion
result = run_injection(dummy_rel, "multi-placeholder-salt")
assert result.returncode == 0
content = dummy_abs.read_text(encoding="utf-8")
assert "CI_INJECT_SALT_HERE" not in content
assert content.count("multi-placeholder-salt") == 2
def test_injection_multiple_placeholders_same_line(repo_tmp):
"""Ensures sed performs a truly global substitution when placeholders repeat on one line."""
dummy_rel = f"{repo_tmp}/dummy_multi_same_line.gd"
dummy_abs = Path(PROJECT_ROOT) / dummy_rel
dummy_abs.write_text(
"extends Node\n"
'var security = {"salt1": "CI_INJECT_SALT_HERE", "salt2": "CI_INJECT_SALT_HERE"}\n',
encoding="utf-8",
)
result = run_injection(dummy_rel, "multi-placeholder-salt")
assert result.returncode == 0
content = dummy_abs.read_text(encoding="utf-8")
assert "CI_INJECT_SALT_HERE" not in content
# Both placeholders on the same line must be replaced
assert content.count("multi-placeholder-salt") == 2
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+106
to
+113
| result = run_injection(dummy_rel, "multi-placeholder-salt") | ||
|
|
||
| assert result.returncode == 0 | ||
| content = dummy_abs.read_text(encoding="utf-8") | ||
| assert "CI_INJECT_SALT_HERE" not in content | ||
| assert content.count("multi-placeholder-salt") == 2 | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a case where the placeholder appears multiple times on the same line
The current test covers multiple occurrences across lines. To better validate the sed behavior, please also add a case where CI_INJECT_SALT_HERE appears multiple times on a single line (e.g., a long JSON-like literal) to confirm it performs a truly global substitution per line.
Suggested change
| result = run_injection(dummy_rel, "multi-placeholder-salt") | |
| assert result.returncode == 0 | |
| content = dummy_abs.read_text(encoding="utf-8") | |
| assert "CI_INJECT_SALT_HERE" not in content | |
| assert content.count("multi-placeholder-salt") == 2 | |
| result = run_injection(dummy_rel, "multi-placeholder-salt") | |
| assert result.returncode == 0 | |
| content = dummy_abs.read_text(encoding="utf-8") | |
| assert "CI_INJECT_SALT_HERE" not in content | |
| assert content.count("multi-placeholder-salt") == 2 | |
| def test_injection_multiple_placeholders_same_line(repo_tmp): | |
| """Ensures sed performs a truly global substitution when placeholders repeat on one line.""" | |
| dummy_rel = f"{repo_tmp}/dummy_multi_same_line.gd" | |
| dummy_abs = Path(PROJECT_ROOT) / dummy_rel | |
| dummy_abs.write_text( | |
| "extends Node\n" | |
| 'var security = {"salt1": "CI_INJECT_SALT_HERE", "salt2": "CI_INJECT_SALT_HERE"}\n', | |
| encoding="utf-8", | |
| ) | |
| result = run_injection(dummy_rel, "multi-placeholder-salt") | |
| assert result.returncode == 0 | |
| content = dummy_abs.read_text(encoding="utf-8") | |
| assert "CI_INJECT_SALT_HERE" not in content | |
| # Both placeholders on the same line must be replaced | |
| assert content.count("multi-placeholder-salt") == 2 |
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
1 participant
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.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Switch encryption salt handling from project settings to CI-injected GDScript bytecode, hardening web exports and improving failure recovery.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests:
Summary by Sourcery
Switch save encryption to use a CI-injected GDScript salt and reinforce web exports and tests against crashes and corrupted state.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Chores:
Summary by CodeRabbit
Chores
Tests
Bug Fixes