feat!: recover malformed elements and preserve repair workflows - #2159
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe project system recovers malformed element sidecars with stable IDs and suppressed writes. Project opening reports deserialization warnings and structured incidents. 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
Suggested labels: 🚥 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 reviewable code changes were analyzed. |
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (44): Last reviewed commit: "feat!: complete recovered reference migr..." | 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}
left a comment
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}
left a comment
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".
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.
left a comment
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).
left a comment
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.
left a comment
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
left a comment
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.
left a comment
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.
left a comment
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".
left a comment
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.
left a comment
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.
left a comment
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".
commented
Aug 15, 2026
|
@codex review |
left a comment
There was a problem hiding this comment.
💡 Codex Review
beutl/src/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cs
Lines 125 to 126 in 9c0ccf9
Propagate fatal failures before creating nested fallbacks
When a nested plugin type with a declared fallback, such as a Transform subclass, throws AccessViolationException or OutOfMemoryException from its constructor or deserializer, this filter converts the wrapped failure into the fallback and the Scene-level fatal-failure guard never sees it. Fresh evidence beyond the resolved Scene-level thread is that this nested catch, along with the equivalent CoreSerializer filters, still calls TryCreateFallback without checking ExceptionHelpers.ContainsFatalFailure; reject fatal exception chains before fallback conversion.
AGENTS.md reference: AGENTS.md:L60-L60
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62b60d2583
ℹ️ 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".
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c798cb6815
ℹ️ 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".
BREAKING CHANGE: Beutl.Engine plugin implementations must implement IProperty.ReplaceCurrentValue, IProperty<T>.ReplaceCurrentValue, IKeyFrame.ReplaceValue, and IReferenceExpression.Rebind, then rebuild. Replacement hooks must preserve validation and notifications while replacing equality-equivalent references. Rebind must preserve expression state for the new ID or explicitly return null. Beutl.Core static property setters now propagate scoped reference-replacement intent through SetAndRaise.
yuto-trd
changed the title
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d892ff17
ℹ️ 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".
Capture referenced storage reads independently of plugin graph traversal, retain URIs on content-failure fallbacks, and preserve original easing JSON until an explicit replacement. Cover Save As, repair, and undo lifecycles. BREAKING CHANGE: Beutl.AgentToolkit.Common.PathComparison and PathBoundary have been removed. Replace PathComparison.ForCurrentPlatform with Beutl.FilePathComparison.Comparison from Beutl.Core. There is no public replacement for PathBoundary.ResolveExistingPath(string) or ResolveDeepestExistingTarget(string); consumers must provide their own symlink-aware canonicalization and boundary policy. Beutl.PathBoundary is internal.
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbec02855e
ℹ️ 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".
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e9f950d01
ℹ️ 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".
Preserve nullable values, dictionary/set comparison policies, wrapped scene identities, and retained sidecars after undoing a rehomed repair. Unify built-in editors on the public repair completion service and verify the contract from a non-friend test assembly. BREAKING CHANGE: Beutl editor subclasses must replace ResumeElementPersistenceAfterFallbackReplacement(previous) and ResumeElementPersistenceAfterKnownRecoveryBlockerReplacement() with CompleteElementRepair(). External editors can use Beutl.Editor.Services.ElementRecoveryService.TryCompleteRepair(element, history) before committing their repair transaction.
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 621e80ba58
ℹ️ 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".
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7c1df2577
ℹ️ 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".
commented
Sep 8, 2026
|
No TODO comments were found. |
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd0075439b
ℹ️ 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".
Rebuild immutable collections with their comparers and order, support opted-in composite expressions, propagate fatal migration failures, and keep transient plugin diagnostics out of incident identity. Preserve Undo restoration after a failed nested copy and reuse the shared platform path comparison policy. BREAKING CHANGE: Beutl.Engine plugin implementations must implement IProperty.ReplaceCurrentValue(object?), IProperty<T>.ReplaceCurrentValue(T), IKeyFrame.ReplaceValue(object?), and IReferenceExpression.Rebind(Guid), then rebuild. Replacement must preserve distinct reference identity; Rebind must preserve custom state or return null. Beutl.Editor.Components custom editors must replace the removed ResumeElementPersistenceAfterFallbackReplacement and ResumeElementPersistenceAfterKnownRecoveryBlockerReplacement helpers with CompleteElementRepair(). Beutl.AgentToolkit consumers must use Beutl.Core FilePathComparison.Comparison instead of PathComparison.ForCurrentPlatform and supply their own policy for removed public PathBoundary helpers.
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 580bb7e034
ℹ️ 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".
|
|
||
| protected ImmutableArray<CoreObject?> GetStorables() => [_element]; | ||
|
|
||
| protected void CompleteElementRepair() |
There was a problem hiding this comment.
When an unavailable DisplacementMapTransform is loaded as a null slot with a non-fallback incident, DisplacementMapTransformEditorViewModel.TryPasteJson still assigns the pasted transform directly at lines 160–168 and commits without reaching this new repair-completion hook. Suppression therefore remains active, so saving preserves the malformed .belm and the pasted repair disappears after reopening. Fresh evidence beyond the resolved ChangeType/null-slot thread is this separate paste path; route it through SetValue or call CompleteElementRepair before committing, and add a lifecycle regression.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
| foreach (Element item in Children) | ||
| { | ||
| CoreSerializer.StoreToUri(item, item.Uri!); | ||
| CoreSerializer.StoreToUri(item, item.Uri!, sidecarRoot); |
There was a problem hiding this comment.
When a recovered element retains a URI-backed source whose path is also the sidecar URI of a later live element—a valid possibility for plugin-owned serialized graphs—this loop has no global reservation for retained destinations. During Save As, the recovered child can copy its retained bytes first and the later StoreToUri then overwrites that same path normally, leaving the recovered .belm pointing at bytes different from those captured for recovery; reversing child order can instead reject the save on a byte mismatch. Preflight live and retained destinations and reject or safely rehome such collisions before writing any child.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
|
|
||
| public static bool ContainsNonRecoverableFileSystemFailure(Exception exception) | ||
| => Contains(exception, static current => current is UnauthorizedAccessException | ||
| or IOException and not FileNotFoundException); |
There was a problem hiding this comment.
When a plugin element constructor or deserializer accesses a resource beneath a directory that has been moved or deleted, .NET throws DirectoryNotFoundException, but this predicate classifies every IOException except FileNotFoundException as non-recoverable. Scene.RestoreElementOrFallback therefore lets that exception abort the entire scene load, while the equivalent missing-file case becomes a disabled recovered element and preserves the other healthy elements. Exclude DirectoryNotFoundException from this predicate as well and add the corresponding recovery regression.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
Description
Opening a scene with a malformed or partially unavailable
.belmsidecar now recovers the affected element and reports structured warnings while healthy elements remain usable. Recovered sidecars retain their original bytes through save, autosave, removal, and Save As. Once the last recovery blocker is repaired, normal persistence resumes; undo restores the protected source bytes.Recovery assigns stable, collision-free IDs and migrates references without replacing a surviving owner. It includes scene-owned plugin property graphs, preserves every logical retained-sidecar path, and avoids overwriting unrelated files during rehoming. Fatal plugin constructor failures propagate instead of being converted to content fallbacks. Declarative edits reject newly introduced fallbacks, including values hidden inside plugin wrappers.
Plugins can complete repairs through
ElementRecoveryService.TryCompleteRepair(element, history)before committing their transaction. The shared service is used by built-in editors and preserves Undo/Redo behavior. Scene-owned identity capture includes serialized wrappers; reference migration preserves read-only and immutable collection comparison policies and sequence order. Composite expressions can opt in through IReferenceRewritable.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)Also changes
Beutl.Coreserialization/property replacement andBeutl.AgentToolkitvalidation/persistence.Breaking changes
Plugins implementing the following
Beutl.Enginecontracts must update their implementations and rebuild against this version:IProperty.ReplaceCurrentValue(object?)andIProperty<T>.ReplaceCurrentValue(T): implement reference replacement that accepts a distinct reference even when value equality reports equality. Preserve the implementation's validation and change-notification behavior. Value types retain value-equality semantics.IKeyFrame.ReplaceValue(object?): implement the same explicit replacement semantics for keyframe values.IReferenceExpression.Rebind(Guid): return an equivalent expression for the new target ID while preserving all implementation-specific state and its property path. Implementations that cannot preserve their state must explicitly returnnull; recovery then retains the original expression. This member intentionally has no compatibility default.These are required authoring contracts, not binary-compatible additions. Static
CorePropertyaccessors usingSetAndRaisenow receive scoped forced-reference replacement intent through their existing setter.The public
Beutl.AgentToolkit.Common.PathComparisonandBeutl.AgentToolkit.Common.PathBoundaryhelpers have been removed. ReplacePathComparison.ForCurrentPlatformwithBeutl.FilePathComparison.ComparisonfromBeutl.Core. There is no public replacement forPathBoundary.ResolveExistingPath(string)orPathBoundary.ResolveDeepestExistingTarget(string): consumers must implement their own symlink-aware canonicalization and boundary policy. The sharedBeutl.PathBoundaryimplementation is internal and cannot be used by external consumers.Custom editor subclasses should replace calls to
ResumeElementPersistenceAfterFallbackReplacement(previous)andResumeElementPersistenceAfterKnownRecoveryBlockerReplacement()withCompleteElementRepair(). Repair completion now checks the remaining graph without requiring the removed value. See element recovery for the public plugin API.Test plan
Fixed issues / References
Extracted from the feature-004 review hardening so it can land independently of
speckit/004-gpu-pass-fusion.BREAKING CHANGE: Beutl.Engine plugin implementations must implement IProperty.ReplaceCurrentValue(object?), IProperty.ReplaceCurrentValue(T), IKeyFrame.ReplaceValue(object?), and IReferenceExpression.Rebind(Guid), then rebuild. Replacement must accept distinct references despite value equality; Rebind must preserve custom state or return null. Beutl.Editor.Components custom editor subclasses must migrate the removed ResumeElementPersistenceAfterFallbackReplacement and ResumeElementPersistenceAfterKnownRecoveryBlockerReplacement helpers to CompleteElementRepair(). Beutl.AgentToolkit consumers must replace PathComparison.ForCurrentPlatform with Beutl.Core's FilePathComparison.Comparison and provide their own policy for the removed public PathBoundary helpers.