Conversation
Move the high-level game state and manager scripts out of the root folder into core/ and managers/.
ikostan
added
documentation
Contributor
Reviewer's GuideReorganizes core Godot scripts into a new scripts/core and scripts/managers structure and updates tooling, tests, and docs to match, while tightening the CI pipeline for linting and automated unit/browser tests. Flow diagram for new Godot scripts folder structure and referencesflowchart LR
project[Project_root]
scripts_dir[scripts/]
core_dir[core/\n- globals.gd\n- main_scene.gd\n- settings.gd]
managers_dir[managers/\n- audio_manager.gd\n- resource_preloader.gd]
project --> scripts_dir
scripts_dir --> core_dir
scripts_dir --> managers_dir
project_godot[project.godot<br/>Autoloads]
main_scene_tscn[scenes/main_scene.tscn]
tests_gdunit4[test/gdunit4/*]
project --> project_godot
project --> main_scene_tscn
project --> tests_gdunit4
project_godot --> autoload_globals[Autoload_globals_from<br/>res://scripts/core/globals.gd]
project_godot --> autoload_settings[Autoload_settings_from<br/>res://scripts/core/settings.gd]
project_godot --> autoload_main_scene[Main_scene_reference_to<br/>res://scripts/core/main_scene.gd]
main_scene_tscn --> main_scene_script_ref[Node_script_reference_to<br/>res://scripts/core/main_scene.gd]
tests_gdunit4 --> test_globals_path[Update_paths_to<br/>res://scripts/core/globals.gd]
tests_gdunit4 --> test_audio_manager_path[Update_paths_to<br/>res://scripts/managers/audio_manager.gd]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughRewires script paths to Changes
Sequence Diagram(s)sequenceDiagram
participant CI as rgba(30,144,255,0.5) CI Runner
participant Repo as rgba(34,139,34,0.5) Repository
participant Lint as rgba(255,165,0,0.5) markdownlint (npx)
participant Gut as rgba(148,0,211,0.5) GUT (addons/gut)
participant Godot as rgba(220,20,60,0.5) Godot Engine
participant Playwright as rgba(0,128,128,0.5) Playwright tests
participant Web as rgba(75,0,130,0.5) Web Server
CI->>Repo: checkout
CI->>Lint: run `npx markdownlint-cli2@0.12.1 --yes` (excl. venv)
CI->>Gut: ensure `addons/gut` present (download if missing)
CI->>Godot: run GUT tests (`res://addons/gut/gut_cmdln.gd`)
CI->>Web: start web server for Playwright
CI->>Playwright: run end-to-end tests, produce `report.xml`
Playwright->>CI: if `report.xml` missing -> CI kills Web server and exits 1
CI->>Repo: report status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
run_pipeline.sh, removing thereport.xmlexistence warning means a missing or failed pytest run will now fail silently; consider restoring a clear message or explicit failure if the report is absent to make pipeline debugging easier. - The
npx --yes markdownlint-cli2@0.12.1invocation couples the pipeline to an on-demand network install of a specific version on every run; consider using a locally installed tool (or caching) and/or moving the version pin to a central config so CI remains faster and less dependent on external availability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_pipeline.sh`, removing the `report.xml` existence warning means a missing or failed pytest run will now fail silently; consider restoring a clear message or explicit failure if the report is absent to make pipeline debugging easier.
- The `npx --yes markdownlint-cli2@0.12.1` invocation couples the pipeline to an on-demand network install of a specific version on every run; consider using a locally installed tool (or caching) and/or moving the version pin to a central config so CI remains faster and less dependent on external availability.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 |  |
Apr 21, 2026 2:16a.m. | Review ↗ |
| JavaScript | |
Apr 21, 2026 2:16a.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.
ikostan
changed the title
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
files/docs/Platforms_for_Web_Deployment_Guide.md (2)
7-12:⚠️ Potential issue | 🟡 MinorJoin the CI/CD phrase back into the sentence.
Line 12 now renders as a standalone
+bullet, which breaks the paragraph flow.📝 Proposed documentation fix
-We're building **SkyLockAssault** — a totally free-to-play browser -game in **Godot v4.5** on **Windows 10 64-bit**. This is our learning -journey into game dev, so we're keeping everything practical, -low-friction, and focused on **automatic deploys** via GitHub Actions - -+ CI/CD where possible. +We're building **SkyLockAssault** — a totally free-to-play browser +game in **Godot v4.5** on **Windows 10 64-bit**. This is our learning +journey into game dev, so we're keeping everything practical, +low-friction, and focused on **automatic deploys** via GitHub Actions ++ CI/CD where possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Platforms_for_Web_Deployment_Guide.md` around lines 7 - 12, The paragraph was split by a stray plus creating a standalone "+" bullet; merge the line containing "CI/CD where possible." back into the previous sentence so the paragraph reads smoothly (e.g., combine "focused on **automatic deploys** via GitHub Actions" with "CI/CD where possible."), removing the extraneous "+" line; edit the text around the project description in Platforms_for_Web_Deployment_Guide.md to produce one continuous sentence mentioning automatic deploys and CI/CD.
79-101:⚠️ Potential issue | 🟡 MinorRemove Game Jolt from the manual phase or reclassify it consistently.
The guide lists Game Jolt as
100% Autoon Line 79, but also includes it in the manual/occasional Phase 3 list on Line 101.📝 Proposed consistency fix
-+ CrazyGames, Y8, GameDistribution, GamePix, Newgrounds, SoftGames, Game Jolt ++ CrazyGames, Y8, GameDistribution, GamePix, Newgrounds, SoftGames🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Platforms_for_Web_Deployment_Guide.md` around lines 79 - 101, The document currently lists "Game Jolt" in both the "100% Auto: itch.io, Poki, Viverse, Game Jolt" line and again under "Phase 3: Manual Once + Occasional" — remove the duplicate by deleting "Game Jolt" from the Phase 3 bullet list (or alternatively remove it from the 100% Auto line if you prefer it to be manual) so that "100% Auto: ... Game Jolt" and "Phase 3: Manual Once + Occasional" are consistent; update the Phase 3 list under the "Phase 3: Manual Once + Occasional" heading to no longer include "Game Jolt."files/docs/Development_Guide.md (1)
101-128:⚠️ Potential issue | 🟡 MinorUpdate the Core Files Reference paths for migrated singletons.
This section still points
GlobalsandSettingsto the old root script paths, while this PR updates the autoloads tores://scripts/core/....📝 Proposed documentation fix
-| **Logic Observer** | `res://scripts/globals.gd` | Connects to the resource to trigger centralized logging and `_save_settings()`. | +| **Logic Observer** | `res://scripts/core/globals.gd` | Connects to the resource to trigger centralized logging and `_save_settings()`. | ... -| **Persistence Settings** | `res://scripts/settings.gd` | Manages low-level `InputMap` serialization and legacy migration logic. | +| **Persistence Settings** | `res://scripts/core/settings.gd` | Manages low-level `InputMap` serialization and legacy migration logic. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@files/docs/Development_Guide.md` around lines 101 - 128, Update the Core Files Reference to use the migrated autoload paths: change the "Logic Observer" entry for Globals to point to res://scripts/core/globals.gd and change the "Persistence Settings" entry for Settings to res://scripts/core/settings.gd (leave other file entries like game_settings_resource.gd, gameplay_settings.gd, and advanced_settings.gd as-is); ensure the table rows referencing "Globals" and "Settings" use these new res://scripts/core/... paths so the documentation matches the autoload updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@run_pipeline.sh`:
- Around line 52-55: run_pipeline.sh currently runs GUT without installing it
and passes -gdir=res://test which ignores .gutconfig.json; update the GUT step
to first ensure GUT is installed (reuse the installation steps from
run_gut_unit_tests.sh or .github/workflows/gut_tests.yml to clone/install
addons/gut into the project) and change the godot invocation in the GUT Unit
Tests block to use -gdir=res://test/gut/ (or rely on
-gconfig=res://.gutconfig.json without overriding the configured dir) so only
the intended test directory is discovered.
---
Outside diff comments:
In `@files/docs/Development_Guide.md`:
- Around line 101-128: Update the Core Files Reference to use the migrated
autoload paths: change the "Logic Observer" entry for Globals to point to
res://scripts/core/globals.gd and change the "Persistence Settings" entry for
Settings to res://scripts/core/settings.gd (leave other file entries like
game_settings_resource.gd, gameplay_settings.gd, and advanced_settings.gd
as-is); ensure the table rows referencing "Globals" and "Settings" use these new
res://scripts/core/... paths so the documentation matches the autoload updates.
In `@files/docs/Platforms_for_Web_Deployment_Guide.md`:
- Around line 7-12: The paragraph was split by a stray plus creating a
standalone "+" bullet; merge the line containing "CI/CD where possible." back
into the previous sentence so the paragraph reads smoothly (e.g., combine
"focused on **automatic deploys** via GitHub Actions" with "CI/CD where
possible."), removing the extraneous "+" line; edit the text around the project
description in Platforms_for_Web_Deployment_Guide.md to produce one continuous
sentence mentioning automatic deploys and CI/CD.
- Around line 79-101: The document currently lists "Game Jolt" in both the "100%
Auto: itch.io, Poki, Viverse, Game Jolt" line and again under "Phase 3: Manual
Once + Occasional" — remove the duplicate by deleting "Game Jolt" from the Phase
3 bullet list (or alternatively remove it from the 100% Auto line if you prefer
it to be manual) so that "100% Auto: ... Game Jolt" and "Phase 3: Manual Once +
Occasional" are consistent; update the Phase 3 list under the "Phase 3: Manual
Once + Occasional" heading to no longer include "Game Jolt."
🪄 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: d40a1533-310f-4e29-9067-d7783d5f7119
📒 Files selected for processing (15)
.gitignoreREADME.mdfiles/docs/Development_Guide.mdfiles/docs/Platforms_for_Web_Deployment_Guide.mdproject.godotrun_gdunit4_unit_tests.shrun_pipeline.shscenes/main_scene.tscnscripts/core/globals.gdscripts/core/globals.gd.uidscripts/core/main_scene.gdscripts/core/main_scene.gd.uidscripts/core/settings.gdscripts/core/settings.gd.uidtest/gdunit4/test_globals.gd
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: GDUnit4 Unit Tests / unit-test
- GitHub Check: Sourcery review
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-10T00:07:55.427Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
test/gdunit4/test_globals.gdfiles/docs/Development_Guide.md
🪛 Shellcheck (0.11.0)
run_pipeline.sh
[warning] 9-9: PW_TIMEOUT appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (5)
.gitignore (1)
50-51: LGTM — generated project-scan artifacts are ignored.These entries fit the existing pattern of excluding local/generated tooling outputs from version control.
scenes/main_scene.tscn (1)
3-3: LGTM — scene script path matches the new core location.The
ExtResourceid is preserved, and the suppliedscripts/core/main_scene.gdcontext confirms the target script exists.project.godot (1)
27-28: LGTM — autoload paths are consistent with the core script migration.Both migrated singleton paths are backed by the supplied
scripts/core/globals.gdandscripts/core/settings.gdsnippets.test/gdunit4/test_globals.gd (1)
16-16: LGTM — the test now targets the migrated Globals script.This keeps the GDUnit4 coverage aligned with the new autoload path in
project.godot.README.md (1)
126-128: LGTM — Markdown spacing and bullet cleanup look correct.The heading spacing and Known Limitations bullets now render cleanly.
Also applies to: 256-290
The command assumes addons/gut/ already exists, but run_pipeline.sh does not install it. The existing run_gut_unit_tests.sh and .github/workflows/gut_tests.yml both include GUT installation logic that run_pipeline.sh lacks. Additionally, passing -gdir=res://test broadens test discovery beyond .gutconfig.json's configured res://test/gut/, so GUT may attempt to load unintended test suites (e.g., GDUnit4 tests under res://test/gdunit4/).
Move to managers/: audio_manager.gd, resource_preloader.gd.
RUN is a Dockerfile instruction and is not valid bash. Running this will cause a command not found error. It should be replaced with a conditional install bloc
This overrides the directory configured in .gutconfig.json (which points to res://test/gut/) and causes GUT to scan all subdirectories under res://test/, including GDUnit4 tests. These flags should be removed so -gconfig alone governs test discovery:
In run_pipeline.sh, removing the report.xml existence warning means a missing or failed pytest run will now fail silently; consider restoring a clear message or explicit failure if the report is absent to make pipeline debugging easier.
Owner
Author
Fixed |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
run_pipeline.sh (1)
9-9: Remove or wirePW_TIMEOUTinto the pytest command. Line 9 definesPW_TIMEOUT=10000, but the pytest command on line 96 doesn't use the--timeoutparameter, so the variable has no effect. Either add--timeout=$PW_TIMEOUTto the pytest invocation or remove the unused variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run_pipeline.sh` at line 9, The PW_TIMEOUT environment variable is set but not used; either remove the PW_TIMEOUT=10000 assignment or wire it into the pytest invocation by adding the pytest timeout flag. Locate the PW_TIMEOUT assignment and the pytest command (the script's pytest invocation) and either delete the PW_TIMEOUT line or modify the pytest command to include --timeout=$PW_TIMEOUT (or the appropriate flag your test runner expects) so the variable is actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@run_pipeline.sh`:
- Around line 40-41: Replace the silent copy of GDUnit4 (the line containing "cp
-r /project/addons/gdUnit4 $PROJECT_DIR/addons/ || true") with an explicit
existence check and fail/install behavior: test for the source directory
(/project/addons/gdUnit4) using [ -d ... ] or similar, and if missing either
exit with a non‑zero error and a clear message or fetch/install GDUnit4 from the
canonical source before copying; ensure the copy command runs only after the
check succeeds and remove the "|| true" so failures are not suppressed.
- Around line 81-89: The readiness check uses the loop index $i as a success
flag which causes a false failure when curl succeeds on the final iteration;
change the logic to track readiness explicitly by introducing a flag (e.g.,
READY=0) before the for-loop, set READY=1 immediately after a successful curl
(inside the if that echoes "Web server ready" and breaks), and after the loop
test READY (e.g., if [ "$READY" -ne 1 ] ...) to decide startup failure;
reference the loop variable i, the curl call, and SERVER_PORT when locating and
updating the code.
---
Nitpick comments:
In `@run_pipeline.sh`:
- Line 9: The PW_TIMEOUT environment variable is set but not used; either remove
the PW_TIMEOUT=10000 assignment or wire it into the pytest invocation by adding
the pytest timeout flag. Locate the PW_TIMEOUT assignment and the pytest command
(the script's pytest invocation) and either delete the PW_TIMEOUT line or modify
the pytest command to include --timeout=$PW_TIMEOUT (or the appropriate flag
your test runner expects) so the variable is actually applied.
🪄 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: 7bd8f29f-de21-49e8-80cb-b2e0d90e2de1
📒 Files selected for processing (8)
project.godotrun_pipeline.shscenes/main_scene.tscnscripts/managers/audio_manager.gdscripts/managers/audio_manager.gd.uidscripts/managers/resource_preloader.gdscripts/managers/resource_preloader.gd.uidtest/gdunit4/test_audio_manager.gd
🚧 Files skipped from review as they are similar to previous changes (1)
- project.godot
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-30T04:02:28.002Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:28.002Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, you MUST include all parameters the method takes, even default ones. GUT cannot fill in default values automatically. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is called as `eval(js_string)`, GUT records it as `eval(js_string, false)`, so the test assertion must be `.bind(js_string, false)` not just `.bind(js_string)`. This applies to project `SkyLockAssault` (ikostan/SkyLockAssault) in `test/gut/test_audio_web_bridge.gd`.
Applied to files:
run_pipeline.sh
🪛 Shellcheck (0.11.0)
run_pipeline.sh
[warning] 9-9: PW_TIMEOUT appears unused. Verify use (or export if used externally).
(SC2034)
[info] 89-89: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (5)
scenes/main_scene.tscn (1)
3-8: LGTM — scene script references were updated consistently.The
Node2DandResourcePreloaderscript paths now point to the newscripts/core/andscripts/managers/locations, and the attached script base types remain compatible with the scene nodes.run_pipeline.sh (3)
51-67: LGTM — GUT installation and discovery are scoped correctly now.The install block uses normal bash, and the test run relies on
.gutconfig.jsoninstead of broad-gdiroverrides.
99-117: LGTM — missing Playwright report is now treated as fatal.The new branch avoids silently completing without
report.xmland cleans up the web server before exiting.
30-31: Consider the context:run_pipeline.shis a local development tool, not a CI/CD gate.Line 31 includes
--fix, which allows the script to mutate files locally. While auto-fixing can be useful for local development (enabling developers to quickly fix linting issues), the concern about uncommitted changes being silently modified is valid only if this script is used in a CI/CD pipeline context.However,
run_pipeline.shis not invoked by the CI/CD pipeline. The actual CI lint gate (.github/workflows/lint_readme.yml) uses the GitHub ActionDavidAnson/markdownlint-cli2-actionwithfix: true, which does present the risk you described—allowing CI to pass with auto-fixed files that were never committed.If the concern is about preventing auto-fixing at the CI gate, focus on the workflow file instead. If
run_pipeline.shis intended as a local-only development tool, the--fixflag is appropriate for convenience.test/gdunit4/test_audio_manager.gd (1)
19-20: LGTM — the test now instantiates the relocated manager script.The updated path matches the new
scripts/managers/location, and the methods used by the test remain present onaudio_manager.gd.
… the environment The npx --yes markdownlint-cli2@0.12.1 invocation couples the pipeline to an on-demand network install of a specific version on every run; consider using a locally installed tool (or caching) and/or moving the version pin to a central config so CI remains faster and less dependent on external availability.
Owner
Author
Fixed |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
9-9: Remove or wire PW_TIMEOUT into the pytest command. Line 9 defines PW_TIMEOUT=10000, but the pytest command on line 96 doesn't use the --timeout parameter, so the variable has no effect. Either add --timeout=$PW_TIMEOUT to the pytest invocation or remove the unused variable.
Line 12 now renders as a standalone + bullet, which breaks the paragraph flow.
Remove Game Jolt from the manual phase or reclassify it consistently. The guide lists Game Jolt as 100% Auto on Line 79, but also includes it in the manual/occasional Phase 3 list on Line 101.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
run_pipeline.sh, consider avoiding downloading the GUT addon at runtime (wget/unzip from GitHub) as part of the CI pipeline, since this adds an external network dependency and potential flakiness—prefer checking it into the repo, using a submodule, or baking it into the Docker image instead. - The hard-coded GUT version URL (
v9.5.0.zip) and extracted directory name inrun_pipeline.shtightly couple the pipeline to a specific release structure; consider centralizing this version in a variable or using a more robust path check so future version bumps are less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_pipeline.sh`, consider avoiding downloading the GUT addon at runtime (wget/unzip from GitHub) as part of the CI pipeline, since this adds an external network dependency and potential flakiness—prefer checking it into the repo, using a submodule, or baking it into the Docker image instead.
- The hard-coded GUT version URL (`v9.5.0.zip`) and extracted directory name in `run_pipeline.sh` tightly couple the pipeline to a specific release structure; consider centralizing this version in a variable or using a more robust path check so future version bumps are less error-prone.
## Individual Comments
### Comment 1
<location path="run_pipeline.sh" line_range="9" />
<code_context>
EXPORT_DIR="$PROJECT_DIR/export/web_thread_off"
SERVER_PORT=8080
-PW_TIMEOUT=10000 # Default timeout in ms; adjustable
+PW_TIMEOUT=10000
# Function to check if a step failed
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify timeout units to match pytest-timeout expectations
This value is now passed directly to `pytest` via `--timeout=$PW_TIMEOUT`, and `pytest-timeout` interprets it as seconds, not milliseconds. As written, `10000` is ~2.8 hours, not 10 seconds. Please either change the value to the intended number of seconds or clearly document that this timeout is in seconds to avoid accidental misconfiguration.
</issue_to_address>
### Comment 2
<location path="run_pipeline.sh" line_range="55-61" />
<code_context>
-# Upload reports (simulate artifact upload by copying to a reports dir)
+# 5. GUT Unit Tests
+echo "Ensuring GUT is installed in addons/..."
+if [ ! -d "$PROJECT_DIR/addons/gut" ]; then
+ mkdir -p "$PROJECT_DIR/addons"
+ wget https://github.com/bitwes/Gut/archive/refs/tags/v9.5.0.zip
+ unzip v9.5.0.zip -d "$PROJECT_DIR/addons"
+ mv "$PROJECT_DIR/addons/Gut-9.5.0/addons/gut" "$PROJECT_DIR/addons/gut"
+ rm -rf "$PROJECT_DIR/addons/Gut-9.5.0" v9.5.0.zip
+fi
+
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid downloading and unpacking GUT on every pipeline run for performance and reliability
This step makes every CI run download and unzip GUT, adding avoidable network and I/O overhead and introducing a runtime dependency on GitHub being available. If GUT is a stable dependency, consider installing it in the Docker image (or caching it via a shared volume) so the pipeline avoids repeated downloads and file moves.
Suggested implementation:
```
# 5. GUT Unit Tests
# Assumes GUT is already installed in the Docker image at "$PROJECT_DIR/addons/gut"
if [ ! -d "$PROJECT_DIR/addons/gut" ]; then
echo "GUT not found at '$PROJECT_DIR/addons/gut'."
echo "Please ensure GUT is installed in the Docker image (or cached volume) before running this pipeline."
exit 1
fi
```
To fully implement the suggestion:
1. Update your Dockerfile (or base image build) to clone/unpack GUT into `$PROJECT_DIR/addons/gut` at image build time, so it is available for the pipeline without network access.
2. If you are using a cache/volume approach instead, ensure that the cache populates `$PROJECT_DIR/addons/gut` before `run_pipeline.sh` executes.
3. If there is (or will be) a command that actually runs the GUT tests (e.g. `godot --headless ...`), place it immediately after this existence check and wrap it with `check_exit` as done for the other steps.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
…tations issue (bug_risk): Clarify timeout units to match pytest-timeout expectations This value is now passed directly to pytest via --timeout=$PW_TIMEOUT, and pytest-timeout interprets it as seconds, not milliseconds. As written, 10000 is ~2.8 hours, not 10 seconds. Please either change the value to the intended number of seconds or clearly document that this timeout is in seconds to avoid accidental misconfiguration.
ikostan
changed the title
…y pipeline run for performance and reliability This step makes every CI run download and unzip GUT, adding avoidable network and I/O overhead and introducing a runtime dependency on GitHub being available. If GUT is a stable dependency, consider installing it in the Docker image (or caching it via a shared volume) so the pipeline avoids repeated downloads and file moves. To fully implement the suggestion: Update your Dockerfile (or base image build) to clone/unpack GUT into $PROJECT_DIR/addons/gut at image build time, so it is available for the pipeline without network access. If you are using a cache/volume approach instead, ensure that the cache populates $PROJECT_DIR/addons/gut before run_pipeline.sh executes. If there is (or will be) a command that actually runs the GUT tests (e.g. godot --headless ...), place it immediately after this existence check and wrap it with check_exit as done for the other steps.
Owner
Author
All addressed. |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
This was referenced Apr 23, 2026
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.
This summary outlines the architectural improvements, pipeline enhancements, and environment optimizations implemented for SkyLockAssault in this PR.
Summary of Changes
1. Directory Reorganization
scripts/directory from a flat structure into a modular, feature-oriented hierarchy.core/,entities/,managers/,resources/,system/, andui/.game_settings_resource.gdandaudio_constants.gd, into theres://scripts/resources/folder for improved asset management.2. Pipeline Infrastructure Updates
run_pipeline.shto execute GUT unit tests alongside existing GDUnit4 tests.res://test/gut/via.gutconfig.json, preventing GUT from scanning and conflicting with GDUnit4 test suites.report.xmlis not generated.3. Docker & Environment Optimization
Dockerfileto pre-install GUT v9.5.0, eliminating the need for on-demand downloads during CI/CD runs.markdownlint-cli2@0.12.1within the image to ensure compatibility with Node.js v18 and prevent breaking changes from upstream updates.run_pipeline.sh, resulting in faster and more resilient build cycles.4. Bug Fixes & Refinement
PW_TIMEOUTto seconds (10s) to align withpytest-timeoutexpectations.venv/directory, preventing false positives from third-party library documentation.scripts/subfolder structure.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")
Based on the technical changes and commit history within the project, here is a summary of the contributions made by @ikostan to this PR:
Core Pipeline Infrastructure
run_pipeline.shscript, ensuring multiple testing frameworks (GDUnit4 and GUT) execute sequentially[cite: 1036].Dockerfileand pipeline script to bake stable dependencies like GUT and specific versions of markdownlint-cli2 directly into the image to eliminate network dependencies and improve build speeds[cite: 1032, 1036].report.xmlgeneration and mapped internal variables topytest-timeoutparameters to prevent silent pipeline hangs[cite: 1036].Project Reorganization
scripts/directory to a feature-oriented structure, specifically moving files into dedicatedui/,core/,managers/,entities/, andsystem/subfolders for better modularity[cite: 263, 264, 265, 266].game_settings_resource.gdandaudio_constants.gd, into a centralizedresources/folder to ensure consistent pathing across the project[cite: 263].Testing & Compliance
res://test/gut/, preventing conflicts with the GDUnit4 test suite[cite: 1036].venv/directory, ensuring that static analysis focuses solely on project source code and adheres to the established Python and GDScript coding standards[cite: 1036].This summary details the AI and automation bot contributions for the project's recent infrastructure and testing updates.
The following bots provided automated code reviews, refactoring suggestions, and static analysis to maintain code quality and security standards:
AI and Bot Contributors:
@deepsource-autofix[bot], @coderabbitai, @sourcery-ai[bot], @imgbot[bot], @all-contributors[bot]
Summary by Sourcery
Update tooling, tests, and documentation to align with the new project structure and web deployment workflow.
Enhancements:
Documentation:
Tests:
Summary by Sourcery
Align tooling, tests, and documentation with the updated Godot project structure and CI pipeline.
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
Chores
Documentation
Tests