Skip to content

Add node_rename, complex node_set_property values, and script_patch#9

Merged
dsarno merged 3 commits into
mainfrom
high-leverage-authoring
Apr 14, 2026
Merged

Add node_rename, complex node_set_property values, and script_patch#9
dsarno merged 3 commits into
mainfrom
high-leverage-authoring

Conversation

@dsarno

@dsarno dsarno commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • node_rename — rename nodes with sibling-collision and invalid-char validation via String.validate_node_name(). Undoable. NodePath/script references in other nodes are not auto-updated (documented in the tool).
  • Complex node_set_property — coerces Resource (res:// path string or empty-string-to-clear), NodePath, StringName, Array, and Dictionary; _serialize_value recurses so typed round-trips stay JSON-safe.
  • script_patch — anchor-based old_textnew_text replace with ambiguity detection and optional replace_all. Picked over line-range (fragile under drift) and unified diff (whitespace-sensitive) after a focused spike.

Closes the remaining High-Leverage Authoring items in docs/implementation-plan.md Phase 3 and makes the script.patch decision.

Test plan

  • ruff check src/ tests/ passes
  • pytest -v — 282 passing (+3 new unit/integration)
  • GDScript test_run — 216 passing (+23 new: 7 rename, 8 complex set_property, 8 patch)
  • Live smoke against running editor: rename (5 paths), set_property (Resource assign/clear/not-found, NodePath, float), script_patch (unique match, not-found, ambiguous, replace_all, file-not-found)

🤖 Generated with Claude Code

High-Leverage Authoring batch from docs/implementation-plan.md Phase 3:

- node_rename: rename a node with sibling-collision and invalid-char checks
  (via String.validate_node_name). Documents that NodePath references in
  OTHER nodes are not auto-updated.
- node_set_property: now coerces Resource (res:// path string or null to
  clear), NodePath, StringName, Array, and Dictionary; _serialize_value
  recurses into Array/Dictionary so round-tripped properties stay JSON-safe.
- script_patch: anchor-based old_text → new_text replace with ambiguity
  detection and optional replace_all. Chosen over line-range (fragile under
  drift) and unified diff (whitespace-sensitive) after a focused spike.

Tests: +3 Python (282 total) and +23 GDScript (216 total). Live-smoked
all three tools against a running editor.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Apr 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

- test_set_property_resource_path: build a temp StandardMaterial3D in
  suite_setup/teardown instead of referencing res://materials/floor.tres
  (which isn't committed to the repo).
- test_rename_node_basic: use a time-suffixed unique name so repeated
  runs don't collide with lingering state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds new authoring capabilities to the Godot AI MCP surface: node renaming, richer node property setting (including Resources/NodePath/containers), and an anchor-based script patch tool.

Changes:

  • Introduce node_rename tool/handler wired through Python runtime ↔ Godot plugin, with undo support and validation.
  • Extend node_set_property coercion/serialization to support Resource path assignment/clearing, NodePath, StringName, Array, and Dictionary.
  • Add script_patch tool/handler for exact-substring replacement with ambiguity detection and optional replace_all, plus tests and plan-doc updates.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/test_runtime_handlers.py Extends stub runtime + unit tests for node_rename and script_patch handlers.
tests/integration/test_mcp_tools.py Adds MCP integration coverage for node_rename and script_patch tool calls.
test_project/tests/test_script.gd Adds Godot-side tests validating patch_script behavior and error cases.
test_project/tests/test_node.gd Adds Godot-side tests for resource/nodepath coercion, serialization recursion, and rename behaviors.
test_project/tests/_mcp_test_script.gd Adds a placeholder script fixture used by the test scene setup.
src/godot_ai/tools/script.py Exposes new script_patch MCP tool with user-facing documentation.
src/godot_ai/tools/node.py Updates node_set_property typing/docs and adds node_rename MCP tool.
src/godot_ai/handlers/script.py Adds Python shared handler script_patch sending patch_script command.
src/godot_ai/handlers/node.py Adds Python shared handler node_rename sending rename_node command.
plugin/addons/godot_ai/plugin.gd Registers rename_node and patch_script commands with the dispatcher.
plugin/addons/godot_ai/handlers/script_handler.gd Implements patch_script file editing logic + response shape.
plugin/addons/godot_ai/handlers/node_handler.gd Implements rename_node, enhances set_property coercion and recursive serialization.
docs/implementation-plan.md Marks Phase 3 items complete and updates test-count totals.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +89 to +97
if path.is_empty():
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: path")
if not path.begins_with("res://"):
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
if not "new_text" in params:
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: new_text")
if old_text.is_empty():
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "old_text must not be empty")

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch_script allows patching any res:// file type, but the tool/docstring frames this as a GDScript (.gd) operation. Unlike create_script (which enforces .gd), this can accidentally rewrite non-text/binary assets (e.g., .tscn/.tres) as UTF-8 text and corrupt them. Add the same .gd extension validation (or otherwise restrict to safe text/script extensions) before reading/writing.

Copilot uses AI. Check for mistakes.
if not path.begins_with("res://"):
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Path must start with res://")
if not "new_text" in params:
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: new_text")

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch_script checks for missing new_text but not missing old_text. If the caller omits old_text, the handler returns old_text must not be empty rather than a consistent Missing required param: old_text, and it also prevents explicitly passing an empty string (even if that might be useful for no-op validation). Consider adding an explicit "old_text" in params check (mirroring new_text) and keep the non-empty constraint as a separate validation.

Suggested change
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: new_text")
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: new_text")
if not "old_text" in params:
return McpErrorCodes.make(McpErrorCodes.INVALID_PARAMS, "Missing required param: old_text")

Copilot uses AI. Check for mistakes.
Comment thread src/godot_ai/tools/node.py Outdated
Comment on lines +148 to +161
Coerces `value` to match the property's declared type:

- Vector2/Vector3: dict with x/y/z keys
- Color: dict with r/g/b/a keys, or hex string ("#ff0000")
- NodePath: string ("../Other/Node")
- Resource: res:// path string (loads and assigns), or null to clear
- StringName: plain string
- Array/Dictionary: pass a JSON list/object
- bool/int/float: JSON primitives

Args:
path: Scene path of the node (e.g. "/Main/Camera3D").
property: Property name (e.g. "fov", "position", "visible").
value: New value for the property.
property: Property name (e.g. "fov", "position", "visible", "mesh", "remote_path").
value: New value for the property. Pass null to clear a Resource/NodePath.

Copilot AI Apr 14, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node_set_property docstring says Resources can be cleared with null only, but the Godot-side implementation also treats an empty string ("") as a signal to clear Resource properties. Either document the empty-string behavior here (since clients may rely on it) or remove that behavior for a single, consistent clearing mechanism.

Copilot uses AI. Check for mistakes.
- patch_script: enforce .gd extension to prevent corrupting .tscn/.tres
  binaries; add explicit "old_text" in params check before the non-empty
  validation so the error mirrors the new_text path.
- node_set_property docstring: document that an empty string also clears
  a Resource property (the implementation already accepts both null and "").
- script_patch docstring: mention the .gd requirement.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dsarno dsarno merged commit b53b505 into main Apr 14, 2026
10 checks passed
@dsarno dsarno deleted the high-leverage-authoring branch April 14, 2026 22:07
dsarno added a commit that referenced this pull request May 3, 2026
* Add update/config data-loss safeguards (#297, PR 2)

Two P0 data-loss bugs from the audit roadmap:

**Finding #9** — `update_reload_runner.gd::_install_zip_paths` had no
rollback for partial extracts. If file 3 of 5 failed to write during
self-update, files 1-2 would be vN+1 and 3-5 would be vN, and the runner
would re-enable the plugin against that mixed tree. Now: every per-file
install records a `.update_backup` snapshot via copy (not rename), and a
mid-loop failure rolls every prior write back to its vN content. If
rollback itself fails, the runner reports `FAILED_MIXED` and refuses to
re-enable the plugin — manual intervention required, never silent
half-state.

**Finding #10** — `clients/_atomic_write.gd`'s rename fallback was
remove-then-rename: when the second rename failed (Windows AV / lock
timing), the user's entire MCP config — Claude Desktop, Cursor, Cline,
etc. — was lost. New fallback is overwrite-copy with byte-size
verification; the original is never removed before the new bytes land,
and any failure restores from `.backup` so the prior config is preserved.

**Test gate** (per the audit's matrix):
- 8 new GDScript tests in `test_update_reload_runner.gd` exercising the
  rollback paths: restore-from-backup, delete-orphan, FAILED_MIXED on
  missing backup, reverse-order processing, finalize-clears-backups, and
  end-to-end mid-loop failure with backup restore.
- 4 new GDScript tests in `test_clients.gd` covering atomic_write's
  backup snapshot, tmp cleanup, destination-content preservation on
  failed swap, and restore-from-backup recovery.
- 12 new Python pin tests in `test_audit_data_loss_safeguards.py`
  locking the structural invariants of both fixes (status enum, no
  remove-before-rename, copy_absolute fallback, UTF-8 byte verify, etc.).

Existing pins in `test_editor_focus_refocus.py` still hold; the runner
keeps `temp_path + TEMP_FILE_SUFFIX`, the `.tmp` rename, and the
non-atomic-FS `remove_absolute(target_path)` fallback — only the
top-level signature, return value, and rollback bookkeeping are new.

Base: beta

https://claude.ai/code/session_01Emx5STgx9s3puE2n7dFmVB

* Clear partial bytes on failed first-time atomic write (#297, PR 2)

Address Copilot review on #299: when `had_original` is false and
`copy_absolute` lands some bytes but `_written_size_matches` rejects the
result, the function returned false but left the partial copy at `path`.
That contradicts the "destination is in its pre-call state on failure"
contract — a first-time write would leave a truncated/invalid new
config behind.

The fix is a guarded elif branch in the failure path: if no original
existed AND there's now a regular file at `path` (the partial copy),
remove it so the failure leaves nothing on disk. `FileAccess.file_exists`
keeps the cleanup off non-file destinations — a path that points at a
directory yields `had_original=false` too, but we must not try to delete
the directory there.

Also adds a structural pin
(`test_atomic_write_clears_partial_new_file_when_no_original_existed`)
to lock the new branch in place against future refactors.

https://claude.ai/code/session_01Emx5STgx9s3puE2n7dFmVB

* Skip directory-collision tests on Windows (#297, PR 2)

The two new tests that force a "neither rename nor copy can land"
failure by pointing at an existing non-empty directory:

  - test_atomic_write_preserves_destination_when_swap_fails
  - test_install_zip_paths_rolls_back_when_mid_loop_write_fails

run cleanly on Linux and macOS but the same assertions are not portable
to Windows. Godot's DirAccess.copy is a byte-loop on FileAccess and
Windows' MoveFileExW / CopyFileW behavior on a directory destination is
not consistent enough to reliably reject the swap in the way the test
expects.

The contracts these tests cover are still locked across all platforms:

  - The structural Python pin tests in test_audit_data_loss_safeguards.py
    enforce the bug-fix patterns (no remove-before-rename, FAILED_MIXED
    refuses re-enable, restore-from-backup on failed copy, etc.) — these
    run on every CI matrix entry.
  - The deterministic GDScript rollback unit tests (`_rollback_paths_written`
    happy / orphan / mixed / reverse-order) cover the runtime contract
    without depending on directory-as-destination filesystem behavior.

Skip rather than rework the directory-collision approach to a different
forced-failure path so the Linux/macOS regression coverage stays as
faithful to the production failure path as we can make it.

https://claude.ai/code/session_01Emx5STgx9s3puE2n7dFmVB

* Bisect: widen Windows skip to all new tests (#297, PR 2)

Targeted directory-collision skip (eb7bdbb) didn't unblock Windows. Widen
the skip so the entire `test_update_reload_runner` suite and all four
new atomic-write tests in `test_clients.gd` are skipped on Windows.

If the next Windows CI passes:
  - failing test is somewhere in this set; re-enable one at a time.

If Windows still fails:
  - regression is in the production code path being exercised by an
    EXISTING test on Windows; investigate _atomic_write.gd /
    update_reload_runner.gd for Windows-specific behavior.

Python source-pin tests in test_audit_data_loss_safeguards.py continue
to lock the bug-fix patterns on every platform.

https://claude.ai/code/session_01Emx5STgx9s3puE2n7dFmVB

* Revert "Bisect: widen Windows skip to all new tests (#297, PR 2)"

This reverts commit 5346663.

* Revert "Skip directory-collision tests on Windows (#297, PR 2)"

This reverts commit eb7bdbb.

* Surface FAILED_MIXED when inner _install_zip_file restore fails (#297, PR 2)

Address PR review on #299: in `_install_zip_file`'s failure path, after the
primary rename and the retry both reject the swap, the function tries to
restore `target_path` from `backup_path` — but the COPY result was ignored
and the backup deleted unconditionally. If that inner restore fails, the
target is gone, the backup is gone, and the failed file is NOT recorded in
`_paths_written`. `_rollback_paths_written` then walks only the prior
records (which restore cleanly) and returns FAILED_CLEAN, letting
`_handle_install_failure` re-enable the plugin against a tree where this
file is missing — the exact mixed-tree scenario PR 2 is meant to prevent.

Fix:
- `_install_zip_file` now only deletes the backup when the restore copy
  actually succeeded. On failure it sets `_restore_failed = true` (a new
  member) and leaves the backup on disk so the user has a manual recovery
  path.
- `_rollback_paths_written` consults `_restore_failed` and surfaces
  FAILED_MIXED even when every recorded entry rolls back cleanly.
- Adds `test_rollback_surfaces_failed_mixed_when_restore_failed_flag_is_set`
  pinning the runtime contract, plus a Python source-pin in
  `test_inner_install_restore_failure_surfaces_failed_mixed` locking the
  fix patterns in place.

Companion fix in `_atomic_write.gd`: same "restore-is-assumed" shape on
the backup_made branch. Drop the unnecessary `remove_absolute(path)`
before the restore copy — `copy_absolute` overwrites by default, and the
pre-remove only opened a window where `path` could disappear if the
copy itself failed. Pinned by
`test_atomic_write_restore_does_not_remove_path_before_copy`.

https://claude.ai/code/session_01Emx5STgx9s3puE2n7dFmVB

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants