Merge from master - #733
Merged
Merged
Conversation
…ps://github.com/ikostan/SkyLockAssault into encryption-key-generation-failure-in-web-export
The logic for injecting the ci feature flag into export_presets.cfg is duplicated in both workspace/run_browser_tests.sh and .github/workflows/browser_test.yml; extracting this into a small shared script would reduce drift and make future changes safer. To fix this, we extract that Python code out of the bash/yaml wrappers and put it into its own dedicated file. Then, we tell both the Bash script and the GitHub Action to just "run that file."
Force UTF-8 for stdout in .github/scripts/inject_ci_flag.py to avoid crashes on legacy Windows terminals; streamline the custom_features injection and harden error printing. Add tests/ci/conftest.py which provides a repo_tmp fixture that yields a project-root-relative POSIX temp dir for WSL compatibility. Update tests/ci/test_ci_flag_injection.py to call the injector via an absolute path, set PYTHONIOENCODING, use UTF-8 output capture, and improve failure assertions. Remove the duplicated repo_tmp fixture from test_salt_injection.py.
Change the injection strategy to a safe "strip-and-append" approach: remove any existing custom_features lines and then insert custom_features="ci" under every [preset.<n>.options] header. This makes the script idempotent, supports multiple presets, and ensures CI-only mode replaces local feature flags. Minor refactors: normalized UTF-8 stdout handling and small formatting tweaks. Tests expanded and hardened to check encoding, backup contents, idempotency, existing-flag handling, and multi-preset behavior.
Only create the .bak backup if it doesn't already exist to avoid overwriting the pristine original during idempotent runs. Tests updated: refine subprocess return type annotation, add/adjust idempotency assertions to verify backup stability and duplicated-flag protection, ensure both presets remain intact, and add a malformed-config test to confirm the script fails safely without corrupting files.
An object has been imported but is not used anywhere in the file. It should either be used or the import should be removed.
…ps://github.com/ikostan/SkyLockAssault into encryption-key-generation-failure-in-web-export
If there are backslashes in a docstring, please use raw strings.
Adjust inject_ci_flag.py to strip existing custom_features and inject the ci flag under root preset headers ([preset.N]) instead of the .options sections, using a multiline-safe regex and preserving backups only when absent. Update unit tests to reflect the new section layout and strengthen assertions/fixtures to avoid brittle newline issues; clarify a subprocess comment and fix malformed-config expectations. Also update run_browser_tests.sh to exclude the tests/ci directory from the Playwright pytest run so CI-only tests are not executed in browser runs.
Replace literal wait_for_timeout values in multiple tests with the TEST_TIMEOUT constant (and one with DEFAULT_TIMEOUT) to centralize and standardize timing behavior. Affected tests: difficulty_flow_test.py, load_main_menu_test.py, navigation_to_audio_test.py, reset_audio_flow_test.py, validate_clean_load_test.py, volume_sliders_mutes_test.py, and adjusted no_error_logs_test.py to use DEFAULT_TIMEOUT instead of a hardcoded 15000ms. This improves consistency and makes it easier to tune test timing globally.
…RLF (\r\n) line endings in export_presets.cfg suggestion (testing): Consider adding a test that explicitly covers CRLF (\r\n) line endings in export_presets.cfg Existing tests are newline-agnostic, but none exercise an export_presets.cfg that actually uses . Because inject_ci_flag.py uses multiline regexes and rewrites the file, please add a variant of an existing test (e.g. test_inject_ci_flag_standard or ..._multiple_presets) that seeds the config with CRLF via "\r\n".join([...]) to verify behavior on Windows-style line endings.
…ps://github.com/ikostan/SkyLockAssault into encryption-key-generation-failure-in-web-export
…ncryption key call may introduce noisy logs and minor information leakage.
🚨 suggestion (security): Logging the CI/test flag at INFO for every encryption key call may introduce noisy logs and minor information leakage.
Given this likely runs on every save/load, this INFO log could flood production logs and reveals whether the build has the ci feature. If it’s mainly for debugging, please lower it to DEBUG or guard it with OS.has_feature("debug") so it doesn’t expose environment details in normal logs.
…can raise an AttributeError in some environments.
issue (bug_risk): Accessing sys.stdout.encoding without a None check can raise an AttributeError in some environments.
In CI or when stdout is redirected, sys.stdout.encoding may be None, so calling .lower() will raise before the script runs. Consider guarding with something like enc = sys.stdout.encoding or "" before using enc.lower(), or wrap the reconfigure block in try/except to avoid hard failures in those environments.
Sourcery-AI is absolutely right here, and this is a great catch for CI stability.
When a script runs in an automated CI pipeline (like GitHub Actions), stdout is often piped directly into log files rather than an interactive terminal. When that happens, Python doesn't assign a default terminal encoding, causing sys.stdout.encoding to evaluate to None. Calling .lower() on None will throw a hard AttributeError and crash the pipeline before the script even starts doing its real work.
The fix is exactly what the bot suggested: we need to use a fallback string ("") if the encoding is None, and it's also smart to wrap the reconfigure step in a try/except block just in case the piped environment doesn't support reconfiguration.
…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.
Why the Compliance Bot is Rejecting Feature #570 The automated code compliance tracking system is blocking your Pull Request for two distinct reasons: Changeset Isolation (Missing Diff Summary): Your audio_manager.gd file already contains the entire high-performance caching, pooling, and hijacking implementation. However, because this code was written previously, it isn't part of your current git commit history/diff context for this PR. The bot checks the incoming code diff explicitly, sees the UI calling the method, but assumes the backend is missing because it isn't being modified in this file list. Missing Explicit Verification Suite: The feature request contains 6 specific validation criteria (caching, eviction, player pools, hijacking, failure suppression, and node tree constraints). To satisfy automated validation, these criteria must be covered by a dedicated unit test suite. Additionally, your audio_manager.gd line 143 currently lists a comment referencing (Issue #565). This typo prevents the tracker from linking the implementation to the correct issue descriptor (#570).
Why the Signal Leak Happens In Godot 4, a CheckButton emits its toggled signal whenever its checked status changes—even when modified programmatically via backend sync events, configuration resets, or WebBridge payload updates. Because your centralized _execute_bus_mute_toggle pipeline was playing the "check" sound effect unconditionally, any incoming automated change that adjusted a button's visual state would inadvertently force the UI to trigger a sound effect. This violated the core requirement that automated pipelines must execute in absolute, total silence.
…because of strict environmental path and isolation rules. Why the Automated Checks Failed Wrong File Location: The compliance script expects a brand-new, standalone validation module created under the test/unit/ directory tree. Right now, modifying existing files or putting them in other directories causes the linter to assume the module doesn't exist. Missing Test Matrix Links: The bot uses sequential string matching to read your test framework outputs. It explicitly scans for test methods targeting the unique criteria of the TC-AM-001 to TC-AM-014 tracking rules.
…values from sliders or fades. This branch only triggers on volume == 0.0. Depending on how sliders/fades quantize values, you may see very small non-zero values (e.g. 0.0001) instead of an exact 0.0, which would skip auto-mute and leave the bus effectively silent but unmuted. Consider using a small epsilon or a minimum threshold (e.g. volume <= 0.001) so near-zero values are handled consistently with zero.
Sourcery AI’s high-level feedback targets important code-hardening principles. Addressing these three points will significantly reduce the long-term maintenance overhead of our test suites.
To prevent false negatives or platform warnings inside strict headless CI environments, we should completely remove DirAccess.dir_exists_absolute() from your integrity test scripts. Calling DirAccess.open() directly acts as both the existence check and the initialization loop. If it returns null, the folder safely skips without a crash.
…set_button.gd Right now, test_audio_reset_button.gd uses exact float comparisons against snapped quantization steps (like 0.495, 0.792, and 0.99). If the layout slider's step value changes in the editor, your tests instantly break. To make the test suite completely resilient to layout or step changes, we change the exact equality assertions (assert_eq) to epsilon range assertions (assert_almost_eq).
…o null, which can hide configuration mistakes. Because callers silently receive null, focus/audio updates are skipped instead of clearly failing, which can obscure new or misconfigured buses during development. Consider adding an assert(false, ...) in the default branch (for debug builds) or at least logging a warning when bus_name is unknown, so missing mappings are caught early.
The _get_slider_for_bus and _get_mute_button_for_bus helpers assert on unknown bus names, which will hard-crash in production if a new bus is added but not wired; consider replacing the assert(false, ...) with a softer runtime check (e.g., logging and early-return) to avoid bringing down the UI in misconfigurations. Several of the new tests (e.g., SFX centralization and auto-mute suites) reach into AudioManager internals like _sfx_pool, _sfx_cache, and _missing_sfx_cache; you might want to introduce minimal public inspection/reset helpers for these concerns so tests don't depend on private fields and future refactors remain easier. Sourcery-AI is bringing up two completely valid points here that we should fix before pushing this branch across the final verification gate. The first point is a legitimate production stability risk. Since Godot automatically strips assert() calls out of exported release builds, our current fallback path would drop down to returning a null reference silently. The moment the calling function tries to use that null value, the UI will hard crash for the player. The second point is an encapsulation cleanup to keep our unit tests from breaking if we ever adjust how the sound effect pool behaves internally.
Add UI auto-mute with click SFX and expand audio integrity tests
ikostan
added this to the
Milestone 19: UI Audio Infrastructure & Centralized SFX Systems milestone
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
1 similar comment
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @ikostan, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jun 2, 2026 4:39a.m. | Review ↗ |
| JavaScript |  |
Jun 2, 2026 4:39a.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.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
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.
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")