Skip to content

fix: agent-initiated destroys register with the Editor undo stack - #968

Open
zorionarrillaga wants to merge 1 commit into
IvanMurzak:mainfrom
zorionarrillaga:fix/destroy-registers-with-undo
Open

fix: agent-initiated destroys register with the Editor undo stack#968
zorionarrillaga wants to merge 1 commit into
IvanMurzak:mainfrom
zorionarrillaga:fix/destroy-registers-with-undo

Conversation

@zorionarrillaga

@zorionarrillaga zorionarrillaga commented Aug 26, 2026

Copy link
Copy Markdown

Thanks — all three taken, and you caught something I had wrong.

The description edits are gone entirely. You are right about Ctrl+Z: it is Cmd+Z on macOS, so
the line was false for half your users, and the token cost was not buying anything either way.

On the third comment — the reason the pre-6.5 file only had a description change is that I had
missed GameObject.Destroy.pre-Unity.6.5.cs completely. So the previous version fixed the code on
the 6.5+ side and the wording on the pre-6.5 side, which is backwards. That is now the code on both
sides and no description changes anywhere.

What changed

Three call sites, nothing else:

  • GameObject.Destroy.cs:62 (#if UNITY_6000_5_OR_NEWER)
  • GameObject.Destroy.pre-Unity.6.5.cs:62 (#if !UNITY_6000_5_OR_NEWER)
  • GameObject.Component.Destroy.cs:88 (shared, no #if)

UnityEngine.Object.DestroyImmediateUnityEditor.Undo.DestroyObjectImmediate, fully qualified
to match the existing UnityEditor.EditorUtility.SetDirty style in that folder, plus a CHANGELOG
entry under [Unreleased] → Fixed.

The other DestroyImmediate calls are untouched — Screenshot.Isolated, Screenshot.SceneView,
Screenshot.Camera, Screenshot.GameView, Assets.Prefab.Create and MainThreadDispatcher all
release temporary objects that do not belong on a user's undo stack.

Verification, and its limits

Compiled against Unity 6000.5.6f1, zero error CS, and
ToolThreadSafetyTests.GameObjectDestroy_BothThreads and GameObjectComponentDestroy_BothThreads
pass there. Undo.PerformUndo() restores the destroyed object after the new call and does not after
the old one, which is the behaviour the change is for.

6000.5.6f1 is the only editor on this machine, so the #if !UNITY_6000_5_OR_NEWER branch was
never compiled.
It is the same one-line substitution as its 6.5+ twin, fully qualified so it needs
no new using, but I have not built it and I would rather say so than imply I had.

Two things I did not decide for you

A Transform can now be destroyed where it previously could not. gameobject-component-destroy
iterates go.GetComponents<Component>() with no Transform guard, and the schema text points at
index 0. Object.DestroyImmediate refuses a Transform and logs an error; Undo.DestroyObjectImmediate
destroys and replaces it silently — the GameObject survives with a new Transform, and any serialized
reference to the old one becomes missing, with the tool still reporting success. It is undoable, but
it is a real behaviour change on an input the tool accepts. Happy to add a Transform/RectTransform
skip, or to leave it and note it, whichever you prefer.

Play mode. editor-application-set-state can put the editor into play mode, where Undo is not
meaningful. The call is unconditional here; a fallback to Object.DestroyImmediate while
EditorApplication.isPlaying is two lines if you want it.

@zorionarrillaga
zorionarrillaga force-pushed the fix/destroy-registers-with-undo branch from 9851971 to 9b1183d Compare August 26, 2026 15:42
"and records the destroyed reference. If no component matches at all, throws with the help text from " +
"`Error.NotFoundComponents` (which includes a preview of all available components on the GameObject).")]
[Description("Destroy one or many components from target GameObject. Can't destroy missed components. " +
"Registered with the Editor undo stack, so the removal can be reverted with Ctrl+Z. " +

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's save tokens, remove this additional information.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done — the whole addition is gone, description unchanged from upstream.

[AiSkillDescription(DestroySkill.Description)]
[AiSkillBody(DestroySkill.Body)]
[Description("Destroy GameObject and all nested GameObjects recursively in opened Prefab or in a Scene. " +
"Registered with the Editor undo stack, so the subtree can be restored with Ctrl+Z. " +

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Remove this line.
Side note: CTRL+Z is only relevant for Windows. On MacOS it is a false information

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, removed. And thank you for the macOS note — it is Cmd+Z there, so the line was false for half your users. It should not have been in a token-counted description in the first place.

Comment on lines +81 to +84
"Validates the `gameObjectRef`, resolves it on the main thread, then calls `Object.DestroyImmediate` " +
"(the immediate variant is required for Editor-mode operations). Returns a `DestroyGameObjectResult` " +
"Validates the `gameObjectRef`, resolves it on the main thread, then calls " +
"`Undo.DestroyObjectImmediate` (the immediate variant is required for Editor-mode operations; " +
"the `Undo.` form registers the destruction with the Editor undo stack, so Ctrl+Z restores the " +
"subtree). Returns a `DestroyGameObjectResult` " +

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This only the description update, but the code was not touched. It should no description changes if code hadn't been modified

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right, and the cause is worse than it looks: I had missed GameObject.Destroy.pre-Unity.6.5.cs entirely, so the branch fixed the code on the 6.5+ side and only the wording on the pre-6.5 side. Now it is the code on both sides and no description changes anywhere. One honest limit: 6000.5.6f1 is the only editor I have, so the !UNITY_6000_5_OR_NEWER branch is not compiled here.

@zorionarrillaga
zorionarrillaga marked this pull request as draft August 26, 2026 16:47
gameobject-destroy and gameobject-component-destroy both carry
DestructiveHint = true, yet called UnityEngine.Object.DestroyImmediate, so the
destroyed subtree or component never reached the Editor undo stack and the
editor's undo shortcut could not bring it back. Undo. appeared nowhere in the
plugin.

Three call sites now use UnityEditor.Undo.DestroyObjectImmediate, fully
qualified to match the existing UnityEditor.EditorUtility.SetDirty style in
that folder - including both sides of the UNITY_6000_5_OR_NEWER split for
gameobject-destroy, which the first version of this branch missed.

No description or skill changes: the wording was costing tokens without buying
anything, and its Ctrl+Z claim was false on macOS.

The transient DestroyImmediate calls in Screenshot.*, Assets.Prefab.Create and
MainThreadDispatcher are unchanged - they release temporary objects that do not
belong on a user's undo stack.
@zorionarrillaga
zorionarrillaga force-pushed the fix/destroy-registers-with-undo branch from 9b1183d to 980342e Compare August 26, 2026 16:49
@zorionarrillaga
zorionarrillaga marked this pull request as ready for review August 26, 2026 16:50
@zorionarrillaga

Copy link
Copy Markdown
Author

All three points are addressed in 980342e — the added text is out of both descriptions, and GameObject.Destroy.pre-Unity.6.5.cs now has the code change instead of only the wording. The diff is three call sites plus a CHANGELOG entry.

The red checks aren't from this change. All 12 Unity jobs stop at license activation:

##[error]Missing Unity License File and no Serial was found.
env:
  UNITY_LICENSE:
  UNITY_EMAIL:
  UNITY_PASSWORD:

test_pull_request.yml runs on pull_request with secrets: inherit, and GitHub withholds secrets from fork-triggered runs, so nothing compiles. The license-free jobs are green — nuget gate and both test-cli legs. The publish-test-results failure on main is downstream: workflow_run with no artifacts to download.

test-pull-request-manual from your side would get the license — I can't trigger that from a fork.

@zorionarrillaga

Copy link
Copy Markdown
Author

Found the hinge for the red checks, in case it saves you a look. Before f473111d, test_unity_plugin.yml read UNITY_EMAIL: ${{ secrets.UNITY_EMAIL || '…' }}: the literal fallback meant a fork run got credentials without needing any secret. That commit dropped the fallback for a bare ${{ secrets.UNITY_LICENSE }}, and test_pull_request.yml has been on pull_request since dfa1699b, so fork runs now reach game-ci with the license empty. Same-repo runs are unaffected: task/r2-release-unity was 12/12 green on 08-24.

Separately: DestroySkill.Body in GameObject.cs and GameObject.pre-Unity.6.5.cs, AiSkillBody in GameObject.Component.Destroy.cs, and both destroy SKILL.md files still name Object.DestroyImmediate. I left them as they are because their code is unchanged, per your review. Say the word if you would rather the name matched.

@IvanMurzak

Copy link
Copy Markdown
Owner

Found the hinge for the red checks, in case it saves you a look. Before f473111d, test_unity_plugin.yml read UNITY_EMAIL: ${{ secrets.UNITY_EMAIL || '…' }}: the literal fallback meant a fork run got credentials without needing any secret. That commit dropped the fallback for a bare ${{ secrets.UNITY_LICENSE }}, and test_pull_request.yml has been on pull_request since dfa1699b, so fork runs now reach game-ci with the license empty. Same-repo runs are unaffected: task/r2-release-unity was 12/12 green on 08-24.

Separately: DestroySkill.Body in GameObject.cs and GameObject.pre-Unity.6.5.cs, AiSkillBody in GameObject.Component.Destroy.cs, and both destroy SKILL.md files still name Object.DestroyImmediate. I left them as they are because their code is unchanged, per your review. Say the word if you would rather the name matched.

Thank you @zorionarrillaga , I tried to fix it in the past. Could you please try to implement the fix in another PR? We can test that fix first, and then get back to this PR.

IvanMurzak added a commit that referenced this pull request Sep 3, 2026
Accuracy corrections to the fork-PR section, from two report-only review helpers
plus this pass's own verification against the GitHub API. Documentation only; no
behavioural change.

docs/claude/ci-unity-license.md
- #543 is CLOSED (state_reason=completed, 2026-03-15), so "tracked in issue #543
  and PR #971" sent a reader to a six-month-dead issue. PR #971 is the live one.
- The run-33758511822 table row said "this same workflow", whose nearest
  antecedent is row 1's test_pull_request.yml. The run actually used
  test_pull_request_manual.yml (API: path=.github/workflows/
  test_pull_request_manual.yml, head_branch=ci/p0-fork-license-repro). Named it.
- "starts and completes in the same second" is falsified by the section's own
  cited runs: 32992648441's 6000.3.1f1-editmode/windows-mono leg is
  04:58:48Z -> 04:58:49Z, and on the repro run 33758511822 one of the two legs is
  13:01:37Z -> 13:01:38Z. Restated as a bound ("within a second").
- "the whole job is 1-3 minutes" understated the measured spread; job wall clocks
  on 32992648441 run 1m23s - 3m14s. Now 1-4 minutes.
- The bad-.ulf ordering claim ("pulls the editor image first, then fails inside
  the container") was a hypothesis in declarative register - no bad-.ulf run is on
  record. Kept the diagnostic, marked it unverified, and grounded the "minutes"
  half on run 33757559794, whose licensed step takes 9-13 min per leg.
- Duration does not discriminate a fork PR from a same-repo run whose secret was
  deleted: UNITY_LICENSE is required:false, so both deliver an empty string and
  fail identically. Points at the head-repository check as the discriminator.
- "every run ... was red" was wrong for 4 of 7. All seven runs in the window are
  forks and none went green, but they are 3 failure / 2 cancelled / 2
  action_required. Corrected, and documented action_required (GitHub's
  first-time-contributor approval gate) as a second fork-PR shape with a
  different signature - conclusion is null and nothing ran.
- refs/pull/<n>/merge only exists while GitHub can compute a clean test-merge,
  neither ref is fetched by default, and workflow_dispatch takes only a branch or
  tag - so --ref refs/pull/<n>/head is rejected. That is what makes the deferral
  to #971 legible rather than unexplained.
- Gave "all 12 legs" its derivation (6 caller jobs x the 2-way platform matrix) so
  it self-corrects if the three commented-out playmode jobs are ever enabled.
- The pre-existing refresh procedure told a reader with an invalid-license run to
  re-issue the .ulf, which is the exact trap this section exists to prevent; added
  a one-clause back-pointer.

.github/workflows/test_unity_plugin.yml
- The comment blamed "secrets: inherit", a construct this file does not contain -
  it is the caller's. Attributed it to test_pull_request.yml.

NOT changed, after verification: "the same commit 91e2472 the fork PRs branched
from". One helper reported this as unverifiable and recommended weakening it. It
is exactly right - PRs #922/#967/#968 all have base.sha=91e2472a, and
`git merge-base --is-ancestor 91e2472 refs/pull/<n>/head` holds for all three
with the merge-base being 91e2472 itself. The claim stands as written.

Gates: Suite 4 (CI-surface) green - check_nuget_gate.py exit 0 ("NuGet gate OK:
15 pins, generation UNITY_MCP_DEPS_3, propagation consistent"), workflow YAML
re-parses with all three secrets keys and three inputs intact. Suites 1-3 not
applicable (nothing under Unity-MCP-Plugin/**, nothing under cli/); the
substantive verification of a CI-docs change is the PR's own CI run.
scan-pipeline-leaks.py: 0 hits over 108 added lines, controls 104/104.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bac1LKpVobv1i1FNGCRNvM
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