feat(project-system): recover malformed element files without ever rewriting them - #2159
feat(project-system): recover malformed element files without ever rewriting them#2159yuto-trd wants to merge 15 commits into
Conversation
…writing them Opening a scene whose .belm sidecar no longer parses previously failed the whole project open. Such elements now load as disabled fallback elements that retain the original raw text: open_project reports a warning naming the file and parser error while healthy elements keep loading and rendering. Recovered elements are unpersistable at the CoreSerializer level, so every save path — Scene.Serialize, the editor's Ctrl+S child loop, the auto-save service, and toolkit saves — leaves the un-parseable file byte-identical on disk, and the auto-save delete branch exempts them so removing a recovered element cannot destroy the recoverable sidecar. Their element Id comes from a quote-aware top-level scan of the raw text when present, or a deterministic UUIDv5 of the filename, so repeated opens agree, and the fallback's declarative projection carries a valid $type and Id so edits to unrelated elements reconcile normally.
📝 WalkthroughWalkthroughThe project system recovers malformed element sidecars with stable IDs and suppressed writes. Project opening reports deserialization warnings. Save, rehome, autosave, rendering, editing, and deletion preserve recovered source bytes. ChangesMalformed element recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OpenProject
participant Scene
participant ElementSidecar
participant CoreSerializer
OpenProject->>Scene: Open project and restore elements
Scene->>ElementSidecar: Read serialized element
Scene->>CoreSerializer: Deserialize element
CoreSerializer-->>Scene: Return fallback or restored object
CoreSerializer-->>Scene: Record fallback incident
Scene->>Scene: Preserve recovery metadata and raw bytes
Scene-->>OpenProject: Return recovered elements
OpenProject->>OpenProject: Collect warnings and incidents
OpenProject-->>OpenProject: Return project summary
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 29 file(s); skipped 3. |
Confidence Score: 4/5This PR is not yet safe to merge because migrated recovered IDs leave reference expressions targeting obsolete object IDs. MigrateRecoveredElementReferences updates only direct IReference property values; serialized ReferenceExpression ObjectIds remain unchanged and later fail hierarchy lookup once the recovered placeholder ID has been replaced. Files Needing Attention: src/Beutl.ProjectSystem/ProjectSystem/Scene.cs Important Files Changed
Reviews (15): Last reviewed commit: "fix(review): resume persistence on every..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the corrupt payload into one field.
The same truncated-JSON literal appears on Lines 35, 79, 93, and 111. A single
private static readonly byte[] s_corruptByteskeeps the four tests in sync when the payload shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs` around lines 35 - 36, Extract the repeated truncated-JSON byte payload into a single private static readonly field named s_corruptBytes in MalformedElementRecoveryTests, then replace the inline literals at all four test locations with that shared field while preserving the existing test behavior.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs (2)
674-723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this fixture in the two warning tests.
CreateProjectWithMalformedElementrepeats the project, healthy element, and malformed element setup thatOpen_project_warns_about_malformed_element_json_and_keeps_healthy_elementsbuilds inline on Lines 98-135. The two blocks differ only in the malformed payload. Add a payload parameter to the fixture builder and call it from that test. Also extract the repeatedRenderToolsconstruction (Lines 66-78 and 144-156) into a helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs` around lines 674 - 723, Refactor the malformed-element warning tests to reuse CreateProjectWithMalformedElement: add a malformed-payload parameter, use it when writing the malformed element JSON, and update Open_project_warns_about_malformed_element_json_and_keeps_healthy_elements to obtain its setup from the fixture. Extract the duplicated RenderTools construction into a helper and use that helper in both warning tests.
165-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo warning assertions depend on error text that Beutl does not own. The shared root cause is that both tests match substrings produced by the JSON layer rather than the message
CollectDeserializationWarningsformats. A .NET or converter update can reword that text and break both tests without any behavior regression. Assert on the element filename plus the Beutl-owned phrasecould not be deserialized, and treat the parser detail as an optional extra assertion.
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs#L165-L169: replace the"JsonReaderException"and"invalid start"substring matches with the Beutl-owned warning phrase.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs#L87-L89: replace the"could not be converted"substring match with the Beutl-owned warning phrase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs` around lines 165 - 169, The warning assertions in SessionToolsTests should rely on Beutl-owned wording rather than parser-specific error text. At tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:165-169, keep the malformed element filename assertion and replace the JsonReaderException and invalid start checks with “could not be deserialized”; at tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:87-89, replace the “could not be converted” check with the same phrase. Parser details may remain only as optional assertions.src/Beutl.ProjectSystem/ProjectSystem/Scene.cs (1)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify recovered-element tracking.
_recoveredElementsonly stores entries made whenMarkRecoveredElementsetselement.IsStorageWriteSuppressed = true, andCoreSerializer.StoreToUrialready returns for that state.RecoveredElementSource.RawTextis not read anywhere in the codebase, and entries are never removed when children are detached. If no future code needs the retained raw text, remove the dictionary and rely onElement.IsStorageWriteSuppressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 64 - 65, Remove the unused _recoveredElements tracking and any associated RecoveredElementSource/RawText bookkeeping in Scene and recovery-related methods. Preserve MarkRecoveredElement’s behavior by setting Element.IsStorageWriteSuppressed directly, and update callers to rely on that flag instead of dictionary lookups or retained raw text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.AgentToolkit/Tools/SessionTools.cs`:
- Around line 80-83: Update the session response construction so
CollectDeserializationWarnings(result.Project) executes through
result.Session.ReadOnSession, matching the dispatch used by CreateSummary. Keep
the warning collection within the session-dispatch callback to ensure traversal
of the live project graph occurs on the owning editor thread.
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 230-234: Update the CoreSerializer flow around StoreToUri so
IsStorageWriteSuppressed does not make skipped writes indistinguishable from
successful writes: when the target URI changes, allow the write and Uri update,
and for new locations copy the original recovered sidecar bytes. Preserve
suppression only when no URI relocation or sidecar copy is required.
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 765-784: Remove the matches[0] fallback branch from
ResolveRecoveredElementId; only accept an ID returned by FindTopLevelIdMatch,
and otherwise fall through to CreateVersion5Guid using the relative path.
- Around line 694-733: The catch in RestoreElementOrFallback currently handles
only JsonException; broaden it to include InvalidOperationException and the
serializer’s unsupported-deserialization exception type so valid sidecars with
unresolvable types enter the existing fallback construction path. Preserve the
current fallback metadata, projection, recovery marking, and return behavior.
In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs`:
- Around line 141-142: Add a success assertion immediately after each
OpenProject call in tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs at
lines 141-142, 183-183, and 219-219, using opened.IsSuccess and
opened.Error?.Message before accessing opened.Value, opened.Value!.Session, or
serializing the result.
In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs`:
- Around line 51-54: Update the recovery test around the first and second
restored IDs to assert that the recovered ID is not Guid.Empty, while retaining
the existing equality assertion between both restores.
---
Nitpick comments:
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 64-65: Remove the unused _recoveredElements tracking and any
associated RecoveredElementSource/RawText bookkeeping in Scene and
recovery-related methods. Preserve MarkRecoveredElement’s behavior by setting
Element.IsStorageWriteSuppressed directly, and update callers to rely on that
flag instead of dictionary lookups or retained raw text.
In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs`:
- Around line 674-723: Refactor the malformed-element warning tests to reuse
CreateProjectWithMalformedElement: add a malformed-payload parameter, use it
when writing the malformed element JSON, and update
Open_project_warns_about_malformed_element_json_and_keeps_healthy_elements to
obtain its setup from the fixture. Extract the duplicated RenderTools
construction into a helper and use that helper in both warning tests.
- Around line 165-169: The warning assertions in SessionToolsTests should rely
on Beutl-owned wording rather than parser-specific error text. At
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:165-169, keep the
malformed element filename assertion and replace the JsonReaderException and
invalid start checks with “could not be deserialized”; at
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:87-89, replace the
“could not be converted” check with the same phrase. Parser details may remain
only as optional assertions.
In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs`:
- Around line 35-36: Extract the repeated truncated-JSON byte payload into a
single private static readonly field named s_corruptBytes in
MalformedElementRecoveryTests, then replace the inline literals at all four test
locations with that shared field while preserving the existing test behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ef5de1e-6292-495a-bc04-9373741552d7
📒 Files selected for processing (7)
src/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/CoreObject.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Editor/AutoSaveService.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10445d024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (obj is CoreObject { IsStorageWriteSuppressed: true }) | ||
| { | ||
| return; |
There was a problem hiding this comment.
This public serializer now silently succeeds without writing only for CoreObject instances carrying an internal flag. Out-of-tree consumers cannot inspect, clear, or implement that policy, while recovery state is split between this flag and Scene's private recovery map; expose an interface, option, or virtual persistence hook so callers can explicitly handle or resolve recovered content instead of encountering an opaque no-op.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping the persistence-suppression contract internal is deliberate for this PR: the only producer is Scene recovery and the only consumers are CoreSerializer and AutoSaveService. Promoting it to a public composable policy changes public serializer surface and deserves its own design change; leaving this thread open for a maintainer decision.
Save-as now copies a recovered element's retained raw bytes to the new location instead of silently skipping the write, so a saved-as project keeps the element while the original file stays untouched. Recovery catches the full deserialization-domain failure set (a resolvable but non-Element $type surfaced as InvalidCastException and aborted the whole open), reads sidecar text only on the recovery path instead of doubling every healthy element's I/O, and never adopts a nested or quoted Id when no top-level Id exists. open_project collects deserialization warnings on the session thread, and the tests assert open success before use, a non-empty recovered Id, save-as rehoming, and non-Element-discriminator recovery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1688213481
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…discriminators before deserializing - SuppressedStorageSource now retains raw bytes, so rehoming keeps a BOM, foreign encodings, and undecodable bytes verbatim. - The rehome path no longer mutates the suppression record; the source location stays skip-protected even if a failed multi-file save rolls Uri back. - TypeFormat.ToType returns null for unparsable names instead of leaking parser exceptions, and the legacy discriminator fill-in keys on string presence so garbage $type recovers instead of loading as the default. - RestoreFromUri rejects a discriminator type incompatible with the expected type before instantiating it, preventing wrong-type load side effects (e.g. a Scene declared in a .belm globbing element files).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8acc3245f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- The legacy discriminator fill-in keys on the $type/@type property key alone, so a non-string or blank discriminator recovers instead of loading as the legacy default. - The recovery filter inverts to catch every non-filesystem failure (value converters throw freely, e.g. FormatException from Color.Parse); IOException/UnauthorizedAccessException still propagate. - Fallback creation records a thread-local incident so elements whose only fallback lives outside the hierarchy (e.g. a plain property or keyframe value) are still byte-frozen. - A recovered element that surfaces an Id another element owns yields it and falls back to its deterministic path-derived identity. - The rehome branch is create-only: it never overwrites an existing file, preserving manual repairs at the destination. - The toolkit's Save As keeps element sidecar file names so path-derived recovery identities stay stable across rehoming.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 243-271: Replace the File.Exists check and subsequent
File.WriteAllBytes call in the suppressedObj rehoming branch with an atomic
FileMode.CreateNew write, preserving the existing-file behavior by catching the
already-exists condition, updating suppressedObj.Uri, and returning without
overwriting the file.
In `@src/Beutl.Core/TypeFormat.cs`:
- Around line 13-25: Update ParseNestedType to guard the resolved type before
calling MakeGenericType, returning null when parent?.GetNestedType or
_assembly?.GetType yields null. Also catch ArgumentException from
Type.MakeGenericType, while preserving the existing null result behavior for
malformed type names.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8db0d964-a15f-463c-8910-0be1fe9be983
📒 Files selected for processing (15)
src/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/CoreObject.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Core/Serialization/DeserializationIncidents.cssrc/Beutl.Core/Serialization/FallbackDeserializationHelper.cssrc/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cssrc/Beutl.Core/Serialization/SuppressedStorageSource.cssrc/Beutl.Core/TypeFormat.cssrc/Beutl.Editor/AutoSaveService.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cstests/Beutl.UnitTests/Serialization/DeserializationIncidentsTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Beutl.Editor/AutoSaveService.cs
- tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e663648f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…riminator parsing - The create-only rehome now opens the destination with FileMode.CreateNew, closing the exists-check/write race; an already-existing file is treated as the protected skip path and other IO failures still propagate. - TypeNameParser.ParseNestedType returns null when the assembly or nested type cannot be resolved instead of calling MakeGenericType on null, and ToType's filter also absorbs ArgumentException so malformed generic discriminators read as unknown types.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d9a6212a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…command, and nested rehome paths - Nested deserialization (TryDeserializeCoreSerializable and DeserializeFromJsonObject) rejects a discriminator type that is not assignable to the expected base before instantiating it, so a Scene declared inside Objects becomes a fallback instead of recursively reopening its own sidecar. - open_project warning collection traverses each property's keyframe animation values, surfacing fallbacks that live only in keyframes. - Scene's DeleteCommand skips File.Delete for elements carrying a suppressed storage source, so deleting a recovered element keeps the retained sidecar bytes. - Save As preserves each sidecar's scene-relative subpath (falling back to the basename for rooted/escaping paths), keeping path-derived recovery identities stable for nested layouts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f89260e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 (4)
src/Beutl.ProjectSystem/ProjectSystem/Scene.cs (3)
712-715: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject generated IDs that are already claimed.
The
claimedIds.Add(child.Id)result is ignored after assigning the path-derived ID. If that ID matches a healthy element or an earlier recovered element, duplicateElement.Idvalues remain. Check each generated candidate and derive another deterministic candidate when the candidate is already claimed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 712 - 715, Update the recovered-element ID assignment in the surrounding recovery method to check the result of claimedIds.Add for the path-derived candidate. When the candidate is already claimed, deterministically derive another candidate and retry until it can be added, then assign that unique ID to child.Id.
741-745: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftLimit recovery to content-related exceptions.
This catch block converts every non-I/O
Exceptioninto a disabled fallback element, including conversion failures as intended, but also programming errors and fatal runtime conditions. Project loading can then succeed while hiding the failure and preserving only raw bytes. Catch the known deserialization and conversion exceptions, and rethrow cancellation plus fatal runtime exceptions such asOutOfMemoryException.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 741 - 745, Update the recovery catch filter in Scene loading to catch only the known deserialization and value-conversion exceptions needed for fallback handling. Explicitly exclude cancellation and fatal runtime exceptions such as OutOfMemoryException, while preserving propagation of filesystem failures and unexpected programming errors.
799-813: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a root object and a non-empty ID before using the recovered ID.
Guid.TryParse("00000000-0000-0000-0000-000000000000", out var id)returnstruewhile reassigningGuid.Empty;FindTopLevelIdMatchalso accepts an inner object of a root array. Require the root JSON object andtopLevelId != Guid.Emptybefore callingreturn topLevelId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 799 - 813, Update ResolveRecoveredElementId to require the recovered JSON root to be an object, ensure FindTopLevelIdMatch does not accept an inner object from a root array, and only return topLevelId when the parsed value is non-empty (topLevelId != Guid.Empty). Otherwise preserve the existing deterministic filename GUID fallback.src/Beutl.Core/Serialization/CoreSerializer.cs (1)
266-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate write failures after a successful
CreateNew.The
tryblock also coversstream.Write; dispose-related failures can follow the same path. If creation succeeds but writing or disposal fails, the destination still exists, so the catch treats the partial file as an existing destination and returns successfully. Catch the existing-file error only aroundFileStreamconstruction. Let write and disposal failures propagate, and remove any partial destination.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Core/Serialization/CoreSerializer.cs` around lines 266 - 280, Restrict the existing-file IOException handling in the recovery flow to the FileStream construction in CoreSerializer, so stream.Write and disposal failures propagate instead of being treated as successful recovery. If writing or disposal fails after creation, remove the partial rehomedPath destination before rethrowing, while preserving the existing-file behavior for CreateNew collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 70-75: Update both CoreSerializer entry points and
TryDeserializeCoreSerializable to resolve an existing $type/@type discriminator
before checking baseType.IsSealed. Validate that the resolved actualType is
assignable to baseType before instantiation; use baseType only when neither
discriminator key is present and fallback is required.
---
Outside diff comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 266-280: Restrict the existing-file IOException handling in the
recovery flow to the FileStream construction in CoreSerializer, so stream.Write
and disposal failures propagate instead of being treated as successful recovery.
If writing or disposal fails after creation, remove the partial rehomedPath
destination before rethrowing, while preserving the existing-file behavior for
CreateNew collisions.
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 712-715: Update the recovered-element ID assignment in the
surrounding recovery method to check the result of claimedIds.Add for the
path-derived candidate. When the candidate is already claimed, deterministically
derive another candidate and retry until it can be added, then assign that
unique ID to child.Id.
- Around line 741-745: Update the recovery catch filter in Scene loading to
catch only the known deserialization and value-conversion exceptions needed for
fallback handling. Explicitly exclude cancellation and fatal runtime exceptions
such as OutOfMemoryException, while preserving propagation of filesystem
failures and unexpected programming errors.
- Around line 799-813: Update ResolveRecoveredElementId to require the recovered
JSON root to be an object, ensure FindTopLevelIdMatch does not accept an inner
object from a root array, and only return topLevelId when the parsed value is
non-empty (topLevelId != Guid.Empty). Otherwise preserve the existing
deterministic filename GUID fallback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bcaef780-8c81-434f-9965-efa26f1768ba
📒 Files selected for processing (8)
src/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cs
- src/Beutl.AgentToolkit/Sessions/FileEditingSession.cs
- tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs
- src/Beutl.AgentToolkit/Tools/SessionTools.cs
- tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs
- tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
…m, and recovery scanning - A rehome write failure after FileMode.CreateNew succeeded deletes the partial file and rethrows instead of being misread as a pre-existing repair; only a create-phase failure with the file present skips. - Recovered-id dedupe processes children in stable sidecar-path order (the parallel load is unordered) and derives further deterministic UUIDv5 candidates when the path-derived replacement is itself taken. - Fallback projections keep the original discriminator; the runtime fallback type is written only when the projection has none. - Save As containment resolves the full path instead of a '..' prefix test, so directories like '..assets' keep their subpath. - The top-level Id scan tracks array nesting, so a root-array sidecar's inner Id is never adopted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be2f05d0a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k edits with non-hierarchical fallbacks - Recovered-id UUIDv5 inputs and the dedupe ordering key normalize the scene-relative path to forward slashes, so identities match across Windows and Unix. - Collision remaps persist in the scene file (RecoveredElementIds, path -> Guid): a remap applies on load regardless of the current claimant set, entries are pruned when their sidecar heals or leaves, and a persisted id a healthy element now owns is dropped in favor of a fresh deterministic derivation. - Recovery warnings name the scene-relative sidecar path, so same-named files in different subdirectories are distinguishable. - Reconciler baseline collection uses the same full serialized-graph traversal as its sandbox (property values and keyframe animation values), so a pre-existing non-hierarchical fallback no longer rejects every apply_edit. - The sealed-baseType discriminator short-circuit is kept and documented: sealed wrappers such as Optional<T> carry the wrapped payload's $type on their own node and interpret it themselves.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59bd61dd4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tively, and copy atomically - A zero-GUID top-level Id reads as unreadable and falls back to the deterministic path-derived identity. - Recovered-id claims are seeded from the scene's own Id, healthy element Ids, and their descendant CoreObject Ids. - The persisted RecoveredElementIds map is authoritative for every recovered element and is rebuilt at serialize time from current paths, so rehoming or flattening a sidecar keeps its identity. - Rehome copies write to a temp sibling and File.Move without overwrite, so an interrupted copy leaves no partial file that a later save would mistake for a manual repair.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b0eb8ff33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…covery incidents
- Fallback projection normalization walks the full serialized graph
(hierarchical children, property values, keyframe values), so an
ID-less fallback outside the hierarchy gets its instance Id into the
projection and reconciliation no longer regenerates it under a new Id.
- open_project additionally returns recoveryIncidents — structured
{ elementFile, reason, typeName, message } records beside the
presentation warnings — and the MCP contract documents both.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: defc57d331
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Beutl.AgentToolkit/Tools/SessionTools.cs (1)
152-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one traversal helper with
Reconciler.
CollectFallbacksrepeats the traversal implemented byTraverseSerializedGraphinsrc/Beutl.AgentToolkit/Reconciliation/Reconciler.cs(Lines 724-832): engine-object properties, keyframe values, enumerables, and reference-identity cycle protection. The two copies already differ.TraverseSerializedGraphskips the visited-set insert for value types, whileCollectFallbacksadds every boxed struct value to the set. Extract one internal traversal helper and call it from both sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.AgentToolkit/Tools/SessionTools.cs` around lines 152 - 188, Extract the shared graph traversal logic from CollectFallbacks and Reconciler’s TraverseSerializedGraph into one internal helper, preserving traversal of EngineObject properties, keyframe values, enumerables, and reference-identity cycle protection. Ensure value types are not inserted into the visited set, then update both callers to use the shared helper while retaining their respective fallback collection behavior.tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs (1)
63-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for the repeated "restore twice, assert stable ID" pattern.
Five new tests in this file repeat the same shape: restore the scene twice, read a
Guid, assert it is notGuid.Empty(and sometimes not equal to a specific forbidden value), then assert the second restore yields the same value as the first. Extract a small helper, for exampleAssertStableRecoveredId(Uri sceneUri, Func<Scene, Guid> selectId, params Guid[] forbiddenIds), and call it from each site. This reduces duplicated restore/assert boilerplate and keeps each test focused on its distinguishing setup.
- Line 63-79 (
Restore_MalformedElementWithEmptyTopLevelId_UsesStableNonEmptyId): replace the manual restore-twice/assert block with the shared helper.- Line 373-389 (
Restore_TopLevelIdMatchingSceneId_IsReassignedStably): same replacement, passingsceneIdas a forbidden value.- Line 391-419 (
Restore_TopLevelIdMatchingHealthyDescendantId_IsReassignedStably): same replacement, passingdescendantIdas a forbidden value.- Line 477-493 (
Restore_MalformedSubdirectoryElementIdUsesForwardSlashRelativePath): same replacement, keeping its extra golden-value assertions alongside the helper call.- Line 578-594 (
Restore_RootArrayId_DoesNotAdoptInnerIdAndRemainsStable): same replacement, passinginnerIdas a forbidden value.Also applies to: 373-389, 391-419, 477-493, 578-594
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs` around lines 63 - 79, Extract a shared AssertStableRecoveredId helper in the test class that restores the scene twice, selects the ID via Func<Scene, Guid>, rejects Guid.Empty and any supplied forbidden IDs, and verifies both restores match. Replace the duplicated restore/assert blocks in the five named tests with helper calls, passing sceneId, descendantId, or innerId where required, while preserving the extra golden-value assertions in Restore_MalformedSubdirectoryElementIdUsesForwardSlashRelativePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/001-agent-editing-toolkit/contracts/mcp-tools.md`:
- Line 100: Update the open_project output contract to use the camelCase keys
warnings and recoveryIncidents, and document reason using the exact possible
FallbackReason enum names. Ensure the listed reason values match the fixed
FallbackReason definition rather than retaining
TypeNotFound/DeserializationFailed unless those are the enum members.
---
Nitpick comments:
In `@src/Beutl.AgentToolkit/Tools/SessionTools.cs`:
- Around line 152-188: Extract the shared graph traversal logic from
CollectFallbacks and Reconciler’s TraverseSerializedGraph into one internal
helper, preserving traversal of EngineObject properties, keyframe values,
enumerables, and reference-identity cycle protection. Ensure value types are not
inserted into the visited set, then update both callers to use the shared helper
while retaining their respective fallback collection behavior.
In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs`:
- Around line 63-79: Extract a shared AssertStableRecoveredId helper in the test
class that restores the scene twice, selects the ID via Func<Scene, Guid>,
rejects Guid.Empty and any supplied forbidden IDs, and verifies both restores
match. Replace the duplicated restore/assert blocks in the five named tests with
helper calls, passing sceneId, descendantId, or innerId where required, while
preserving the extra golden-value assertions in
Restore_MalformedSubdirectoryElementIdUsesForwardSlashRelativePath.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 882a0b5c-35cf-4fe8-971f-feb8a3cf75a2
📒 Files selected for processing (10)
docs/specs/001-agent-editing-toolkit/contracts/mcp-tools.mdsrc/Beutl.AgentToolkit/Reconciliation/Reconciler.cssrc/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Reconciliation/ReconcilerIdIntegrityTests.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Beutl.AgentToolkit/Sessions/FileEditingSession.cs
- tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
- src/Beutl.Core/Serialization/CoreSerializer.cs
…stically - Recovered-id claims now cover every element's full serialized-graph descendants; healthy elements and their objects claim first. - A recovered element's descendant whose Id another scene object owns is reassigned to a deterministic UUIDv5 derived from the sidecar's relative path and the original Id, stable across loads, with fallback projections refreshed to the final Id. The byte-frozen sidecar is untouched.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e69843fe1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ecovery lifecycle - Serializing a Uri-less in-memory scene no longer throws; the recovered id rebuild skips gracefully. - An unresolvable keyframe Easing records a deserialization incident, so the element is byte-frozen instead of silently losing the string. - Repairing the last fallback (paste or fallback editor SetJsonString) clears the element's storage suppression, so saves resume once the graph is clean. - A repaired sidecar loading healthy with a new id migrates group references from the persisted placeholder id to the actual id. - Recovered descendant remaps persist (RecoveredDescendantIds keyed by path!originalId) and apply before duplicate detection, surviving claimant removal deterministically. - The recovery filter unwraps TargetInvocationException/AggregateException chains, so a plugin constructor's wrapped IO failure propagates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c285943d00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @yuto-trd. * #2159 (comment) The following files were modified: * `src/Beutl.AgentToolkit/Reconciliation/Reconciler.cs` * `src/Beutl.AgentToolkit/Sessions/FileEditingSession.cs` * `src/Beutl.AgentToolkit/Tools/SessionTools.cs` * `src/Beutl.Core/Serialization/CoreSerializer.cs` * `src/Beutl.Core/Serialization/DeserializationIncidents.cs` * `src/Beutl.Core/Serialization/FallbackDeserializationHelper.cs` * `src/Beutl.Core/TypeFormat.cs` * `src/Beutl.Editor/AutoSaveService.cs` * `src/Beutl.Editor/Services/ElementObjectService.cs` * `src/Beutl.Engine/Animation/KeyFrame.cs` * `src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` * `src/Beutl/ViewModels/Editors/AudioEffectEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/BaseEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/BrushEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/FilterEffectEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/GeometryEditorViewModel.cs` * `src/Beutl/ViewModels/Editors/TransformEditorViewModel.cs`
…ery incidents - Descendant remaps key on path!originalId#occurrence, so two descendants sharing a serialized id keep distinct persisted identities without swapping across sessions. - Save As reserves unique destinations when flattened escaping sidecars share a basename. - Recovery incidents carry sceneId/sceneName so multi-scene projects stay distinguishable; the contract documents them. - Clearing storage suppression after a fallback repair is recorded in the same history transaction, so undoing the repair restores the freeze. - The recovery placeholder projection preserves an extractable top-level $type string, so incidents report the original declared type.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f6048ed1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ries, and migrate references - SuppressedStorageSource tracks non-fallback incidents (a lossy value substitution such as an unresolvable easing); repairing the last fallback no longer unfreezes such elements — only an external file repair can. - open_project reports elements frozen solely by the incident tally (no fallback object) as structured incidents and warnings. - Placeholder-id migrations now also rewrite IReference-typed property values, not just Groups, so healthy references follow a repaired element's new id. - Descendant remap occurrences are anchored at load time and reused verbatim when rebuilding the persisted map, so in-editor mutations cannot shift which key maps to which frozen-file descendant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef21083e83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… order and scanning - Every editor value-replacement path (SetValue overloads, ApplyTemplate, TryPasteJson, drawable-brush inner replacement) runs the fallback resume hook with the clear recorded in the same history transaction. - Recovered ELEMENT ids claim before any recovered descendant, so a descendant sharing its element's id is the side that remaps and groups keep resolving. - The top-level scan requires an object root and stops at its close, so trailing JSON after the first root can no longer donate an Id. - Storing a suppressed object to a non-file URI throws like the ordinary path instead of reporting silent success.
|
No TODO comments were found. |
| if (property.CurrentValue is not IReference reference | ||
| || !_pendingRecoveredElementIdMigrations.TryGetValue(reference.Id, out Guid migratedId)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| Element? target = Children.FirstOrDefault(child => child.Id == migratedId); | ||
| property.CurrentValue = target is not null && reference.ObjectType.IsInstanceOfType(target) | ||
| ? reference.Resolved(target) | ||
| : Activator.CreateInstance(reference.GetType(), migratedId)!; |
There was a problem hiding this comment.
When a ReferenceExpression targets a recovered element whose placeholder ID is migrated, this loop updates only property.CurrentValue and leaves the expression's ObjectId unchanged, causing later expression evaluation to miss its target and return a null or default value.
Knowledge Base Used: Beutl.Core
Minimum allowed line rate is |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs (1)
116-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest does not exercise a real
..parent-directory segment.
Path.Combine("..assets", "clip.belm")builds a path segment literally named"..assets". This is a folder name that starts with two dot characters. It is not a..parent-directory reference.A real parent-directory reference needs
..as its own path segment, for examplePath.Combine("..", "assets", "clip.belm").The stated test intent describes a sidecar path that starts with
..as parent-directory escaping. The current code does not test that case. It tests an unrelated folder-naming edge case instead. The real..-escaping scenario for sidecar rehoming remains unverified.🧪 Proposed fix to test a real parent-directory segment
- string relativePath = Path.Combine("..assets", "clip.belm"); + string relativePath = Path.Combine("..", "assets", "clip.belm");Based on the line-range change details describing this test as covering "an element sidecar path beginning with
..(../assets/clip.belm)", which the current code does not actually exercise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs` around lines 116 - 141, Update the test method SetProjectPath_keeps_element_sidecar_subpaths_starting_with_two_dots so relativePath uses a real parent-directory segment, such as Path.Combine("..", "assets", "clip.belm"), instead of the literal "..assets" folder name. Preserve the existing setup and assertion so the test verifies sidecar rehoming for ../assets/clip.belm.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Engine/Animation/KeyFrame.cs`:
- Around line 57-63: Update the easing recovery logic in KeyFrame
deserialization around TypeFormat.ToType and Activator.CreateInstance to reject
abstract or otherwise non-instantiable Easing types, catch expected activation
failures, and route both cases through DeserializationIncidents.RecordFallback()
with new LinearEasing(). Preserve successful concrete easing activation.
In `@src/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cs`:
- Around line 181-190: Update ApplyTemplate to replace the direct
PropertyAdapter.SetValue(instance) call with the keyframe-aware SetValue
overload used by TryPasteJson, passing the previous value, new instance, and
CommandNames.ApplyTemplate. Preserve the existing template creation, expansion,
persistence-resume, commit, and success/failure behavior.
---
Outside diff comments:
In `@tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs`:
- Around line 116-141: Update the test method
SetProjectPath_keeps_element_sidecar_subpaths_starting_with_two_dots so
relativePath uses a real parent-directory segment, such as Path.Combine("..",
"assets", "clip.belm"), instead of the literal "..assets" folder name. Preserve
the existing setup and assertion so the test verifies sidecar rehoming for
../assets/clip.belm.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 863cea3b-2a74-4c02-bdcc-0c506d75372f
📒 Files selected for processing (23)
docs/specs/001-agent-editing-toolkit/contracts/mcp-tools.mdsrc/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/Properties/AssemblyInfo.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Core/Serialization/SuppressedStorageSource.cssrc/Beutl.Editor/Services/ElementObjectService.cssrc/Beutl.Engine/Animation/KeyFrame.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cssrc/Beutl.ProjectSystem/Properties/AssemblyInfo.cssrc/Beutl/ViewModels/Editors/AudioEffectEditorViewModel.cssrc/Beutl/ViewModels/Editors/BaseEditorViewModel.cssrc/Beutl/ViewModels/Editors/BrushEditorViewModel.cssrc/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cssrc/Beutl/ViewModels/Editors/FilterEffectEditorViewModel.cssrc/Beutl/ViewModels/Editors/GeometryEditorViewModel.cssrc/Beutl/ViewModels/Editors/GroupedEditorHelper.cssrc/Beutl/ViewModels/Editors/TransformEditorViewModel.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.HeadlessUITests/FallbackEditorPersistenceTests.cstests/Beutl.UnitTests/Editor/Services/ElementObjectServiceTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Beutl.Core/Serialization/SuppressedStorageSource.cs
- docs/specs/001-agent-editing-toolkit/contracts/mcp-tools.md
- src/Beutl.AgentToolkit/Tools/SessionTools.cs
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
- src/Beutl.Core/Serialization/CoreSerializer.cs
- src/Beutl.AgentToolkit/Sessions/FileEditingSession.cs
| Type? type = TypeFormat.ToType(easingType); | ||
| if (type is null || !type.IsAssignableTo(typeof(Easing))) | ||
| { | ||
| DeserializationIncidents.RecordFallback(); | ||
| Easing = new LinearEasing(); | ||
| } | ||
| else if (Activator.CreateInstance(type) is Easing easing) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 --glob '*.cs' \
'class\s+\w+\s*:\s*Easing|abstract\s+class\s+\w+\s*:\s*Easing|Activator\.CreateInstance|TypeFormat\.ToType|RecordFallback' \
src testsRepository: b-editor/beutl
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KeyFrame.cs =="
sed -n '1,130p' src/Beutl.Engine/Animation/KeyFrame.cs
echo
echo "== Easing definitions and discriminator usages =="
rg -n -C 4 --glob '*.cs' 'abstract class .*Easing|class .*: Easing|EasingType|Easing.*Discriminator|TypeFormat\.ToType\(easing|RecordFallback\(\)\)' src/Beutl.Engine src tests
echo
echo "== Behavioral probe for Activator.CreateInstance on abstract/no-public-ctor types =="
python3 - <<'PY'
# Read-only structural probe: determine whether source contains an abstract Easing subclass,
# and whether KeyFrame falls back if activation throws.
from pathlib import Path
import re
keyframe = Path('src/Beutl.Engine/Animation/KeyFrame.cs').read_text()
m = re.search(r'else\s+if\s+Activator\.CreateInstance\(type\)\s+is\s+Easing\s+easing\s*\{(?P<body>.*?)\}\s*(?:else\s+\{(?P<else>.*?)\})?\}', keyframe, re.S)
print("has_createinstance_else_if_block:", bool(m))
if m:
print("block_body_has_record_fallback_before_else_or_inside:", "RecordFallback" in (m.group('body') or "") + (m.group('else') or ""))
print("block_body_ends_with_assignment_to_Easing:", bool(re.search(r'#?\s*Easing\s*=\s*new\s+LinearEasing', (m.group('body') or "") + (m.group('else') or ""))))
print("block_body_has_try_catch_or_catch:", bool(re.search(r'\btry\b|\bcatch\b', (m.group('body') or "") + (m.group('else') or ""))))
for p in Path('src').rglob('*.cs'):
text = p.read_text(errors='ignore')
if ': Easing' in text:
for i, line in enumerate(text.splitlines(), 1):
if ': Easing' in line or 'abstract class' in line:
print(f"{p}:{i}:{line.strip()}")
PYRepository: b-editor/beutl
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KeyFrame deserialization tests =="
rg -n -C 6 --glob '*.cs' 'KeyFrame|Easing' tests | rg -n -C 6 'Deserialize|LinearEasing|Fallback|FallbackCount|easing|KeyTime|Value|Spline' || true
echo
echo "== Easing constructors =="
for f in src/Beutl.Engine/Animation/Easings/*.cs; do
echo "--- $f"
sed -n '1,70p' "$f"
done
echo
echo "== Static extractor for assignable/non-instantiable Easing candidates in repo =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('src').glob('**/*.cs'):
text = p.read_text(errors='ignore')
for i, line in enumerate(text.splitlines(), 1):
if re.search(r'(abstract\s+)?\bclass\s+\w+\s*:\s*\w*\w*Easing\b|:\s*Easing\b|\beasingType\b', line):
print(f"{p}:{i}:{line.strip()}")
PYRepository: b-editor/beutl
Length of output: 50371
Handle non-instantiable easing discriminators during recovery.
An Easing subtype can pass IsAssignableTo(typeof(Easing)) while still be abstract or lack an accessible parameterless constructor. The Activator.CreateInstance(type) at line 63 can throw before the fallback path runs, so a malformed sidecar can still abort deserialization. Reject these types and catch expected activation failures, then record the fallback and assign new LinearEasing().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Beutl.Engine/Animation/KeyFrame.cs` around lines 57 - 63, Update the
easing recovery logic in KeyFrame deserialization around TypeFormat.ToType and
Activator.CreateInstance to reject abstract or otherwise non-instantiable Easing
types, catch expected activation failures, and route both cases through
DeserializationIncidents.RecordFallback() with new LinearEasing(). Preserve
successful concrete easing activation.
| public override bool ApplyTemplate(ObjectTemplateItem template) | ||
| { | ||
| if (template.CreateInstance() is not T instance) return false; | ||
| T? previous = PropertyAdapter.GetValue(); | ||
| IsExpanded.Value = true; | ||
| PropertyAdapter.SetValue(instance); | ||
| ResumeElementPersistenceAfterFallbackReplacement(previous); | ||
| Commit(CommandNames.ApplyTemplate); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ApplyTemplate bypasses keyframe-aware replacement.
ApplyTemplate reads and writes PropertyAdapter directly. It does not check EditingKeyFrame.Value. TryPasteJson in this same file (Lines 192-212) checks EditingKeyFrame.Value and routes through SetValue(oldValue, newValue, commandName), which assigns kf.Value when a keyframe is being edited.
If a user edits a specific keyframe and applies a template, ApplyTemplate writes the new instance to the property adapter instead of to the active keyframe. The keyframe value stays unchanged. The edit target is wrong.
Route ApplyTemplate through the same keyframe-aware SetValue overload used by TryPasteJson.
🐛 Proposed fix to route ApplyTemplate through the keyframe-aware SetValue
public override bool ApplyTemplate(ObjectTemplateItem template)
{
if (template.CreateInstance() is not T instance) return false;
- T? previous = PropertyAdapter.GetValue();
IsExpanded.Value = true;
- PropertyAdapter.SetValue(instance);
- ResumeElementPersistenceAfterFallbackReplacement(previous);
- Commit(CommandNames.ApplyTemplate);
+ if (EditingKeyFrame.Value is { } kf)
+ {
+ SetValue(kf.Value, instance, CommandNames.ApplyTemplate);
+ }
+ else
+ {
+ SetValue(PropertyAdapter.GetValue(), instance, CommandNames.ApplyTemplate);
+ }
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public override bool ApplyTemplate(ObjectTemplateItem template) | |
| { | |
| if (template.CreateInstance() is not T instance) return false; | |
| T? previous = PropertyAdapter.GetValue(); | |
| IsExpanded.Value = true; | |
| PropertyAdapter.SetValue(instance); | |
| ResumeElementPersistenceAfterFallbackReplacement(previous); | |
| Commit(CommandNames.ApplyTemplate); | |
| return true; | |
| } | |
| public override bool ApplyTemplate(ObjectTemplateItem template) | |
| { | |
| if (template.CreateInstance() is not T instance) return false; | |
| IsExpanded.Value = true; | |
| if (EditingKeyFrame.Value is { } kf) | |
| { | |
| SetValue(kf.Value, instance, CommandNames.ApplyTemplate); | |
| } | |
| else | |
| { | |
| SetValue(PropertyAdapter.GetValue(), instance, CommandNames.ApplyTemplate); | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Beutl/ViewModels/Editors/CoreObjectEditorViewModel.cs` around lines 181 -
190, Update ApplyTemplate to replace the direct
PropertyAdapter.SetValue(instance) call with the keyframe-aware SetValue
overload used by TryPasteJson, passing the previous value, new instance, and
CommandNames.ApplyTemplate. Preserve the existing template creation, expansion,
persistence-resume, commit, and success/failure behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e14fe9660
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| internal static SuppressedStorageSource? TryResumeElementPersistence(Element element) | ||
| { | ||
| if (element.SuppressedStorageSource is not { } source | ||
| || source.HasNonFallbackIncidents | ||
| || EnumerateSerializedGraphFallbacks(element).Any()) |
There was a problem hiding this comment.
Fresh evidence after the earlier UI repair fix is that the final tree still has repair paths which never invoke this helper: Reconciler.ApplyCore can remove the last fallback through Documents.Write, and TextureSourceEditorViewModel.SetDrawableType can replace a nested FallbackDrawable directly. In either case the edit appears successful, but SuppressedStorageSource remains set, so the next save skips or copies the old sidecar and the repair disappears after reopening. Route these paths through the reversible resumption logic and add NUnit regressions for both entry points.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| if (property.CurrentValue is not IReference reference | ||
| || !_pendingRecoveredElementIdMigrations.TryGetValue(reference.Id, out Guid migratedId)) | ||
| { |
There was a problem hiding this comment.
Fresh evidence against the earlier migration response is that this final implementation inspects only property.CurrentValue; references stored in property.Expression are serialized separately as IReferenceExpression.ObjectId and never enter this branch. When an externally repaired sidecar adopts a new ID, a healthy ReferenceExpression<T> that still names the persisted placeholder therefore stops resolving even though ordinary IReference values and groups are migrated. Migrate reference expressions while preserving their property paths, and cover that serialized form with an NUnit regression.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| if (fallbacks.Count == 0 | ||
| && element.SuppressedStorageSource is { HasNonFallbackIncidents: true }) |
There was a problem hiding this comment.
When one sidecar contains both an IFallback and a lossy non-fallback substitution such as an invalid easing, HasNonFallbackIncidents is true but this fallbacks.Count == 0 condition suppresses the corresponding incident. open_project then reports only the fallback; after that fallback is repaired, persistence remains frozen because the unreported incident still blocks TryResumeElementPersistence, leaving callers unable to explain why saves retain the old file. Emit the non-fallback incident independently of whether fallback objects also exist, with a mixed-incident NUnit test.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| private static readonly Regex s_typePattern = new( | ||
| "\"\\$type\"\\s*:\\s*(?<type>\"(?:\\\\.|[^\"\\\\])*\")", | ||
| RegexOptions.CultureInvariant); |
There was a problem hiding this comment.
For a recovered sidecar whose top-level discriminator uses the supported legacy @type key, this scanner never captures the original type because it matches only $type, even though JsonHelper.TryGetDiscriminator and the new compatibility checks explicitly accept both keys. The placeholder consequently receives the runtime fallback discriminator, which the warning collector removes, and RecoveryIncident.TypeName becomes null precisely when the missing type is needed for repair. Scan both discriminator keys and add an NUnit case for an unknown or malformed @type.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| .OrderBy(static item => item.RelativePath, StringComparer.Ordinal) | ||
| .ToArray(); | ||
|
|
||
| var claimedIds = new HashSet<Guid> { Guid.Empty, Id }; |
There was a problem hiding this comment.
Fresh evidence after the earlier scene-wide collision fix is that claimedIds still starts with only the scene ID and is subsequently populated solely from elements and their descendants; TimelineLayer and SceneMarker are hierarchical CoreObjects in the same scene but are never reserved. If a malformed sidecar declares one of their IDs as its top-level ID, the recovered element keeps the duplicate and hierarchy-based lookup can resolve the layer or marker instead of the element, misdirecting references and queries. Seed claims from every non-recovered scene object and add NUnit coverage for both collections.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| internal static SuppressedStorageSource? TryResumeElementPersistence(Element element) | ||
| { | ||
| if (element.SuppressedStorageSource is not { } source | ||
| || source.HasNonFallbackIncidents |
There was a problem hiding this comment.
Fresh evidence after introducing HasNonFallbackIncidents is that the flag is immutable after load: it is only set when suppression is created, and this condition rejects resumption forever. For example, after an invalid easing is substituted, explicitly choosing a valid easing in the graph or timeline editor still leaves the element suppressed, so saving retains the malformed bytes and the visible repair disappears on reopen. Track resolvable incidents by location and clear them through an undoable repair path, with an NUnit regression proving that repairing the last such incident resumes persistence.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| if (easingNode is JsonValue easingTypeValue | ||
| && easingTypeValue.TryGetValue(out string? easingType)) |
There was a problem hiding this comment.
Fresh evidence beyond the malformed-string easing fix is that this branch records an incident only when a JsonValue can be read as a string. If serialized Easing is a number, boolean, or another unsupported primitive, neither this branch nor the JsonObject branch runs, so the default easing is silently retained, the element is considered healthy, and the next save overwrites the malformed value instead of preserving the sidecar. Record an incident for every present easing node that is neither a valid type-name string nor the supported spline object, with NUnit coverage for primitive and null inputs.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| var fallback = new FallbackEngineObject | ||
| { | ||
| Name = "Unreadable element data", | ||
| Reason = FallbackReason.DeserializationFailed, |
There was a problem hiding this comment.
For a syntactically valid element whose top-level discriminator names an unavailable plugin type, recovery always assigns DeserializationFailed here even though the discriminator was successfully extracted and cannot be resolved. The documented structured contract exposes reason specifically as TypeNotFound | DeserializationFailed, so clients will misclassify the common missing-plugin case and may offer generic corruption guidance instead of installation or plugin-repair actions. Choose TypeNotFound when the captured discriminator is unresolvable, retain DeserializationFailed for known types that fail to load, and add an NUnit assertion for the top-level case.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
Description
.belmsidecar no longer parses previously failed the whole project open. Such elements now load as disabled fallback elements that retain the original raw text, andopen_projectreports a warning naming the file and the parser error while healthy elements keep loading and rendering.CoreSerializerlevel, so every save path —Scene.Serialize, the editor's Ctrl+S child loop, the auto-save service, and toolkit saves — leaves the un-parseable file byte-identical on disk. The auto-save delete branch exempts them, so removing a recovered element cannot destroy the recoverable sidecar.$typeandIdso edits to unrelated elements reconcile normally.Affected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Breaking changes
None. Opening a project with a corrupt element previously failed outright; it now succeeds with a warning and never rewrites the corrupt file.
Test plan
tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs— byte-identity across open/edit/save for truncated and wrong-$typesidecars, stable Ids across repeated opens (including nested-Id-first inputs), auto-save delete exemption, direct childStoreToUriandAutoSaveService.SaveObjectsdesktop paths.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs—open_projectwarnings naming the file and parser error; no false positives on healthy projects; edits to unrelated elements succeed while a recovered element exists.Fixed issues / References
Extracted from the feature-004 review hardening so it can land independently of
speckit/004-gpu-pass-fusion.Summary by CodeRabbit
New Features
Bug Fixes