Fix 10 friction points from cyberpunk HUD stress test#21
Merged
Conversation
Four high-impact fixes discovered during the MCP-only HUD build exercise: 1. theme_create: auto-create parent directories before saving (same DirAccess.make_dir_recursive_absolute pattern as script_create) 2. ui_build_layout: support theme_override_* pseudo-properties (theme_override_colors/, _constants/, _font_sizes/, _styles/ prefixes now route to add_theme_*_override methods with proper coercion) 3. theme_set_stylebox_flat: add 12 per-side parameters (border_width_top/bottom/left/right, corner_radius per-corner, content_margin per-side — all override uniform values when provided) 4. editor_screenshot source="game": fix viewport type mismatch (variable was typed SubViewport but game viewport is a Viewport; implicit cast returned null, cascading to Python KeyError) All fixes include GDScript tests, Python unit tests, and integration tests. pytest: 377 passed | test_run: 312 passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t rename Three new capabilities discovered during the cyberpunk HUD stress test: 1. Scene instancing via node_create: new scene_path parameter loads a PackedScene and instantiates it (mutually exclusive with type param). Supports undo, sets owner recursively for proper serialization. 2. animation_delete tool + overwrite parameter: new animation_delete command removes a clip with full undo support. Both animation_create and animation_create_simple now accept overwrite=true to replace existing animations in one atomic undo action. 3. Allow renaming scene root: removed the artificial guard that blocked renaming the scene root node. Godot's editor UI allows this natively. Root rename doesn't affect the .tscn file path. All fixes include GDScript tests, Python unit tests, and handler updates. pytest: 382 passed | test_run: 327 passed, 1 pre-existing failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion validate Three "nice to have" improvements from the cyberpunk HUD stress test: 1. project_stop readiness race: add 150ms settle delay in Python handler after stop_project returns, giving the editor time to emit readiness_changed before the next tool call arrives. 2. signal_connect autoload support: when ScenePath.resolve fails, fall back to checking ProjectSettings autoload entries and resolving via Engine.get_main_loop().root. Both source and target paths now resolve autoload singletons. Error messages clarify "not in scene tree or autoloads". 3. animation_validate tool: read-only tool that checks all track paths in an animation resolve to actual nodes. Reports valid_count, broken_count, and broken_tracks with index/path/issue for each stale reference. Does not require writable session. All fixes include GDScript tests and Python unit tests. pytest: 385 passed | test_run: 332 passed, 0 failed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The CI script waited for a Godot session to connect but not for the editor to actually open a scene. Tests that resolve scene paths would get EDITOR_NOT_READY because readiness was still "no_scene". Now polls editor_state readiness until it transitions to "ready" (up to 60s) before running test_run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes the patch coverage gap — the tool wrapper functions in tools/animation.py now have integration tests exercising the full MCP call path through the mock plugin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Headless Godot (--headless --editor) does not auto-open the last scene. After the readiness wait detects no_scene, explicitly call scene_open via MCP to load main.tscn before running tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Headless Godot --editor no longer auto-opens the last scene on recent CI runners. Pass res://main.tscn as a positional arg to force the scene open on launch. Also log scene_open response for debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Headless Godot --import creates .godot/ but does not record any open scenes. When --editor launches, the editor_layout has no scenes to reopen, leaving readiness stuck at no_scene. Append an [EditorNode] section with open_scenes after import to ensure main.tscn reopens on editor launch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous fix appended to the import-generated layout, but Godot's ConfigFile parser may not handle duplicate sections. Write a clean minimal layout file with just the EditorNode section. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move scene_open call to right after session detection, before the readiness poll loop. This gives the editor time to load the scene while we wait for readiness to transition from no_scene to ready. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The cyberpunk HUD exercise (7cad020) committed a dirty main.tscn with test-run leftovers: stale ext_resource refs to _mcp_test_material.tres (does not exist in repo), ~20 test nodes, modified Camera3D, and the HUDLoader script. project.godot also had stray GameState autoload and ability_* input actions. On CI, the missing resource caused scene_open to fail, keeping readiness stuck at no_scene, so test_run returned 0 tests. Removed: cyberpunk HUD files, test artifacts, stray autoloads/inputs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add debug prints to _discover_suites() and dump raw test_run response in ci-godot-tests script. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When 0 suites found, include dir listing, gd file names, and per-file load errors so CI raw dump shows exactly why discovery fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous _debug_discovery() returned {} — likely a runtime error.
Inline the check directly to avoid function call issues.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CACHE_MODE_IGNORE creates fresh script instances with different class identities, causing typed Array[McpTestSuite] appends to silently fail on CI where the class cache is freshly built by --import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause: calling scene_open via MCP before test_run in CI left the headless editor in a state where _discover_suites() returned empty. Possibly related to #10 (scene_open re-entrancy crash). On main (which has no scene_open in CI, tests discover 312 suites successfully. Changes: - Remove scene_open + readiness wait from ci-godot-tests script - Remove editor_layout.cfg force-write from ci.yml Linux job - Use untyped Array in _discover_suites() to avoid potential typed array silent rejection with CACHE_MODE_IGNORE script loading - Restore CACHE_MODE_IGNORE for dev workflow (pick up test changes) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> EOF )
When 0 suites are found, return dir listing, load results, and McpTestSuite check status so CI logs reveal the root cause. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause: CACHE_MODE_IGNORE in _discover_suites() breaks class_name resolution when test scripts reference plugin types (e.g. `var _handler: ThemeHandler` in test_theme.gd). Fresh-loaded scripts can't resolve these type declarations against the global class registry, causing ResourceLoader.load() to silently crash. This only affects the PR branch because main has no test files that reference the new ThemeHandler/UiHandler classes. Fix: use default cache mode so scripts resolve class_name types from the cached global class registry. Use untyped Array for extra safety. Remove scene_open additions from CI script and editor_layout.cfg hack (tests don't need a scene open — main CI proves this. Closes #10 (partially — removes scene_open from CI to avoid crash risk) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> EOF )
When a GDScript handler crashes, .call() returns default empty dict.
Detect this and return a proper error so CI logs reveal the crash
instead of silently returning {}.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts test_handler.gd, dispatcher.gd, ci-godot-tests, and ci.yml to exactly match main. If CI still fails, the issue is in the PR's feature code (handler changes, test files, scene cleanup), not in the test infrastructure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These 22 extra .uid files were from a local Godot run. They may cause the import step to register stale UIDs, corrupting class resolution and breaking test discovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
test_editor.gd had two functions named test_screenshot_invalid_source (lines 93 and 315). GDScript parse error on ResourceLoader.load() crashed _discover_suites(), returning 0 tests on all CI platforms. Also sync ci.yml Linux pip timeout from main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three guardrails to prevent silent test discovery failures:
1. _discover_suites() now catches per-file load errors and continues.
Returns {suites, errors} so the error message names the broken
file instead of 'No test suites found'.
2. New script/ci-check-gdscript validates all .gd files for parse
errors before the editor starts. Would have caught the duplicate
function name in seconds.
3. CI workflow runs validation after --import, before --editor.
Parse errors fail fast with clear output.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The barichello/godot-ci container ships with an old pip that hangs on dependency resolution. Upgrading pip first and adding --timeout 60 should prevent the infinite hang.
barichello/godot-ci:4.6.2 hangs at pip install every run. Switch to chickensoft-games/setup-godot + actions/setup-python on ubuntu-latest, matching the macOS and Windows setup that works reliably.
# Conflicts: # .github/workflows/ci.yml
- Extract _commit_animation_add() helper in animation_handler.gd to deduplicate the overwrite undo/redo block between create_animation and create_simple (was copy-pasted verbatim) - Replace _resolve_autoload property list scan with O(1) ProjectSettings.has_setting() lookup - Remove TOCTOU dir_exists check before make_dir_recursive in theme_handler.gd (mkdir is idempotent) - Optimize ci-check-gdscript: reuse --import output instead of spawning one Godot process per .gd file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ening Sync CLAUDE.md, implementation-plan, testing-strategy, tool-taxonomy, and skill file with recent changes: chickensoft CI runner, ci-check-gdscript validation step, step timeouts, resilient test discovery, animation_delete and animation_validate tools, overwrite parameter on animation_create, node_create scene_path for scene instancing, rename_node root support, theme_set_stylebox_flat per-side overrides, and updated test counts (387 Python + 333 GDScript = 720 total). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dsarno
added a commit
that referenced
this pull request
Apr 16, 2026
Nine follow-ups from a retroactive code review of the animation_* tool family (#17) and the cyberpunk HUD friction fixes (#21). All 388 Python + 345 GDScript tests pass; smoke-tested live against the editor. 1. node_create scene_path now uses PackedScene.GEN_EDIT_STATE_INSTANCE so the editor treats the result as a real scene instance (foldout icon, .tscn stores a reference, not an exploded subtree). Removes the _set_owner_recursive call that was running outside the undo action and also broke the instance link by forcing descendant ownership. 2. Animation track undo no longer caches the baseline track_count at do time - it resolves the index via anim.find_track(path, type) at undo time. Robust to any history interleaving. New test stresses this path. 3. _coerce_value_for_track now errors with INVALID_PARAMS when the target node exists but the property doesnt - previously the raw value passed through and produced garbage keyframes at playback. Matches the handlers own docstring. 4. add_method_track validates keyframe args is an Array and method is a non-empty string. Previously a typed-assign crash or silent garbage. 5. animation_delete now uses _resolve_animation to locate the owning library, matching the read-side symmetry with animation_get / animation_play. Delete from non-default libraries works end-to-end. 6. theme_set_stylebox_flat flattened from 27 parameters to a nested-dict API: border / corners / margins / shadow each take {all, sides} with side-specific keys overriding all. Unknown keys fail loudly. Breaking change - tool was only merged in #21 and has no external clients yet. 7. project_stop replaces the fixed 150ms sleep with a bounded poll on session.readiness. Reaches truth as soon as the existing readiness_changed event arrives (~17ms) instead of blanket-sleeping 150ms, with a 1s timeout for a hung play process. 8. ui_build_layout pseudo-property tests now read back via get_theme_*_override (not the fallback get_theme_* getters), per the CLAUDE.md "assert on the stored Variant" rule. Added missing theme_override_styles/ test. 9. signal_connect distinguishes declared-but-uninstantiated autoloads from genuinely missing nodes, with a clear error pointing users at @onready/runtime-connect as the workaround. Previous "not found" error masked the real reason most runtime-only autoloads cant be connected at edit time. Also updates CLAUDE.md with reference patterns for find_track-at-undo, GEN_EDIT_STATE_INSTANCE, and get_theme_*_override readback tests. Deferred to a follow-up PR: plugin dispatcher coroutine support + readiness Option C (plugin-side await). That refactor touches the hot path every command flows through and deserves its own bisect-friendly landing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 16, 2026
dsarno
added a commit
that referenced
this pull request
Apr 16, 2026
* Address retroactive review of PRs #17 and #21 Nine follow-ups from a retroactive code review of the animation_* tool family (#17) and the cyberpunk HUD friction fixes (#21). All 388 Python + 345 GDScript tests pass; smoke-tested live against the editor. 1. node_create scene_path now uses PackedScene.GEN_EDIT_STATE_INSTANCE so the editor treats the result as a real scene instance (foldout icon, .tscn stores a reference, not an exploded subtree). Removes the _set_owner_recursive call that was running outside the undo action and also broke the instance link by forcing descendant ownership. 2. Animation track undo no longer caches the baseline track_count at do time - it resolves the index via anim.find_track(path, type) at undo time. Robust to any history interleaving. New test stresses this path. 3. _coerce_value_for_track now errors with INVALID_PARAMS when the target node exists but the property doesnt - previously the raw value passed through and produced garbage keyframes at playback. Matches the handlers own docstring. 4. add_method_track validates keyframe args is an Array and method is a non-empty string. Previously a typed-assign crash or silent garbage. 5. animation_delete now uses _resolve_animation to locate the owning library, matching the read-side symmetry with animation_get / animation_play. Delete from non-default libraries works end-to-end. 6. theme_set_stylebox_flat flattened from 27 parameters to a nested-dict API: border / corners / margins / shadow each take {all, sides} with side-specific keys overriding all. Unknown keys fail loudly. Breaking change - tool was only merged in #21 and has no external clients yet. 7. project_stop replaces the fixed 150ms sleep with a bounded poll on session.readiness. Reaches truth as soon as the existing readiness_changed event arrives (~17ms) instead of blanket-sleeping 150ms, with a 1s timeout for a hung play process. 8. ui_build_layout pseudo-property tests now read back via get_theme_*_override (not the fallback get_theme_* getters), per the CLAUDE.md "assert on the stored Variant" rule. Added missing theme_override_styles/ test. 9. signal_connect distinguishes declared-but-uninstantiated autoloads from genuinely missing nodes, with a clear error pointing users at @onready/runtime-connect as the workaround. Previous "not found" error masked the real reason most runtime-only autoloads cant be connected at edit time. Also updates CLAUDE.md with reference patterns for find_track-at-undo, GEN_EDIT_STATE_INSTANCE, and get_theme_*_override readback tests. Deferred to a follow-up PR: plugin dispatcher coroutine support + readiness Option C (plugin-side await). That refactor touches the hot path every command flows through and deserves its own bisect-friendly landing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: bump test counts and reflect PR #28 API reshape - CLAUDE.md: 387 → 388 Python tests - implementation-plan.md: 387+333=720 → 388+345=733 total; theme_* blurb now describes the nested border/corners/margins/shadow dicts and animation undo-robust-to-history-interleaving Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * simplify: hoist property-type resolution + use get_running_loop 1. animation_handler.gd: _coerce_value_for_track split into _resolve_track_prop_context (per-track) + _coerce_with_context (per-key). add_property_track resolves the target property type once before the keyframe loop instead of re-walking get_property_list() for every keyframe. Dense clips (100+ keys) avoid 100x the work. Existing API (_coerce_value_for_track) preserved for create_simple, which still resolves per-tween since target/property change per tween. 2. project.py: replace asyncio.get_event_loop() with get_running_loop() (former is deprecated in Python 3.12+ inside async functions). All 388 Python + 345 GDScript tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 29, 2026
Closed
dsarno
added a commit
that referenced
this pull request
Jun 9, 2026
…dator The strong traversal validator from #347 was wired into only filesystem and script handlers; ~12 other path-taking handlers used a bare begins_with("res://") check (which does not reject "..") or no check at all (relying solely on ResourceLoader.exists/load). This unifies all of them on McpPathValidator and extends the validator with two checks. Validator (utils/path_validator.gd): - Reject embedded null bytes (truncation trap — the path written could differ from the one validated). - New for_write flag: write callers additionally refuse res://project.godot, the res://.godot/ metadata dir, and .import sidecars (overwriting these corrupts the project / import cache). Reads still permit inspecting them. Signature is backward-compatible (for_write defaults to false). Writers now validate with for_write=true (closes the traversal-write primitive): resource_io.save_to_disk (backs resource/environment/texture/curve saves), scene create_scene/save_scene_as, material/theme save helpers, filesystem write_text, script create/patch. Load/read sites now validate (closes the unvalidated-load surface, incl. the @tool-script-execution risk from open_scene/create_node): resource load/assign + nested object-property loads, scene open_scene, script attach_script, node create_node scene_path + set_property resource values, audio set_stream + list root, material assign/shader/list, ui theme + stylebox, autoload path, curve set_points, particle mesh/ material/texture, material_values.load_texture. Path-validation rejections report VALUE_OUT_OF_RANGE (the code these handlers already used for "must start with res://"), keeping the INVALID_PARAMS catch-all count under the audit-v2 #21 ceiling enforced by test_error_code_distribution. The four pre-existing validator sites that already wrapped errors as INVALID_PARAMS (filesystem, script create/read/patch/find, resource_io save, material _validate_material_path) are left unchanged. set_stream and the other load handlers now reject user:// (consistent with the validator's existing test_user_prefix_rejected policy); the audio test fixture moves from user:// to a res:// .tres registered via EditorFileSystem.update_file. Tests (run against live Godot 4.6.3): validator unit tests for null-byte + write blocklist + read-allows; scene traversal/manifest-overwrite rejection. All affected suites pass (path_validator/scene/resource/node/material/theme/ curve/particle/ui/audio/autoload/filesystem/script). test_error_code_distribution passes. Parse check clean. Addresses advisory GHSA-p5x8-v25q-qw69 (GH-1, GH-2, GH-3, GH-4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dsarno
added a commit
that referenced
this pull request
Jun 10, 2026
…dator The strong traversal validator from #347 was wired into only filesystem and script handlers; ~12 other path-taking handlers used a bare begins_with("res://") check (which does not reject "..") or no check at all (relying solely on ResourceLoader.exists/load). This unifies all of them on McpPathValidator and extends the validator with two checks. Validator (utils/path_validator.gd): - Reject embedded null bytes (truncation trap — the path written could differ from the one validated). - New for_write flag: write callers additionally refuse res://project.godot, the res://.godot/ metadata dir, and .import sidecars (overwriting these corrupts the project / import cache). Reads still permit inspecting them. Signature is backward-compatible (for_write defaults to false). Writers now validate with for_write=true (closes the traversal-write primitive): resource_io.save_to_disk (backs resource/environment/texture/curve saves), scene create_scene/save_scene_as, material/theme save helpers, filesystem write_text, script create/patch. Load/read sites now validate (closes the unvalidated-load surface, incl. the @tool-script-execution risk from open_scene/create_node): resource load/assign + nested object-property loads, scene open_scene, script attach_script, node create_node scene_path + set_property resource values, audio set_stream + list root, material assign/shader/list, ui theme + stylebox, autoload path, curve set_points, particle mesh/ material/texture, material_values.load_texture. Path-validation rejections report VALUE_OUT_OF_RANGE (the code these handlers already used for "must start with res://"), keeping the INVALID_PARAMS catch-all count under the audit-v2 #21 ceiling enforced by test_error_code_distribution. The four pre-existing validator sites that already wrapped errors as INVALID_PARAMS (filesystem, script create/read/patch/find, resource_io save, material _validate_material_path) are left unchanged. set_stream and the other load handlers now reject user:// (consistent with the validator's existing test_user_prefix_rejected policy); the audio test fixture moves from user:// to a res:// .tres registered via EditorFileSystem.update_file. Tests (run against live Godot 4.6.3): validator unit tests for null-byte + write blocklist + read-allows; scene traversal/manifest-overwrite rejection. All affected suites pass (path_validator/scene/resource/node/material/theme/ curve/particle/ui/audio/autoload/filesystem/script). test_error_code_distribution passes. Parse check clean. Addresses advisory GHSA-p5x8-v25q-qw69 (GH-1, GH-2, GH-3, GH-4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dsarno
added a commit
that referenced
this pull request
Jun 10, 2026
…dator (#546) * harden(handlers): route every path-taking handler through McpPathValidator The strong traversal validator from #347 was wired into only filesystem and script handlers; ~12 other path-taking handlers used a bare begins_with("res://") check (which does not reject "..") or no check at all (relying solely on ResourceLoader.exists/load). This unifies all of them on McpPathValidator and extends the validator with two checks. Validator (utils/path_validator.gd): - Reject embedded null bytes (truncation trap — the path written could differ from the one validated). - New for_write flag: write callers additionally refuse res://project.godot, the res://.godot/ metadata dir, and .import sidecars (overwriting these corrupts the project / import cache). Reads still permit inspecting them. Signature is backward-compatible (for_write defaults to false). Writers now validate with for_write=true (closes the traversal-write primitive): resource_io.save_to_disk (backs resource/environment/texture/curve saves), scene create_scene/save_scene_as, material/theme save helpers, filesystem write_text, script create/patch. Load/read sites now validate (closes the unvalidated-load surface, incl. the @tool-script-execution risk from open_scene/create_node): resource load/assign + nested object-property loads, scene open_scene, script attach_script, node create_node scene_path + set_property resource values, audio set_stream + list root, material assign/shader/list, ui theme + stylebox, autoload path, curve set_points, particle mesh/ material/texture, material_values.load_texture. Path-validation rejections report VALUE_OUT_OF_RANGE (the code these handlers already used for "must start with res://"), keeping the INVALID_PARAMS catch-all count under the audit-v2 #21 ceiling enforced by test_error_code_distribution. The four pre-existing validator sites that already wrapped errors as INVALID_PARAMS (filesystem, script create/read/patch/find, resource_io save, material _validate_material_path) are left unchanged. set_stream and the other load handlers now reject user:// (consistent with the validator's existing test_user_prefix_rejected policy); the audio test fixture moves from user:// to a res:// .tres registered via EditorFileSystem.update_file. Tests (run against live Godot 4.6.3): validator unit tests for null-byte + write blocklist + read-allows; scene traversal/manifest-overwrite rejection. All affected suites pass (path_validator/scene/resource/node/material/theme/ curve/particle/ui/audio/autoload/filesystem/script). test_error_code_distribution passes. Parse check clean. Addresses advisory GHSA-p5x8-v25q-qw69 (GH-1, GH-2, GH-3, GH-4). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * harden(handlers): address review — uid:// loads, case-fold blocklist, shared helper Follow-up to the McpPathValidator unification, fixing the code-review findings. Correctness: - Restore uid:// and user:// support on load handlers. ResourceLoader accepts both (uid:// is an opaque resource id that can't express traversal; user:// is the user data sandbox), but routing every load through the strict res:// validator rejected them — a regression for uid:// refs copied from .tscn/.uid and for user:// runtime assets. New validate_loadable_path() accepts res:// (confined), uid:// (as-is), and user:// (confined under the user root), and load sites now use it: set_property/assign_resource/resource property dicts, open_scene, create_node, attach_script, set_stream, particle mesh/material/ texture, material assign/shader, ui theme/stylebox, curve, material_values. - Case-fold the write blocklist. macOS (APFS) and Windows (NTFS) are case-insensitive by default, so res://Project.godot resolved to the real project.godot and slipped past a case-sensitive compare. - Add override.cfg to the write blocklist (applied over project.godot at startup — same takeover surface as the manifest). - Reads no longer run the write blocklist: _validate_material_path / _validate_res_path / _load_material_from_path / _load_theme_from_params take for_write, passed true only by the create/save callers. get_material and apply_theme (pure reads) no longer return a spurious "Refusing to write". - Stop blocking .import writes — editing import config then reimporting is a legitimate, recoverable workflow; the blocklist is the startup-execution surface (manifest, override.cfg, .godot/) only. Cleanup / altitude: - Single error-code choke point: McpPathValidator.path_error / loadable_error return a ready error dict (MISSING_REQUIRED_PARAM for empty, VALUE_OUT_OF_RANGE for invalid) so every handler reports the same code for the same failure. filesystem/script move off their old INVALID_PARAMS wrapping onto this. - curve set_points validates the load path (the save layer, resource_io.save_to_disk, remains the authoritative write-confinement check) instead of double-running the write validator. - Drop the redundant is_empty() pre-check in material_values.load_texture. - Document the lexical-containment (no symlink resolution) limitation in the validator — GDScript has no realpath; the loopback boundary is the control. Tests (live Godot 4.6.3, all suites 0 failures): uid:// / user:// acceptance, user:// traversal + unknown-scheme rejection, case-insensitive blocklist, override.cfg block, .import now allowed; audio fixture moved back to user:// (no repo/EFS pollution). test_path_traversal_guard counts any McpPathValidator delegation; missing-path tests now assert MISSING_REQUIRED_PARAM. Addresses review findings 1-10 on GHSA-p5x8-v25q-qw69 (path confinement). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * harden(handlers): address Copilot review on path validation - Guard the null-byte check against an empty String.chr(0) sentinel in both validate_resource_path and validate_loadable_path. On builds that normalize embedded nulls away (e.g. 4.3), contains("") would be true and reject every path; a String that can't hold a null can't smuggle one, so skip the check. - Update stale "res:// path" comments in ui_handler (stylebox override) and material_values.load_texture — both now accept uid:// / user:// via validate_loadable_path. - Tighten test_path_traversal_guard: assert attach_script is present and require >=5 McpPathValidator delegations in script_handler, so a regression where attach_script stops validating its path is caught. Live Godot 4.6.3: path_validator/filesystem/script/ui/material/resource/scene/ audio suites all pass; test_path_traversal_guard green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: wrap long func-name tuple in test_path_traversal_guard (ruff E501) The 5-entry func list exceeded the 100-char line limit after adding attach_script. Pure formatting; the assertion is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(handlers): align remaining res:// messages with uid:// / user:// support Copilot re-review: now that the theme / stylebox / shader load paths accept uid:// and user:// via loadable_error, the surrounding comments, the build_layout docstring, the stylebox fallback error ("expects a res:// path"), and the shader_path missing-arg error still said res:// only. Text-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Fixes 10 of the 12 friction points documented in the cyberpunk HUD stress test, organized by priority tier.
Tier 1 — High impact, straightforward fixes
Tier 2 — New capabilities
Tier 3 — Nice to have
Test plan