Conversation
Issue #285, "[FEATURE] Convert Hard-Coded Globals to Godot Resource," focuses on migrating static configuration and game settings from the current globals.gd singleton into a more modular Godot Resource system.
Line 7 exports an unconstrained int. The code directly indexes LogLevel.keys() with this value in globals.gd (lines 42, 126), which will crash if the value falls outside 0–4. Enforce bounds in the property itself.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…//github.com/ikostan/SkyLockAssault into convert-hard-coded-globals-to-godot-resource
Update trivy.yml
Update codeql.yml
Update trivy.yml
Add a reusable Trivy workflow (.github/workflows/_trivy.yml) callable via workflow_call that runs the aquasecurity/trivy-action (pinned SHA), outputs SARIF, and uploads results to the GitHub Security tab. Update the existing .github/workflows/trivy.yml to use actions/checkout@v4, remove the explicit ref/persist-credentials, and replace the Trivy action with an explicit install of Trivy v0.48.3 and a `trivy fs` scan that writes trivy-results.sarif. The SARIF upload step is kept to ensure results appear in the Security tab. These changes provide a centralized reusable workflow while keeping a standalone CLI-based scan option.
Add reusable Trivy workflow and update scan
Maintenance
Update trivy.yml
Update trivy.yml
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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 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 |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
Contributor
Reviewer's GuideThis PR migrates several global settings (log level, difficulty, remap prompts, and scene references) into a new GameSettingsResource, updates gameplay and UI scripts plus tests to use the resource, and refreshes CI/security tooling and static analysis configuration. Sequence diagram for Globals.load_key_mapping using GameSettingsResourcesequenceDiagram
actor Player
participant Menu as MenuNode
participant Globals
participant Settings as GameSettingsResource
participant Root as SceneTreeRoot
participant KMScene as KeyMappingSceneInstance
Player->>Menu: Trigger key mapping
Menu->>Globals: load_key_mapping(menu_to_hide)
activate Globals
Globals->>Menu: hide menu_to_hide
Globals->>Globals: hide_other_menus_and_pause_video()
Globals->>Settings: check key_mapping_scene
alt key_mapping_scene is null
Globals->>Globals: log_message(Error: Key mapping scene not configured., ERROR)
alt hidden_menus not empty
Globals->>Globals: prev_menu = hidden_menus.pop_back()
Globals->>prev_menu: set visible true
end
Globals-->>Menu: return
else key_mapping_scene configured
Globals->>Settings: key_mapping_scene.instantiate()
Settings-->>Globals: CanvasLayer instance
Globals->>Root: add_child(km_instance)
Globals-->>Menu: return
end
deactivate Globals
Sequence diagram for Globals.load_options using settings.options_scenesequenceDiagram
actor Player
participant Menu as MenuNode
participant Globals
participant Settings as GameSettingsResource
participant Root as SceneTreeRoot
participant Options as OptionsMenuInstance
Player->>Menu: Open options menu
Menu->>Globals: load_options(menu_to_hide)
activate Globals
Globals->>Globals: push menu_to_hide into hidden_menus
Globals->>Menu: set visible false
Globals->>Settings: check options_scene
alt options_scene is null
Globals->>Globals: log_message(Failed to open options scene., ERROR)
Globals-->>Menu: return
else options_scene configured
Globals->>Globals: options_open = true
Globals->>Settings: options_scene.instantiate()
Settings-->>Globals: CanvasLayer instance
Globals->>Globals: assign to options_instance
alt options_instance is null
Globals->>Globals: log_message(Failed to instantiate options scene, ERROR)
Globals->>Globals: options_open = false
Globals-->>Menu: return
else
Globals->>Root: add_child(options_instance)
Globals->>Globals: connect_teardown_signals()
Globals-->>Menu: return
end
end
deactivate Globals
Class diagram for new GameSettingsResource and updated Globals usageclassDiagram
class GameSettingsResource {
+int current_log_level
+bool enable_debug_logging
+float difficulty
+String remap_prompt_keyboard
+String remap_prompt_gamepad
+PackedScene key_mapping_scene
+PackedScene options_scene
-float _difficulty
}
class Globals {
+GameSettingsResource settings
+CanvasLayer options_instance
+Array~Node~ hidden_menus
+bool options_open
+String previous_scene
+String next_scene
+String current_input_device
+void _ready()
+void _load_settings(path)
+void _save_settings(path)
+void load_options(menu_to_hide)
+void load_key_mapping(menu_to_hide)
+void log_message(message, level)
}
class AdvancedSettings {
+void _ready()
+void _on_advanced_reset_button_pressed()
+void _on_log_level_item_selected(index)
}
class GameplaySettings {
+void _ready()
+void _on_difficulty_value_changed(value)
}
class InputRemapButton {
+bool listening
+int current_device
+void _ready()
+void _on_pressed()
}
class Bullet {
+float fire_rate
+Timer timer
+bool can_fire
+void fire()
}
class Player {
+Dictionary speed
+Dictionary fuel
+float MAX_SPEED
+void _on_fuel_timer_timeout()
}
Globals --> GameSettingsResource : owns_settings
AdvancedSettings ..> GameSettingsResource : reads_writes_logging
GameplaySettings ..> GameSettingsResource : reads_writes_difficulty
InputRemapButton ..> GameSettingsResource : reads_remap_prompts
Bullet ..> GameSettingsResource : reads_difficulty
Player ..> GameSettingsResource : reads_difficulty
AdvancedSettings ..> Globals : calls_log_and_save
GameplaySettings ..> Globals : calls_log_and_save
InputRemapButton ..> Globals : calls_log_message
Bullet ..> Globals : uses_logging
Player ..> Globals : uses_logging
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 found 4 issues, and left some high level feedback:
- In
GameSettingsResource.difficultythe setter callsGlobals.log_message, which couples the data container back to the autoload and may break usage in the editor or in isolation; consider removing the logging from the Resource and letting the caller (e.g.,_load_settings) handle any validation/logging instead. - The CI changes switch
actions/checkoutto@v6andaquasecurity/trivy-actionto@master, which removes the previous pinning to immutable SHAs; to avoid supply-chain risk, please restore SHA pinning (or at least a fixed version tag) for these actions. - Several tests (e.g.,
test_globals_resource.gd) assignGlobals.settings = GameSettingsResource.new(), which mutates global state for the whole test run; it would be safer to snapshot and restore the originalGlobals.settingsin setup/teardown to avoid subtle cross-test interactions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GameSettingsResource.difficulty` the setter calls `Globals.log_message`, which couples the data container back to the autoload and may break usage in the editor or in isolation; consider removing the logging from the Resource and letting the caller (e.g., `_load_settings`) handle any validation/logging instead.
- The CI changes switch `actions/checkout` to `@v6` and `aquasecurity/trivy-action` to `@master`, which removes the previous pinning to immutable SHAs; to avoid supply-chain risk, please restore SHA pinning (or at least a fixed version tag) for these actions.
- Several tests (e.g., `test_globals_resource.gd`) assign `Globals.settings = GameSettingsResource.new()`, which mutates global state for the whole test run; it would be safer to snapshot and restore the original `Globals.settings` in setup/teardown to avoid subtle cross-test interactions.
## Individual Comments
### Comment 1
<location path="scripts/globals.gd" line_range="22-25" />
<code_context>
+# @export var difficulty: float = 1.0 # Multiplier: 1.0=Normal, <1=Easy, >1=Hard
+
+# Add the resource reference here
+@export var settings: GameSettingsResource = preload("res://settings/default_settings.tres")
# In globals.gd (add after @export vars)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard against `settings` being null or misconfigured to avoid hard-to-debug crashes.
`settings` is dereferenced in `_ready` and `log_message` assuming it’s always a valid `GameSettingsResource`. If it’s cleared or swapped in the inspector, this will cause runtime crashes. Either enforce non-null once at startup with a clear assertion/error, or centralize access through a `get_settings()` helper that provides a safe default or logs a clear error instead of crashing.
Suggested implementation:
```
# Add the resource reference here
const DEFAULT_SETTINGS_RESOURCE: GameSettingsResource = preload("res://settings/default_settings.tres")
@export var settings: GameSettingsResource = DEFAULT_SETTINGS_RESOURCE
func get_settings() -> GameSettingsResource:
# Centralized, safe access to game settings.
if settings == null:
push_error("Globals.settings is null. Falling back to DEFAULT_SETTINGS_RESOURCE.")
settings = DEFAULT_SETTINGS_RESOURCE
elif not settings is GameSettingsResource:
push_error("Globals.settings is not a GameSettingsResource. Resetting to DEFAULT_SETTINGS_RESOURCE.")
settings = DEFAULT_SETTINGS_RESOURCE
return settings
# In globals.gd (add after @export vars)
var options_instance: CanvasLayer = null
var hidden_menus: Array[Node] = []
var options_open: bool = false
## Key Mapping scene for direct loading from warning dialogs.
# var key_mapping_scene: PackedScene = preload("res://scenes/key_mapping_menu.tscn")
var previous_scene: String = "res://scenes/main_menu.tscn" # Default fallback
# var options_scene: PackedScene = preload("res://scenes/options_menu.tscn")
var next_scene: String = "" # Path to the next scene to load via loading screen.
```
To fully implement the safety you requested, update all direct uses of `settings` in this file (and anywhere else it’s treated as a global) to go through the helper:
- In `_ready`, replace any `settings.<property>` with `get_settings().<property>`.
- In `log_message` (and any other logging/utility functions), replace `settings.<property>` with `get_settings().<property>`.
This ensures that if `settings` is cleared or misconfigured in the inspector, the code logs a clear error and uses a safe default instead of crashing.
</issue_to_address>
### Comment 2
<location path="scripts/game_settings_resource.gd" line_range="22-29" />
<code_context>
+# Multiplier: 1.0=Normal, <1=Easy, >1=Hard
+# In globals.gd, change the difficulty variable in the Resource script:
+# game_settings_resource.gd
+@export var difficulty: float = 1.0:
+ set(value):
+ if value < 0.5 or value > 2.0:
+ Globals.log_message(
+ "Invalid difficulty loaded (" + str(value) + ") - clamping to valid range.",
+ Globals.LogLevel.WARNING
+ )
+ _difficulty = clamp(value, 0.5, 2.0)
+ get:
+ return _difficulty
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling `Globals.log_message` from the Resource setter to prevent tight coupling and editor-time errors.
Because this Resource’s setter calls `Globals.log_message`, it now depends on the `Globals` autoload being present. That can throw errors when editing in the inspector or using this Resource in tools before `Globals` is initialized, and introduces an unnecessary dependency from data to global logic. Keep the clamping here but move logging elsewhere (e.g. via a signal or status flag that `Globals` listens to), or at least guard with an editor/runtime check and ensure `Globals` exists before calling it.
</issue_to_address>
### Comment 3
<location path=".github/workflows/trivy.yml" line_range="15-23" />
<code_context>
steps:
- name: "Checkout repository"
- uses: "actions/checkout@v6.0.1"
+ uses: "actions/checkout@v6"
</code_context>
<issue_to_address>
**🚨 issue (security):** Pin GitHub Actions to immutable SHAs instead of tags or `master` to avoid supply-chain risk.
This now uses `actions/checkout@v6` (and still uses `aquasecurity/trivy-action@master`), so the workflow’s behavior can change without any code changes here. Please pin both actions to specific commit SHAs (optionally with a comment noting the version) to keep the scans reproducible and auditable.
</issue_to_address>
### Comment 4
<location path=".github/workflows/codeql.yml" line_range="26-27" />
<code_context>
steps:
- name: "Checkout repository"
- uses: "actions/checkout@v6.0.1"
+ uses: "actions/checkout@v6"
+ with:
+ # This explicitly tells the runner to use the branch that triggered the workflow
</code_context>
<issue_to_address>
**🚨 issue (security):** Use a commit SHA instead of a version tag for `actions/checkout` in security-related workflows.
For security-sensitive jobs like CodeQL, please pin `actions/checkout` to a specific commit SHA instead of `@v6`, per GitHub’s hardening guidance. You can add a comment beside the SHA to document which version it represents.
</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
+22
to
25
| @export var settings: GameSettingsResource = preload("res://settings/default_settings.tres") | ||
|
|
||
| # In globals.gd (add after @export vars) | ||
| var options_instance: CanvasLayer = null |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Guard against settings being null or misconfigured to avoid hard-to-debug crashes.
settings is dereferenced in _ready and log_message assuming it’s always a valid GameSettingsResource. If it’s cleared or swapped in the inspector, this will cause runtime crashes. Either enforce non-null once at startup with a clear assertion/error, or centralize access through a get_settings() helper that provides a safe default or logs a clear error instead of crashing.
Suggested implementation:
# Add the resource reference here
const DEFAULT_SETTINGS_RESOURCE: GameSettingsResource = preload("res://settings/default_settings.tres")
@export var settings: GameSettingsResource = DEFAULT_SETTINGS_RESOURCE
func get_settings() -> GameSettingsResource:
# Centralized, safe access to game settings.
if settings == null:
push_error("Globals.settings is null. Falling back to DEFAULT_SETTINGS_RESOURCE.")
settings = DEFAULT_SETTINGS_RESOURCE
elif not settings is GameSettingsResource:
push_error("Globals.settings is not a GameSettingsResource. Resetting to DEFAULT_SETTINGS_RESOURCE.")
settings = DEFAULT_SETTINGS_RESOURCE
return settings
# In globals.gd (add after @export vars)
var options_instance: CanvasLayer = null
var hidden_menus: Array[Node] = []
var options_open: bool = false
## Key Mapping scene for direct loading from warning dialogs.
# var key_mapping_scene: PackedScene = preload("res://scenes/key_mapping_menu.tscn")
var previous_scene: String = "res://scenes/main_menu.tscn" # Default fallback
# var options_scene: PackedScene = preload("res://scenes/options_menu.tscn")
var next_scene: String = "" # Path to the next scene to load via loading screen.
To fully implement the safety you requested, update all direct uses of settings in this file (and anywhere else it’s treated as a global) to go through the helper:
- In
_ready, replace anysettings.<property>withget_settings().<property>. - In
log_message(and any other logging/utility functions), replacesettings.<property>withget_settings().<property>.
This ensures that if settings is cleared or misconfigured in the inspector, the code logs a clear error and uses a safe default instead of crashing.
Comment on lines
+22
to
+29
| @export var difficulty: float = 1.0: | ||
| set(value): | ||
| if value < 0.5 or value > 2.0: | ||
| Globals.log_message( | ||
| "Invalid difficulty loaded (" + str(value) + ") - clamping to valid range.", | ||
| Globals.LogLevel.WARNING | ||
| ) | ||
| _difficulty = clamp(value, 0.5, 2.0) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid calling Globals.log_message from the Resource setter to prevent tight coupling and editor-time errors.
Because this Resource’s setter calls Globals.log_message, it now depends on the Globals autoload being present. That can throw errors when editing in the inspector or using this Resource in tools before Globals is initialized, and introduces an unnecessary dependency from data to global logic. Keep the clamping here but move logging elsewhere (e.g. via a signal or status flag that Globals listens to), or at least guard with an editor/runtime check and ensure Globals exists before calling it.
Comment on lines
+15
to
+23
| - name: "Checkout repository" | ||
| uses: "actions/checkout@v6" | ||
| with: | ||
| # This explicitly tells the runner to use the branch that triggered the workflow | ||
| ref: ${{ github.ref_name }} | ||
| fetch-depth: 0 | ||
| persist-credentials: false | ||
| - name: "Run Trivy FS Scan" | ||
| uses: "aquasecurity/trivy-action@master" |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Pin GitHub Actions to immutable SHAs instead of tags or master to avoid supply-chain risk.
This now uses actions/checkout@v6 (and still uses aquasecurity/trivy-action@master), so the workflow’s behavior can change without any code changes here. Please pin both actions to specific commit SHAs (optionally with a comment noting the version) to keep the scans reproducible and auditable.
Comment on lines
26
to
+27
| - name: "Checkout repository" | ||
| uses: "actions/checkout@v6.0.1" | ||
| uses: "actions/checkout@v6" |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Use a commit SHA instead of a version tag for actions/checkout in security-related workflows.
For security-sensitive jobs like CodeQL, please pin actions/checkout to a specific commit SHA instead of @v6, per GitHub’s hardening guidance. You can add a comment beside the SHA to document which version it represents.
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
2 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")
Summary by Sourcery
Migrate global gameplay and logging settings to a shared Resource object and update usage across code, tests, and CI configuration.
New Features:
Bug Fixes:
Enhancements:
CI:
Tests:
Chores: